Skip to content

Conversation

gxalpha
Copy link
Member

@gxalpha gxalpha commented Aug 31, 2025

Description

Enables Qt strict mode, and makes the changes needed in the codebase to do so.
Strict mode disables "a number of Qt APIs that are deemed suboptimal or dangerous" and that, while not yet officially deprecated, will be removed in the long term. This includes (but isn't limited to) the foreach macro, contextless connects (connect() calls without a receiver), implicit conversions from QByteArray to const char *, and implicit conversions from QString to QUrl.

Motivation and Context

We have previously done cleanups on foreach (see #9662), and things like the contextless connect calls sometimes come up during PR reviews. As can be seen by the first commit, especially the implicit QByteArray->const char * conversions can cause unneeded back-and-forth conversions in some scenarios, not having them is actually advantageous as it reveals the conversions that are happening.
Qt is also moving towards using strict mode themselves (see QTBUG-132327).
By fixing all of these occurrences and turning on strict mode, we prevent the dangerous APIs from being used again in the future.

How Has This Been Tested?

macOS 26.
The code compiles.
I tested in all cases that the connections are still hit, and that conversions still yield the correct results (yes, this took a while.)

Types of changes

  • Code cleanup (non-breaking change which makes code smaller or more readable)

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.

SetComboByName, SetComboByValue and SetInvalidValue all pass their
parameters to Qt functions which only take QStrings. As a consequence,
we have cases we'd convert a QString to a const char* to call these
functions, only for the functions to implicitly convert them back into
QStrings. We can avoid this by passing QStrings directly. In cases where
we did actually pass const chars, the (implicit) conversion now just
takes place earlier.
@gxalpha gxalpha requested a review from RytoEX August 31, 2025 12:05
@gxalpha gxalpha added the Code Cleanup Non-breaking change which makes code smaller or more readable label Aug 31, 2025
@tytan652
Copy link
Collaborator

tytan652 commented Aug 31, 2025

If calling connect() as an object method is meaningless since you add the object in the parameter, wouldn't it be better to replace every call with the closest static definition (e.g. QObject::connect()) ?

@gxalpha
Copy link
Member Author

gxalpha commented Aug 31, 2025

If calling connect() as an object method is meaningless since you add the object in the parameter, wouldn't it be better to replace every call with the closest static definition (e.g. QObject::connect()) ?

I'm not sure what you mean, all connect calls are already using the static declarations (both before and after this PR). Prepending QObject:: wouldn't change anything, except make the code more verbose.
The only non-static connect function that exists in Qt is inline QMetaObject::Connection connect(const QObject *sender, const char *signal, const char *member, Qt::ConnectionType type = Qt::AutoConnection) const; which we're not using as it would need the SIGNAL/SLOT macros.

@tytan652
Copy link
Collaborator

tytan652 commented Aug 31, 2025

Things like object->connect( are completely meaningless with your changes, you can just replace them with connect( or QObject::connect( when outside of a QObject.

@gxalpha
Copy link
Member Author

gxalpha commented Aug 31, 2025

Ah, that's what you mean. I'd argue technically that's not exactly in scope for this change but there's only one instance of that that doesn't use the contextless connect, so I guess I'll add it to this PR.

@gxalpha gxalpha force-pushed the qt-strict branch 3 times, most recently from 6a296f8 to 1f87d4d Compare August 31, 2025 16:56
Qt strict mode disables APIs deemed "'suboptimal' or 'dangerous'" [1]
and "clearly undesirable" [2] by Qt that will be removed in the long
term.
Usages of the APIs in OBS have been removed in the previous commits, and
by setting this flag we keep ourselves from using them again.
The versioning works in a way where e.g. a new addition to this in 6.9
would only be disabled if at least 0x060900 is set. By setting 0xFF0000,
we're effectively disabling any APIs that are deemed to be bad in any
future Qt version (up to 255).
While this could lead to OBS not being buildable against bleeding edge
Qt, it also means that it will be noticed early. Should the disabled API
turn out to be too complex to remove, the value can be downgraded to the
last working one then.

[1] qt/qtbase@3a6c8e0
[2] qt/qtbase@f9163ae
@Penwy
Copy link
Contributor

Penwy commented Aug 31, 2025

Tested on Ubuntu 24.04, compiles without issues, played around with every different bit of UI I could think of, found no issues.
I am available for any further specific testing if needed.

@RytoEX RytoEX self-assigned this Sep 2, 2025
@RytoEX
Copy link
Member

RytoEX commented Sep 2, 2025

Will probably target this for 32.1.

Copy link
Member

@PatTheMav PatTheMav left a comment

Choose a reason for hiding this comment

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

Looks good overall, I'd prefer if we could replace use of QT_TO_UTF8 with the explicit .toUtf8().constData() call, particularly as some changed code uses that or constData() already.

I'd probably also like to see a variant of QTStr that accepts a QString and a renaming of QTStr itself as that function name is incredibly uninformative, but those two can wait for another PR.

lastErrorReason.clear();
const QByteArray url = YOUTUBE_LIVE_CHANNEL_URL "?part=snippet,contentDetails,statistics"
"&mine=true";
const char *url = YOUTUBE_LIVE_CHANNEL_URL "?part=snippet,contentDetails,statistics"
Copy link
Member

Choose a reason for hiding this comment

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

Use std::string_view instead of raw char pointers in C++ (prefer C++ types over C types).

Notably string views can also be static constexpr and either be resolved at compile time or (when reaching to their underlying character data via .data()) become read accesses into the data region (no allocations).

@gxalpha
Copy link
Member Author

gxalpha commented Sep 25, 2025

Looks good overall, I'd prefer if we could replace use of QT_TO_UTF8 with the explicit .toUtf8().constData() call, particularly as some changed code uses that or constData() already.

I actually have a branch already to at least get rid of = QT_TO_UTF8 patters (which all implicitly cast to std::string cause otherwise they'd be in big trouble as you can't keep that pointer). It's already tested from my side, I just need to open the PR. Getting rid of QT_TO_UTF8 completely is on my dream list as well. These changes-everywhere PRs are just quite a bit of work to test because I like to check every codepath separately.

Use std::string_view instead of raw char pointers in C++ (prefer C++ types over C types).

Will change that.

@PatTheMav
Copy link
Member

Looks good overall, I'd prefer if we could replace use of QT_TO_UTF8 with the explicit .toUtf8().constData() call, particularly as some changed code uses that or constData() already.

I actually have a branch already to at least get rid of = QT_TO_UTF8 patters (which all implicitly cast to std::string cause otherwise they'd be in big trouble as you can't keep that pointer). It's already tested from my side, I just need to open the PR. Getting rid of QT_TO_UTF8 completely is on my dream list as well. These changes-everywhere PRs are just quite a bit of work to test because I like to check every codepath separately.

Great job, let's do that in a separate PR then. 💪🏻

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Code Cleanup Non-breaking change which makes code smaller or more readable
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants