Remove unused self parameters in Q*Application static methods

[jnbooth]

Resolves #1205.

Note: This PR contains breaking changes.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (626371a) to head (7799c44).

Additional details and impacted files

@@            Coverage Diff            @@
##              main     #1216   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           73        73           
  Lines        12612     12612           
=========================================
  Hits         12612     12612           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[LeonMatthesKDAB]

Sorry for not thinking of this earlier, but I just realized we may have a safety issue with these methods (which may be why we made them "members" initially).

Some of them seem to be not thread-safe!
E.g. setActiveWindow just accesses the QApplicationPrivate::active_window without any locking.

So they are effectively only safe-to-call from a single thread (presumably the "main thread")
We'd have to make these methods unsafe or introduce some kind of locking ourselves.

The Qt docs also state that e.g. only one QGuiApplication should be active: https://doc.qt.io/qt-6/qguiapplication.html#QGuiApplication

So the way these could be made safe is by keeping them as member functions and enforcing in the constructor that only a single QGuiApplication can actually be constructed (and disabling Send+Sync).
We currently don't do that second part though.

I'm also open to other suggestions, especially as keeping member functions is inconvenient.

[jnbooth]

So they are effectively only safe-to-call from a single thread (presumably the "main thread")

I believe this is true of all GUI operations, not just the ones related to QGuiApplication. Per KDAB's own documentation:

Qt's GUI operations are not thread safe, so non-main threads cannot safely perform any GUI operation. That means no widgets, QtQuick, QPixmap, or anything that touches the window manager. There are some exceptions: GUI functions that only manipulate data but don't touch the window manager can be called on secondary threads, things like QImage and QPainter. Be careful though, as classes like QBitmap or QPixmap actually are not safe. Check the documentation for each API: if you don't see a note at the top of the documentation saying that the function is reentrant, it's not safe to be called except from the main thread.

setActiveWindow touches the window manager, so it would be included in that. Is it doable for cxx-qt to provide guarantees of thread safety where GUI operations are concerned? Send prevents moving between threads, but that doesn't stop someone from constructing an object outside of the main thread, among other things. It might need to be up to users.

EDIT: It could be possible to throw a runtime error if code executes outside the main thread. That seems like a method of last resort, though.

[LeonMatthesKDAB]

So they are effectively only safe-to-call from a single thread (presumably the "main thread")

I believe this is true of all GUI operations, not just the ones related to QGuiApplication. Per KDAB's own documentation:

Qt's GUI operations are not thread safe, so non-main threads cannot safely perform any GUI operation. That means no widgets, QtQuick, QPixmap, or anything that touches the window manager. There are some exceptions: GUI functions that only manipulate data but don't touch the window manager can be called on secondary threads, things like QImage and QPainter. Be careful though, as classes like QBitmap or QPixmap actually are not safe. Check the documentation for each API: if you don't see a note at the top of the documentation saying that the function is reentrant, it's not safe to be called except from the main thread.

setActiveWindow touches the window manager, so it would be included in that. Is it doable for cxx-qt to provide guarantees of thread safety where GUI operations are concerned? Send prevents moving between threads, but that doesn't stop someone from constructing an object outside of the main thread, among other things. It might need to be up to users.

EDIT: It could be possible to throw a runtime error if code executes outside the main thread. That seems like a method of last resort, though.

Hm, with this kind of design by Qt, I think we may not get around doing runtime checks tbh.

In general, we should make sure that the objects that need to be used from the main thread are not Send, which should get us halfway there.
However, as you mentioned we need to enforce that you cannot create such an object outside the main thread...
I don't see a good way to do this apart from runtime checks and returning an Err if you call such a constructor outside the main thread (which might need to apply to QGuiApplication as well).

There is a method since Qt 6.8 that we can use to check this:
https://doc.qt.io/qt-6/qthread.html#isMainThread

For the static functions in Q*Application, we could then return an Err or just panic.

Unfortunately it uses some private API internally, so it would probably be hard to backport for Qt5 and Qt 6.7-.

More ideas are certainly welcome... @ahayzen-kdab @jnbooth ?

[ahayzen-kdab]

@LeonMatthesKDAB yup, this is all quite fun, another one of the reasons why avoiding having GUI code in the Rust side is useful and just exposing models :-) For the actual Q*Application classes you can only have one instance of them and they all inherit from QCoreApplication and note as the docs of the isMainThread state The main thread is the thread in which QCoreApplication was created. This is usually the thread that called the main() function, but not necessarily so. It is the thread that is processing the GUI events and in which graphical objects (https://doc.qt.io/qt-6/qwindow.html, QWidget can be created. https://doc.qt.io/qt-6/qthread.html#isMainThread

So for finding out if you are the main thread after creating the application could you not do QCoreApplication::instance()->thread() == QThread::currentThread() ? This would at least tell you "am I in the same thread as the q*application" which is then effectively "is this the main thread" for < Qt 6.8 ? There may also be a simpler way but i haven't really looked around :-)

[LeonMatthesKDAB]

Hm, I know that at least on MacOS, any event processing has to actually happen on the main thread, (e.g. the one that executes main).
The docs are a bit unclear on how this relates, as you can create at least a QCoreApplication on other threads.
Can you do the same with e.g. QGuiApplication?

It's possible that just checking the instances thread is good enough though.
However, there's a bit of a catch-22 here: QCoreApplication::instance() itself isn't thread-safe.
So using that to check whether we are on the right thread doesn't work, right?

[jnbooth]

I've added (unused) &self and self: Pin<&mut Self> parameters to the functions that access the application instance. A few functions don't need them because they don't access the instance, and instead Qt uses a mutex to guard them:

• add_library_path: locked by Qt's libraryPathMutex
• font: locked by Qt's applicationFontMutex
• library_path: locked by Qt's libraryPathMutex
• remove_library_path: locked by Qt's libraryPathMutex
• set_font: locked by Qt's applicationFontMutex
• set_library_paths: locked by Qt's libraryPathMutex

The Q*Application structs do not implement Send or Sync, so that should be sufficient to keep everything safe.

I know that at least on MacOS, any event processing has to actually happen on the main thread, (e.g. the one that executes main).

Does it? I don't see that in the documentation.