D11352: [RFC] Auto ordered systray

[Konrad Materka]

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 the `onParentChanged` hook (not sure why it was needed in 
the first place...)
  
  
  Due to a bug in Repeater it is needed sometimes. Repeater randomly changes 
the parent item after it was created and re-parented (race condition?). I had 
big troubles with Repeater, especially combining with Loader, 
DelegateModel.Package etc. Repeater is very buggy, in other words, it works by 
happy coincidence. :)
  
  BTW, my idea is to create a common model for all tray items and make 
sorting/filtering there. First version is in D23413 
, it has no sorting yet. I plan to add 
filtering: "Active", "Hidden/Passive", "Invisible" and then 3 different Views 
which will use filtered model. This way `updateItemVisibility` won't even be 
needed! Anyway, this is not a place for this discussion.
  
  PS. comment:
  `// return negative integer if a < b, 0 if a === b, and positive otherwise`
  is incorrect, it will never return 0, thanks to `creationId` ;-)

REPOSITORY
  R120 Plasma Workspace

BRANCH
  stableSystray2

REVISION DETAIL
  https://phabricator.kde.org/D11352

To: Pitel, #vdg, #plasma, mart, ngraham
Cc: kmaterka, ognarb, ngraham, wsdfhjxc, mart, plasma-devel, LeGast00n, 
The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, ragreen, ZrenBot, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol

D11352: [RFC] Auto ordered systray

[Nathaniel Graham]

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, fbampaloukas, GB_2, ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol

D11352: [RFC] Auto ordered systray

[Marco Martin]

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, The-Feren-OS-Dev, 
jraleigh, fbampaloukas, GB_2, ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol

D11352: [RFC] Auto ordered systray

[Nathaniel Graham]

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, #vdg, #plasma, mart, ngraham
Cc: ognarb, ngraham, wsdfhjxc, mart, plasma-devel, LeGast00n, The-Feren-OS-Dev, 
jraleigh, fbampaloukas, GB_2, ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol

D11352: [RFC] Auto ordered systray

[Radek Hušek]

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&id=66209

BRANCH
  stableSystray2

REVISION DETAIL
  https://phabricator.kde.org/D11352

AFFECTED FILES
  applets/systemtray/package/contents/ui/items/AbstractItem.qml
  applets/systemtray/package/contents/ui/main.qml

To: Pitel, #vdg, #plasma, mart
Cc: ognarb, ngraham, wsdfhjxc, mart, plasma-devel, LeGast00n, The-Feren-OS-Dev, 
jraleigh, fbampaloukas, GB_2, ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol

D11352: [RFC] Auto ordered systray

[Nathaniel Graham]

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: Pitel, #vdg, #plasma, mart
Cc: ognarb, ngraham, wsdfhjxc, mart, plasma-devel, jraleigh, GB_2, ragreen, 
Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol

D11352: [RFC] Auto ordered systray

[Nathaniel Graham]

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, jraleigh, GB_2, ragreen, Pitel, 
ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol

D11352: [RFC] Auto ordered systray

[Nathaniel Graham]

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 
, and D11750 
.

REPOSITORY
  R120 Plasma Workspace

BRANCH
  stableSystray2

REVISION DETAIL
  https://phabricator.kde.org/D11352

To: Pitel, #vdg, #plasma, mart
Cc: ngraham, wsdfhjxc, mart, plasma-devel, GB_2, ragreen, Pitel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol

D11352: [RFC] Auto ordered systray

[Radek Hušek]

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&id=30859

BRANCH
  stableSystray2

REVISION DETAIL
  https://phabricator.kde.org/D11352

AFFECTED FILES
  applets/systemtray/package/contents/ui/items/AbstractItem.qml
  applets/systemtray/package/contents/ui/main.qml

To: Pitel, #vdg, #plasma, mart
Cc: ngraham, wsdfhjxc, mart, plasma-devel, ragreen, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol

D11352: [RFC] Auto ordered systray

[Radek Hušek]

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 plasmoid 
> in systray but I am not so sure about statusnotifiers, spec says that `Id` 
> should be unique for given app, but what if user launches two instances of 
> the app? Or am I overthinking this?

I tested multiple instances of vlc and all their properties exported by 
`statusnotifier` engine have the same values so `creationId` is required after 
all.

REPOSITORY
  R120 Plasma Workspace

BRANCH
  stableSystray2

REVISION DETAIL
  https://phabricator.kde.org/D11352

To: Pitel, #vdg, #plasma, mart
Cc: ngraham, wsdfhjxc, mart, plasma-devel, ragreen, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol

D11352: [RFC] Auto ordered systray

[Radek Hušek]

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 
> statusnotifiers, which would then make them ordered after plasmoids but 
> that's fine)

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 plasmoid in systray but I am 
not so sure about statusnotifiers, spec says that `Id` should be unique for 
given app, but what if user launches two instances of the app? Or am I 
overthinking 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, ragreen, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol

D11352: [RFC] Auto ordered systray

[Marco Martin]

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 necessary: both plasmoids and statusnotifieritems have ids 
that you can access (numeric for plasmoids, alphanumeric for statusnotifiers, 
which would then make them ordered after plasmoids but that's fine)

REPOSITORY
  R120 Plasma Workspace

BRANCH
  stableSystray2

REVISION DETAIL
  https://phabricator.kde.org/D11352

To: Pitel, #vdg, #plasma, mart
Cc: ngraham, wsdfhjxc, mart, plasma-devel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol

D11352: [RFC] Auto ordered systray

[Marco Martin]

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
  
  
  i would prefer the final version to be quiet
  
  > - Fix sorting algorithm (the order was wrong if item was created with empty 
text and filled it in later)
  > 
  >   Note that I do not have write access, so I cannot land this myself.
  
  you can apply for it, you're more than welcome

REPOSITORY
  R120 Plasma Workspace

BRANCH
  stableSystray2

REVISION DETAIL
  https://phabricator.kde.org/D11352

To: Pitel, #vdg, #plasma, mart
Cc: ngraham, wsdfhjxc, mart, plasma-devel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol

D11352: [RFC] Auto ordered systray

[Radek Hušek]

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 item was created with empty 
text and filled it in later)
  
  Note that I do not have write access, so I cannot land this myself.

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D11352?vs=29977&id=30111

BRANCH
  stableSystray2

REVISION DETAIL
  https://phabricator.kde.org/D11352

AFFECTED FILES
  applets/systemtray/package/contents/ui/items/AbstractItem.qml
  applets/systemtray/package/contents/ui/main.qml

To: Pitel, #vdg, #plasma, mart
Cc: ngraham, wsdfhjxc, mart, plasma-devel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol

D11352: [RFC] Auto ordered systray

[Marco Martin]

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: Pitel, #vdg, #plasma, mart
Cc: ngraham, wsdfhjxc, mart, plasma-devel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol

D11352: [RFC] Auto ordered systray

[Nathaniel Graham]

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, 
jensreuterberg, abetts, sebas, apol

D11352: [RFC] Auto ordered systray

[Radek Hušek]

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 just a fixed random order. I would rather 
not implement some crazy compare function just because it has somewhat 
reasonable results.
  
  To me just the consistent order is a big step forward compared to the current 
state. I would also like to hear opinion of someone working in ui design. #VDG 
?

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D11352

To: Pitel, #vdg, #plasma
Cc: wsdfhjxc, mart, plasma-devel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol

D11352: [RFC] Auto ordered systray

[Wojciech Stachurski]

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 be better to compare by `item.itemId`. 
Although `StatusNotifierItem` may have it undefined initially, it can be fixed 
with a timer hack and waiting till a proper `Id` is available, then updating 
the `itemId` property and calling `updateItemVisibility()` function.
  
  Modifying the comparing function to look at `item.itemId` instead of 
`item.text` makes the result no better than it was originally. However, if 
compared strings are slightly altered in a specific way, the result might be 
better (something like a seed to get the most satisfying pattern). For example:
  
function compareItems(a, b) {
var categoryDiff = indexForItemCategory(a) - indexForItemCategory(b)
var altA = a.itemId ? a.itemId.replace("org.kde.", 
"").replace("plasma.", "") : ""
var altB = b.itemId ? b.itemId.replace("org.kde.", 
"").replace("plasma.", "") : ""
altA = altA.substring(altA.length / 2, altA.length)
altB = altB.substring(altB.length / 2, altB.length)
return categoryDiff != 0 ? categoryDiff : altA.localeCompare(altB)
}
  
  Screenshot below presents the item ordering result after this modification, 
which is consistent regardless of system's language setting and which looks 
nicer in my opinion.
  F5759917: Items.png 
  
  I don't really have any other ideas on further improvements, so forgive me if 
you find this one utterly stupid, I promise there will bo no more of them :)

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D11352

To: Pitel
Cc: wsdfhjxc, mart, plasma-devel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol

D11352: [RFC] Auto ordered systray

[Radek Hušek]

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&id=29977

BRANCH
  stableSystray2

REVISION DETAIL
  https://phabricator.kde.org/D11352

AFFECTED FILES
  applets/systemtray/package/contents/ui/items/AbstractItem.qml
  applets/systemtray/package/contents/ui/main.qml

To: Pitel
Cc: wsdfhjxc, mart, plasma-devel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol

D11352: [RFC] Auto ordered systray

[Wojciech Stachurski]

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, abetts, sebas, apol

D11352: [RFC] Auto ordered systray

[Radek Hušek]

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 category are from diferent repos, so till they are fixed I added this 
hack, also does not contain Keyboard and Discover applets because I was unable 
to find their itemIds)

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D11352?vs=29690&id=29973

BRANCH
  stableSystray2

REVISION DETAIL
  https://phabricator.kde.org/D11352

AFFECTED FILES
  applets/systemtray/package/contents/ui/items/AbstractItem.qml
  applets/systemtray/package/contents/ui/main.qml

To: Pitel
Cc: wsdfhjxc, mart, plasma-devel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol

D11352: [RFC] Auto ordered systray

[Wojciech Stachurski]

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  -> `Hardware`
  - KDE Connect   -> `Hardware`
  - Touchpad  -> `Hardware`
  - Keyboard Indicator-> `Hardware`
  - Discover Notifier -> `SystemServices`
  
  Does it make any sense? F5759406: Items.png 


REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D11352

To: Pitel
Cc: wsdfhjxc, mart, plasma-devel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol

D11352: [RFC] Auto ordered systray

[Radek Hušek]

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 function 
are, rest of arguments does not matter. I somehow managed to run an older 
version of patch on my machine were `callLater` was applied to local helper 
function so it worked for me...
  
  - Removed `callLater` stuff and also removed the `onParentChanged` hook (not 
sure why it was needed in the first place...)
  - Renamed `getCategoryOrder` to `indexForItemCategory` and `itemCompare` to 
`compareItems`.

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D11352?vs=29592&id=29690

BRANCH
  stableSystray2

REVISION DETAIL
  https://phabricator.kde.org/D11352

AFFECTED FILES
  applets/systemtray/package/contents/ui/items/AbstractItem.qml
  applets/systemtray/package/contents/ui/main.qml

To: Pitel
Cc: wsdfhjxc, mart, plasma-devel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol

D11352: [RFC] Auto ordered systray

[Radek Hušek]

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, ali-mohamed, 
jensreuterberg, abetts, sebas, apol

D11352: [RFC] Auto ordered systray

[Marco Martin]

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, sebas, apol

D11352: [RFC] Auto ordered systray

[Wojciech Stachurski]

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. The more precise item categories are, the 
more sensible auto ordering will be.
  
  In D11352#227210 , @mart wrote:
  
  > did you just copy over the qml files on an old installation or rebuilt all 
workspace?
  
  
  I've applied the patch on a clean branch and rebuilt the `plasma-workspace` 
(even from scratch). If it works for you, then something else is definitely 
broken on my side, so never mind me.

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D11352

To: Pitel
Cc: wsdfhjxc, mart, plasma-devel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol

D11352: [RFC] Auto ordered systray

[Marco Martin]

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 doesn't make any difference and the items are 
neither in panel nor in hidden panel, they just disappear. Only after going 
through configuration and manually switching every item to hidden and
  
  
  did you just copy over the qml files on an old installation or rebuilt all 
workspace?

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D11352

To: Pitel
Cc: wsdfhjxc, mart, plasma-devel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol

D11352: [RFC] Auto ordered systray

[Marco Martin]

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 rather hardcoding half random long list 
into `main.qml`. (Long list in default config is fine by me because user can 
modify that to their wishes.)
  >
  > Judging by previous responses, manually arrangeable system tray is not 
going to be a thing. Although, having it consistent is a nice improvement. What 
bothers me is those groups being vague and also that some default items (such 
as Networks, Touchpad, Updates) are assigned to `UnknownCategory` instead of 
one of specialized ones. Yes, this makes the
  
  
  yes, categories of those items need to be fixed, this was kinda forgotten as 
in plasma5 unfortunately the systray wasn't really using them

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D11352

To: Pitel
Cc: wsdfhjxc, mart, plasma-devel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol

D11352: [RFC] Auto ordered systray

[Radek Hušek]

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 time 
(wrong time is right at reparent before changing order of items, and I believe 
the exact time is implementation detail we should not depend on anyway).

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D11352

To: Pitel
Cc: wsdfhjxc, mart, plasma-devel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol

D11352: [RFC] Auto ordered systray

[Radek Hušek]

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 indexForCategory and have the 
> category as parameter instead of the item

What about `indexForItemCategory`? I need to pass in item not category because 
of special treatment of notifications.

Also I'll rename `itemCompare` to `compareItems`.

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D11352

To: Pitel
Cc: wsdfhjxc, mart, plasma-devel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol

D11352: [RFC] Auto ordered systray

[Wojciech Stachurski]

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 which is unrelated. Not sure what I 
did wrong.
  
  > I do not like the idea of large per applet order list because it will 
always be incomplete and give ugly results if user chooses to replace e.g. 
usual battery applet with some custom one.
  
  Yes, that would have worked only if the user didn't replace embedded items 
with some custom ones for the same indication purpose. So, you are right, it's 
probably a no-go.
  
  > If we conclude that sort based on categories + text is no sufficient I 
would strongly prefer using D11292  
possibly with a larger default config rather hardcoding half random long list 
into `main.qml`. (Long list in default config is fine by me because user can 
modify that to their wishes.)
  
  Judging by previous responses, manually arrangeable system tray is not going 
to be a thing. Although, having it consistent is a nice improvement. What 
bothers me is those groups being vague and also that some default items (such 
as Networks, Touchpad, Updates) are assigned to `UnknownCategory` instead of 
one of specialized ones. Yes, this makes the system tray consistent, but the 
ordering gives a messy result where perceptually unrelated items might be 
placed between related ones. Still, better than random first-in-first-shown 
behavior.

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D11352

To: Pitel
Cc: wsdfhjxc, mart, plasma-devel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol

D11352: [RFC] Auto ordered systray

[Radek Hušek]

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 doesn't make any difference and the items are 
neither in panel nor in hidden panel, they just disappear. Only after going 
through configuration and manually switching every item to hidden and then to 
shown makes most of them visible.
  
  
  That seems like you are getting some javascript error in 
`updateItemVisibility`. Can look at what plasma shell is reporting (or paste it 
here)?
  
  > As for the item order, those categories are rather broad and sorting the 
items by their ids or texts within them will always produce odd results. Given 
there is a limited pool of known embedded items, maybe it would be better to 
predefine ordering values for some of them (battery=0, volume=1, networks=2, 
..., kdeconnect=5, devicenotifier=6, printers=7, etc.) to make the actual 
groups more sensible for humans?
  
  I do not like the idea of large per applet order list because it will always 
be incomplete and give ugly results if user chooses to replace e.g. usual 
battery applet with some custom one. If we conclude that sort based on 
categories + text is no sufficient I would strongly prefer using D11292 
 possibly with a larger default config 
rather hardcoding half random long list into `main.qml`. (Long list in default 
config is fine by me because user can modify that to their wishes.)

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D11352

To: Pitel
Cc: wsdfhjxc, mart, plasma-devel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol

D11352: [RFC] Auto ordered systray

[Wojciech Stachurski]

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, they just disappear. Only after going 
through configuration and manually switching every item to hidden and then to 
shown makes most of them visible.
  
  As for the item order, those categories are rather broad and sorting the 
items by their ids or texts within them will always produce odd results. Given 
there is a limited pool of known embedded items, maybe it would be better to 
predefine ordering values for some of them (battery=0, volume=1, networks=2, 
..., kdeconnect=5, devicenotifier=6, printers=7, etc.) to make the actual 
groups more sensible for humans?

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D11352

To: Pitel
Cc: wsdfhjxc, mart, plasma-devel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol

D11352: [RFC] Auto ordered systray

[Marco Martin]

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 in c++ already anyways)

INLINE COMMENTS

> AbstractItem.qml:81
> +function updateVisibility() {
> +Qt.callLater(updateItemVisibility, abstractItem)
> +}

what's the reson for using callLater?

> main.qml:65
> +]
> +function getCategoryOrder(item) {
> +if (item.itemId == "org.kde.plasma.notifications") {

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 indexForCategory and have the 
category as parameter instead of the item

> main.qml:79
> +function reorderItem(item, container) {
> +if (container.children.length == 0) {
> +item.parent = container

i like the logic, but add some comments on what is doing

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D11352

To: Pitel
Cc: wsdfhjxc, mart, plasma-devel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol

D11352: [RFC] Auto ordered systray

[Radek Hušek]

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 
, D11292 
) but prefers auto
  ordering instead. To make my opinion I hacked up this and started testing on 
myself.
  Current version breaks if two items have the same category and text (I guess 
this
  should not happen but...).
  
  Note that category order is more or less randomly chosen (I do believe 
someone else
  has an opinion what the correct order is).
  
  This is mutually exclusive with D11292 .

REPOSITORY
  R120 Plasma Workspace

BRANCH
  stableSystray2

REVISION DETAIL
  https://phabricator.kde.org/D11352

AFFECTED FILES
  applets/systemtray/package/contents/ui/items/AbstractItem.qml
  applets/systemtray/package/contents/ui/main.qml

To: Pitel
Cc: wsdfhjxc, mart, plasma-devel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol