port the ksplash kcm to the new design
ClosedPublic

Authored by mart on Apr 10 2018, 2:07 PM.

Details

Summary

redically simplifying the code, port the ksplash kcm to the new design

Test Plan

selecting and preview work correctly, resize behavior is correct

Diff Detail

Repository
R119 Plasma Desktop
Branch
phab/ksplashredesign
Lint
No Linters Available
Unit
No Unit Test Coverage
mart created this revision.Apr 10 2018, 2:07 PM
Restricted Application added a project: Plasma. · View Herald TranscriptApr 10 2018, 2:07 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
mart requested review of this revision.Apr 10 2018, 2:07 PM

I like patches with more red than green. It's a very positive sign ++

Minor comments below

kcms/ksplash/package/contents/ui/main.qml
52

Doing this *and* the conection on line 64 seems wrong.

61

redundant given the onCurrentIndexChanged binding above

mart added inline comments.Apr 11 2018, 9:00 AM
kcms/ksplash/package/contents/ui/main.qml
61

the onCountChanged connection thinghie it's an hack that serves at startup: we don't have a signal from listview "i'm done populating" which happens way after component.oncompleted, so the only way we have is when the list gets populated, which also means that its contents grow in size.

*probably* i can remove onCurrentIndexChanged

kcms/ksplash/package/contents/ui/main.qml
61

You shouldn't have told me that :)

I have one consistent rule about working round Qt bugs, it needs an upstream report and a link in the code.


but in any case, your comment doesn't explain why we need:
view.positionViewAtIndex(grid.currentIndex, GridView.Visible)

If we've just changed the index, we'll run onCurrentIndexChanged which does it's own view.positionViewAt() .

Unless we need to position the view even when we don't change the index?

mart updated this revision to Diff 31901.Apr 11 2018, 3:48 PM
  • remove one redundant setcurrentindex
mart added inline comments.Apr 11 2018, 3:49 PM
kcms/ksplash/package/contents/ui/main.qml
61

You shouldn't have told me that :)
I have one consistent rule about working round Qt bugs, it needs an upstream report and a link in the code.

I am not completely sure is actually a Qt bug... not sure it would be possible to have some finished signal, as depending on how the model behaves, you can never be sure that the representation is "completed" as the model can change at any moment?

davidedmundson added inline comments.Apr 11 2018, 4:45 PM
kcms/ksplash/package/contents/ui/main.qml
61

ou can never be sure that the representation is "completed" as the model can change at any moment?

It must know internally as it has that separate populate and add transition.

It shouldn't need an external signal. In a perfect world:

ListView {

model: someValidModel
currentIndex: 4

}

should "just work"

mart updated this revision to Diff 31956.Apr 12 2018, 9:48 AM
  • remove one redundant setcurrentindex
mart updated this revision to Diff 31958.Apr 12 2018, 9:53 AM
  • fix minor warnings
mart added inline comments.Apr 12 2018, 9:55 AM
kcms/ksplash/package/contents/ui/main.qml
61

ListView {
model: someValidModel
currentIndex: 4
}

annoyingly, you are right :p
this actually works, the problem was that i was loading the model too late, after componentComplete.

now, loading the model in the ctor and just relying on one single binding for currentIndex works perfectly.

note: i removed onCurrentIndexChanged: positionViewAtIndex as i want to put it on the gridview kcm component itself, as i think it belongs there.

abetts added a subscriber: abetts.Apr 12 2018, 2:39 PM

Is there a way that the additional buttons are centered along the bottom edge of the preview?

mart added a comment.Apr 12 2018, 4:18 PM

like that, tough i don't like how it makes the code become

davidedmundson accepted this revision.Apr 12 2018, 4:22 PM

somethign something centre buttons

Alternative is to maximise the viewport. There was bug report on the cursors KCM about that, but I can't remember what happened.

This revision is now accepted and ready to land.Apr 12 2018, 4:22 PM

If the buttons are bound to the edges of the chooser thingy, doesn't that make them jump around and have different positions when the window is horizontally resized? https://bugs.kde.org/show_bug.cgi?id=391910 strikes again...

mart added a comment.EditedApr 13 2018, 4:32 PM

this is an experiment (still full of binding loops) which implements the way we talked about of

This revision was automatically updated to reflect the committed changes.