D28033: Create ExpandableListItem

ratijas added inline comments. INLINE COMMENTS > ngraham wrote in ExpandableListItem.qml:269 > It was nothing more than a bad API design decision on my part. Luckily, it is easily fixable, with an alias for backward compatibility. However, I don't know how to enforce deprecation warnings on

D28033: Create ExpandableListItem

ngraham added inline comments. INLINE COMMENTS > ratijas wrote in ExpandableListItem.qml:269 > 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. It was nothing more than a bad API

D28033: Create ExpandableListItem

ratijas added inline comments. INLINE COMMENTS > ExpandableListItem.qml:269 > + */ > +property bool isEnabled: true > + 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.

D28033: Create ExpandableListItem

meven added a comment. Great consistency work ! CC @niccolove REPOSITORY R242 Plasma Framework (Library) REVISION DETAIL https://phabricator.kde.org/D28033 To: ngraham, #vdg, #plasma, davidedmundson Cc: meven, dfaure, mart, davidedmundson, bruns, niccolove, cblack, davidre,

D28033: Create ExpandableListItem

ngraham added a comment. Fixed in 96d203b4b86e829fa958a8aa2ac42c8f5e1a4d31 . REPOSITORY R242 Plasma Framework (Library) REVISION DETAIL https://phabricator.kde.org/D28033 To: ngraham, #vdg, #plasma,

D28033: Create ExpandableListItem

ngraham added a comment. Oops, sorry. Will fix. REPOSITORY R242 Plasma Framework (Library) REVISION DETAIL https://phabricator.kde.org/D28033 To: ngraham, #vdg, #plasma, davidedmundson Cc: dfaure, mart, davidedmundson, bruns, niccolove, cblack, davidre, kde-frameworks-devel, LeGast00n,

D28033: Create ExpandableListItem

dfaure added a comment. This commit broke the i18ndcheck unittest. https://build.kde.org/job/Frameworks/view/Platform%20-%20SUSEQt5.14/job/plasma-framework/job/kf5-qt5%20SUSEQt5.14/9/testReport/junit/projectroot/autotests/i18ndcheck/ Reproducible locally. Works before and breaks

D28033: Create ExpandableListItem

This revision was automatically updated to reflect the committed changes. Closed by commit R242:3bd31386abbf: Create ExpandableListItem (authored by ngraham). REPOSITORY R242 Plasma Framework (Library) CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D28033?vs=79366=79367 REVISION

D28033: Create ExpandableListItem

ngraham updated this revision to Diff 79366. ngraham marked an inline comment as done. ngraham added a comment. height -> contentHeight REPOSITORY R242 Plasma Framework (Library) CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D28033?vs=78593=79366 BRANCH ExpandableListItem

D28033: Create ExpandableListItem

davidedmundson accepted this revision. davidedmundson added a comment. This revision is now accepted and ready to land. Tagging is tomorrow, don't merge before then. INLINE COMMENTS > ExpandableListItem.qml:491 > +Item { > +height: actionsList.height > +width:

D28033: Create ExpandableListItem

ngraham added a dependent revision: D28321: [WIP] [applets/devicenotifier] Port to ExpandableListItem. REPOSITORY R242 Plasma Framework (Library) REVISION DETAIL https://phabricator.kde.org/D28033 To: ngraham, #vdg, #plasma, davidedmundson Cc: mart, davidedmundson, bruns, niccolove,

D28033: Create ExpandableListItem

ngraham added a comment. Ready for re-review. @davidedmundson REPOSITORY R242 Plasma Framework (Library) REVISION DETAIL https://phabricator.kde.org/D28033 To: ngraham, #vdg, #plasma, davidedmundson Cc: mart, davidedmundson, bruns, niccolove, cblack, davidre, kde-frameworks-devel,

D28033: Create ExpandableListItem

ngraham updated this revision to Diff 78593. ngraham added a comment. Add new properties `defaultActionButtonVisible` and `subtitleColor`, used in the device notifier REPOSITORY R242 Plasma Framework (Library) CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D28033?vs=78324=78593

D28033: Create ExpandableListItem

ngraham updated this revision to Diff 78324. ngraham added a comment. Rebase better (one of these days I'll remember to do it right the first time around...) REPOSITORY R242 Plasma Framework (Library) CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D28033?vs=78323=78324 BRANCH

D28033: Create ExpandableListItem

ngraham updated this revision to Diff 78323. ngraham added a comment. Rebase REPOSITORY R242 Plasma Framework (Library) CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D28033?vs=78177=78323 BRANCH ExpandableListItem (branched from master) REVISION DETAIL

D28033: Create ExpandableListItem

ngraham added a comment. 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

D28033: Create ExpandableListItem

ngraham updated this revision to Diff 78177. ngraham added a comment. Fix documentation formatting REPOSITORY R242 Plasma Framework (Library) CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D28033?vs=78104=78177 BRANCH ExpandableListItem (branched from master) REVISION DETAIL

D28033: Create ExpandableListItem

ngraham marked 4 inline comments as done. REPOSITORY R242 Plasma Framework (Library) REVISION DETAIL https://phabricator.kde.org/D28033 To: ngraham, #vdg, #plasma, davidedmundson Cc: mart, davidedmundson, bruns, niccolove, cblack, davidre, kde-frameworks-devel, LeGast00n, GB_2, michaelh,

D28033: Create ExpandableListItem

ngraham updated this revision to Diff 78104. ngraham added a comment. Use the new `veryLongDuration` duration instead a multiple of `longDuration` REPOSITORY R242 Plasma Framework (Library) CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D28033?vs=78103=78104 BRANCH

D28033: Create ExpandableListItem

ngraham updated this revision to Diff 78103. ngraham added a comment. Rebase REPOSITORY R242 Plasma Framework (Library) CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D28033?vs=77947=78103 BRANCH ExpandableListItem (branched from master) REVISION DETAIL

D28033: Create ExpandableListItem

ngraham edited the summary of this revision. ngraham added a dependency: D28144: Make animation durations consistent with Kirigami values. REPOSITORY R242 Plasma Framework (Library) REVISION DETAIL https://phabricator.kde.org/D28033 To: ngraham, #vdg, #plasma, davidedmundson Cc: mart,

D28033: Create ExpandableListItem

ngraham added a comment. D28143: Add "veryLongDuration" animation duration D28144: Make animation durations consistent with Kirigami values REPOSITORY R242 Plasma Framework (Library) REVISION DETAIL

D28033: Create ExpandableListItem

ngraham added a comment. 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. REPOSITORY R242 Plasma Framework (Library) REVISION DETAIL https://phabricator.kde.org/D28033 To:

D28033: Create ExpandableListItem

mart added a comment. In D28033#630450 , @ngraham wrote: > > shortDuration: 150ms > longDuration: 250ms > veryLongDuration: 500ms veryLongDuration doesn't sound too good, but if longduration goes 500ms probably a lot of

D28033: Create ExpandableListItem

ngraham edited the summary of this revision. REPOSITORY R242 Plasma Framework (Library) REVISION DETAIL https://phabricator.kde.org/D28033 To: ngraham, #vdg, #plasma, davidedmundson Cc: mart, davidedmundson, bruns, niccolove, cblack, davidre, kde-frameworks-devel, LeGast00n, GB_2,

D28033: Create ExpandableListItem

ngraham added a comment. I agree 150ms for shortDuration is much more sensible; the current speed it 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

D28033: Create ExpandableListItem

mart added inline comments. INLINE COMMENTS > davidedmundson wrote in ExpandableListItem.qml:230 > If it's multiplied by a factor it's not a "standard duration" anymore. to me this tells more that - default durations right now are waaay too short - we don't take into account that for movements

D28033: Create ExpandableListItem

mart added a comment. In D28033#630073 , @ngraham wrote: > (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

D28033: Create ExpandableListItem

ngraham edited the test plan for this revision. REPOSITORY R242 Plasma Framework (Library) REVISION DETAIL https://phabricator.kde.org/D28033 To: ngraham, #vdg, #plasma, davidedmundson Cc: mart, davidedmundson, bruns, niccolove, cblack, davidre, kde-frameworks-devel, LeGast00n, GB_2,

D28033: Create ExpandableListItem

ngraham updated this revision to Diff 77947. ngraham added a comment. Fix a whitespace thingy REPOSITORY R242 Plasma Framework (Library) CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D28033?vs=77946=77947 BRANCH ExpandableListItem (branched from master) REVISION DETAIL

D28033: Create ExpandableListItem

ngraham updated this revision to Diff 77946. ngraham added a comment. Add API docs REPOSITORY R242 Plasma Framework (Library) CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D28033?vs=77935=77946 BRANCH ExpandableListItem (branched from master) REVISION DETAIL

D28033: Create ExpandableListItem

ngraham added a comment. (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

D28033: Create ExpandableListItem

davidedmundson added inline comments. INLINE COMMENTS > ngraham wrote in ExpandableListItem.qml:230 > Not sure what you mean by "random." It's a multiple of a standard duration > that I chose to look good (IMO). If it's multiplied by a factor it's not a "standard duration" anymore. REPOSITORY

D28033: Create ExpandableListItem

ngraham added a comment. Will beef up the docs next INLINE COMMENTS > davidedmundson wrote in ExpandableListItem.qml:230 > I meant the duration Not sure what you mean by "random." It's a multiple of a standard duration that I chose to look good (IMO). REPOSITORY R242 Plasma Framework

D28033: Create ExpandableListItem

ngraham updated this revision to Diff 77935. ngraham marked 4 inline comments as done. ngraham added a comment. Move to PlasmaExtraComponents REPOSITORY R242 Plasma Framework (Library) CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D28033?vs=77702=77935 BRANCH

D28033: Create ExpandableListItem

mart added a comment. 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) REPOSITORY R242 Plasma Framework (Library) REVISION DETAIL

D28033: Create ExpandableListItem

davidedmundson added inline comments. INLINE COMMENTS > ngraham wrote in ExpandableListItem.qml:2 > Where should it live? PC2? Kirigami? 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

D28033: Create ExpandableListItem

ngraham updated this revision to Diff 77702. ngraham added a comment. 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 REPOSITORY R242 Plasma Framework

D28033: Create ExpandableListItem

ngraham updated this revision to Diff 77701. ngraham added a comment. - 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 REPOSITORY R242 Plasma Framework (Library)

D28033: Create ExpandableListItem

ngraham updated this revision to Diff 77695. ngraham added a comment. Use `StyledText` instead of `RichText` because `RichText` doesn't support eliding (see https://bugreports.qt.io/browse/QTBUG-16567) REPOSITORY R242 Plasma Framework (Library) CHANGES SINCE LAST UPDATE

D28033: Create ExpandableListItem

ngraham updated this revision to Diff 77691. ngraham added a comment. Add TODOs REPOSITORY R242 Plasma Framework (Library) CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D28033?vs=77688=77691 BRANCH ExpandableListItem (branched from master) REVISION DETAIL

D28033: Create ExpandableListItem

ngraham updated this revision to Diff 77687. ngraham added a comment. Rebase REPOSITORY R242 Plasma Framework (Library) CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D28033?vs=77686=77687 BRANCH ExpandableListItem (branched from master) REVISION DETAIL

D28033: Create ExpandableListItem

ngraham updated this revision to Diff 77688. ngraham added a comment. Fix one thing REPOSITORY R242 Plasma Framework (Library) CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D28033?vs=77687=77688 BRANCH ExpandableListItem (branched from master) REVISION DETAIL

D28033: Create ExpandableListItem

ngraham marked 5 inline comments as done. ngraham added a comment. 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:

D28033: Create ExpandableListItem

ngraham added inline comments. INLINE COMMENTS > davidedmundson wrote in ExpandableListItem.qml:2 > It shouldn't be in PC3. It's new API from qqc2.A Where should it live? PC2? Kirigami? > davidedmundson wrote in ExpandableListItem.qml:23 > Unused? Not unused, it's needed for `Highlight`,

D28033: Create ExpandableListItem

ngraham updated this revision to Diff 77686. ngraham marked 4 inline comments as done. ngraham added a comment. - 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

D28033: Create ExpandableListItem

davidedmundson requested changes to this revision. This revision now requires changes to proceed. REPOSITORY R242 Plasma Framework (Library) REVISION DETAIL https://phabricator.kde.org/D28033 To: ngraham, #vdg, #plasma, davidedmundson Cc: davidedmundson, bruns, niccolove, cblack, davidre,

D28033: Create ExpandableListItem

niccolove added a comment. 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

D28033: Create ExpandableListItem

ngraham updated this revision to Diff 77636. ngraham added a comment. Rebase better REPOSITORY R242 Plasma Framework (Library) CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D28033?vs=77635=77636 BRANCH ExpandableListItem (branched from master) REVISION DETAIL

D28033: Create ExpandableListItem

ngraham updated this revision to Diff 77635. ngraham added a comment. Rebase REPOSITORY R242 Plasma Framework (Library) CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D28033?vs=77603=77635 BRANCH ExpandableListItem (branched from master) REVISION DETAIL

D28033: Create ExpandableListItem

davidedmundson added inline comments. INLINE COMMENTS > ExpandableListItem.qml:2 > +/* > + * Copyright 2020 Nate Graham > + * It shouldn't be in PC3. It's new API from qqc2.A > ExpandableListItem.qml:23 > +import org.kde.plasma.core 2.0 as PlasmaCore > +import org.kde.plasma.plasmoid 2.0 >

D28033: Create ExpandableListItem

ngraham updated this revision to Diff 77603. ngraham added a comment. - Add an opacity animation when expanded view is shown - Add a button on the right to expand and collapse the expanded view - Clean and simplify expand()/collapse() code a bit REPOSITORY R242 Plasma Framework

D28033: Create ExpandableListItem

ngraham added a comment. If it were a flat button, I'd want to make the default action button flat too, or else they wouldn't match. Here's how that would look: F8174983: Toolbuttons.png And here's how it would look with both of them as non-flat

D28033: Create ExpandableListItem

ngraham edited the test plan for this revision. REPOSITORY R242 Plasma Framework (Library) REVISION DETAIL https://phabricator.kde.org/D28033 To: ngraham, #vdg, #plasma Cc: bruns, niccolove, cblack, davidre, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham

D28033: Create ExpandableListItem

davidre added a comment. In D28033#627192 , @ngraham wrote: > In D28033#627184 , @cblack wrote: > > > In D28033#627180 , @davidre wrote: > > > > > I

D28033: Create ExpandableListItem

ngraham added a comment. In D28033#627184 , @cblack wrote: > In D28033#627180 , @davidre wrote: > > > I would love if we could find a way to show that the item is expandable > > > I would

D28033: Create ExpandableListItem

ngraham added a subscriber: bruns. ngraham added a comment. Discussion points: - Is this even the right paradigm to use for extra actions? Or should we instead use a SwipeListItem and put the extra actions in an overflow menu, and use the expanded view only for custom content? -

D28033: Create ExpandableListItem

niccolove added a comment. Strongly agree on having a single component. By the way, all these lists have the issue that the button only appearing on hover is very touch-unfriendly. This does not have to be addressed in this patch, but it having it implemented in only one place makes it much

D28033: Create ExpandableListItem

cblack added a comment. In D28033#627180 , @davidre wrote: > I would love if we could find a way to show that the item is expandable I would probably use a pointing down arrow without a stem when collapsed, and pointing up when open.

D28033: Create ExpandableListItem

davidre added a comment. I would love if we could find a way to show that the item is expandable REPOSITORY R242 Plasma Framework (Library) REVISION DETAIL https://phabricator.kde.org/D28033 To: ngraham, #vdg, #plasma Cc: davidre, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh,

D28033: Create ExpandableListItem

ngraham added a dependent revision: D28037: [Applet] Port to ExpandableListItem. REPOSITORY R242 Plasma Framework (Library) REVISION DETAIL https://phabricator.kde.org/D28033 To: ngraham, #vdg, #plasma Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D28033: Create ExpandableListItem

ngraham added a dependent revision: D28035: [Applet] Port to ExpandableListItem. REPOSITORY R242 Plasma Framework (Library) REVISION DETAIL https://phabricator.kde.org/D28033 To: ngraham, #vdg, #plasma Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D28033: Create ExpandableListItem

ngraham added a dependent revision: D28034: [WIP] [Applet] Port to ExpandableListItem. REPOSITORY R242 Plasma Framework (Library) REVISION DETAIL https://phabricator.kde.org/D28033 To: ngraham, #vdg, #plasma Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D28033: Create ExpandableListItem

ngraham added a dependent revision: D28036: [Applet] Port to ExpandableListItem. REPOSITORY R242 Plasma Framework (Library) REVISION DETAIL https://phabricator.kde.org/D28033 To: ngraham, #vdg, #plasma Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D28033: Create ExpandableListItem

ngraham created this revision. ngraham added reviewers: VDG, Plasma. Herald added a project: Frameworks. Herald added a subscriber: kde-frameworks-devel. ngraham requested review of this revision. REVISION SUMMARY This patch creates the `ExpandableListItem`, a re-usable PlasmaComponents3 item