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

D25223: Avoid side effects during menu initialization

broulik accepted this revision. This revision is now accepted and ready to land. REPOSITORY R135 Integration for Qt applications in Plasma BRANCH master REVISION DETAIL https://phabricator.kde.org/D25223 To: kmaterka, #plasma, #frameworks, broulik Cc: nicolasfella, davidedmundson,

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

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,

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. > > >

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

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

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

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.

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) > -,

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

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

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,

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

D25223: Avoid side effects during menu initialization

kmaterka edited the summary of this revision. 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_2, ragreen,

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=69489 BRANCH master REVISION DETAIL

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,

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