Skip to content

Conversation

@tytan652
Copy link
Collaborator

@tytan652 tytan652 commented Oct 22, 2022

Description

Depends on:

Dependency of:

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_qdock() 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_qdock() 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_qdock() 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()

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.

Motivation and Context

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

Also make the API more neutral toward the used dock system for non-custom methods.

How Has This Been Tested?

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

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

@tytan652 tytan652 added Bug Fix Non-breaking change which fixes an issue Enhancement Improvement to existing functionality New Feature New feature or plugin labels Oct 22, 2022
@tytan652 tytan652 force-pushed the unique_id_for_docks branch from e9a60db to 03a9519 Compare October 22, 2022 15:32
@PatTheMav
Copy link
Member

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.

@tytan652
Copy link
Collaborator Author

tytan652 commented Oct 27, 2022

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 tytan652 force-pushed the unique_id_for_docks branch from 03a9519 to 347c807 Compare November 20, 2022 14:03
@tytan652 tytan652 added this to the OBS Studio 29.1 milestone Nov 20, 2022
@PatTheMav
Copy link
Member

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?

@tytan652
Copy link
Collaborator Author

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

@PatTheMav
Copy link
Member

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.

@tytan652 tytan652 force-pushed the unique_id_for_docks branch from 347c807 to ff91a93 Compare December 21, 2022 08:38
@tytan652 tytan652 force-pushed the unique_id_for_docks branch 2 times, most recently from 1038eb8 to ebd3e60 Compare January 20, 2023 20:12
@tytan652 tytan652 force-pushed the unique_id_for_docks branch 2 times, most recently from 80f8041 to aa98043 Compare January 29, 2023 13:05
@tytan652 tytan652 force-pushed the unique_id_for_docks branch from aa98043 to db0b175 Compare February 7, 2023 17:28
@tytan652 tytan652 force-pushed the unique_id_for_docks branch 3 times, most recently from dbeee22 to 2611e11 Compare February 20, 2023 11:43
@tytan652 tytan652 force-pushed the unique_id_for_docks branch 2 times, most recently from 5c1b501 to 5caf6e8 Compare February 27, 2023 12:24
@tytan652 tytan652 force-pushed the unique_id_for_docks branch from 5caf6e8 to 342d8bd Compare March 8, 2023 10:07
@tytan652
Copy link
Collaborator Author

tytan652 commented Mar 8, 2023

Renamed obs_frontend_add_custom_dock to obs_frontend_add_custom_qdock.

@tytan652 tytan652 force-pushed the unique_id_for_docks branch 3 times, most recently from 717c37d to c75dac5 Compare March 11, 2023 09:27
@tytan652 tytan652 force-pushed the unique_id_for_docks branch from c75dac5 to 63052c9 Compare March 17, 2023 08:11
@tytan652 tytan652 force-pushed the unique_id_for_docks branch from 63052c9 to 2de16d8 Compare March 17, 2023 09:22
@tytan652 tytan652 marked this pull request as ready for review March 17, 2023 09:22
@tytan652 tytan652 force-pushed the unique_id_for_docks branch from 2de16d8 to 2de8ec0 Compare March 17, 2023 09:51
@tytan652
Copy link
Collaborator Author

Replace QUuid usage in obs_frontend_add_dock by os_generate_uuid.

@tytan652 tytan652 force-pushed the unique_id_for_docks branch from 2de8ec0 to f9bab9d Compare March 17, 2023 10:03
@tytan652
Copy link
Collaborator Author

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 tytan652 force-pushed the unique_id_for_docks branch 3 times, most recently from 44bfb83 to c0d6ea0 Compare May 3, 2023 06:53
@tytan652 tytan652 force-pushed the unique_id_for_docks branch from c0d6ea0 to b51c07d Compare May 10, 2023 12:43
@tytan652 tytan652 force-pushed the unique_id_for_docks branch from b51c07d to cb1e0f4 Compare May 27, 2023 22:49
tytan652 added 3 commits May 31, 2023 08:30
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.
obs_frontend_add_dock() is deprecated in favor of
obs_frontend_add_dock_by_id()
Some plugin does that by deleting the QAction returned by
obs_frontend_add_dock().

Now that obs_frontend_add_dock() is deprecated,
obs_frontend_add_custom_qdock() replace this usage.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Bug Fix Non-breaking change which fixes an issue Enhancement Improvement to existing functionality New Feature New feature or plugin

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants