Skip to content

frontend: Use qobject_cast when casting QObjects - #11784

Open
cg2121 wants to merge 2 commits into
obsproject:masterfrom
cg2121:replace-dynamic-cast
Open

frontend: Use qobject_cast when casting QObjects#11784
cg2121 wants to merge 2 commits into
obsproject:masterfrom
cg2121:replace-dynamic-cast

Conversation

@cg2121

@cg2121 cg2121 commented Jan 28, 2025

Copy link
Copy Markdown
Contributor

Description

When casting between QObjects, qobject_cast should always be used. This increases performance as there is no RTTI (Run Time Type Information) with qobject_cast, like there is with dynamic_cast.

Using reinterpret_cast is bad practice, as you should use it only in very specific cases.

Motivation and Context

Better code

How Has This Been Tested?

compiled and ran OBS

Types of changes

  • Bug fix (non-breaking change which fixes an issue) -->
  • Performance enhancement (non-breaking change which improves efficiency)

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.

@cg2121
cg2121 force-pushed the replace-dynamic-cast branch from eb3f82d to 5582ea1 Compare January 28, 2025 11:49
@cg2121 cg2121 changed the title frontend: Use qobject_cast instead of dynamic_cast frontend: Use qobject_cast when casting QObjects Jan 28, 2025
@cg2121 cg2121 added kind/bug Categorizes issue or PR as related to a bug. area/ui-ux Anything to do with changes or additions to UI/UX elements. labels Jan 28, 2025
@cg2121
cg2121 marked this pull request as draft January 28, 2025 12:50
@cg2121

cg2121 commented Jan 28, 2025

Copy link
Copy Markdown
Contributor Author

Converting to draft, as #11785 needs to be merged first.

@cg2121
cg2121 marked this pull request as ready for review May 2, 2025 05:39
@cg2121
cg2121 force-pushed the replace-dynamic-cast branch from 5582ea1 to 7808d32 Compare May 2, 2025 05:41
@cg2121
cg2121 force-pushed the replace-dynamic-cast branch from 7808d32 to a9342f8 Compare May 5, 2025 18:34
@cg2121

cg2121 commented May 5, 2025

Copy link
Copy Markdown
Contributor Author

This PR now builds correctly

@cg2121
cg2121 requested a review from PatTheMav May 5, 2025 18:47
@cg2121
cg2121 force-pushed the replace-dynamic-cast branch 2 times, most recently from 3623034 to c2398aa Compare May 5, 2025 18:51
@RytoEX RytoEX self-assigned this May 5, 2025

@PatTheMav PatTheMav left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall I'm fine with this change, though it obviously touches a whole lot casts in the code that touch a whole lot of aspects of our frontend code, so this might be too late for the next version but should possibly merged after the next version is stabilised to give it some time to exist on the master branch.

@RytoEX WDYT?

@RytoEX

RytoEX commented Nov 19, 2025

Copy link
Copy Markdown
Member

Overall I'm fine with this change, though it obviously touches a whole lot casts in the code that touch a whole lot of aspects of our frontend code, so this might be too late for the next version but should possibly merged after the next version is stabilised to give it some time to exist on the master branch.

@RytoEX WDYT?

I think this should be merged for 32.1, merge timing TBD. It does have a merge conflict though.

@Warchamp7
Warchamp7 force-pushed the replace-dynamic-cast branch from c2398aa to 6789390 Compare November 27, 2025 22:09
@Warchamp7

Copy link
Copy Markdown
Member

I've rebased this PR

@Warchamp7 Warchamp7 moved this from In Review to Ready For Review in OBS Studio 32.1 PR Considerations Nov 27, 2025
@Warchamp7 Warchamp7 moved this from Ready For Review to Ready For Merge in OBS Studio 32.1 PR Considerations Nov 27, 2025
@RytoEX RytoEX added this to the OBS Studio 32.1 milestone Dec 18, 2025
@RytoEX

RytoEX commented May 7, 2026

Copy link
Copy Markdown
Member

This requires a rebase.

@Warchamp7
Warchamp7 force-pushed the replace-dynamic-cast branch from f6596c5 to 5c91477 Compare May 8, 2026 02:51
@Warchamp7

Copy link
Copy Markdown
Member

This requires a rebase.

Rebased and conflicts resolved, however I've also added a to-be-squashed commit with some additional casts that were either introduced in the rebase, or missed originally.

@Warchamp7
Warchamp7 force-pushed the replace-dynamic-cast branch from 5c91477 to f6cc03e Compare May 8, 2026 02:54

@sebastian-s-beckmann sebastian-s-beckmann left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't gone through all the files (only about one third) but the idea should be clear.
There are still a lot of qobject_casts that should be static_casts. qobject_cast is only needed where it isn't clear (or assumed) that the cast succeeds. Especially in cases where the pointer is dereferenced immediately after the cast, qobject_cast doesn't make sense. Also in some cases, instead of casting from the main window it should just be OBSBasic::Get().
Also commit authorship is still changed.

Comment thread frontend/settings/OBSBasicSettings.cpp Outdated
Comment thread frontend/components/VolumeControl.cpp
Comment thread frontend/dialogs/OBSBasicSourceSelect.cpp Outdated
Comment thread frontend/importer/ImporterEntryPathItemDelegate.cpp Outdated
Comment thread frontend/utility/audio-encoders.cpp Outdated
Comment thread frontend/widgets/OBSBasic.cpp Outdated
Comment thread frontend/widgets/OBSBasic_Clipboard.cpp Outdated
Comment thread frontend/widgets/OBSBasic_ContextToolbar.cpp
@Warchamp7

Copy link
Copy Markdown
Member

I haven't gone through all the files (only about one third) but the idea should be clear. There are still a lot of qobject_casts that should be static_casts. qobject_cast is only needed where it isn't clear (or assumed) that the cast succeeds. Especially in cases where the pointer is dereferenced immediately after the cast, qobject_cast doesn't make sense. Also in some cases, instead of casting from the main window it should just be OBSBasic::Get(). Also commit authorship is still changed.

Commit authorship is intentional since at this point it contains changes from both of us.

Will address all the feedback.

@Warchamp7
Warchamp7 force-pushed the replace-dynamic-cast branch 2 times, most recently from 15aa946 to 87c6774 Compare May 8, 2026 18:48
Comment thread shared/properties-view/properties-view.cpp Outdated
@Warchamp7
Warchamp7 force-pushed the replace-dynamic-cast branch from 87c6774 to 677a0d8 Compare May 10, 2026 18:55

@sebastian-s-beckmann sebastian-s-beckmann left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems correct

@PatTheMav PatTheMav left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even though it might seem superfluous to use qobject_cast or dynamic_cast without checking the result for nullptr after, even a missing nullptr check will create deterministic crash points (mainly when that pointer is dereferenced).

But as the crash by UpdateContextBar demonstrates, static_cast will produce a valid pointer and thus dereferencing will succeed and if both types happen to have identical member or method names (like GetSource) code might potentially succeed and produce corrupted data or crash at arbitrary points later.

Otherwise I agree that where no checks and balances are used (and code is pretty blasé about safety so far anyway) using static_cast directly seems more "honest".

Comment thread frontend/widgets/OBSBasic_ContextToolbar.cpp Outdated
Comment thread frontend/dialogs/OBSYoutubeActions.cpp Outdated
Comment thread frontend/docks/YouTubeAppDock.cpp Outdated
Comment thread frontend/importer/ImporterEntryPathItemDelegate.cpp
Comment thread frontend/settings/OBSBasicSettings.cpp Outdated
Comment thread frontend/settings/OBSBasicSettings_Stream.cpp Outdated
@Warchamp7
Warchamp7 force-pushed the replace-dynamic-cast branch 2 times, most recently from 8e97660 to c788f7f Compare May 18, 2026 20:44
@github-project-automation github-project-automation Bot moved this to Ready For Review in 33.0 Release Tracker Jul 16, 2026
@github-project-automation github-project-automation Bot moved this to Ready For Review in 33.1 Release Tracker Jul 16, 2026
@Warchamp7 Warchamp7 removed this from the OBS Studio 32.2 milestone Jul 30, 2026
@Warchamp7
Warchamp7 force-pushed the replace-dynamic-cast branch 2 times, most recently from b6fa120 to c1c6bed Compare August 14, 2026 04:26
When casting between QObjects, qobject_cast should always be used for dynamic
casts. This increases performance as there is no RTTI (Run Time Type
Information) with qobject_cast, like there is with dynamic_cast. In other cases
where we are sure or assume the cast is valid, use static_cast.

Using reinterpret_cast is bad practice, as you should use it in very
specific cases.

Co-Authored-By: Clayton Groeneveld <19962531+cg2121@users.noreply.github.com>
@Warchamp7
Warchamp7 force-pushed the replace-dynamic-cast branch 2 times, most recently from 29ebe44 to 359c1fa Compare August 19, 2026 21:27
@Warchamp7
Warchamp7 force-pushed the replace-dynamic-cast branch from 359c1fa to f502b57 Compare September 5, 2026 00:05
SourceTree is designed for SourceTreeModel and should avoid
using any other model. Enforce this on the SourceTree itself
@Warchamp7
Warchamp7 force-pushed the replace-dynamic-cast branch from f502b57 to 4e8d78c Compare September 5, 2026 00:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/ui-ux Anything to do with changes or additions to UI/UX elements. kind/bug Categorizes issue or PR as related to a bug. kind/cleanup Non-breaking change which makes code smaller or more readable

Projects

Status: Ready For Review

Development

Successfully merging this pull request may close these issues.

5 participants