Create ExpandableListItem
Needs ReviewPublic

Authored by ngraham on Fri, Mar 13, 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 24297
Build 24315: arc lint + arc unit
There are a very large number of changes, so older changes are hidden. Show Older Changes
davidedmundson added inline comments.Sat, Mar 14, 4:11 AM
src/declarativeimports/plasmacomponents3/ExpandableListItem.qml
2 ↗(On Diff #77603)

It shouldn't be in PC3. It's new API from qqc2.A

23 ↗(On Diff #77603)

Unused?

153 ↗(On Diff #77603)

Why pc2?

179 ↗(On Diff #77603)

Shouldn't it run the default action?

190 ↗(On Diff #77603)

Neve rdefine an implicit size from an actual size.

230 ↗(On Diff #77603)

Seems random

235 ↗(On Diff #77603)

Sounds like you want 2 classes. One a subclass with the list as the custom view.

Then you don't need the container, or this switching.

ngraham updated this revision to Diff 77636.Sat, Mar 14, 8:32 PM

Rebase better

AFAIK, there should be a easy way to add animations to the highlight effect. An example is the print one, where clicking on it will animate the highlight. It should also be possible to make the highlight effect fly over elements instead of always being on the hovered element. Not a must for this patch, but it could be nice to have.

davidedmundson requested changes to this revision.Sun, Mar 15, 6:23 PM
This revision now requires changes to proceed.Sun, Mar 15, 6:23 PM
ngraham updated this revision to Diff 77686.Sun, Mar 15, 11:44 PM
ngraham marked 4 inline comments as done.
  • Address review comments
  • Improve inline comments for clarify
  • Use a pointing hand cursor when hovering over the item to indicate that it's clickable
  • Make subtitle wrapping optional
  • Substantially simplify the expanded view handling
ngraham added inline comments.Sun, Mar 15, 11:44 PM
src/declarativeimports/plasmacomponents3/ExpandableListItem.qml
2 ↗(On Diff #77603)

Where should it live? PC2? Kirigami?

23 ↗(On Diff #77603)

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

179 ↗(On Diff #77603)

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

230 ↗(On Diff #77603)

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.Sun, Mar 15, 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.Sun, Mar 15, 11:49 PM

Fix one thing

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

Add TODOs

ngraham updated this revision to Diff 77695.Mon, Mar 16, 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.Mon, Mar 16, 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.Mon, Mar 16, 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.Wed, Mar 18, 3:12 PM
src/declarativeimports/plasmacomponents3/ExpandableListItem.qml
2 ↗(On Diff #77603)

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

23 ↗(On Diff #77603)

I meant this one

import org.kde.plasma.plasmoid 2.0

having that in library code is surprising.

230 ↗(On Diff #77603)

I meant the duration

mart added a subscriber: mart.Wed, Mar 18, 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.Wed, Mar 18, 4:15 PM
ngraham marked 4 inline comments as done.

Move to PlasmaExtraComponents

Will beef up the docs next

src/declarativeimports/plasmacomponents3/ExpandableListItem.qml
230 ↗(On Diff #77603)

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.Wed, Mar 18, 4:25 PM
src/declarativeimports/plasmacomponents3/ExpandableListItem.qml
230 ↗(On Diff #77603)

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.Wed, Mar 18, 5:34 PM

Add API docs

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

Fix a whitespace thingy

ngraham edited the test plan for this revision. (Show Details)Wed, Mar 18, 11:32 PM
mart added a comment.Thu, Mar 19, 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.Thu, Mar 19, 9:34 AM
src/declarativeimports/plasmacomponents3/ExpandableListItem.qml
230 ↗(On Diff #77603)

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.EditedThu, Mar 19, 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)Thu, Mar 19, 3:07 PM
mart added a comment.Thu, Mar 19, 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.Fri, Mar 20, 3:49 PM

Use the new veryLongDuration duration instead a multiple of longDuration

ngraham marked 4 inline comments as done.Fri, Mar 20, 3:52 PM
ngraham updated this revision to Diff 78177.Sat, Mar 21, 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.Mon, Mar 23, 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.Thu, Mar 26, 9:15 PM

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