obs-studio icon indicating copy to clipboard operation
obs-studio copied to clipboard

Avoid having two docks with the same object name

Open tytan652 opened this issue 3 years ago • 5 comments

Description

Depends on:

  • https://github.com/obsproject/obs-studio/pull/7636

Dependency of:

  • https://github.com/obsproject/obs-studio/pull/7638

https://doc.qt.io/qt-6/qmainwindow.html#saveState

You should make sure that this property is unique for each QToolBar and QDockWidget you add to the QMainWindow.

We should avoid letting plugins add docks with the same object name. So to implement that:

  • obs_frontend_add_dock() is deprecated in favor of obs_frontend_add_dock_by_id() and obs_frontend_add_custom_dock() which ask for an unique id (object name). The dock is not added if the id is already used and return false in this case.
  • Also the new method does not return a QAction since internally we use the one included in QDockWidget.
  • obs_frontend_add_dock() implements a temporary workaround to avoid this.
  • obs_frontend_add_custom_dock() replace the usage of obs_frontend_add_dock() where the returned QAction is deleted (e.g. Sources Dock plugin). The dock is not added if the id is already used and return false in this case.
  • If the object name of docks added with obs_frontend_add_dock() and obs_frontend_add_custom_dock() happen to be changed, the object name is changed back to its original.
  • Docks added with new methods can be removed from the UI with obs_frontend_remove_dock()

Motivation and Context

Fix a potential issue and preparation for PRs that depends on this one.

How Has This Been Tested?

  • Modified Downstream Keyer plugin to use obs_frontend_add_dock_by_id() or obs_frontend_add_custom_dock()

Types of changes

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

Checklist:

  • [x] My code has been run through clang-format.
  • [x] I have read the contributing document.
  • [x] My code is not on the master branch.
  • [x] The code has been tested.
  • [x] All commit messages are properly formatted and commits squashed where appropriate.
  • [x] I have included updates to all appropriate documentation.

tytan652 avatar Oct 22 '22 14:10 tytan652

Personally I'd prefer to avoid the whole "function_call[insert integer here]" shenanigans, it always seems half-arsed to me. If code uses the old signature it should break and throw a compile-time error, forcing any authors relying on it to update to the new API, especially as this probably won't end up in a minor version update anyway.

Otherwise we're dragging a whole ecosystem of mixed implementations with us until we end up with obs_frontend_add_dock5.

PatTheMav avatar Oct 27 '22 13:10 PatTheMav

Then how should I name it rather than adding a 2, because the old function is deprecated because the custom QAction is no longer created and and the new one require an id.

Edit: C does not support having two function named the same even with different parameters and return type.

tytan652 avatar Oct 27 '22 13:10 tytan652

Is there other code that uses this function? Because if it is only us, then we should fix all invocations of the code, but I'm afraid you might tell me that plugins are allowed to use this function?

PatTheMav avatar Nov 22 '22 21:11 PatTheMav

If you are talking about obs_frontend_add_dock(), it's a Frontend API function used by many plugins.

tytan652 avatar Nov 22 '22 21:11 tytan652

I was afraid you'd say that - internal UI methods shouldn't be exposed as-is to the API, but that's for later to fix.

I'd suggest naming the function obs_frontend_add_dock_by_id then, because it's a) different and b) clearly communicates what it does.

PatTheMav avatar Nov 22 '22 21:11 PatTheMav

Renamed obs_frontend_add_custom_dock to obs_frontend_add_custom_qdock.

tytan652 avatar Mar 08 '23 10:03 tytan652

Replace QUuid usage in obs_frontend_add_dock by os_generate_uuid.

tytan652 avatar Mar 17 '23 09:03 tytan652

Added this note to the PR description:

Note: If obs_frontend_add_custom_qdock() usecase is something we want to avoid, review the code about removing it and deprecate the usecase through obs_frontend_add_dock() deprecation.

tytan652 avatar Mar 21 '23 11:03 tytan652