Rewrite Window List plasmoid.
ClosedPublic

Authored by hein on Jul 4 2016, 6:26 PM.

Details

Summary
  • Ported to libtaskmanager-ng, dropping dependency on legacytaskmanager
  • Made virtual desktop sectioning in the listview actually work
    • Bonus: Don't section when there is only one virtual desktop
  • Made Unclutter/Cascade window actions actually work, and fix the wonky way they were shown in the UI
    • Code added to KWindowSystem QML plugin
  • Implemented full keyboard navigation: enter/return, arrows and tab/backtab
  • Added popup window pin
  • Added basic Accessibility support
  • Made UI sizes and margins consistent with Kicker and Folder View listviews

Depends on https://git.reviewboard.kde.org/r/128362/

Diff Detail

Repository
R119 Plasma Desktop
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
hein updated this revision to Diff 4945.Jul 4 2016, 6:26 PM
hein retitled this revision from to Rewrite Window List plasmoid..
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 TranscriptJul 4 2016, 6:26 PM
hein updated this revision to Diff 4946.Jul 4 2016, 6:39 PM

Adjust to changed API in RB128362.

broulik added a subscriber: broulik.Jul 4 2016, 6:44 PM
broulik added inline comments.
applets/window-list/contents/ui/main.qml
94

Does this ever happen? focus != activeFocus

113

Doesn't ListView take care of highlight placement and visibility?

121

Unused id

142

Just make this the MouseArea, saves you one Item per delegate

157

This is the default

186

Layout.preferredWidth? I've seen wonkiness with IconItem and Layouts as it has an implicitWidth/height and that has priority over width/height as far as Layout is concerned

199

Use RowLayout instead of Row and then Layout.fillWidth?

211

There's a KeyNavigation attached property where you can tell it which the next tab/backtab/up/down item is

218

event.accepted twice, inside and outside the if

309

QtQuick Controls Button from which PlasmaComponents Button inherits automatically handles Accessible automatically

388

Is that API even supported still? We have Plasmoid.toolTipMainText, Plasmoid.toolTipSubText etc

hein marked 10 inline comments as done.Jul 4 2016, 7:03 PM

I'm doing some other tweaks and then updating with the fixes.

applets/window-list/contents/ui/main.qml
94

Yes. ScrollArea is a FocusScope. The ListView never loses focus, the ScrollArea does.

113

Removed and replaced with highlightMoveDuration.

121

Dropped.

142

Changed.

157

I prefer being explicit, but dropped.

186

Done.

199

Done.

211

I know. KeyNavigation never really works well for me, and has issues when you try to combine it with Keys.* as you usually have to when working with the item views. I prefer just stickig to Keys for consistency. It's more or less the same code anyways.

218

Thanks, copy and paste accident. Now I can actually remove the onUpPressed/onDownPressed again :-).

309

Dropped.

388

Well, it worked ... I kept that part from the old code. I changed it now though.

hein marked 8 inline comments as done.Jul 4 2016, 7:10 PM
hein added inline comments.
applets/window-list/contents/ui/main.qml
199

Actually not done; I need to manage this for other reasons.

hein updated this revision to Diff 4947.Jul 4 2016, 7:22 PM
  • Fix Kai's issues.
  • Fix delegate right margin when scrolling.
  • When scrolling, move window pin to avoid clash with scrollbar.
  • Elide delegate labels intersecting window pin early.
hein added a comment.Jul 4 2016, 7:25 PM

Hang on -- the RowLayout doesn't work right, I need to touch the label stuff again.

hein updated this revision to Diff 4948.Jul 4 2016, 7:28 PM

Drop RowLayout use again.

Row is fine; Kicker and other UIs do it like this and it works.

hein updated this revision to Diff 4962.Jul 5 2016, 2:17 PM

Drop the Unclutter/Cascade buttons.

Moving the existing code implementing those actions from the tasks
data engine to kdeclarative didn't get through review and this is
the most I have time for.

This revision was automatically updated to reflect the committed changes.
hein reopened this revision.Jul 10 2016, 8:25 AM

Sorry, I pushed this accidentally. Reopening.

hein added a comment.Jul 10 2016, 8:26 AM

Reverted - please continue reviewing so this can actually go in ...

davidedmundson added inline comments.
applets/window-list/contents/ui/main.qml
241–242

You're using the margins from this frame, but you're not even rendering this frame anywhere?
Using the margins from something we're not rendering doesn't make really make sense semantically.

Then later we later use the left/right margins from the highlight which makes even less sense. No-one else does that.

We have a consistency problem with list view delegates over Plasma generally, so I this probably isn't any worse, but it's not right.

There is a PlasmaComponents.ListItem which does a fairly good job of handling some of this vaguely consistently. Though that's currently only used by 4 things.

313

Not tested, but will this work with RTL?

Note if not, you just need an anchors.left: parent.left and it will

359

it's better to do

height: parent.height
you already have the centering of the text in the text alignment

hein marked an inline comment as done.Jul 10 2016, 4:09 PM
hein added inline comments.
applets/window-list/contents/ui/main.qml
241–242

We use this same technique in Kicker and FV and other places. It's the only way to get element sizes from a themer and cope with themes that miss certain elements. I have asked for better APIs over the years and got a "no" back from Marco every time. This works, and anything else breaks for the user. Our UIs rely on it.

313

Worked in my --reverse test.

hein marked 2 inline comments as done.Jul 10 2016, 4:12 PM
hein added inline comments.Jul 10 2016, 4:16 PM
applets/window-list/contents/ui/main.qml
241–242

Oh the Math.max() is because the margin sizes differ for some themes and this avoids ui jumps in cases like that -- another real world fix ...

applets/window-list/contents/ui/main.qml
241–242

The main bug I'm complaining about isn't you using the margins, it's about how you're then not putting the items in that frame - which is literally the point of the frame.

It happens to be that viewitem/normal is completely transparent on Breeze, but you can't rely on that being the case for all themes.

Plus you don't get the mousedown->selected effect. Krunner is a good example of that working. (btw, that uses ViewItem and most certainly doesn't "break for the user")

...and if you do think it's better to get rid viewitem/normal viewitem/selected - (which tbh I'd be fine with, the current themeing doesn't match ListView's highligh API as it didn't exist at the time) then we *need* to make sure our component matches.

Not s

hein marked an inline comment as done.Jul 11 2016, 2:58 AM
hein added inline comments.
applets/window-list/contents/ui/main.qml
241–242

Can you please summarize all this into a concrete change request for this patch so it can proceed?

(Sorry for replying to the wrong parent, Phab is for some reason hiding your comment.)

hein marked an inline comment as not done.Jul 11 2016, 2:59 AM
hein marked an inline comment as done.Jul 12 2016, 9:49 AM
hein added inline comments.
applets/window-list/contents/ui/main.qml
359

Done.

hein updated this revision to Diff 5094.Jul 12 2016, 9:49 AM
hein marked an inline comment as done.

Fix Label height setting.

davidedmundson accepted this revision.Jul 12 2016, 1:38 PM
davidedmundson added a reviewer: davidedmundson.

Can you please summarize all this into a concrete change request for this patch so it can proceed?

I'm saying it "should" be

delegate:
FrameSvgItem {

prefix: "viewitem/normal"
Item {
   //content here, with the margins set to the frame above
}

}

That way if a theme does have a viewitem/normal it gets rendered.

However, if you want you can merge this as-is and I'll start a ML discussion trying to unify all the delegates.

This revision is now accepted and ready to land.Jul 12 2016, 1:38 PM
This revision was automatically updated to reflect the committed changes.