Setting some attributes, like visible, enabled, etc has side effects. Do not set them if these are not changed.
- Group Reviewers
- R135:3c786666453b: Avoid side effects during menu initialization
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.
The problem was with:
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:
QMenu::clear() deletes all attached actions (if not used somewhere else) but not menus! It is weird, but someone explained it here:
This line was causing issues, even if menu is (or should be) visible, setting true has side effects.
I will send fixes in 5 minutes
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.
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?
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.
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:
- 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.