Overhaul group popup dialog
ClosedPublic

Authored by hein on Jan 20 2017, 8:46 PM.

Details

Summary

A set of UI improvements to the group popup dialog:

  • The dialog was height-limited but not scrollable. It now shows a vertical scrollbar if needed.
  • The existing keyboard handling is extended to scroll any item navigated to by keyboard into view.
  • At opening time (before the dialog causes a change in window focus) the currently-active task (if any) is collected, and then made the active item, scrolling it into view if needed. Keyboard nav will then start there. This improves initial focus handling.
  • The normal wheel-handling is short-circuited in favor of scrolling when the dialog overflows.

BUG:375196

Diff Detail

Repository
R119 Plasma Desktop
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
hein updated this revision to Diff 10398.Jan 20 2017, 8:46 PM
hein retitled this revision from to Overhaul group popup dialog.
hein updated this object.
hein edited the test plan for this revision. (Show Details)
hein added a reviewer: Plasma.
hein added a subscriber: plasma-devel.
Restricted Application added a project: Plasma. · View Herald TranscriptJan 20 2017, 8:46 PM
hein added a comment.Jan 20 2017, 8:56 PM

Note: I think I need to make collecting the active task more robust still. Feel free to review, but I will probably update after the weekend.

hein updated this revision to Diff 10411.Jan 21 2017, 3:32 PM

Make active task pre-collection reliable.

broulik added inline comments.
applets/taskmanager/package/contents/ui/ContextMenu.qml
537

Unrelated

applets/taskmanager/package/contents/ui/GroupDialog.qml
63

Remove

119–134

Why this change?

125

FTR: Qt 5.9 will add a "positioningComplete" signal :)

161

Can you perhaps have the task do that automatically since fwict you always do forceActiveFocus and then ensureItemVisible:

onActiveFocusChanged: {
    if (activeFocus) {
        ensureItemVisible(this);
    }
}
250

?

256

Why not assign it directly above?

property TextMetrics textMetrics: TextMetrics {}
hein marked 7 inline comments as done.Jan 23 2017, 2:09 PM

Will update once I have a Qt again.

applets/taskmanager/package/contents/ui/ContextMenu.qml
537

Will do.

applets/taskmanager/package/contents/ui/GroupDialog.qml
63

Will do.

119–134

I was doing heavy refactoring and changing the nesting and trying different things a few times, happened along the way. I'll clean it out of the diff.

125

Nifty, about time. That also means we can remove some fugly code in main.qml for the layouting and delegate geo publishs.

161

I see the appeal, but I'd rather not do it implicitly and calling GroupDialog API ... I'm rather unhappy with how much coupling there is between Task and GroupDialog already (writing a delegate that relies on API outside of it is always pretty ugly). I'll think about whether I can find some other nice solution.

250

Doing it declaratively creates a binding loop in this case.

256

I think this wasn't possible in older Qt Quicks? Nice that it's possible now, will do.

broulik added inline comments.Jan 23 2017, 4:45 PM
applets/taskmanager/package/contents/ui/GroupDialog.qml
250

I don't get how assigning a number as a child works. This line makes no sense to me.

256

I think this has always been possible :) definitely for years.

What doesn't (until Qt 5.8 which fixes this) is having name spaced declarations, ie. property PlasmaCore.Foo bar will choke on the dot.

hein marked 7 inline comments as done.Jan 23 2017, 9:09 PM
hein added inline comments.
applets/taskmanager/package/contents/ui/GroupDialog.qml
250

Actually, to me it doesn't, either ;). I was asleep at the wheel/rushed earlier, will remove. Thanks for being persistent.

hein updated this revision to Diff 10468.Jan 23 2017, 9:40 PM

Various cleanups suggested by Kai.

mart added a subscriber: mart.Jan 25 2017, 11:45 AM
mart added inline comments.
applets/taskmanager/package/contents/ui/GroupDialog.qml
91

is this timer working around some bug? should have at least some comments explaining why this needs to be delayed

hein added a comment.Jan 25 2017, 2:09 PM

There's extensive comments explaining it in the patch actually ...?

hein updated this revision to Diff 10552.Jan 25 2017, 4:08 PM

Collect active task on hover but invalidate when child count changes.

This should be reasonably safe-ish with regard to the stored task
index still being valid at click time.

hein added a comment.Jan 25 2017, 4:09 PM

Also added a more localized comment on Marco's request.

hein updated this revision to Diff 10555.Jan 25 2017, 5:04 PM

Bite the bullet and do the active task collection the brute force way.

broulik accepted this revision.Jan 25 2017, 5:35 PM
broulik added a reviewer: broulik.
This revision is now accepted and ready to land.Jan 25 2017, 5:35 PM
This revision was automatically updated to reflect the committed changes.