Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add procedure handler based API #431

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

tytan652
Copy link
Contributor

@tytan652 tytan652 commented Jan 10, 2024

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: Browser proc handler api tytan652/obs-studio#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.

@tytan652 tytan652 marked this pull request as draft January 10, 2024 18:02
@tytan652 tytan652 changed the title Add procedure handler based API header Add procedure handler based API Jan 10, 2024
@tytan652 tytan652 marked this pull request as ready for review January 10, 2024 18:31
@PatTheMav
Copy link
Member

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
Copy link
Contributor Author

tytan652 commented Jan 12, 2024

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
Copy link
Member

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
Copy link
Contributor Author

tytan652 commented Jan 12, 2024

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
Copy link
Member

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
Copy link
Contributor Author

tytan652 commented Jan 12, 2024

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 tytan652/obs-studio@5ef1362 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
Copy link
Member

PatTheMav commented Jan 12, 2024

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
Copy link
Contributor Author

tytan652 commented Jan 12, 2024

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
Copy link
Member

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
Copy link
Contributor Author

tytan652 commented Jan 12, 2024

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

@PatTheMav
Copy link
Member

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
Copy link
Contributor Author

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
Copy link
Contributor Author

tytan652 commented Jan 13, 2024

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".

@Lain-B Lain-B self-assigned this Jan 20, 2024
@tytan652 tytan652 force-pushed the proc_handler_api branch 2 times, most recently from 4ceb23d to 9680ac2 Compare January 27, 2024 16:43
@tytan652
Copy link
Contributor Author

Update CMake part to match obsproject/obs-websocket#1196 CMake as Pat approved it, like I said there obsproject/obs-websocket#1196 (comment).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement New feature or improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants