Create ExpandableListItem
ClosedPublic

Authored by ngraham on Mar 13 2020, 5:26 PM.

Details

Summary

This patch creates the ExpandableListItem, a re-usable PlasmaExtras item that can
be used for the views in various Plasma applet pop-ups, such as Vaults, Printers,
Bluetooth, Networks, and Device Notifier. This way those applets can share more code and
not have to implement this paradigm themselves in five different ways, as they currently
do.

All of these applets currently use slightly different visual styles. For example the
network applet uses a pushbutton with no icon as its "default action" button, while
other applets use icons-only toolbuttons. I tried my best to create a component that's
flexible but also consistent, so various applets that adopt it will see minor visual
changes as a result. Hopefully this is acceptable.

Closes T12812

Depends on D28144

Test Plan

Diff Detail

Repository
R242 Plasma Framework (Library)
Branch
ExpandableListItem (branched from master)
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 23729
Build 23747: arc lint + arc unit
There are a very large number of changes, so older changes are hidden. Show Older Changes
ngraham added inline comments.Mar 15 2020, 11:44 PM
src/declarativeimports/plasmacomponents3/ExpandableListItem.qml
3

Where should it live? PC2? Kirigami?

24

Not unused, it's needed for Highlight, which has no PC3 version.

180

It already does by virtue of having defaultActionButtonAction being an alias to defaultActionButton.action; this just adds an additional behavior on top of that. I'll add a comment to make this clearer

231

Not random; this is the easing curve we agreed to and is now in the HIG: https://hig.kde.org/style/animations.html#guidelines

ngraham marked 5 inline comments as done.Mar 15 2020, 11:47 PM

By switching to the PC3 BusyIndicator, I'm now seeing a binding loop that doesn't make sense to me:

file:///home/nate/kde/usr/lib64/qml/org/kde/plasma/components.3/ExpandableListItem.qml:191:13: QML BusyIndicator: Binding loop detected for property "implicitHeight"

I'm not touching the implicitWidth of the item here, so it seems like it could be inherent to the control itself.

ngraham updated this revision to Diff 77688.Mar 15 2020, 11:49 PM

Fix one thing

ngraham updated this revision to Diff 77691.Mar 16 2020, 12:14 AM

Add TODOs

ngraham updated this revision to Diff 77695.Mar 16 2020, 3:16 AM

Use StyledText instead of RichText because RichText doesn't support eliding (see https://bugreports.qt.io/browse/QTBUG-16567)

ngraham updated this revision to Diff 77701.Mar 16 2020, 3:37 AM
  • Create a re-usable public toggleExpanded() function and use it internally
  • Make the busy indicator a bit smaller so it doesn't make the list item taller when it appears
ngraham updated this revision to Diff 77702.Mar 16 2020, 3:45 AM

Don't auto-collapse when the default action button is clicked; this can interfere with actions where you want the button to show a custom expanded view, as in the Networks widget

davidedmundson added inline comments.Mar 18 2020, 3:12 PM
src/declarativeimports/plasmacomponents3/ExpandableListItem.qml
3

Definitely can't be in PC2.

Kirigami is doable, but it'd require some code changes to not use any Plasma.* imports and go through relevant abstractions

Failing that plasmaextracomponents

24

I meant this one

import org.kde.plasma.plasmoid 2.0

having that in library code is surprising.

231

I meant the duration

mart added a subscriber: mart.Mar 18 2020, 4:12 PM

to me it should be in plasma extracomponents

(and with a warning in the docs that such an item should be used only in lists that are known to have always very few items)

ngraham updated this revision to Diff 77935.Mar 18 2020, 4:15 PM
ngraham marked 4 inline comments as done.

Move to PlasmaExtraComponents

Will beef up the docs next

src/declarativeimports/plasmacomponents3/ExpandableListItem.qml
231

Not sure what you mean by "random." It's a multiple of a standard duration that I chose to look good (IMO).

davidedmundson added inline comments.Mar 18 2020, 4:25 PM
src/declarativeimports/plasmacomponents3/ExpandableListItem.qml
231

If it's multiplied by a factor it's not a "standard duration" anymore.

(WRT duration) What should I do then? Hardcode something? Add a new duration in a separate patch? The only standard durations we have are all quite short; neither one feels suitable for the animation I'm using here.

Also we multiply distance units by factors all the time so I don't understand why it's not acceptable to do so for a duration.

ngraham updated this revision to Diff 77946.Mar 18 2020, 5:34 PM

Add API docs

ngraham updated this revision to Diff 77947.Mar 18 2020, 5:34 PM

Fix a whitespace thingy

ngraham edited the test plan for this revision. (Show Details)Mar 18 2020, 11:32 PM
mart added a comment.Mar 19 2020, 9:33 AM

(WRT duration) What should I do then? Hardcode something? Add a new duration in a separate patch? The only standard durations we have are all quite short; neither one feels suitable for the animation I'm using here.

to me the real problem is really the default settings for durations longDuration became 150ms or so, (shortduration is something so short that usually the animation frames just gets dropped altogether so it's pointless to even use it ever)
I know this was done in the attempt of improving animations, but the end effect is ridiculous and makes animations actually looking worse, either comically fast (super fast fades are ok, item movements or resizes shouldn't actually be too fast, or be perceived as threatening) or no frames rendered at all even if there is code for it
the original default values of 150 and 250 msecs for short and long were quite fine actually

also, we should probably start into looking using SmoothedAnimation speed more rather than durations (to not fix travel time, but travel speed) (as animators don't support it and we would need something that reacts to global animations settings probably we would need our implementation of the speed concept)

https://valhead.com/2016/05/05/how-fast-should-your-ui-animations-be/
https://www.nngroup.com/articles/too-fast-ux/
https://uxdesign.cc/the-ultimate-guide-to-proper-use-of-animation-in-ux-10bd98614fa9

mart added inline comments.Mar 19 2020, 9:34 AM
src/declarativeimports/plasmacomponents3/ExpandableListItem.qml
231

to me this tells more that

  • default durations right now are waaay too short
  • we don't take into account that for movements and resizes, the actual duration should be a function of the travel distance, rather than a fixed duration
ngraham added a comment.EditedMar 19 2020, 1:20 PM

I agree 150ms for shortDuration is much more sensible; the current speed is too short to even notice that something is animating. I think we need more durations though. My proposal would be something like this:

shortDuration: 150ms
mediumDuration: 250ms
longDuration: 500ms

Or, for compatibility with existing types:

shortDuration: 150ms
longDuration: 250ms
veryLongDuration: 500ms

ngraham edited the summary of this revision. (Show Details)Mar 19 2020, 3:07 PM
mart added a comment.Mar 19 2020, 4:49 PM

shortDuration: 150ms
longDuration: 250ms
veryLongDuration: 500ms

veryLongDuration doesn't sound too good, but if longduration goes 500ms probably a lot of existing things are going to look way too slow so +1 for compatibility

In fact it looks like Kirigami already uses 150 for short and 250 for long, so there's an argument for making Plasma consistent with that. Will submit patches.

ngraham updated this revision to Diff 78104.Mar 20 2020, 3:49 PM

Use the new veryLongDuration duration instead a multiple of longDuration

ngraham marked 4 inline comments as done.Mar 20 2020, 3:52 PM
ngraham updated this revision to Diff 78177.Mar 21 2020, 4:37 PM

Fix documentation formatting

Idea: instead of allowing a context menu to be defined, we require that the contextual actions always be defined as actions and appear in the expanded view. Then, we make the expanded view contain a tab view and put the contextual actions into one tab, hiding the tab bar when there's only one tab, and allowing additional tabs to be defined. With this, you wouldn't define a whole separate custom expanded view; you would define additional tabs to put in the tabbed view. This would allow us to remove the context menu to make the contextual actions discoverable and touch-friendly even in more complex delegates where you want contextual actions as well as some kind of custom view.

Thoughts?

ngraham updated this revision to Diff 78324.Mar 23 2020, 11:22 PM

Rebase better (one of these days I'll remember to do it right the first time around...)

ngraham updated this revision to Diff 78593.Mar 26 2020, 9:15 PM

Add new properties defaultActionButtonVisible and subtitleColor, used in the device notifier

davidedmundson accepted this revision.Apr 3 2020, 12:19 PM

Tagging is tomorrow, don't merge before then.

src/declarativeimports/plasmaextracomponents/qml/ExpandableListItem.qml
491 ↗(On Diff #78593)

not contentHeight?

This revision is now accepted and ready to land.Apr 3 2020, 12:19 PM
ngraham updated this revision to Diff 79366.Apr 5 2020, 2:52 AM
ngraham marked an inline comment as done.

height -> contentHeight

This revision was automatically updated to reflect the committed changes.

Oops, sorry. Will fix.

meven added a subscriber: meven.May 10 2020, 6:36 PM

Great consistency work !

CC @niccolove

ratijas added a subscriber: ratijas.Feb 2 2022, 7:47 PM
ratijas added inline comments.
src/declarativeimports/plasmaextracomponents/qml/ExpandableListItem.qml
269 ↗(On Diff #79367)

Why simply Item::enabled wasn't enough? Is there a real use-case that justifies an extra property with overlapping name? Should be documented if there is one.

ngraham added inline comments.Feb 2 2022, 8:00 PM
src/declarativeimports/plasmaextracomponents/qml/ExpandableListItem.qml
269 ↗(On Diff #79367)

It was nothing more than a bad API design decision on my part.

ratijas added inline comments.Feb 2 2022, 8:05 PM
src/declarativeimports/plasmaextracomponents/qml/ExpandableListItem.qml
269 ↗(On Diff #79367)

Luckily, it is easily fixable, with an alias for backward compatibility. However, I don't know how to enforce deprecation warnings on aliases. It might be possible in Qt/QML 6 though.