Changeset View
Standalone View
src/kstatusnotifieritem.cpp
| Show First 20 Lines • Show All 64 Lines • ▼ Show 20 Line(s) | 64 | { | |||
|---|---|---|---|---|---|
| 65 | d->init(id); | 65 | d->init(id); | ||
| 66 | } | 66 | } | ||
| 67 | 67 | | |||
| 68 | KStatusNotifierItem::~KStatusNotifierItem() | 68 | KStatusNotifierItem::~KStatusNotifierItem() | ||
| 69 | { | 69 | { | ||
| 70 | delete d->statusNotifierWatcher; | 70 | delete d->statusNotifierWatcher; | ||
| 71 | delete d->notificationsClient; | 71 | delete d->notificationsClient; | ||
| 72 | delete d->systemTrayIcon; | 72 | delete d->systemTrayIcon; | ||
| 73 | if (!qApp->closingDown()) { | 73 | if (!qApp->closingDown() && d->menuOwnership) { | ||
| 74 | delete d->menu; | 74 | delete d->menu; | ||
| 75 | } | 75 | } | ||
| 76 | if (d->associatedWidget) { | 76 | if (d->associatedWidget) { | ||
| 77 | KWindowSystem::self()->disconnect(d->associatedWidget); | 77 | KWindowSystem::self()->disconnect(d->associatedWidget); | ||
| 78 | } | 78 | } | ||
| 79 | delete d; | 79 | delete d; | ||
| 80 | } | 80 | } | ||
| 81 | 81 | | |||
| ▲ Show 20 Lines • Show All 329 Lines • ▼ Show 20 Line(s) | 410 | #endif | |||
| 411 | emit d->statusNotifierItemDBus->NewToolTip(); | 411 | emit d->statusNotifierItemDBus->NewToolTip(); | ||
| 412 | } | 412 | } | ||
| 413 | 413 | | |||
| 414 | QString KStatusNotifierItem::toolTipSubTitle() const | 414 | QString KStatusNotifierItem::toolTipSubTitle() const | ||
| 415 | { | 415 | { | ||
| 416 | return d->toolTipSubTitle; | 416 | return d->toolTipSubTitle; | ||
| 417 | } | 417 | } | ||
| 418 | 418 | | |||
| 419 | void KStatusNotifierItem::setContextMenu(QMenu *menu) | 419 | void KStatusNotifierItem::setContextMenu(QMenu *menu, bool takeOwnership) | ||
| 420 | { | 420 | { | ||
| 421 | if (d->menu && d->menu != menu) { | 421 | if (d->menu && d->menu != menu) { | ||
| 422 | d->menu->removeEventFilter(this); | 422 | d->menu->removeEventFilter(this); | ||
| 423 | delete d->menu; | 423 | delete d->menu; | ||
| 424 | } | 424 | } | ||
| 425 | 425 | | |||
| 426 | if (!menu) { | 426 | if (!menu) { | ||
| 427 | d->menu = nullptr; | 427 | d->menu = nullptr; | ||
| Show All 16 Lines | 443 | #if HAVE_DBUSMENUQT | |||
| 444 | new DBusMenuExporter(d->menuObjectPath, menu, d->statusNotifierItemDBus->dbusConnection()); | 444 | new DBusMenuExporter(d->menuObjectPath, menu, d->statusNotifierItemDBus->dbusConnection()); | ||
| 445 | #endif | 445 | #endif | ||
| 446 | } | 446 | } | ||
| 447 | 447 | | |||
| 448 | connect(menu, SIGNAL(aboutToShow()), this, SLOT(contextMenuAboutToShow())); | 448 | connect(menu, SIGNAL(aboutToShow()), this, SLOT(contextMenuAboutToShow())); | ||
| 449 | } | 449 | } | ||
| 450 | 450 | | |||
| 451 | d->menu = menu; | 451 | d->menu = menu; | ||
| 452 | d->menuOwnership = takeOwnership; | ||||
| 452 | Qt::WindowFlags oldFlags = d->menu->windowFlags(); | 453 | Qt::WindowFlags oldFlags = d->menu->windowFlags(); | ||
| 453 | d->menu->setParent(nullptr); | 454 | d->menu->setParent(nullptr); | ||
anthonyfieroni: Do not take ownership of the menu and delete it when it does not have a parent. takeOwnership… | |||||
In theory that should be the correct approach, the "Qt way". But we have existing contract (also discussed in D24667), documentation is clear, menu ownership is always transferred and menu removed, regardless of the parent (please check my comment in line 790, parent might be null or might not be null). kmaterka: In theory that should be the correct approach, the "Qt way". But we have existing contract… | |||||
| 454 | d->menu->setWindowFlags(oldFlags); | 455 | d->menu->setWindowFlags(oldFlags); | ||
| 455 | } | 456 | } | ||
| 456 | 457 | | |||
| 457 | QMenu *KStatusNotifierItem::contextMenu() const | 458 | QMenu *KStatusNotifierItem::contextMenu() const | ||
| 458 | { | 459 | { | ||
| 459 | return d->menu; | 460 | return d->menu; | ||
| 460 | } | 461 | } | ||
| 461 | 462 | | |||
| ▲ Show 20 Lines • Show All 297 Lines • ▼ Show 20 Line(s) | 756 | : q(item), | |||
| 759 | menu(nullptr), | 760 | menu(nullptr), | ||
| 760 | associatedWidget(nullptr), | 761 | associatedWidget(nullptr), | ||
| 761 | titleAction(nullptr), | 762 | titleAction(nullptr), | ||
| 762 | statusNotifierWatcher(nullptr), | 763 | statusNotifierWatcher(nullptr), | ||
| 763 | notificationsClient(nullptr), | 764 | notificationsClient(nullptr), | ||
| 764 | systemTrayIcon(nullptr), | 765 | systemTrayIcon(nullptr), | ||
| 765 | hasQuit(false), | 766 | hasQuit(false), | ||
| 766 | onAllDesktops(false), | 767 | onAllDesktops(false), | ||
| 767 | standardActionsEnabled(true) | 768 | standardActionsEnabled(true), | ||
| 769 | menuOwnership(true) | ||||
| 768 | { | 770 | { | ||
| 769 | } | 771 | } | ||
| 770 | 772 | | |||
| 771 | void KStatusNotifierItemPrivate::init(const QString &extraId) | 773 | void KStatusNotifierItemPrivate::init(const QString &extraId) | ||
| 772 | { | 774 | { | ||
| 773 | qDBusRegisterMetaType<KDbusImageStruct>(); | 775 | qDBusRegisterMetaType<KDbusImageStruct>(); | ||
| 774 | qDBusRegisterMetaType<KDbusImageVector>(); | 776 | qDBusRegisterMetaType<KDbusImageVector>(); | ||
| 775 | qDBusRegisterMetaType<KDbusToolTipStruct>(); | 777 | qDBusRegisterMetaType<KDbusToolTipStruct>(); | ||
| 776 | 778 | | |||
| 777 | statusNotifierItemDBus = new KStatusNotifierItemDBus(q); | 779 | statusNotifierItemDBus = new KStatusNotifierItemDBus(q); | ||
| 778 | q->setAssociatedWidget(qobject_cast<QWidget *>(q->parent())); | 780 | q->setAssociatedWidget(qobject_cast<QWidget *>(q->parent())); | ||
| 779 | 781 | | |||
| 780 | QDBusServiceWatcher *watcher = new QDBusServiceWatcher(QString::fromLatin1(s_statusNotifierWatcherServiceName), | 782 | QDBusServiceWatcher *watcher = new QDBusServiceWatcher(QString::fromLatin1(s_statusNotifierWatcherServiceName), | ||
| 781 | QDBusConnection::sessionBus(), | 783 | QDBusConnection::sessionBus(), | ||
| 782 | QDBusServiceWatcher::WatchForOwnerChange, | 784 | QDBusServiceWatcher::WatchForOwnerChange, | ||
| 783 | q); | 785 | q); | ||
| 784 | QObject::connect(watcher, SIGNAL(serviceOwnerChanged(QString,QString,QString)), | 786 | QObject::connect(watcher, SIGNAL(serviceOwnerChanged(QString,QString,QString)), | ||
| 785 | q, SLOT(serviceChange(QString,QString,QString))); | 787 | q, SLOT(serviceChange(QString,QString,QString))); | ||
| 786 | 788 | | |||
| 787 | //create a default menu, just like in KSystemtrayIcon | 789 | //create a default menu, just like in KSystemtrayIcon | ||
| 788 | QMenu *m = new QMenu(associatedWidget); | 790 | QMenu *m = new QMenu(associatedWidget); | ||
"associatedWidget" is a KSNI parent (line 780). It might be or might not be set. If parent is not set, then "associatedWidget" is null and QMenu does not have parent. This is fine, we will delete it. But if there is parent then menu won't be deleted and we will have memory leak. Eventually this menu will be deleted, when parent is destroyed, but current contract is different. new QMenu() Then it will be removed, because there is no parent. It should not have any important side effects. So far so good. What we want to achieve is have an ability to retake ownership after it is passed to setContextMenu. With your approach, it could be done this way:
The final goal is to fix KDEPlatformSystemTrayIcon and there is no QWidget to use as a parent :( Exactly: m_sni->setContextMenu(ourMenu->menu()); ourMenu can live longer than KStatusNotifierItem *m_sni and can be reused for another KStatusNotifierItem instance. The situation is described in BUG 365105. In other word, in QPA, menu can and should live independently to system tray icon, which is not the case in KStatusNotifierItem. I really like your idea! Maybe I'm missing something obvious and is possible to set parent in KDEPlatformSystemTrayIcon... kmaterka: "associatedWidget" is a KSNI parent (line 780). It might be or might not be set. If parent is… | |||||
That's easy checkable if (!qApp->closingDown() && (!d->menu->parent() || d->menu->parent() == d->associatedWidget) {
delete d->menu;
}We should not own the menu, that's not tight approach at least. anthonyfieroni: That's easy checkable
```
if (!qApp->closingDown() && (!d->menu->parent() || d->menu->parent()… | |||||
This check is not reliable, assosiatedWidget can change. Anyway, this doesn't matter here. kmaterka: This check is not reliable, assosiatedWidget can change. Anyway, this doesn't matter here.
Did… | |||||
Probably KSNI should not own the menu anthonyfieroni: `Probably KSNI should not own the menu`
Yes, widget that creates the menu should, like in bug… | |||||
| 789 | 791 | | |||
| 790 | title = QGuiApplication::applicationDisplayName(); | 792 | title = QGuiApplication::applicationDisplayName(); | ||
| 791 | if (title.isEmpty()) { | 793 | if (title.isEmpty()) { | ||
| 792 | title = QCoreApplication::applicationName(); | 794 | title = QCoreApplication::applicationName(); | ||
| 793 | } | 795 | } | ||
| 794 | #ifdef Q_OS_MACOS | 796 | #ifdef Q_OS_MACOS | ||
| 795 | // OS X doesn't have texted separators so we emulate QAction::addSection(): | 797 | // OS X doesn't have texted separators so we emulate QAction::addSection(): | ||
| 796 | // we first add an action with the desired text (title) and icon | 798 | // we first add an action with the desired text (title) and icon | ||
| ▲ Show 20 Lines • Show All 362 Lines • Show Last 20 Lines | |||||
Do not take ownership of the menu and delete it when it does not have a parent. takeOwnership is wrong approach, you can remove it.