[KStatusNotifierItem] Allow left click when menu is null
ClosedPublic

Authored by kmaterka on Oct 15 2019, 2:34 PM.

Details

Summary

If associatedWidget and menu are the same then instead of "activate"
action context menu is used. When both are null coparition is always
true but context menu can't be shown, since it is null.
It is partial solution for a problem described in https://bugs.kde.org/show_bug.cgi?id=365105.

CCBUG: 365105

Test Plan

As described in the bug:

  • create QSystemTrayIcon
  • assign QMenu
  • show, hide and show again

Both right click and left click do nothing.

After change left click works

Diff Detail

Repository
R289 KNotifications
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 17715
Build 17733: arc lint + arc unit
kmaterka created this revision.Oct 15 2019, 2:34 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptOct 15 2019, 2:34 PM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
kmaterka requested review of this revision.Oct 15 2019, 2:34 PM

This fixes issue from first comment made by @davidedmundson . Second is still not fixed. Let me copy it here:

We have one menu for the whole tray instance
We create and delete KSNI object in init() and cleanup() which are called on show() hide()

The SNI takes ownership of the menu

so for the the second init, we set the SNI to a now deleted menu.

The assumption in QPA is that context menu is persistent, tray icon is not.

There are few way to fix that, but I don't like any of these:

  • create new QMenu when old one is destroyed, try to recover menu content
    • menu actions are stored safely in m_items, but title, icon and other flags are not
    • we can have two menus, one for KSNI, second to store values for recovery but this is silly
  • create a proxy/delegate to internal QMenu, so that the internal QMenu won't be deleted
    • overkill? a lot of methods/signals/slots to delegate
  • do not delete menu in KSNI
    • not possible, other apps are relaying on this behavior
    • add new parameter to skip menu deletion? Isn't it another overkill ad unnecessary API change?
  • remove SystemTray support from KdePlatformTheme
    • easiest, but...
    • Qt has implementation based on DBus, but it behaves differently to KSNI: different position of context menu, different implementation of activated, scroll is not supported etc

What do you think?

kmaterka added inline comments.Oct 15 2019, 3:11 PM
src/kstatusnotifieritem.cpp
619

This should fix a situation when both are NULL

src/kstatusnotifieritemdbus_p.cpp
293

This check is duplicated, it should be safe to remove it here.

kmaterka retitled this revision from Activate when both associatedWidget and menu are null to [KStatusNotifierItem] Allow left click when menu is null.Oct 16 2019, 8:43 AM
kmaterka added a reviewer: Frameworks.

I think the best would be to add additional parameter to KSNI. Change from:

void KStatusNotifierItem::setContextMenu(QMenu *menu);

to something like this:

void KStatusNotifierItem::setContextMenu(QMenu *menu, bool takeOwnership = true);

Then, after next release of Frameworks, change implementation in plasma-integration.

ngraham edited the summary of this revision. (Show Details)Oct 16 2019, 3:20 PM

bool takeOwnership = true);

If only Qt/we used modern C++ features to communicate object ownership :)

Good rundown of the options.

remove SystemTray support from KdePlatformTheme

It's not trivial. There's no way to access the relevant Qt classes. I should revisit the patch that allowed that.

Our hopeful longterm plans for KF6 are to kill KStatusNotifierItem add the missing methods to QSystemTray, fix some other parts of their implementation and then drop it here.


do not delete menu in KSNI

Makes the most sense in some form.
It /could/ be done without additional API by abusing QObject parentship: https://phabricator.kde.org/P479

Then this code being a special case would just set the parent back afterwards. Whether that's actually any better than any explicit boolean method is debatable though.

davidedmundson accepted this revision.Oct 16 2019, 4:28 PM
This revision is now accepted and ready to land.Oct 16 2019, 4:28 PM
kmaterka marked 2 inline comments as done.Oct 17 2019, 3:43 PM

bool takeOwnership = true);

If only Qt/we used modern C++ features to communicate object ownership :)

Hehe, yes. Unfortunately KSNI messes with parent of menu object :/

do not delete menu in KSNI

Makes the most sense in some form.
It /could/ be done without additional API by abusing QObject parentship: https://phabricator.kde.org/P479

Then this code being a special case would just set the parent back afterwards. Whether that's actually any better than any explicit boolean method is debatable though.

Believe me, I checked that :) I was thinking about this, but it won't work in this form. There are two reasons. First:

void KStatusNotifierItemPrivate::init(const QString &extraId)
//...
QMenu *m = new QMenu(associatedWidget);

associatedWidget can be null or not. I don't know if this is needed, what is the purpose of this code and what are side effects. Probably this can be safely removed.
Second reason is more serious: QMenu parent must be of QWidget type and KSNI is not a widget.

This revision was automatically updated to reflect the committed changes.