D28208: Move sni icon handling logic from data engine to applet

kmaterka added a comment. @davidre can you extract your test improvement to a separate patch? it would help me in another issue and I don't want to "steal" your code :) REPOSITORY R120 Plasma Workspace REVISION DETAIL https://phabricator.kde.org/D28208 To: davidre, kmaterka, broulik,

D28470: [PlasmaCore.IconItem] Refactor source handling for different types

kmaterka added a comment. Regression fixed in D29314 . REPOSITORY R242 Plasma Framework (Library) REVISION DETAIL https://phabricator.kde.org/D28470 To: kmaterka, #plasma, broulik, apol, davidedmundson, #frameworks Cc: ngraham, mart, davidre, cblack,

D29314: [PlasmaCore.IconItem] Regression: fix crash on source change

This revision was automatically updated to reflect the committed changes. Closed by commit R242:4b0d4c4bdaed: [PlasmaCore.IconItem] Regression: fix crash on source change (authored by kmaterka). REPOSITORY R242 Plasma Framework (Library) CHANGES SINCE LAST UPDATE

D29314: [PlasmaCore.IconItem] Regression: fix crash on source change

kmaterka edited the summary of this revision. REPOSITORY R242 Plasma Framework (Library) REVISION DETAIL https://phabricator.kde.org/D29314 To: kmaterka, #plasma, #frameworks, ngraham, davidedmundson Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29314: [PlasmaCore.IconItem] Regression: fix crash on source change

kmaterka edited the summary of this revision. kmaterka edited the test plan for this revision. kmaterka added a reviewer: davidedmundson. REPOSITORY R242 Plasma Framework (Library) REVISION DETAIL https://phabricator.kde.org/D29314 To: kmaterka, #plasma, #frameworks, ngraham, davidedmundson

D29314: [PlasmaCore.IconItem] Regression: fix crash on source change

kmaterka created this revision. kmaterka added reviewers: Plasma, Frameworks, ngraham. Herald added a project: Frameworks. Herald added a subscriber: kde-frameworks-devel. kmaterka requested review of this revision. REVISION SUMMARY When SvgSource is changed, old one is deleted. Connections are

D28470: [PlasmaCore.IconItem] Refactor source handling for different types

kmaterka added a comment. In D28470#660492 , @ngraham wrote: > `git bisect` says this caused https://bugs.kde.org/show_bug.cgi?id=420801. I will check that, sorry... REPOSITORY R242 Plasma Framework (Library) REVISION DETAIL

D28470: [PlasmaCore.IconItem] Refactor source handling for different types

This revision was automatically updated to reflect the committed changes. Closed by commit R242:5a3fb570feda: [PlasmaCore.IconItem] Refactor source handling for different types (authored by kmaterka). REPOSITORY R242 Plasma Framework (Library) CHANGES SINCE LAST UPDATE

D28470: [PlasmaCore.IconItem] Refactor source handling for different types

kmaterka added a reviewer: Frameworks. REPOSITORY R242 Plasma Framework (Library) REVISION DETAIL https://phabricator.kde.org/D28470 To: kmaterka, #plasma, broulik, apol, davidedmundson, #frameworks Cc: mart, davidre, cblack, kde-frameworks-devel, #plasma, LeGast00n, michaelh, ngraham,

D28470: [PlasmaCore.IconItem] Refactor source handling for different types

kmaterka updated this revision to Diff 81031. kmaterka added a comment. Rebase to master (includes D29102 ) REPOSITORY R242 Plasma Framework (Library) CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D28470?vs=80148=81031 BRANCH master

D28470: [PlasmaCore.IconItem] Refactor source handling for different types

kmaterka added a comment. Any other comment? Is it OK and can be approved? REPOSITORY R242 Plasma Framework (Library) REVISION DETAIL https://phabricator.kde.org/D28470 To: kmaterka, #plasma, broulik, apol, davidedmundson Cc: mart, davidre, cblack, kde-frameworks-devel, #plasma,

D28470: [PlasmaCore.IconItem] Refactor source handling for different types

kmaterka updated this revision to Diff 80148. kmaterka marked an inline comment as done. kmaterka added a comment. Do not inherit from QObject REPOSITORY R242 Plasma Framework (Library) CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D28470?vs=79283=80148 BRANCH master REVISION

D28470: [PlasmaCore.IconItem] Refactor source handling for different types

kmaterka marked an inline comment as done. kmaterka added a comment. In D28470#647757 , @davidedmundson wrote: > Note there's a unit test for IconItem worth running if you haven't already. I've checked that already, these test were

D28208: Move sni icon handling logic from data engine to applet

kmaterka added a reviewer: Frameworks. REPOSITORY R120 Plasma Workspace REVISION DETAIL https://phabricator.kde.org/D28208 To: davidre, kmaterka, broulik, mart, #plasma, #vdg, #frameworks Cc: bruns, ngraham, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus,

D28470: [PlasmaCore.IconItem] Refactor source handling for different types

kmaterka added a comment. ping, review needed :) REPOSITORY R242 Plasma Framework (Library) REVISION DETAIL https://phabricator.kde.org/D28470 To: kmaterka, #plasma, broulik, apol, davidedmundson Cc: davidre, cblack, kde-frameworks-devel, #plasma, LeGast00n, GB_2, michaelh, ngraham,

D28470: [PlasmaCore.IconItem] Refactor source handling for different types

kmaterka added a comment. Is it OK now? Any other review comments? REPOSITORY R242 Plasma Framework (Library) REVISION DETAIL https://phabricator.kde.org/D28470 To: kmaterka, #plasma, broulik, apol, davidedmundson Cc: davidre, cblack, kde-frameworks-devel, #plasma, LeGast00n, GB_2,

D28470: [PlasmaCore.IconItem] Refactor source handling for different types

kmaterka updated this revision to Diff 79283. kmaterka added a comment. Change class name: *Strategy -> *Source REPOSITORY R242 Plasma Framework (Library) CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D28470?vs=79281=79283 BRANCH master REVISION DETAIL

D28470: [PlasmaCore.IconItem] Refactor source handling for different types

kmaterka added a comment. In D28470#641230 , @davidre wrote: > In D28470#640468 , @cblack wrote: > > > Splitting into multiple classes seems like a good idea, but "strategy"? Seems like an odd

D28470: [PlasmaCore.IconItem] Refactor source handling for different types

kmaterka retitled this revision from "WIP: IconItem: Refactor source handling for different types" to "[PlasmaCore.IconItem] Refactor source handling for different types". kmaterka edited the test plan for this revision. REPOSITORY R242 Plasma Framework (Library) REVISION DETAIL

D28470: WIP: IconItem: Refactor source handling for different types

kmaterka updated this revision to Diff 79281. kmaterka marked an inline comment as done. kmaterka added a comment. be consistent with old implementation REPOSITORY R242 Plasma Framework (Library) CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D28470?vs=79279=79281 BRANCH master

D28470: WIP: IconItem: Refactor source handling for different types

kmaterka marked 2 inline comments as done. kmaterka added inline comments. INLINE COMMENTS > cblack wrote in iconitem.cpp:58 > Is this class necessary? I feel like this class's behaviour should be what > its parent does without a child implementation. I wanted `IconItemStrategy` to be just an

D28470: WIP: IconItem: Refactor source handling for different types

kmaterka updated this revision to Diff 79279. kmaterka added a comment. fix some silly errors REPOSITORY R242 Plasma Framework (Library) CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D28470?vs=79011=79279 BRANCH master REVISION DETAIL https://phabricator.kde.org/D28470

D28470: WIP: IconItem: Refactor source handling for different types

kmaterka added a comment. Do you think it is a good direction? REPOSITORY R242 Plasma Framework (Library) REVISION DETAIL https://phabricator.kde.org/D28470 To: kmaterka, #plasma, broulik, apol, davidedmundson Cc: kde-frameworks-devel, #plasma, LeGast00n, cblack, GB_2, michaelh,

D28470: WIP: IconItem: Refactor source handling for different types

kmaterka created this revision. kmaterka added reviewers: Plasma, broulik, apol, davidedmundson. Herald added a project: Frameworks. Herald added a subscriber: kde-frameworks-devel. kmaterka requested review of this revision. REVISION SUMMARY There are 3 possible strategies: QIcon, QImage and

D28197: [KSortFilterProxyModel QML] Make invalidateFilter public

kmaterka accepted this revision. REPOSITORY R275 KItemModels REVISION DETAIL https://phabricator.kde.org/D28197 To: broulik, ahiemstra, davidedmundson, iasensio, kmaterka Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D25326: Move Plasma's SortFilterProxyModel into KItemModel's QML plugin

kmaterka added inline comments. INLINE COMMENTS > ahiemstra wrote in ksortfilterproxymodel_qml.cpp:111 > I don't think a unit test is the right place for example code. It's probably > better to either improve the documentation of the property or add an example > somewhere where it makes sense.

D25326: Move Plasma's SortFilterProxyModel into KItemModel's QML plugin

kmaterka added a comment. Few comments from someone who was recently using `PlasmaCore.SortFilterModel` and had trouble understanding sorting :) INLINE COMMENTS > ksortfilterproxymodel_qml.cpp:111 > + > +void tst_KSortFilterProxyModelQml::testSortRole() > +{ Can you add a test for

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

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

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

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

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

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,

D24843: [KDEPlatformSystemTrayIcon] Recreate deleted menu

kmaterka added a comment. In D24843#560188 , @kmaterka wrote: > In D24843#560023 , @broulik wrote: > > > This causes menus (mostly submenus) to randomly show up when the SNI is updated, e.g. every

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

D24843: [KDEPlatformSystemTrayIcon] Recreate deleted menu

kmaterka added a comment. In D24843#560023 , @broulik wrote: > This causes menus (mostly submenus) to randomly show up when the SNI is updated, e.g. every time VLC changes a track I get its "speed (slower, normal, faster)" menu open: >

D24825: Add hideOnWindowDeactivate to PlasmaComponents.Dialog

This revision was automatically updated to reflect the committed changes. Closed by commit R242:96b7fc21f5b5: Add hideOnWindowDeactivate to PlasmaComponents.Dialog (authored by kmaterka). REPOSITORY R242 Plasma Framework (Library) CHANGES SINCE LAST UPDATE

D24825: Add hideOnWindowDeactivate to PlasmaComponents.Dialog

kmaterka added a comment. Should I proceed? This component has serious issues anyway, maybe there is no point in fixing this? For example: There is Loader that load always the same QML because... "if (true || )" - line 241. Even if this is obsolete, cleanup would be good idea.

D24843: [KDEPlatformSystemTrayIcon] Recreate deleted menu

This revision was automatically updated to reflect the committed changes. Closed by commit R135:bc1c85144adb: [KDEPlatformSystemTrayIcon] Recreate deleted menu (authored by kmaterka). REPOSITORY R135 Integration for Qt applications in Plasma CHANGES SINCE LAST UPDATE

D24843: [KDEPlatformSystemTrayIcon] Recreate deleted menu

kmaterka added a comment. Any chance to get this reviewed? :) REPOSITORY R135 Integration for Qt applications in Plasma REVISION DETAIL https://phabricator.kde.org/D24843 To: kmaterka, apol, davidedmundson, #plasma, #frameworks, broulik, nicolasfella Cc: plasma-devel, LeGast00n,

D24843: [KDEPlatformSystemTrayIcon] Recreate deleted menu

kmaterka added a comment. This is a proper solution to BUG 365105. Can someone review it? Is everything OK with this? REPOSITORY R135 Integration for Qt applications in Plasma REVISION DETAIL https://phabricator.kde.org/D24843 To: kmaterka, apol, davidedmundson, #plasma, #frameworks,

D24843: [KDEPlatformSystemTrayIcon] Recreate deleted menu

kmaterka added reviewers: Frameworks, broulik, nicolasfella. REPOSITORY R135 Integration for Qt applications in Plasma REVISION DETAIL https://phabricator.kde.org/D24843 To: kmaterka, apol, davidedmundson, #plasma, #frameworks, broulik, nicolasfella Cc: plasma-devel, LeGast00n,

D24755: [KStatusNotifierItem] Optionaly, do not take menu ownership

kmaterka added a comment. Moved to D24843 REPOSITORY R289 KNotifications REVISION DETAIL https://phabricator.kde.org/D24755 To: kmaterka, #frameworks, davidedmundson, broulik, nicolasfella Cc: anthonyfieroni, kde-frameworks-devel, LeGast00n, GB_2,

D24755: [KStatusNotifierItem] Optionaly, do not take menu ownership

kmaterka added a comment. > First, it will not have memory leaks, we take ownership on parentless menu, on menu that has a parent, it will destroy it by parent-child cleanups. Yes, eventually menu will be deleted. I'm thinking about the situation when someone has: if

D24755: [KStatusNotifierItem] Optionaly, do not take menu ownership

kmaterka abandoned this revision. kmaterka added a comment. This is a dead end, I will implement different solution in QPA (KDEPlatformSystemTrayIcon) plugin. REPOSITORY R289 KNotifications REVISION DETAIL https://phabricator.kde.org/D24755 To: kmaterka, #frameworks, davidedmundson,

D24755: [KStatusNotifierItem] Optionaly, do not take menu ownership

kmaterka added a comment. In D24755#550415 , @anthonyfieroni wrote: > That should be fine, in QPA we have a tray icon per sni, update menu should be on same object which will not be a problem, check it. There are two objects in QPA that

D24825: Add hideOnWindowDeactivate to PlasmaComponents.Dialog

kmaterka added a comment. This is a first step to solve 401016 : "Touchpad plasmoid [confirmation dialog] remains open when losing focus, I think that it is inconsistent since other elements of tray close themselves after switching away."

D24825: Add hideOnWindowDeactivate to PlasmaComponents.Dialog

kmaterka edited the summary of this revision. kmaterka added reviewers: Plasma, Frameworks. REPOSITORY R242 Plasma Framework (Library) REVISION DETAIL https://phabricator.kde.org/D24825 To: kmaterka, #plasma, #frameworks Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D24825: Add hideOnWindowDeactivate to PlasmaComponents.Dialog

kmaterka created this revision. Herald added a project: Frameworks. Herald added a subscriber: kde-frameworks-devel. kmaterka requested review of this revision. REVISION SUMMARY When using Dialog or QueryDialog it should be possible to set hideOnWindowDeactivate flag. REPOSITORY R242 Plasma

D24755: [KStatusNotifierItem] Optionaly, do not take menu ownership

kmaterka added inline comments. INLINE COMMENTS > anthonyfieroni wrote in kstatusnotifieritem.cpp:790 > That's easy checkable > > if (!qApp->closingDown() && (!d->menu->parent() || d->menu->parent() == > d->associatedWidget) { > delete d->menu; > } > > We should not own the menu,

D24755: [KStatusNotifierItem] Optionaly, do not take menu ownership

kmaterka added inline comments. INLINE COMMENTS > anthonyfieroni wrote in kstatusnotifieritem.cpp:454 > Do not take ownership of the menu and delete it when it does not have a > parent. takeOwnership is wrong approach, you can remove it. In theory that should be the correct approach, the "Qt

D24755: [KStatusNotifierItem] Optionaly, do not take menu ownership

kmaterka marked 5 inline comments as done. kmaterka added a comment. In D24755#550140 , @davidedmundson wrote: > I don't exactly like it, but I don't have anything better. +1 Me neither. Probably the best would be to use shared pointer,

D24755: [KStatusNotifierItem] Optionaly, do not take menu ownership

kmaterka updated this revision to Diff 68292. kmaterka added a comment. Wrap menu in QPointer REPOSITORY R289 KNotifications CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D24755?vs=68229=68292 BRANCH master REVISION DETAIL https://phabricator.kde.org/D24755 AFFECTED FILES

D24755: [KStatusNotifierItem] Optionaly, do not take menu ownership

kmaterka added inline comments. INLINE COMMENTS > davidedmundson wrote in kstatusnotifieritemprivate_p.h:158 > QPointer here won't solve the other hacks needed. > > We have MenuSource, Menu and KSNI > > QPointer would fix the case of MenuSource still owning the Menu and KSNI > lasting longer

D24755: [KStatusNotifierItem] Optionaly, do not take menu ownership

kmaterka added inline comments. INLINE COMMENTS > anthonyfieroni wrote in kstatusnotifieritemprivate_p.h:158 > It should be QPointer, no other changes are needed. Sure, but this is not my code and this is not related. What I want to do here is more like hack/workaround, so it is better to keep

D24755: [KStatusNotifierItem] Optionaly, do not take menu ownership

kmaterka added a comment. Continuation of D24667 . I know this solution sucks, but I have no idea how this can be done better. I have another idea, probably even crazier: set "DO_NOT_DELETE" flag as a QObject property. Then in destructor check it:

D24755: [KStatusNotifierItem] Optionaly, do not take menu ownership

kmaterka created this revision. kmaterka added reviewers: Frameworks, davidedmundson, broulik, nicolasfella. Herald added a project: Frameworks. Herald added a subscriber: kde-frameworks-devel. kmaterka requested review of this revision. REVISION SUMMARY It is not possible to handle ownership

D24667: [KStatusNotifierItem] Allow left click when menu is null

This revision was automatically updated to reflect the committed changes. Closed by commit R289:aa6bb9b06cfb: [KStatusNotifierItem] Allow left click when menu is null (authored by kmaterka). REPOSITORY R289 KNotifications CHANGES SINCE LAST UPDATE

D24667: [KStatusNotifierItem] Allow left click when menu is null

kmaterka marked 2 inline comments as done. kmaterka added a comment. In D24667#548245 , @broulik wrote: > > bool takeOwnership = true); > > If only Qt/we used modern C++ features to communicate object ownership :) Hehe, yes.

D24667: [KStatusNotifierItem] Allow left click when menu is null

kmaterka added a comment. I think the best would be to add additional parameter to KSNI. Change from: void KStatusNotifierItem::setContextMenu(QMenu *menu); to something like this: void KStatusNotifierItem::setContextMenu(QMenu *menu, bool takeOwnership = true); Then,

D24667: [KStatusNotifierItem] Allow left click when menu is null

kmaterka added a reviewer: Frameworks. REPOSITORY R289 KNotifications REVISION DETAIL https://phabricator.kde.org/D24667 To: kmaterka, davidedmundson, broulik, nicolasfella, #frameworks Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D24667: [KStatusNotifierItem] Allow left click when menu is null

kmaterka retitled this revision from "Activate when both associatedWidget and menu are null" to "[KStatusNotifierItem] Allow left click when menu is null". REPOSITORY R289 KNotifications REVISION DETAIL https://phabricator.kde.org/D24667 To: kmaterka, davidedmundson, broulik, nicolasfella

D24667: Activate when both associatedWidget and menu are null

kmaterka added inline comments. INLINE COMMENTS > kstatusnotifieritem.cpp:619 > > -if (d->associatedWidget == d->menu) { > +if (d->associatedWidget && d->associatedWidget == d->menu) { > d->statusNotifierItemDBus->ContextMenu(pos.x(), pos.y()); This should fix a situation

D24667: Activate when both associatedWidget and menu are null

kmaterka added a comment. This fixes issue from first comment made by @davidedmundson . Second is still not fixed. Let me copy it here: > We have one menu for the whole tray instance >

D24667: Activate when both associatedWidget and menu are null

kmaterka added reviewers: broulik, nicolasfella. REPOSITORY R289 KNotifications REVISION DETAIL https://phabricator.kde.org/D24667 To: kmaterka, davidedmundson, broulik, nicolasfella Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D24667: Activate when both associatedWidget and menu are null

kmaterka created this revision. kmaterka added a reviewer: davidedmundson. Herald added a project: Frameworks. Herald added a subscriber: kde-frameworks-devel. kmaterka requested review of this revision. REVISION SUMMARY If associatedWidget and menu are the same then instead of "activate"