gh-obsproject/obs-browser
Watch
1
0
Fork
You've already forked obs-browser
 0
Code Issues 37 Pull requests 13 Releases 15 Wiki Activity

Add procedure handler based API #431

Open
tytan652 wants to merge 1 commit from tytan652/proc_handler_api into master
pull from: tytan652/proc_handler_api
merge into: gh-obsproject:master
Conversation 14 Commits 1 Files changed 9 +966 -3
tytan652 commented 2024-01-10 12:01:12 -06:00 (Migrated from github.com)

Description

Warning

Not meant to be used by third-party, if they do it's at their own risk as if it's an unstable API.

I would like some feedback before stating that it can be used by third-party.

Relies on:

Adds a procedure handler that enables the use of obs-browser feature (only panels now) outside of the plugin itself without having to rely on symbols search inside the plugin file.

TODO:

  • Rename classes with a better namespace
  • Make the interface a installable header-only library with a CMake package

Motivation and Context

Inspired by how obs-websocket did its API and by the fact that implementing an browser API in the frontend-api looks more and more like a bad idea. I made a procedure handler based API for obs-browser.

Related to the wish to split service integration from the UI as plugins, but this is not strictly bond to services.

How Has This Been Tested?

  • Branch that replace make the whole UI use the new API which enables to test all the API: https://github.com/tytan652/obs-studio/pull/21
    • Integrations needs to be tested to test all API functions…
  • Header installation needs to be checked for Windows and macOS.
    • Tested on Windows, --component Development adds the header

Types of changes

  • New feature (non-breaking change which adds functionality)

Checklist:

  • My code has been run through clang-format.
  • I have read the contributing document.
  • My code is not on the master branch.
  • The code has been tested.
    • Me alone is not enough to test that
  • All commit messages are properly formatted and commits squashed where appropriate.
  • I have included updates to all appropriate documentation.
<!--- Please fill out the following template, which will help other contributors review your Pull Request. --> <!--- Make sure you’ve read the contribution guidelines here: https://github.com/obsproject/obs-studio/blob/master/CONTRIBUTING.rst --> ### Description <!--- Describe your changes in detail. --> <!--- If this change includes UI elements, please include screenshots. --> > [!WARNING] > ~Not meant to be used by third-party, if they do it's at their own risk as if it's an unstable API.~ > > **I would like some feedback before stating that it can be used by third-party.** - Supersedes https://github.com/obsproject/obs-studio/pull/5679 - Supersedes https://github.com/obsproject/obs-studio/pull/7638 Relies on: - https://github.com/obsproject/obs-studio/pull/10100 Adds a procedure handler that enables the use of obs-browser feature (only panels now) outside of the plugin itself without having to rely on symbols search inside the plugin file. TODO: - [x] Rename classes with a better namespace - [x] Make the interface a installable header-only library with a CMake package ### Motivation and Context <!--- Why is this change required? What problem does it solve? --> <!--- If it fixes an open GitHub Issue, or implements feature request --> <!--- from the Ideas page, please link to the issue here. --> Inspired by how obs-websocket did its API and by the fact that implementing an browser API in the frontend-api looks more and more like a bad idea. I made a procedure handler based API for obs-browser. Related to the wish to split service integration from the UI as plugins, but this is not strictly bond to services. ### How Has This Been Tested? <!--- Please describe in detail how you tested your changes. --> <!--- Include details of your testing environment (hardware, OS version, etc.),--> <!--- and the tests you ran, including how it may affect other areas of code. --> - Branch that replace make the whole UI use the new API which enables to test all the API: https://github.com/tytan652/obs-studio/pull/21 - Integrations needs to be tested to test all API functions… - Header installation needs to be checked for Windows and macOS. - Tested on Windows, `--component Development` adds the header ### Types of changes <!--- What types of changes does your PR introduce? Uncomment all that apply --> <!--- - Bug fix (non-breaking change which fixes an issue) --> - New feature (non-breaking change which adds functionality) <!--- - Tweak (non-breaking change to improve existing functionality) --> <!--- - Performance enhancement (non-breaking change which improves efficiency) --> <!--- - Code cleanup (non-breaking change which makes code smaller or more readable) --> <!--- - Breaking change (fix or feature that would cause existing functionality to change) --> <!--- - Documentation (a change to documentation pages) --> ### Checklist: <!--- Go over all the following points, and put an `x` in all the boxes that apply. --> <!--- If you're unsure about any of these, don't hesitate to ask. We're here to help! --> - [x] My code has been run through [clang-format](https://github.com/obsproject/obs-studio/blob/master/.clang-format). - [x] I have read the [**contributing** document](https://github.com/obsproject/obs-studio/blob/master/CONTRIBUTING.rst). - [x] My code is not on the master branch. - [x] The code has been tested. - Me alone is not enough to test that - [x] All commit messages are properly formatted and commits squashed where appropriate. - [x] I have included updates to all appropriate documentation.
PatTheMav commented 2024-01-12 12:25:01 -06:00 (Migrated from github.com)

I think the approach is correct and barring some concerns regarding the name (PhQCefWidget is a mouthful, OBSBrowserWidget would be more expressive, especially if this becomes a public API).

As for the API - how can we make this API available as part of obs-frontend-api without also exposing obs-browser publicly?

Because plugins need to be built out-of-tree, the browser API functions need to be available via the frontend API only, so it needs to be exposed there and needs to be compiled and linked without the browser plugin present.

I think the approach is correct and barring some concerns regarding the name (`PhQCefWidget` is a mouthful, `OBSBrowserWidget` would be more expressive, especially if this becomes a public API). As for the API - how can we make this API available as part of `obs-frontend-api` without also exposing `obs-browser` publicly? Because plugins need to be built out-of-tree, the browser API functions need to be available via the frontend API only, so it needs to be exposed there and needs to be compiled and linked without the browser plugin present.
tytan652 commented 2024-01-12 12:33:58 -06:00 (Migrated from github.com)

As for the API - how can we make this API available as part of obs-frontend-api without also exposing obs-browser publicly?

This design is meant to make it unrelated to obs-frontend-api (beside obs-browser internals using it).

…the browser API functions need to be available via the frontend API only…

This API does not need/depend on the frontend API.

> As for the API - how can we make this API available as part of `obs-frontend-api` without also exposing `obs-browser` publicly? This design is meant to make it unrelated to `obs-frontend-api` (beside obs-browser internals using it). > …the browser API functions need to be available via the frontend API only… This API does not need/depend on the frontend API.
PatTheMav commented 2024-01-12 12:36:58 -06:00 (Migrated from github.com)

As for the API - how can we make this API available as part of obs-frontend-api without also exposing obs-browser publicly?

This design is meant to make it unrelated to obs-frontend-api (beside obs-browser internal using it).

…the browser API functions need to be available via the frontend API only…

This API does not need/depend on the frontend API.

Got it. In that case this PR cannot supersede the PRs mentioned in the OP - the goal of at least one of those is to allow external plugins to create and manage their own browser docks.

As those plugins only link with libobs and obs-frontend-api, such functionality would need to be made available in the latter.

I still think it's worthwhile to have an API for internal use that does not require internal knowledge of the obs-browser plugin to interact with panels, but it's not solving the use case for external plugins then.

> > As for the API - how can we make this API available as part of `obs-frontend-api` without also exposing `obs-browser` publicly? > > This design is meant to make it unrelated to `obs-frontend-api` (beside obs-browser internal using it). > > > …the browser API functions need to be available via the frontend API only… > > This API does not need/depend on the frontend API. Got it. In that case this PR cannot supersede the PRs mentioned in the OP - the goal of at least one of those is to allow external plugins to create and manage their own browser docks. As those plugins only link with `libobs` and `obs-frontend-api`, such functionality would need to be made available in the latter. I still think it's worthwhile to have an API for internal use that does not require internal knowledge of the `obs-browser` plugin to interact with panels, but it's not solving the use case for external plugins then.
tytan652 commented 2024-01-12 12:39:03 -06:00 (Migrated from github.com)

Got it. In that case this PR cannot supersede the PRs mentioned in the OP - the goal of at least one of those is to allow external plugins to create and manage their own browser docks.

Beside the native flag issue, because of the QCefWidget in a window/dock without the native flag can go wrong.
This API can be used with the dock-related front-end API to make docks with a QCefWidget.

> Got it. In that case this PR cannot supersede the PRs mentioned in the OP - the goal of at least one of those is to allow external plugins to create and manage their own browser docks. Beside the native flag issue, because of the QCefWidget in a window/dock without the native flag can go wrong. This API can be used with the dock-related front-end API to make docks with a QCefWidget.
PatTheMav commented 2024-01-12 12:40:28 -06:00 (Migrated from github.com)

Got it. In that case this PR cannot supersede the PRs mentioned in the OP - the goal of at least one of those is to allow external plugins to create and manage their own browser docks.

Beside the native flag issue. This API can be used with the dock-related front-end API to make docks with a QCefWidget.

So with this merged, a leaner frontend API can be based on it you mean?

