Initialise DBus notifications in another thread #152
Conversation
luke-jr
force-pushed
the
gui_notify_setup_bg
branch
from
ce07314 to
0530dd0
Compare
| interface->moveToThread(m_notificator.thread()); | ||
| m_notificator.interface = interface; | ||
| m_notificator.mode = Notificator::Freedesktop; | ||
| } |
There was a problem hiding this comment.
Emit finished here and clean up instead of keeping the thread for the Notificator lifecycle? DBusInitThread name implies it's disposable as soon as init is done.
There was a problem hiding this comment.
Isn't that supposed to be an automatic part of Qt? The docs say the user can't emit it...
There was a problem hiding this comment.
You're right. I think it used to be public signal but seems that is no longer the case.
There was a problem hiding this comment.
I'd say move m_dbus_init_thread->wait(); call into ~DBusInitThread(), so it no longer can be possible to misuse (forget). Also, connecting finished() signal could perform cleanup:
m_dbus_init_thread = new DBusInitThread(*this);
connect(m_dbus_init_thread, &DBusInitThread::finished, this, [this]() {
m_dbus_init_thread->deleteLater();
m_dbus_init_thread = nullptr;
});
m_dbus_init_thread->start();
Qt docs says:
|
QDBusInterface's constructor can hang if org.freedesktop.Notifications is missing, so avoid delaying startup waiting for a timeout
luke-jr
force-pushed
the
gui_notify_setup_bg
branch
from
0530dd0 to
a9f3096
Compare
| interface->moveToThread(m_notificator.thread()); | ||
| m_notificator.interface = interface; | ||
| m_notificator.mode = Notificator::Freedesktop; | ||
| } |
There was a problem hiding this comment.
I'd say move m_dbus_init_thread->wait(); call into ~DBusInitThread(), so it no longer can be possible to misuse (forget). Also, connecting finished() signal could perform cleanup:
m_dbus_init_thread = new DBusInitThread(*this);
connect(m_dbus_init_thread, &DBusInitThread::finished, this, [this]() {
m_dbus_init_thread->deleteLater();
m_dbus_init_thread = nullptr;
});
m_dbus_init_thread->start();
| QDBusInterface *interface; | ||
| friend class DBusInitThread; |
There was a problem hiding this comment.
friend could be avoided if DBusInitThread would have interfaceCreated(QDBusInterface* interface) signal.
Signal can be invoked from within run() using:
QTimer::singelShot(0, this, [this, interface]() {
emit interfaceCreated(interface);
});
singleShot will detect that this (context) lives in different thread and lambda will be invoked in that (receiver) context thread (where DBusInitThread was created).
In the result, std::atomic<Mode> mode; "atomicity" will not be needed, because
this->interface = interface;
mode = Notificator::Freedesktop;
..would be performed on thread Notificator lives, within a slot (or just lambda) that connected to the DBusInitThread::interfaceCreated(QDBusInterface*) signal, and so without any race condition, as we are setting two variables, and only one is atomic.
DBusInitThread constructor will have to get QThread* though for moveToThread().. Or jus use DBusInitThread::thread() to get that "destination" thread? But that's might break if interface-receiving thread is not the same that created DBusInitThread object...
Also, adding interface->setParent(this) after this->interface = interface; would remove need for manual delete, unless there's deletion ordering is involved...
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
hebasto
changed the title
|
A related discussion on IRC:
|
|
🐙 This pull request conflicts with the target branch and needs rebase. |
|
There hasn't been much activity lately and the patch still needs rebase. What is the status here?
|
|
Closing this due to lack of activity. Feel free to reopen. |
QDBusInterface's constructor can hang if org.freedesktop.Notifications is missing, so avoid delaying startup waiting for a timeout