obsproject
frontend: Enable Qt strict mode
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 https://github.com/obsproject/obs-studio/pull/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:
- [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.
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()) ?
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.
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.
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.
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.
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.
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_UTF8patters (which all implicitly cast tostd::stringcause 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 ofQT_TO_UTF8completely 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. 💪🏻
@gxalpha Did you test the YouTube API stuff? Any breakage surrounding service docks and service connections are very visible to users, so would be great if we can check that off as explicitly tested.
Yes, I've tested all requests except for UpdateAccessToken and ResetBroadcast as I don't know how to trigger those.