> > Got it. In that case this PR cannot supersede the PRs mentioned in the OP - the goal of at least one of those is to allow external plugins to create and manage their own browser docks. > > Beside the native flag issue. This API can be used with the dock-related front-end API to make docks with a QCefWidget. So with this merged, a leaner frontend API can be based on it you mean?
tytan652 commented 2024-01-12 12:45:42 -06:00 (Migrated from github.com)

So with this merged, a leaner frontend API can be based on it you mean?

No need for new frontend API.

  1. Plugins creates everything to have a PhQCefWidget instance
  2. Use obs_frontend_add_dock_by_id("test", "Test", ph_qcef_widget.qwidget())
  3. A dock with browser widget is created

Like I said earlier, there is an because of the missing native flag but it's inside the Frontend API that the issue should be solved.

Edit: I have made this github.com/tytan652/obs-studio@5ef1362ada in the past to fix the flag issue which rely to put a custom property in the given widget which is checked by the frontend API. This flag thing is a nightmare.

> So with this merged, a leaner frontend API can be based on it you mean? No need for new frontend API. 1. Plugins creates everything to have a PhQCefWidget instance 2. Use `obs_frontend_add_dock_by_id("test", "Test", ph_qcef_widget.qwidget())` 3. A dock with browser widget is created Like I said earlier, there is an because of the missing native flag but it's inside the Frontend API that the issue should be solved. Edit: I have made this https://github.com/tytan652/obs-studio/commit/5ef1362ada511f6ed62e3a4aa5c6dd0b663e58f1 in the past to fix the flag issue which rely to put a custom property in the given widget which is checked by the frontend API. This flag thing is a nightmare.
PatTheMav commented 2024-01-12 12:48:02 -06:00 (Migrated from github.com)

So with this merged, a leaner frontend API can be based on it you mean?

No need for new frontend API.

  1. Plugins creates everything to have a PhQCefWidget instance
  2. Use obs_frontend_add_dock_by_id("test", "Test", ph_qcef_widget.qwidget())
  3. A dock with browser widget is created

Like I said earlier, there is an because of the missing native flag but it's inside the Frontend API that the issue should be solved.

How can a plugin do that without access to the PhQCefWidget class? A plugin needs to be able to do that without access to obs-browser. Or is that just a string identifier and the plugin needs to throw around a void pointer?

> > So with this merged, a leaner frontend API can be based on it you mean? > > No need for new frontend API. > > 1. Plugins creates everything to have a PhQCefWidget instance > 2. Use `obs_frontend_add_dock_by_id("test", "Test", ph_qcef_widget.qwidget())` > 3. A dock with browser widget is created > > Like I said earlier, there is an because of the missing native flag but it's inside the Frontend API that the issue should be solved. How can a plugin do that without access to the `PhQCefWidget` class? A plugin needs to be able to do that without access to `obs-browser`. Or is that just a string identifier and the plugin needs to throw around a void pointer?
tytan652 commented 2024-01-12 12:49:15 -06:00 (Migrated from github.com)

How can a plugin do that without access to the PhQCefWidget class? A plugin needs to be able to do that without access to obs-browser. Or is that just a string identifier and the plugin needs to throw around a void pointer?

The API header (with PhCef* classes) is "public", no change in CMake was made to install it for now.

> How can a plugin do that without access to the `PhQCefWidget` class? A plugin needs to be able to do that without access to `obs-browser`. Or is that just a string identifier and the plugin needs to throw around a void pointer? The API header (with PhCef* classes) is "public", no change in CMake was made to install it for now.
PatTheMav commented 2024-01-12 12:52:24 -06:00 (Migrated from github.com)

How can a plugin do that without access to the PhQCefWidget class? A plugin needs to be able to do that without access to obs-browser. Or is that just a string identifier and the plugin needs to throw around a void pointer?

The header with PhCef* classes is "public", no change in CMake was made to install it for now.

That's what I was hinting at - obs-browser is not allowed to install headers. Only libobs and obs-frontend-api are. If PhCef* needs to be made available, it needs to happen in obs-frontend-api. No other module is allowed to expose anything to plugins.

> > How can a plugin do that without access to the `PhQCefWidget` class? A plugin needs to be able to do that without access to `obs-browser`. Or is that just a string identifier and the plugin needs to throw around a void pointer? > > The header with PhCef* classes is "public", no change in CMake was made to install it for now. That's what I was hinting at - `obs-browser` is not allowed to install headers. Only `libobs` and `obs-frontend-api` are. If `PhCef*` needs to be made available, it needs to happen in `obs-frontend-api`. No other module is allowed to expose anything to plugins.
tytan652 commented 2024-01-12 12:53:34 -06:00 (Migrated from github.com)

