D11352: [RFC] Auto ordered systray

kmaterka added a comment. Sorry for interrupting this late in the review. I like the idea of consistent ordering very much, I even planned to implement this myself :) I have few comments: In D11352#227354 , @Pitel wrote: > - [...] remove

D11352: [RFC] Auto ordered systray

ngraham added a comment. Cool, will do! REPOSITORY R120 Plasma Workspace BRANCH stableSystray2 REVISION DETAIL https://phabricator.kde.org/D11352 To: Pitel, #vdg, #plasma, mart, ngraham Cc: ognarb, ngraham, wsdfhjxc, mart, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh,

D11352: [RFC] Auto ordered systray

mart added a comment. ok for me at beginning of 5.18 cycle, not yet for 5.17 REPOSITORY R120 Plasma Workspace BRANCH stableSystray2 REVISION DETAIL https://phabricator.kde.org/D11352 To: Pitel, #vdg, #plasma, mart, ngraham Cc: ognarb, ngraham, wsdfhjxc, mart, plasma-devel, LeGast00n,

D11352: [RFC] Auto ordered systray

ngraham accepted this revision. ngraham added a comment. This seems sensible enough to me. @mart, do you think we can do this for 5.17, or should we wait until 5.18? REPOSITORY R120 Plasma Workspace BRANCH stableSystray2 REVISION DETAIL https://phabricator.kde.org/D11352 To: Pitel,

D11352: [RFC] Auto ordered systray

Pitel updated this revision to Diff 66209. Pitel added a comment. Rebase on current master and squash REPOSITORY R120 Plasma Workspace CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D11352?vs=30859=66209 BRANCH stableSystray2 REVISION DETAIL

D11352: [RFC] Auto ordered systray

ngraham added a comment. @pitel, I'm sorry this has been sitting for so long! We'd like to land this but it no longer merges cleanly on master. Can you rebase it please? REPOSITORY R120 Plasma Workspace BRANCH stableSystray2 REVISION DETAIL https://phabricator.kde.org/D11352 To:

D11352: [RFC] Auto ordered systray

ngraham added a comment. #plasma folks? Are we landing this? REPOSITORY R120 Plasma Workspace BRANCH stableSystray2 REVISION DETAIL https://phabricator.kde.org/D11352 To: Pitel, #vdg, #plasma, mart Cc: ngraham, wsdfhjxc, mart, plasma-devel,

D11352: [RFC] Auto ordered systray

ngraham added a comment. Where are we with this? Can it land? Any objections from other #plasma people? If it does, it looks like we will also need to land D11748 , D11749

D11352: [RFC] Auto ordered systray

Pitel updated this revision to Diff 30859. Pitel added a comment. - Remove debug print REPOSITORY R120 Plasma Workspace CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D11352?vs=30111=30859 BRANCH stableSystray2 REVISION DETAIL https://phabricator.kde.org/D11352 AFFECTED

D11352: [RFC] Auto ordered systray

Pitel added inline comments. INLINE COMMENTS > Pitel wrote in AbstractItem.qml:80 > Probably not, I have not observed any problems even without this rule, I just > wanted to be safe. I see that I can use `applet.id` or even > `applet.pluginName` as there should be at most one instace of given

D11352: [RFC] Auto ordered systray

Pitel added a comment. > i would prefer the final version to be quiet Sure. INLINE COMMENTS > mart wrote in AbstractItem.qml:80 > This is probably not necessary: both plasmoids and statusnotifieritems have > ids that you can access (numeric for plasmoids, alphanumeric for >

D11352: [RFC] Auto ordered systray

mart added inline comments. INLINE COMMENTS > AbstractItem.qml:80 > > -onEffectiveStatusChanged: updateItemVisibility(abstractItem); > +property int creationId // used for item order tie breaking > +onEffectiveStatusChanged: updateItemVisibility(abstractItem) This is probably not

D11352: [RFC] Auto ordered systray

mart added a comment. In D11352#230681 , @Pitel wrote: > Great, but I found one more bug (and it affected order of items). This fixes it. > > - Add tie breaking (for the unlikely case of the same category & text) > - Add debug print

D11352: [RFC] Auto ordered systray

Pitel updated this revision to Diff 30111. Pitel added a comment. Great, but I found one more bug (and it affected order of items). This fixes it. - Add tie breaking (for the unlikely case of the same category & text) - Add debug print - Fix sorting algorithm (the order was wrong if

D11352: [RFC] Auto ordered systray

mart accepted this revision. mart added a comment. This revision is now accepted and ready to land. got trough some testing of the last version, for me is now good to go REPOSITORY R120 Plasma Workspace BRANCH stableSystray2 REVISION DETAIL https://phabricator.kde.org/D11352 To:

D11352: [RFC] Auto ordered systray

ngraham added a comment. I rather like this auto-order-by-category approach, myself. +1 conceptually. REPOSITORY R120 Plasma Workspace REVISION DETAIL https://phabricator.kde.org/D11352 To: Pitel, #vdg, #plasma Cc: ngraham, wsdfhjxc, mart, plasma-devel, ZrenBot, lesliezhai, ali-mohamed,

D11352: [RFC] Auto ordered systray

Pitel added a comment. I am using Czech translation and I am satisfied with item order (but I keep only a few items in my systray). I like the current rule because it is simple and have some internal logic (even though it might not be obvious what the `text` of an applet is) and it is not

D11352: [RFC] Auto ordered systray

wsdfhjxc added a comment. Unfortunately, comparing by `item.text` with a non-US language enabled makes the result worse than on previous screenshot (especially the order of items within the `Hardware` group is very odd). To get it consistent regardless of system's language setting it might

D11352: [RFC] Auto ordered systray

Pitel updated this revision to Diff 29977. Pitel added a comment. ty, updated REPOSITORY R120 Plasma Workspace CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D11352?vs=29973=29977 BRANCH stableSystray2 REVISION DETAIL https://phabricator.kde.org/D11352 AFFECTED FILES

D11352: [RFC] Auto ordered systray

wsdfhjxc added a comment. They are `org.kde.plasma.keyboardindicator` and `org.kde.discovernotifier`. REPOSITORY R120 Plasma Workspace REVISION DETAIL https://phabricator.kde.org/D11352 To: Pitel Cc: wsdfhjxc, mart, plasma-devel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg,

D11352: [RFC] Auto ordered systray

Pitel updated this revision to Diff 29973. Pitel added a comment. - put UnknownCategory first - treat applets with category not in the list as UnknownCategory - split reorderItem function into two - add temporary hack whcih allows overriding applet's category (applets with wrong

D11352: [RFC] Auto ordered systray

wsdfhjxc added a comment. Thanks for a fix. I've changed the category order to - `UnknownCategory` - `ApplicationStatus` - `Communications` - `SystemServices` - `Hardware` and updated `metadata.desktop` for some Plasma components - Networks ->

D11352: [RFC] Auto ordered systray

Pitel updated this revision to Diff 29690. Pitel added a comment. @mart you were very right to point out the `callLater` because that is exactly what was broken. The code assumed that all calls of `callLater` with different arguments are executed but in fact only those eith different

D11352: [RFC] Auto ordered systray

Pitel planned changes to this revision. Pitel added a comment. Ok, that looks that I did screw something. Time to investigate... REPOSITORY R120 Plasma Workspace REVISION DETAIL https://phabricator.kde.org/D11352 To: Pitel Cc: wsdfhjxc, mart, plasma-devel, ZrenBot, lesliezhai,

D11352: [RFC] Auto ordered systray

mart added a comment. i can confirm systray is broken for me as well, with plasma-workspace master REPOSITORY R120 Plasma Workspace REVISION DETAIL https://phabricator.kde.org/D11352 To: Pitel Cc: wsdfhjxc, mart, plasma-devel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts,

D11352: [RFC] Auto ordered systray

wsdfhjxc added a comment. In D11352#227154 , @mart wrote: > yes, categories of those items need to be fixed, this was kinda forgotten as in plasma5 unfortunately the systray wasn't really using them It would be great if that happened.

D11352: [RFC] Auto ordered systray

mart added a comment. In D11352#227065 , @wsdfhjxc wrote: > Doesn't seem to work for me. Only Notifications item is visible by default while there are multiple items set to be shown in the configuration. Also, changing the visibility state

D11352: [RFC] Auto ordered systray

mart added a comment. In D11352#227120 , @wsdfhjxc wrote: > > If we conclude that sort based on categories + text is no sufficient I would strongly prefer using D11292 possibly with a larger default config

D11352: [RFC] Auto ordered systray

Pitel added a comment. Phabricator:Pitel 2:0 INLINE COMMENTS > mart wrote in AbstractItem.qml:81 > what's the reson for using callLater? The main reason is to avoid calling `updateItemVisibility` from itself which might happen if `stackAfter` fires the `onParentChanged` event at wrong

D11352: [RFC] Auto ordered systray

Pitel added a comment. I guess Phabricator does not let me only respond to inline comments. INLINE COMMENTS > mart wrote in main.qml:65 > as convention we usually don't have getFoo as names (and this is not getting > a property anyways) > i would like a more descriptive name like

D11352: [RFC] Auto ordered systray

wsdfhjxc added a comment. > That seems like you are getting some javascript error in `updateItemVisibility`. Can look at what plasma shell is reporting (or paste it here)? There are no other errors except of `ConfigEntries.qml:228:34: Unable to assign [undefined] to QKeySequence` spam

D11352: [RFC] Auto ordered systray

Pitel added a comment. In D11352#227065 , @wsdfhjxc wrote: > Doesn't seem to work for me. Only Notifications item is visible by default while there are multiple items set to be shown in the configuration. Also, changing the visibility state

D11352: [RFC] Auto ordered systray

wsdfhjxc added a comment. Doesn't seem to work for me. Only Notifications item is visible by default while there are multiple items set to be shown in the configuration. Also, changing the visibility state doesn't make any difference and the items are neither in panel nor in hidden panel,

D11352: [RFC] Auto ordered systray

mart added a comment. I like where this is going! (will still have to test it running tough) one general thing i wonder, is if would be better to implementthe whole reorderItem() logic on the c++ side as is potentially heavy and complicated.. (and the actual reordering needs to be done

D11352: [RFC] Auto ordered systray

Pitel created this revision. Restricted Application added a project: Plasma. Restricted Application added a subscriber: plasma-devel. Pitel requested review of this revision. REVISION SUMMARY @mart dislikes idea of manually ordering the systray (D11233 ,