D25223: Avoid side effects during menu initialization

This revision was automatically updated to reflect the committed changes. Closed by commit R135:3c78453b: Avoid side effects during menu initialization (authored by kmaterka). REPOSITORY R135 Integration for Qt applications in Plasma CHANGES SINCE LAST UPDATE https://phabricator.kde.org/

D25223: Avoid side effects during menu initialization

kmaterka added a comment. In D25223#561638 , @nicolasfella wrote: > 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

D25223: Avoid side effects during menu initialization

nicolasfella added a comment. My long-term goal is to get rid of the application side KStatusNotifierItem and amend the QSystemTrayIcon API REPOSITORY R135 Integration for Qt applications in Plasma REVISION DETAIL https://phabricator.kde.org/D25223 To: kmaterka, #plasma, #frameworks, br

D25223: Avoid side effects during menu initialization

kmaterka added a comment. In D25223#561570 , @davidedmundson wrote: > 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-proj

D25223: Avoid side effects during menu initialization

davidedmundson added a comment. 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 REPOSITORY R135 Integration for Qt applications in Plasma

D25223: Avoid side effects during menu initialization

nicolasfella added subscribers: davidedmundson, nicolasfella. nicolasfella added a comment. In D25223#561561 , @kmaterka wrote: > Off-topic idea: This QPA integration uses KStatusNotifierItem, which then translates it to DBus. Wouldn't it be bett

D25223: Avoid side effects during menu initialization

kmaterka marked an inline comment as done. kmaterka added a comment. 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 discusse

D25223: Avoid side effects during menu initialization

kmaterka updated this revision to Diff 69634. kmaterka marked an inline comment as done. kmaterka added a comment. QVariant related changes suggested in comments REPOSITORY R135 Integration for Qt applications in Plasma CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D25223?vs=69548

D25223: Avoid side effects during menu initialization

kmaterka marked an inline comment as done. kmaterka added a comment. I will send fixes in 5 minutes INLINE COMMENTS > broulik wrote in kdeplatformsystemtrayicon.cpp:33 > Is this explicit initialization neccessary? Not mandatory, works without this. I just wanted to be... explicit. Documenta

D25223: Avoid side effects during menu initialization

broulik added inline comments. INLINE COMMENTS > kdeplatformsystemtrayicon.cpp:27 > #include > +#include > Already included in the header file > kdeplatformsystemtrayicon.cpp:33 > : QPlatformMenu() > -, m_enabled(true) > -, m_visible(true) > -, m_separatorsCollapsible(true

D25223: Avoid side effects during menu initialization

kmaterka added inline comments. INLINE COMMENTS > kdeplatformsystemtrayicon.cpp:191 > +if (!m_visible.isNull()) { > +m_menu->setVisible(m_visible.value()); > +} This line was causing issues, even if menu is (or should be) visible, setting true has side effects. I checked the cod

D25223: Avoid side effects during menu initialization

kmaterka updated this revision to Diff 69548. kmaterka added a comment. Use QVariant for tri-state boolean. This way atributes are set only when they were really changed in the past. REPOSITORY R135 Integration for Qt applications in Plasma CHANGES SINCE LAST UPDATE https://phabricator.k

D25223: Avoid side effects during menu initialization

cgiboudeaux added a comment. tested successfully locally. I don't see menus opening on the top left corner when running vlc or hp-systray. REPOSITORY R135 Integration for Qt applications in Plasma REVISION DETAIL https://phabricator.kde.org/D25223 To: kmaterka, #plasma, #frameworks, bro

D25223: Avoid side effects during menu initialization

kmaterka added a comment. 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 QMen

D25223: Avoid side effects during menu initialization

kmaterka updated this revision to Diff 69489. kmaterka added a comment. Final version, ready for review. REPOSITORY R135 Integration for Qt applications in Plasma CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D25223?vs=69484&id=69489 BRANCH master REVISION DETAIL https://pha

D25223: Avoid side effects during menu initialization

kmaterka added a comment. Too soon, it is still not finished. REPOSITORY R135 Integration for Qt applications in Plasma REVISION DETAIL https://phabricator.kde.org/D25223 To: kmaterka, #plasma, #frameworks, broulik Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, GB

D25223: Avoid side effects during menu initialization

kmaterka created this revision. kmaterka added reviewers: Plasma, Frameworks, broulik. Herald added a project: Plasma. Herald added a subscriber: plasma-devel. kmaterka requested review of this revision. REVISION SUMMARY Setting some attributes, like visible, enabled, etc has side effects. Do