The header is not part of the obs-browser CMake module, for now it's a standalone interface.

The header is not part of the obs-browser CMake module, for now it's a standalone interface.
PatTheMav commented 2024-01-12 12:55:51 -06:00 (Migrated from github.com)

The header is not part of the obs-browser CMake module, for now it's a standalone interface.

Ah my bad - that header is not part of this PR then but would be added in an associated obs-studio PR?

> The header is not part of the obs-browser CMake module, for now it's a standalone interface. Ah my bad - that header is not part of this PR then but would be added in an associated obs-studio PR?
tytan652 commented 2024-01-12 12:58:03 -06:00 (Migrated from github.com)

Ah my bad - that header is not part of this PR then but would be added in an associated obs-studio PR?

lib/obs-browser-api.hpp is the header, it is in the PR and the API is made with procedure handler to avoid being dependent of the module. But obs-browser needs the header to have the API version macro.

> Ah my bad - that header is not part of this PR then but would be added in an associated obs-studio PR? `lib/obs-browser-api.hpp` is the header, it is in the PR and the API is made with procedure handler to avoid being dependent of the module. But obs-browser needs the header to have the API version macro.
tytan652 commented 2024-01-13 10:31:30 -06:00 (Migrated from github.com)

The API header is now installed with other OBS Studio headers and I changed the namespace of the API classes (Ph-->OBSBrowser).

Edit: Sorry I made a typo it was "now" not "not".

The API header is now installed with other OBS Studio headers and I changed the *namespace* of the API classes (`Ph`-->`OBSBrowser`). Edit: Sorry I made a typo it was "now" not "not".
tytan652 commented 2024-01-27 10:45:31 -06:00 (Migrated from github.com)

Update CMake part to match https://github.com/obsproject/obs-websocket/pull/1196 CMake as Pat approved it, like I said there https://github.com/obsproject/obs-websocket/pull/1196#discussion_r1468463474.

Update CMake part to match https://github.com/obsproject/obs-websocket/pull/1196 CMake as Pat approved it, like I said there https://github.com/obsproject/obs-websocket/pull/1196#discussion_r1468463474.
This pull request has changes conflicting with the target branch.
  • CMakeLists.txt
View command line instructions

Manual merge helper

Use this merge commit message when completing the merge manually.

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u origin 'tytan652/proc_handler_api:tytan652/proc_handler_api'
git switch 'tytan652/proc_handler_api'

Merge

Merge the changes and update on Forgejo.

Warning: The "Autodetect manual merge" setting is not enabled for this repository, you will have to mark this pull request as manually merged afterwards.

git switch 'master'
git merge --no-ff 'tytan652/proc_handler_api'
git switch 'tytan652/proc_handler_api'
git rebase 'master'
git switch 'master'
git merge --ff-only 'tytan652/proc_handler_api'
git switch 'tytan652/proc_handler_api'
git rebase 'master'
git switch 'master'
git merge --no-ff 'tytan652/proc_handler_api'
git switch 'master'
git merge --squash 'tytan652/proc_handler_api'
git switch 'master'
git merge --ff-only 'tytan652/proc_handler_api'
git switch 'master'
git merge 'tytan652/proc_handler_api'
git push origin 'master'
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels   
area/accessibility
Improves accessibility of the program

  
area/dependency
Issue is in external library or dependency, not OBS itself

  
Confirmed

   
Crash Report

   
Delayed
Merging has been delayed due to an issue

  
Design Issue
There is an issue with incorporating this pull request's design. It may take some time to finalize.

  
Duplicate

   
Feedback required
Issue does not contain enough information or is missing the template

  
Good first issue
New to contributing to OBS? Start here!

  
kind/bug
Categorizes issue or PR as related to a bug.

  
kind/cleanup
Non-breaking change which makes code smaller or more readable

  
kind/enhancement
Enhancements are not bugs or new features but can improve usability or performance.

  
platform/linux

   
platform/macos

   
platform/windows

   
priority/high
This is an important PR

  
Question

   
Request for Comments
More feedback & discussion is requested

  
Seeking Other Contributors
This PR needs help from any other developers who are willing to give a crack at this code

  
Seeking Testers

   
Won't Fix / By Design
No labels
area/accessibility
area/dependency
Confirmed
Crash Report
Delayed
Design Issue
Duplicate
Feedback required
Good first issue
kind/bug
kind/cleanup
kind/enhancement
platform/linux
platform/macos
platform/windows
priority/high
Question
Request for Comments
Seeking Other Contributors
Seeking Testers
Won't Fix / By Design
Milestone
Clear milestone
No items
No milestone
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
gh-obsproject/obs-browser!431
No description provided.