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

UI: Add API to create Browser Docks #5679

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

Conversation

Elgato-AStory
Copy link
Contributor

@Elgato-AStory Elgato-AStory commented Dec 21, 2021

Description

Provides two new frontend API calls to add, and delete browser docks that tap into the internal CEF instance.

Motivation and Context

This change will allow plugins to automatically install and manage browser docks as a convenience. As of the latest master commit, the only way to do this was to modify the ini file while OBS is not running, which would be a more fragile approach and less convenient.

I recognize that the custom browser docks are still under active development but this is a set of API calls that I envision would be quite useful to have.

How Has This Been Tested?

This change has been manually tested on a recent version of Windows 10 by writing a basic plugin to trigger these API calls.

Types of changes

  • Tweak (non-breaking change to improve existing 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.
  • All commit messages are properly formatted and commits squashed where appropriate.
  • I have included updates to all appropriate documentation.

@Elgato-AStory Elgato-AStory marked this pull request as ready for review December 21, 2021 02:46
@WizardCM WizardCM added the Enhancement Improvement to existing functionality label Dec 21, 2021
@WizardCM
Copy link
Member

WizardCM commented Dec 21, 2021

Would it also make sense to provide an "update" API where you specify the UUID of the dock you want to update, and the new title/url?

This also makes me wonder if it'd be worth having a way to set custom docks as "private", so that plugins can create a web dock easily that cannot be accidentally modified/deleted by the user.

I'm currently not 100% sold on allowing the plugin to specify the UUID when creating the dock, but as the completed object isn't returned to allow the API user to do follow-up actions, it's probably fine.

Edit: Additionally, as the docs change is specifically related to the new APIs, it can be in the same commit.

@tt2468
Copy link
Member

tt2468 commented Dec 21, 2021

Depending on how this PR goes, I think that we should see at least one of the two things happen:

  • If we allow the caller to specify UUID, then there should be collision checking for that implemented, with a boolean return value on the frontend request.
  • If we generate all of our own UUIDs, then the frontend request should have a string return with the new UUID.

Since the create function only needs to be called once (OBS will save these browsers in the normal list and recreate them across loads), I would lean more towards having the frontend API generate the UUID and return it back to the caller.

@Elgato-AStory
Copy link
Contributor Author

One of the ideas that I was thinking of was that a plugin could have UUIDs that would be used to determine at a glance if the dock that was installed was installed by the plugin, e.g. the plugin just uses a browser dock for its UI, and it can hardcode a specific UUID for that dock. Then the plugin can determine if it's already installed the dock without needing to maintain its own separate state on the user's machine since the UUID will always be the same for that plugin (or at least one of a few possible UUIDs). On the other hand, maintaining that separate state isn't particularly hard.

As long as the UUIDs are generated non-adversarially, I don't think there's really an issue with letting the plugin choose its UUID. If you think that conflicting UUIDs will be a genuine problem though, I can understand wanting to restrict that. Ultimately, an adversary could just inspect/modify the ini file, but it would just be more annoying for them since they can't do it while OBS is running.

I'm up for changing it to suit either approach.

@tt2468
Copy link
Member

tt2468 commented Dec 21, 2021

Would it be possible to validate the URL of the dock? I would think that since the URL can be modified by the end user anyway, validating the URL itself would be more appropriate. That would allow you to more effectively determine if a dock is properly installed.

@Elgato-AStory
Copy link
Contributor Author

Elgato-AStory commented Dec 21, 2021

Would it also make sense to provide an "update" API where you specify the UUID of the dock you want to update, and the new title/url?

I think it would, especially as there will likely be no APIs for getting/setting the actual dock layout. An update will let the plugin update the URL without disrupting the dock's position if needed.

Edit: Additionally, as the docs change is specifically related to the new APIs, it can be in the same commit.

Alright, I'll just squash that in.

@Elgato-AStory
Copy link
Contributor Author

I would think that since the URL can be modified by the end user anyway, validating the URL itself would be more appropriate. That would allow you to more effectively determine if a dock is properly installed.

Yeah, that would probably be the smartest choice rather than just blindly assuming the user hasn't changed it. It would still be useful to be able to tell if the user has changed it at least. It would give the plugin author a chance to ask the user if they're fine with either clobbering their changes or if they should just add in a new dock.

@tt2468
Copy link
Member

tt2468 commented Dec 21, 2021

Perhaps this PR should also implement a lock property to docks, which would disable user input for a dock's row in the editor if locked. That way, you can have the dock show up in the list, but not have to worry about the end user modifying it.

@Elgato-AStory
Copy link
Contributor Author

Elgato-AStory commented Dec 21, 2021

I'm not sure how I feel about that. Perhaps if it was purely an advisory lock that the user could override with minimal effort it might be fine? Or maybe the locked state comes with the requirement that any locked rows are unlocked and/or removed if the plugin that made them is no longer present? I just don't want a misbehaving plugin to be able to make hard-to-reverse changes that persist even beyond the removal of said plugin that inhibit the user's ability to use the software.

@WizardCM
Copy link
Member

WizardCM commented Dec 21, 2021

I can see the logic behind a lock, but private might suit better - the value of private could be the identifier for the plugin (though we could label it owner instead?). Once the plugin is no longer detected, OBS could delete the private browser dock automatically.

@tt2468
Copy link
Member

tt2468 commented Dec 21, 2021

I think that private would make more sense, where the browser dock would have to be registered on every load by the plugin, and would show in the browser docks dialog, but not be editable. I'm not sure if it's necessary to track owners, since we don't really do that for things like sources either.

@WizardCM
Copy link
Member

Hmm, if we're registering it on every load of the plugin, then it doesn't make sense to load it as part of Custom Browser Docks. If that's what we want to do, we could use CBD internally for now and just not save private browser docks to the config so that this PR can be merged without changing too much behind the scenes, and those changes could always be made later.

@Elgato-AStory
Copy link
Contributor Author

I put together a proof of concept for an API that just makes a plugin register the browser dock at every load. It doesn't need to run as a part of the Custom Browser Dock code, though the amount of code needed is low enough that it can probably just live with it. It would probably be best to share any webpage patching needed in the custom browser docks anyway, so I'm thinking I'll factor that out into a separate function that is used by both systems. It behaves well and I think I'll rewrite this PR to use this new approach.

The only API call will be void *obs_frontend_add_browser_dock(const char *id, const char *title, const char *url) and it returns the associated QAction* pointer. On the first registration, the dock will be hidden and the default position will be the same as the custom browser docks when they are unhidden. The ID parameter is used to allow the title to change without losing layout info for the dock.

I am kind of torn on the return value though. I think it may be more useful to return the underlying QDockWidget* and the caller must pass it into obs_frontend_add_dock to register the menu item and to retrieve the associated QAction* pointer. I could also see making passing the dock into obs_frontend_add_dock optional and just make any attempts to pass in the same dock into obs_frontend_add_dock a second time do nothing other than return the associated QAction* pointer. What are your thoughts?

@Elgato-AStory
Copy link
Contributor Author

Elgato-AStory commented Dec 23, 2021

I'm going to go with the plan to return the QDockWidget* and allow the caller to optionally pass it in to obs_frontend_add_dock to retrieve the QAction* and deduplicate any attempts to call obs_frontend_add_dock.

I'm planning to deduplicate based on the UUID generated. This would carry a vanishingly small chance of a UUID collision causing an extra dock to not show up in the menu. Should I serialize the last 12 bits of the UUIDs to remove any chance of a UUID collision or should I not bother? It would only realistically be an issue if the PRNG fails hard.

I'm also going to add a second API call to delete a dock and its corresponding QAction. This will prevent a plugin from having to link in Qt to delete a dock it creates using the new API, and it will also allow scripts that are using the API to also delete these docks without forcing the user to restart OBS or rely on a Qt wrapper. It will just be void obs_frontend_remove_browser_dock(void *dock)

@Elgato-AStory Elgato-AStory changed the title UI: Add API to modify Custom Browser Docks UI: Add API to create Browser Docks Jan 7, 2022
@WizardCM WizardCM self-requested a review January 8, 2022 23:06
@tytan652
Copy link
Collaborator

tytan652 commented Jan 16, 2022

I want to point out that if OBS is running under Wayland, dock(panels) are disabled.
So there is a context where browser docks are not available even if OBS is built with obs-browser.

Maybe adding bool obs_frontend_browser_available() to the API that:

  • return false if OBS is not built with it
  • return true on Windows, macOS and Linux/FreeBSD if OBS is built with it and without Wayland support for Linux/FreeBSD
  • return value of !(obs_get_nix_platform() == OBS_NIX_PLATFORM_WAYLAND) on Linux/FreeBSD if OBS is built with it and with Wayland support

It could allow plugins to know the availability of browser feature before trying to use others and check for a nullptr.

@John-Dean
Copy link

Hello,
Is there any update on this PR? I saw @tytan652 has made another PR (#7638).

Am I misunderstanding, or can we already create browser docks? I noticed when you sync a Twitch account it generates 2 panels ([code here]) for stream controls and chat, is there a reason I can't mimic this code?

John

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement Improvement to existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants