[SystemTray] Rework system tray settings
ClosedPublic

Authored by kmaterka on Jan 11 2020, 4:58 PM.

Details

Summary

Combines settings of SNI icons and plasmoids in one list. Ability to disable whole section is removed.

Initial idea was proposed by Nicolas Fella (D22176).

BUG: 360307
FIXED-IN: 5.18.0

Test Plan
  1. Disable/enable plasmoids
  2. Set Shown/Hidden for plasmoid
  3. Set shortcut for plasmoid
  4. Previously hidden categories are shown
  5. Show all icons and revert
  6. Set Auto/Shown/Hidden for SNI icons

Diff Detail

Repository
R120 Plasma Workspace
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 20964
Build 20982: arc lint + arc unit
kmaterka requested review of this revision.Jan 11 2020, 4:58 PM
kmaterka created this revision.
kmaterka edited the summary of this revision. (Show Details)Jan 11 2020, 5:00 PM
kmaterka added a comment.EditedJan 11 2020, 5:04 PM

This change is based on the idea from D22176.

It needs review and probably more testing. Is it visually correct?

Much better! The scrollview needs a frame around it though. You can do this by adding this to it:

Component.onCompleted: scrollview.background.visible = true;

Why this isn't the default, I have no idea...

Also for toggling individual sections, I would recommend making the button say "disable all" or "hide all" (depending on what's most accurate) and use the view-hidden icon. Then when all items in that category are disabled/hidden, disable all controls and labels for all items in that section.

Finally, if it's possible to use alternating stripey background colors instead of lines between items, that's generally preferred for multi-column table-style views.

I would just kill the per-section control. There's no point to that IMHO

Also German locale strikes again: The shortcut label (Tastatur-Kurzbefehle) is too long here

meven added a subscriber: meven.Jan 11 2020, 6:03 PM

I would just kill the per-section control. There's no point to that IMHO

I don't share this opinion.
A list with dozens of elements is a maze for users.
The categories seems quite helpful when looking for things and relieves the eye in marking breaks.
It really makes the Ui richer.

I agree that separating the list into categories makes it a lot less visually overwhelming to look at. If @nicolasfella was saying that maybe we could remove the feature that allows disabling all elements within a section at once, that I might be able to get behind. I don't really see a use for it.

I'm not against keeping the separators, but the button to en/disable the whole section seems overkill to me. Chances are that the set of entries you want to have does not exactly align with the categories anyway

Much better! The scrollview needs a frame around it though. You can do this by adding this to it:

Is it decided (D26530)? Correct me if I'm wrong, in Kirigami scrollbars are overlaying with transparent background, Kirigami just adds some paddings when needed. Anyway, to have scrollbar seprated it is better to just add one margin to the list (and remove "rightPadding" I added to header, section and list item).

Also for toggling individual sections, I would recommend making the button say "disable all" or "hide all" (depending on what's most accurate) and use the view-hidden icon. Then when all items in that category are disabled/hidden, disable all controls and labels for all items in that section.

OK, will change the icon. I will disable all items. Or I'll just remove this feature...

Finally, if it's possible to use alternating stripey background colors instead of lines between items, that's generally preferred for multi-column table-style views.

In plasmoidviewer all are white, I don't know why it is not the case in real usage. I will fix that.

Also German locale strikes again: The shortcut label (Tastatur-Kurzbefehle) is too long here

OK, I will fix that. My idea is to get max size of all items in the column and apply it to all elements (like in real table).

I'm not against keeping the separators, but the button to en/disable the whole section seems overkill to me. Chances are that the set of entries you want to have does not exactly align with the categories anyway

It is not that it is the first feature in KDE that almost no one uses :) But I lean to the idea of removing it. It really does not make much sense to hide whole category, because it applies to all apps - even those you don't know yet. Definitely, it is confusing. For me, as a develop, it is hard to remove features, it break my heart :) From the other side, I understand that is is equally important to add features and to remove useless ones. Do we have any vote against/to keep this feature?
Note to myself: to remove this, you need to remove all usage of configuration properties: cfg_*Shown.

Much better! The scrollview needs a frame around it though. You can do this by adding this to it:

Is it decided (D26530)? Correct me if I'm wrong, in Kirigami scrollbars are overlaying with transparent background, Kirigami just adds some paddings when needed. Anyway, to have scrollbar seprated it is better to just add one margin to the list (and remove "rightPadding" I added to header, section and list item).

The discussion about overlay vs non-overlay scrollbars is unrelated to whether or not to show a frame and background behind a scrollview. :) Just add the background for now I think.

The discussion about overlay vs non-overlay scrollbars is unrelated to whether or not to show a frame and background behind a scrollview. :) Just add the background for now I think.

Oh, true, my mistake. It looks much better with background, thanks!

Finally, if it's possible to use alternating stripey background colors instead of lines between items, that's generally preferred for multi-column table-style views.

It looks bad with alternating background, list items are merged with sections:

kmaterka edited the summary of this revision. (Show Details)Jan 13 2020, 10:59 AM
kmaterka updated this revision to Diff 73389.Jan 13 2020, 11:05 AM

Review fixes:
Key shortcut header width
Background
Few other small changes

kmaterka updated this revision to Diff 73395.Jan 13 2020, 12:06 PM

Workaround for Kirigami.AbstractListItem

kmaterka edited the summary of this revision. (Show Details)Jan 13 2020, 12:06 PM

Final decision: should I remove feature which allows to hide whole category?

mart added a subscriber: mart.Jan 13 2020, 1:39 PM

Final decision: should I remove feature which allows to hide whole category?

fine for me too.
the reason for it in the beginning was t just be able to say "i don't want normal apps to be able to spam my systray" tough probably things aren't categorize well enough to be able to rely on that.
if that "no normal apps such as mediaplayers" is deemed an useful feature(which i'm not sure it is) , should probably be a checkbox that says exactly that, rather than having the concept of enabling or disabling a whole category

mart added a comment.EditedJan 13 2020, 1:59 PM

on the code side, i have mainly only one small gripe

applets/systemtray/package/contents/ui/ConfigEntries.qml
103

this should never be necessary and will probably cause bugs.
It looks like you're trying to workaround some problem? (which should be rather fixed)

In D26586#593114, @mart wrote:

the reason for it in the beginning was t just be able to say "i don't want normal apps to be able to spam my systray" tough probably things aren't categorize well enough to be able to rely on that.
if that "no normal apps such as mediaplayers" is deemed an useful feature(which i'm not sure it is) , should probably be a checkbox that says exactly that, rather than having the concept of enabling or disabling a whole category

I think that makes sense, yeah.

In D26586#593114, @mart wrote:

fine for me too.

OK, I will remove this feature.

applets/systemtray/package/contents/ui/ConfigEntries.qml
103

Yes, I don't like this neither. It is a workaround for a problem in Kirigami.AbstractListItem. It automatically calculates paddings when scrollbars are detected:
kirigami/src/controls/templates/AbstractListItem.qml$144-165
It checks if view has scrollbars attached: internal.view.T2.ScrollBar.vertical.
I checked other projects (Discover), Kirigami.AbstractListItem is used inside ScrollablePage, which add scrollbars directly to the ListView (defined in custom templates/private/ScrollView.qml.

I use plain QQC2.ScrollView, which attaches scrollbars to itself, not ListView. As a result scrollbars are not detected and padding is not added.

I don't know how to fix that correctly without duplicating a code in ConfigEntries.qml. I can override leftPadding and rightPadding, but I prefer to use original logic (in case it changes in the future). Can you advice?

When I try this out, here's what I see:

There are no sections and everything's disabled.

There are no sections and everything's disabled.

Hmm, there is one small change in the C++ model (category added), maybe you need to restart/logout so that C++ library is reloaded?

kmaterka updated this revision to Diff 73414.Jan 13 2020, 2:48 PM

Removed feature that allowed to hide whole category.

kmaterka edited the summary of this revision. (Show Details)Jan 13 2020, 2:52 PM
kmaterka edited the test plan for this revision. (Show Details)

@mart @ngraham I see heated discussion in D26530. What should I do with scrollbars? I can change it to something like this:

Ah, I see it now.

The alignment for items with shortcuts is a bit odd:

Maybe for the combobox item texts, try this:

  • Shown when relevant
  • Always shown
  • Always hidden
  • Disabled

The alignment for items with shortcuts is a bit odd:

Yes, I know. Shortcuts can be really long, you can have some crazy combinations like "Meta+Ctrl+Shift+Z" which is very wide. From the other side, it should not be that bad... I will work on this, let's see what can be achieved.

Maybe for the combobox item texts, try this:

  • Shown when relevant
  • Always shown
  • Always hidden
  • Disabled

OK, I forgot about this, I will change that.

kmaterka updated this revision to Diff 73470.Jan 13 2020, 11:15 PM

Fixes for column size, it is calculated dynamically now

kmaterka updated this revision to Diff 73504.Jan 14 2020, 11:00 AM

Replaced hack with explicit padding settings

kmaterka marked 2 inline comments as done.Jan 14 2020, 11:02 AM

Anything else to change/fix? When is the code freeze, this Friday?

applets/systemtray/package/contents/ui/ConfigEntries.qml
103

I replaced this hack with explicit paddings settings

There should be no need to explicitly specify paddings now that D26530 has landed. But there's a bunch of workarounds that we still need to remove, so you may have some intermittent weird results.

kmaterka marked an inline comment as done.Jan 14 2020, 12:07 PM

There should be no need to explicitly specify paddings now that D26530 has landed. But there's a bunch of workarounds that we still need to remove, so you may have some intermittent weird results.

I saw that and hoped that it will apply automatically. I recompiled KDE (with qqc2-desktop-style), restarted but it is not working for me. Is there anything needed on my side to enable D26530?

ngraham accepted this revision.Jan 14 2020, 3:44 PM

Yep, the freeze is on Friday. I think we can get this in before then.

UI and behavior look good. Some of the code looks a bit hairy to me, but I don't see anything catastrophic. :) I'll let Plasma folks do the final code review.

This revision is now accepted and ready to land.Jan 14 2020, 3:44 PM

Some of the code looks a bit hairy to me, but I don't see anything catastrophic. :)

What exactly? I can quickly fix/refactor that.

There's a minor problem (that I've seen in other place as well). When scrolling the content overflows the frame on the top a bit:

See how the top frame is white while the side is blue

Also the text/icon sometimes overflows:

There's a minor problem (that I've seen in other place as well). When scrolling the content overflows the frame on the top a bit:

Should I use import QtQuick.Controls 2.5 as QQC2 or import org.kde.plasma.components 3.0 as PlasmaComponents is preferred? With PlasmaComponents there is no problem with overflow, but there is no frame.

Use the QQC2 ScrollView, and we should fix that elsewhere.

Is the review (by other Plasma members) done? Please let me know when can I push changes :)

mart accepted this revision.Jan 15 2020, 3:15 PM

Push it! Push it real good! :)

This revision was automatically updated to reflect the committed changes.

This change break settings for me due to currentValue in ConfigEntries.qml conflicting. It works if I rename the variable to something else

This change break settings for me due to currentValue in ConfigEntries.qml conflicting. It works if I rename the variable to something else

Are you using Qt 5.14?

This change break settings for me due to currentValue in ConfigEntries.qml conflicting. It works if I rename the variable to something else

Are you using Qt 5.14?

Yes. Btw see https://phabricator.kde.org/D26923