Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Initialise DBus notifications in another thread #152

Closed
wants to merge 1 commit into from
Closed

Initialise DBus notifications in another thread #152

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 17 additions & 8 deletions src/qt/notificator.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,18 @@
#ifdef USE_DBUS
// https://wiki.ubuntu.com/NotificationDevelopmentGuidelines recommends at least 128
const int FREEDESKTOP_NOTIFICATION_ICON_SIZE = 128;

void DBusInitThread::run() {
auto interface = new QDBusInterface("org.freedesktop.Notifications", "/org/freedesktop/Notifications", "org.freedesktop.Notifications");
if (!interface->isValid()) {
delete interface;
return;
}
interface->moveToThread(m_notificator.thread());
m_notificator.interface = interface;
m_notificator.mode = Notificator::Freedesktop;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

Isn't that supposed to be an automatic part of Qt? The docs say the user can't emit it...

Copy link
Contributor

Choose a reason for hiding this comment

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

You're right. I think it used to be public signal but seems that is no longer the case.

Copy link

Choose a reason for hiding this comment

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

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();


#endif

Notificator::Notificator(const QString &programName, QSystemTrayIcon *trayicon, QWidget *parent) :
Expand All @@ -48,12 +60,8 @@ Notificator::Notificator(const QString &programName, QSystemTrayIcon *trayicon,
mode = QSystemTray;
}
#ifdef USE_DBUS
interface = new QDBusInterface("org.freedesktop.Notifications",
"/org/freedesktop/Notifications", "org.freedesktop.Notifications");
if(interface->isValid())
{
mode = Freedesktop;
}
m_dbus_init_thread = new DBusInitThread(*this);
m_dbus_init_thread->start();
#endif
#ifdef Q_OS_MAC
// check if users OS has support for NSUserNotification
Expand Down Expand Up @@ -82,6 +90,8 @@ Notificator::Notificator(const QString &programName, QSystemTrayIcon *trayicon,
Notificator::~Notificator()
{
#ifdef USE_DBUS
m_dbus_init_thread->wait();
delete m_dbus_init_thread;
delete interface;
#endif
}
Expand Down Expand Up @@ -296,8 +306,7 @@ void Notificator::notifyMacUserNotificationCenter(Class cls, const QString &titl

void Notificator::notify(Class cls, const QString &title, const QString &text, const QIcon &icon, int millisTimeout)
{
switch(mode)
{
switch (Mode(mode)) {
#ifdef USE_DBUS
case Freedesktop:
notifyDBus(cls, title, text, icon, millisTimeout);
Expand Down
26 changes: 25 additions & 1 deletion src/qt/notificator.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,9 @@

#include <QIcon>
#include <QObject>
#include <QThread>

#include <atomic>

QT_BEGIN_NAMESPACE
class QSystemTrayIcon;
Expand All @@ -20,6 +23,23 @@ class QDBusInterface;
#endif
QT_END_NAMESPACE

class Notificator;

#ifdef USE_DBUS
class DBusInitThread : public QThread
{
Q_OBJECT

Notificator& m_notificator;

public:
DBusInitThread(Notificator& notificator) : m_notificator(notificator) {};

protected:
void run() override;
};
#endif

/** Cross-platform desktop notification client. */
class Notificator: public QObject
{
Expand Down Expand Up @@ -63,11 +83,15 @@ public slots:
UserNotificationCenter /**< Use the 10.8+ User Notification Center (Mac only) */
};
QString programName;
Mode mode;
std::atomic<Mode> mode;
QSystemTrayIcon *trayIcon;
#ifdef USE_DBUS
QThread *m_dbus_init_thread{nullptr};
protected:
QDBusInterface *interface;
friend class DBusInitThread;
Copy link

Choose a reason for hiding this comment

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

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


private:
void notifyDBus(Class cls, const QString &title, const QString &text, const QIcon &icon, int millisTimeout);
#endif
void notifySystray(Class cls, const QString &title, const QString &text, const QIcon &icon, int millisTimeout);
Expand Down