Avoid side effects during menu initialization
Needs ReviewPublic

Authored by kmaterka on Fri, Nov 8, 8:57 PM.

Details

Reviewers
broulik
Group Reviewers
Plasma
Frameworks
Summary

Setting some attributes, like visible, enabled, etc has side effects. Do not set them if these are not changed.

Test Plan

Without this change menus (mostly submenus) randomly shows up when the SNI is updated, e.g. every time VLC changes a track I get its "speed (slower, normal, faster)" menu open.

Diff Detail

Repository
R135 Integration for Qt applications in Plasma
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 18714
Build 18732: arc lint + arc unit
kmaterka created this revision.Fri, Nov 8, 8:57 PM
Restricted Application added a project: Plasma. · View Herald TranscriptFri, Nov 8, 8:57 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
kmaterka requested review of this revision.Fri, Nov 8, 8:57 PM

Too soon, it is still not finished.

kmaterka updated this revision to Diff 69489.Fri, Nov 8, 9:18 PM

Final version, ready for review.

The problem was with:

m_menu->setVisible(m_visible);

setters/getters should not have side effects or any logic, but this method has. Detached menu stayed as top level window and rendered itself.

VLC has a bug, each systray menu update creates new QMenu, but old one is not deleted:
https://github.com/videolan/vlc/blob/master/modules/gui/qt/menus.cpp#L655

QMenu::clear() deletes all attached actions (if not used somewhere else) but not menus! It is weird, but someone explained it here:
https://bugreports.qt.io/browse/QTBUG-11070

kmaterka edited the summary of this revision. (Show Details)Fri, Nov 8, 9:27 PM

tested successfully locally. I don't see menus opening on the top left corner when running vlc or hp-systray.

kmaterka updated this revision to Diff 69548.Sun, Nov 10, 8:10 PM

Use QVariant for tri-state boolean. This way atributes are set only when they were really changed in the past.

kmaterka added inline comments.Sun, Nov 10, 8:12 PM
src/platformtheme/kdeplatformsystemtrayicon.cpp
190

This line was causing issues, even if menu is (or should be) visible, setting true has side effects.
I checked the code of QMenu, QPlatformMenu::setVisible is never used anyway...

broulik added inline comments.Tue, Nov 12, 8:10 AM
src/platformtheme/kdeplatformsystemtrayicon.cpp
27

Already included in the header file

32–34

Is this explicit initialization neccessary?

187

Prefer the specific methods, if exist, toBool()

src/platformtheme/kdeplatformsystemtrayicon.h
60–62

(This looks like a job for std::optional?)

kmaterka marked an inline comment as done.Tue, Nov 12, 1:56 PM

I will send fixes in 5 minutes

src/platformtheme/kdeplatformsystemtrayicon.cpp
32–34

Not mandatory, works without this. I just wanted to be... explicit. Documentation does not say what is the result of "isNull" when QVariant is not valid.
Remove?

187

OK

src/platformtheme/kdeplatformsystemtrayicon.h
60–62

Exactly! But I checked the code of KDE and it is never used. There are even some custom implementations (optional_view, 3 duplicates). QVariant was the closest thing available.

BTW, is KDE officially using C++17?

kmaterka updated this revision to Diff 69634.Tue, Nov 12, 1:58 PM
kmaterka marked an inline comment as done.

QVariant related changes suggested in comments

kmaterka marked an inline comment as done.Tue, Nov 12, 2:11 PM

Off-topic idea: This QPA integration uses KStatusNotifierItem, which then translates it to DBus. Wouldn't it be better to talk to DBus directly? From the other side, this may be a duplication of work... Was the idea discussed?

Off-topic idea: This QPA integration uses KStatusNotifierItem, which then translates it to DBus. Wouldn't it be better to talk to DBus directly? From the other side, this may be a duplication of work... Was the idea discussed?

We did discuss it.

IMHO the best solution would be to reuse the DBus tray implementation that is part of Qt (https://code.woboq.org/qt5/qtbase/src/platformsupport/themes/genericunix/dbustray/qdbustrayicon_p.h.html). However, it is part of the genericunix theme and thus not "public". @davidedmundson had an idea on how we can "inherit" from the gericunix theme and reuse the implementation

I had this. I abandoned because we ended up forking some special wayland stuff in our DBus menu, so would always want our implementation.

?https://codereview.qt-project.org/c/qt/qtbase/+/168458

I had this. I abandoned because we ended up forking some special wayland stuff in our DBus menu, so would always want our implementation.

?https://codereview.qt-project.org/c/qt/qtbase/+/168458

Having an option to fallback to default/predefined theme would be great, but I guess KDE needs to have it's own SystemTray implementation anyway (?). Some cleanup is required, because currently:

  • in Plasma-integration (QPA theme):
    • there is a forked QDBusMenuBar from Qt
    • for tray icon it uses KStatusNotifierItem which has it's drawbacks (additional layer)
  • in KNotifications, KStatusNotifierItem uses libdbusmenu-qt (not forked). It is optional dependency, is there any reason for that in 2019?
  • in plasma-workspace StatusNotifierItemSource uses DBusMenuImporter forked from libdbusmenu-qt
  • Qt has it's own implementation of DBus menu exporter:
    • in genericunix qpa theme, unfortuantelly QDBusPlatformMenu is private
    • it uses undocumented "NewMenu" signal
    • and is buggy: QTBUG-79287, KDE 383202
  • libdbusmenu-qt was not actively updated/maintained for years, I see that David made few commits in 2015. Shouldn't DBusMenuImporter be upstreamed?
  • Freedesktop spec is still in draft phase

Anyway, this is not part of this code review, sorry for the off topic.

My long-term goal is to get rid of the application side KStatusNotifierItem and amend the QSystemTrayIcon API

My long-term goal is to get rid of the application side KStatusNotifierItem and amend the QSystemTrayIcon API

OK, I will think about this. This change is only to fix one regression.
@broulik Is it OK now? Or should I use std::optional (and require C++17)?