frontend: Use qobject_cast when casting QObjects

[cg2121]

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]

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

[cg2121]

This PR now builds correctly

[PatTheMav]

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]

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]

I've rebased this PR

[gxalpha]

Just looking at it, I think most of these should use static_cast instead of either dynamic_cast or qobject_cast. qobject_cast is better than dynamic_cast if you want the dynamic behavior (i.e. have the object be null if the cast fails), but if you already know/assume that the cast will succeed (and dont check for new_obj == nullptr), you can just use static_cast.

Also, original git commit authorship appears to have gotten lost, that should probably be reverted.

[Warchamp7]

Just looking at it, I think most of these should use static_cast instead of either dynamic_cast or qobject_cast. qobject_cast is better than dynamic_cast if you want the dynamic behavior (i.e. have the object be null if the cast fails), but if you already know/assume that the cast will succeed (and dont check for new_obj == nullptr), you can just use static_cast.

Also, original git commit authorship appears to have gotten lost, that should probably be reverted.

Adjusted the PR for this, as well as removed a macro in the AutoConfig files that were casting the wizard() method in favor of an inline method on those pages.

[RytoEX]

This requires a rebase.

[Warchamp7]

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.

[gxalpha]

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.

[Warchamp7]

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.

[gxalpha]

Seems correct

[PatTheMav]

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

[PatTheMav]

This change crashes OBS with a media source because the cast always succeeds and then toolbar->GetSource() will interpret arbitrary memory in the MediaControls object as an OBSWeakSource and crash deep within libobs's reference counting system.

As can be seen below, this entire code depends on this cast failing if the widget is indeed a MediaSource and not a SourceToolbar (hence the branch below), so dynamic_cast or qobject_cast is necessary here.

[PatTheMav]

This will introduce a potential crash if a TwitchAuth or any other object with the common Auth base class is passed into the constructor.

I'd prefer to keep the dynamic_cast here and add a TODO to refactor this (and open a new GitHub issue to keep track).

IMO the constructor should require a YoutubeApiWrappers object here and should not even allow a base class object to be passed in (it doesn't really make sense from an API design point of view).

It's the caller's responsibility to properly upcast the generic Auth object OBSBasic provides into the YoutubeApiWrappers pointer and potentially abort if that isn't possible.

[PatTheMav]

I'm unsure about this, as Auth can be any other derived auth object apart from YoutubeApiWrappers which just inherits from Auth and adds a whole lot of additional functionality.

Indeed this whole function's purpose is to return the "API object" (so probably a whole lot of stuff other than OAuth-related functionality), so it needs to fail if the object in memory cannot be cast into YoutubeApiWrappers (and otherwise the if (apiYouTube) conditional doesn't make much sense).

Once again I'd probably opt for keeping dynamic_cast here and add a TODO for refactoring (probably decoupling the API implementation from the OAuth implementation to make this safer).

[PatTheMav]

Is there value to add a small comment here (and throughout this PR) to explain and point out why the type can be assumed safely, referencing the base types in an associated UI file or where the associated widget is initialised?

In cases like these it would be helpful to establish the logical connection between this event handler and the code that sets up the handler, in a way pointing out that

if that code over there is changed, this code over here will crash

which might otherwise get lost in the thousands of lines of code we have.

This would apply to all correct uses of static_cast throughout, like:

• Casting to a specific layout type from generic QLayout - this creates an implicit link between the UI file (or explicit UI setup code) creating and setting the specific layout sub-type and the static_cast.
• Casting to a specific model type from QAbstractItemModel - QWidgets have a setModel method that allow new models to be set, so if any API method outside of the abstract item model is used, why can we be "sure" that it's the specific item model we're casting to (and are we sure that nobody else calls setModel ever?).
• Casting any generic QWidget to a more specific type - similar situation to the cast to a specific QLayout.

The main purpose would be to create a paper trail for these connections that are non-obvious in code, as the compiler will not be able to catch these mistakes.

[PatTheMav]

This change will allow creation of OBSBasicSettings objects with arbitrary QWidgets as parent pointers, leading to potential crashes at arbitrary points when main is interacted with in a way that is only possible with an actual OBSBasic instance.

The qobject_cast could at least ensure that main is a nullptr, but then the constructor body should actually check if main is not nulled and throw an exception otherwise. Instead it currently calls EnableOutputs unconditionally.

So if it is indeed required that OBSBasicSettings can only ever be instantiated with an OBSBasic parent, that should be explicitly communicated by the API instead of accepting an arbitrary QWidget pointer. This is a class of errors and constraints that we can and should enforce at compile time.

[PatTheMav]

I guess it's "fine" for now because every OAuth implementation in the program currently uses OAuthStreamKey.

But wouldn't it make more sene if the pointer is passed into the function directly, like so:

	if (type == Auth::Type::OAuth_StreamKey || type == Auth::Type::OAuth_LinkedAccount) {
        OAuthStreamKey *streamKey = static_cast<OAuthStreamKey *>(auth.get());

		OnOAuthStreamKeyConnected(streamKey);
	}

At least OBSBasicSettings::OnAuthConnected runs some checks and balances here, but it also requires that whatever ui->service->currentText() returns as the UI label for the service selection is already in sync with the global auth object (again there is no other subclass of it yet, but as designed there could be). 🫠