Setting some attributes, like visible, enabled, etc has side effects. Do not set them if these are not changed.
Details
- Reviewers
broulik - Group Reviewers
Plasma Frameworks - Commits
- 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.
Diff Detail
- Repository
- R135 Integration for Qt applications in Plasma
- Lint
Automatic diff as part of commit; lint not applicable. - Unit
Automatic diff as part of commit; unit tests not applicable.
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
tested successfully locally. I don't see menus opening on the top left corner when running vlc or hp-systray.
Use QVariant for tri-state boolean. This way atributes are set only when they were really changed in the past.
src/platformtheme/kdeplatformsystemtrayicon.cpp | ||
---|---|---|
190 | 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
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. | |
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? |
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:
- 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
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)?