Use consistent ordering for System Tray items
ClosedPublic

Authored by Pitel on Mar 15 2018, 12:40 PM.

Details

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.

Diff Detail

Repository
R120 Plasma Workspace
Branch
stableSystray2
Lint
No Linters Available
Unit
No Unit Test Coverage
Pitel created this revision.Mar 15 2018, 12:40 PM
Restricted Application added a project: Plasma. · View Herald TranscriptMar 15 2018, 12:40 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
Pitel requested review of this revision.Mar 15 2018, 12:40 PM
mart added a comment.Mar 15 2018, 7:58 PM

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)

applets/systemtray/package/contents/ui/items/AbstractItem.qml
81

what's the reson for using callLater?

applets/systemtray/package/contents/ui/main.qml
67

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

81

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

This comment was removed by 7hdk8g5b.
Pitel added a comment.Mar 16 2018, 8:04 AM
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.)

This comment was removed by 7hdk8g5b.

I guess Phabricator does not let me only respond to inline comments.

applets/systemtray/package/contents/ui/main.qml
67

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

Also I'll rename itemCompare to compareItems.

Phabricator:Pitel 2:0

applets/systemtray/package/contents/ui/items/AbstractItem.qml
81

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

mart added a comment.Mar 16 2018, 11:18 AM
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

mart added a comment.Mar 16 2018, 12:01 PM
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?

This comment was removed by 7hdk8g5b.
mart added a comment.EditedMar 16 2018, 4:02 PM

i can confirm systray is broken for me as well, with plasma-workspace master
but i don't get warnings from the terminal about qml errors

Pitel planned changes to this revision.Mar 16 2018, 4:24 PM

Ok, that looks that I did screw something. Time to investigate...

Pitel updated this revision to Diff 29690.EditedMar 16 2018, 5:04 PM

@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 with different function are, rest of arguments does not matter. I somehow managed to run an older version of the patch on my machine were callLater was applied to a local helper function so it worked for me...

  • Remove callLater stuff and also remove the onParentChanged hook (not sure why it was needed in the first place...)
  • Rename getCategoryOrder to indexForItemCategory and itemCompare to compareItems.
This comment was removed by 7hdk8g5b.
Pitel updated this revision to Diff 29973.Mar 20 2018, 8:08 AM
  • 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)
This comment was removed by 7hdk8g5b.
Pitel updated this revision to Diff 29977.Mar 20 2018, 8:32 AM

ty, updated

This comment was removed by 7hdk8g5b.
Pitel added a comment.Mar 20 2018, 5:12 PM

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?

I rather like this auto-order-by-category approach, myself. +1 conceptually.

mart accepted this revision.Mar 21 2018, 11:03 AM

got trough some testing of the last version, for me is now good to go

This revision is now accepted and ready to land.Mar 21 2018, 11:03 AM
Pitel updated this revision to Diff 30111.Mar 21 2018, 11:14 AM

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.

mart added a comment.Mar 23 2018, 12:45 PM

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

mart added inline comments.Mar 23 2018, 12:49 PM
applets/systemtray/package/contents/ui/items/AbstractItem.qml
80–87

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)

Pitel added a comment.Mar 25 2018, 7:09 PM

i would prefer the final version to be quiet

Sure.

applets/systemtray/package/contents/ui/items/AbstractItem.qml
80–87

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?

Pitel added inline comments.Mar 29 2018, 4:26 PM
applets/systemtray/package/contents/ui/items/AbstractItem.qml
80–87

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

Pitel updated this revision to Diff 30859.Mar 29 2018, 4:30 PM
  • Remove debug print

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.

Plasma folks? Are we landing this?

ognarb added a subscriber: ognarb.Mar 3 2019, 11:34 PM

@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?

Pitel updated this revision to Diff 66209.Sep 16 2019, 12:19 PM

Rebase on current master and squash

ngraham accepted this revision.Sep 16 2019, 4:14 PM

This seems sensible enough to me. @mart, do you think we can do this for 5.17, or should we wait until 5.18?

mart added a comment.Sep 16 2019, 4:17 PM

ok for me at beginning of 5.18 cycle, not yet for 5.17

Cool, will do!

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:

  • [...] 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 ;-)

ngraham retitled this revision from [RFC] Auto ordered systray to Use consistent ordering for System Tray items.Sep 19 2019, 8:54 PM
This revision was automatically updated to reflect the committed changes.