D7260: System tray icon's context menu isn't updated properly in plasma/x11

[Aleksei Nikiforov]

i.Dark_Templar created this revision.
Restricted Application added a project: Plasma.
Restricted Application added a subscriber: plasma-devel.

REVISION SUMMARY
This is a fix for a bug I recently reported at: https://bugs.kde.org/show_bug.cgi?id=383202

Basically, if you make system tray menu with multiple levels of nesting, browse this menu (making sure plasma fetches all data for menu via dbus) and then replace menu by different menu with multiple levels of nesting, plasma uses old data for all menu items, usually except of top level menu.

Proposed patch makes sure that no menus are cached, and if top-level menu is updated, all nested menus are updated as well (on aboutToShow event).

TEST PLAN
I've attached test application to the mentioned bug, it started working correctly with this patch on plasma-workspace-5.9.5.

REPOSITORY
R120 Plasma Workspace

REVISION DETAIL
https://phabricator.kde.org/D7260

AFFECTED FILES
libdbusmenuqt/dbusmenuimporter.cpp

To: i.Dark_Templar
Cc: plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart, lukas

[Aleksei Nikiforov]

i.Dark_Templar added reviewers: Plasma, davidedmundson.

REPOSITORY
R120 Plasma Workspace

REVISION DETAIL
https://phabricator.kde.org/D7260

To: i.Dark_Templar, #plasma, davidedmundson
Cc: plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart, lukas

[David Edmundson]

davidedmundson added a comment.


We cache for a reason, simply removing the cache seems a hack for whatever the actual bug is.

Deleting menus and recreating them causes big issues for the appmenu bar; as their we're displaying the menu widget.
See comment on 1754c135c134e04d584d79e011ac38c4eb30d300 I'm pretty sure this will break that.

REPOSITORY
R120 Plasma Workspace

REVISION DETAIL
https://phabricator.kde.org/D7260

To: i.Dark_Templar, #plasma, davidedmundson
Cc: plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart, lukas

[Aleksei Nikiforov]

i.Dark_Templar added a comment.


Current implementation is buggy. Cached menu items may be invalid: after 'LayoutUpdate' event updated menu all of it submenus are no longer valid (and thus cache contains invalid data) and should be updated. Menu itself is updated, but it's submenus are not. It's just usually happens that new submenu data is same as old submenu data, but in case it's not, you'll never get new submenu data and never see updated submenu. See example in menubugtest.tar.bz2 attached to linked bug (and description there).

If you compare implementation of upstream libdbusmenu-qt and current implementation bundled into kde, you'll notice that upstream on 'LayoutUpdate' event updates all submenus synchronously and thus it obtains new correct submenus. Current code never obtains new submenus and always shows old ones.
To work properly, you have to receive new data, either on 'LayoutUpdate' event (as upstream does), on 'AboutToShow' event (as my patch does) or somehow else. Current implementation never does. Just showing old submenus is incorrect. Current change doesn't seem like a hack if you compare current state of kde code to upstream libdbusmenu-qt.

REPOSITORY
R120 Plasma Workspace

REVISION DETAIL
https://phabricator.kde.org/D7260

To: i.Dark_Templar, #plasma, davidedmundson
Cc: plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart, lukas

[David Edmundson]

davidedmundson added a comment.


I get that there's a bug - I'm not remotely suggesting we drop the issue.
Your supplied test is really useful, it's valid and I do want to see this problem get fixed.

But, we can't knowingly replace one bug with a different one, we don't improve anything. The old implementation is buggy in other ways.

Try adding this patch, add the appmenubar widget, run the test app in the libdbusmenuqt folder and click "Menu C".
With this patch you can't open that menu at all.

Post by Aleksei Nikiforov
Current code never obtains new submenus and always shows old ones.

That's not entirely true. We do show the cached version, but also immediately call aboutToShow which will update the subtree. In theory at least :)

----

I /think/ all we need to do is make sure the else branch of the old code handle reparenting?
If you can have another look at that, that'd be great, otherwise I'll try later this week.

----

Note also that the client code will go in one of two differnet paths depending on whether it's going through the plasma platform theme or not.

REPOSITORY
R120 Plasma Workspace

REVISION DETAIL
https://phabricator.kde.org/D7260

To: i.Dark_Templar, #plasma, davidedmundson
Cc: plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart, lukas

[Aleksei Nikiforov]

i.Dark_Templar added a comment.

Post by David Edmundson
That's not entirely true. We do show the cached version, but also immediately call aboutToShow which will update the subtree. In theory at least :)

There's condition in function 'void DBusMenuImporter::slotAboutToShowDBusCallFinished(QDBusPendingCallWatcher *watcher)' around line 494:

if (needRefresh || menu->actions().isEmpty()) {

needRefresh is a value from dbus. Since another application already issued 'LayoutUpdate' event, needRefresh is false (it should be already refreshed, while it's actually not). And menu->actions() is not empty since it uses cached submenu (this is no longer true with this patch, this patch ensures that menu->actions() is empty).
I think another flag may be stored, for example ActionForId map may be expanded for that use case. But I don't see much difference in the end result.

Post by David Edmundson
Try adding this patch, add the appmenubar widget, run the test app in the libdbusmenuqt folder and click "Menu C".
With this patch you can't open that menu at all.

I couldn't reproduce the issue with this patch (I'm already using it). I'm using 'oxygen' theme in plasma/X11. I'm setting in "systemsettings -> Application style -> Widget style -> Fine tuning" value of "Menubar style" to "Application Menu widget", then I'm adding appmenu widget to my desktop and running appmenutest from plasma-workspace/libdbusmenu-qt/test and "Menu C" is working fine, it's showing up and displaying correct time (updating displayed time every time I'm opening "Menu C" again). Did I miss something?

REPOSITORY
R120 Plasma Workspace

REVISION DETAIL
https://phabricator.kde.org/D7260

To: i.Dark_Templar, #plasma, davidedmundson
Cc: plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart, lukas

[David Edmundson]

davidedmundson added a comment.

Post by Aleksei Nikiforov
Did I miss something?

One of us did.

I did have to redo your patch manually as the diff here doesn't apply to either 5.10 or master. It's possible I did it wrong, maybe you can update it?

The only difference is I did right click -> add panel -> application menu bar
It's important to test it in the mode where Menu A and Menu C are both visible in the bar.

REPOSITORY
R120 Plasma Workspace

REVISION DETAIL
https://phabricator.kde.org/D7260

To: i.Dark_Templar, #plasma, davidedmundson
Cc: plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart, lukas

[Aleksei Nikiforov]

i.Dark_Templar updated this revision to Diff 18717.
i.Dark_Templar added a comment.


Updated patch for plasma-workspace 5.10, I'm using it with 5.10.4, but it should apply to master too. I've tried adding appmenu panel to desktop, still works fine for me. A bit of clarification. I'm not sure it's important, but I'm currently using 'Oxygen' workspace theme and 'Air' desktop theme.

REPOSITORY
R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D7260?vs=18023&id=18717

REVISION DETAIL
https://phabricator.kde.org/D7260

AFFECTED FILES
libdbusmenuqt/dbusmenuimporter.cpp

To: i.Dark_Templar, #plasma, davidedmundson
Cc: plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart, lukas

[Christoph Feck]

cfeck added a comment.


David, can you test again?

REPOSITORY
R120 Plasma Workspace

REVISION DETAIL
https://phabricator.kde.org/D7260

To: i.Dark_Templar, #plasma, davidedmundson
Cc: cfeck, plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

[Kai Uwe Broulik]

broulik added a comment.


With this patch the "Bookmarks" menu in KWrite does no longer open (as if it were empty) with global menu enabled.
In application
F3903010: Screenshot_20170908_112004.png <https://phabricator.kde.org/F3903010>
In global menu (affects both the plasmoid and the title bar button) *without* this patch working fine:
F3903013: Screenshot_20170908_112042.png <https://phabricator.kde.org/F3903013>
In global menu *with* this patch is broken:
F3903015: Screenshot_20170908_112126.png <https://phabricator.kde.org/F3903015>
Sometimes the menu briefly appears with the first "Lesezeichen setzen" entry in the middle of the screen and then closes again.

REPOSITORY
R120 Plasma Workspace

REVISION DETAIL
https://phabricator.kde.org/D7260

To: i.Dark_Templar, #plasma, davidedmundson
Cc: broulik, cfeck, plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

[Aleksei Nikiforov]

i.Dark_Templar added a comment.

Post by Kai Uwe Broulik
With this patch the "Bookmarks" menu in KWrite does no longer open (as if it were empty) with global menu enabled.

Hmm, for some reason it still works for me. But when I use global menu, I see a small flickering: menu appears, hides and reappears. I assume it updates for some reason and thus causes flickering.
I ran dbus-monitor to check this assumption and noticed that kwrite sends a lot of 'LayoutUpdate' events even though I didn't update bookmarks.

REPOSITORY
R120 Plasma Workspace

REVISION DETAIL
https://phabricator.kde.org/D7260

To: i.Dark_Templar, #plasma, davidedmundson
Cc: broulik, cfeck, plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

[Aleksei Nikiforov]

i.Dark_Templar added a comment.


Any news on this bug and this change yet?

REPOSITORY
R120 Plasma Workspace

REVISION DETAIL
https://phabricator.kde.org/D7260

To: i.Dark_Templar, #plasma, davidedmundson
Cc: broulik, cfeck, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart