Improve the look of the Plymouth Boot Splash Screen KCM UI
ClosedPublic

Authored by GB_2 on Apr 14 2019, 4:36 PM.

Details

Summary

BUG: 398469
BUG: 408573

Port the KCM to the new GriedView KCM design.


Credits go to @broulik, he basically did all the work.

Test Plan

Open the Plymouth Boot Splash Screen KCM.

Diff Detail

Repository
R258 Plymouth KCM
Branch
arcpatch-D20549
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 14638
Build 14656: arc lint + arc unit
GB_2 created this revision.Apr 14 2019, 4:36 PM
Restricted Application added a project: Plasma. · View Herald TranscriptApr 14 2019, 4:36 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
GB_2 requested review of this revision.Apr 14 2019, 4:36 PM
GB_2 edited the summary of this revision. (Show Details)Apr 14 2019, 4:37 PM
GB_2 added a subscriber: broulik.

Neato, thanks!

Bugs I found:

  • The current splash screen isn't selected in the KCM when you open it (regression from current version)
  • After changing the theme and clicking Apply, the whole window hangs for about 20 seconds (happens with current version, but maybe we can fix it here or at least display a progress bar or something?)
GB_2 updated this revision to Diff 56312.Apr 15 2019, 4:56 PM

Fix initial selection bug and add load indication

GB_2 edited the summary of this revision. (Show Details)Apr 15 2019, 5:01 PM
ngraham accepted this revision.Apr 15 2019, 5:43 PM
ngraham added a reviewer: broulik.

Perfect, UI-wise. Now it just needs a Plasma/@broulik code review. :)

This revision is now accepted and ready to land.Apr 15 2019, 5:43 PM
broulik added inline comments.Apr 17 2019, 3:34 PM
src/kcm.cpp
135–138

Ideally here we would reload the model and then set the newly installed theme as current

212

I think the model population must be split from the KCM load() so you can reload the model without marking it as non-dirty.

Right now when installing a new theme, the Apply button doesn't enable and no theme is selected. Ideally it would select the newly installed theme.

Also needs a emit selectedPluginIndexChanged(); after reloading the model as a model reset will cause ListView to forget its currentIndex

232–235

In error handling we should probably check for error() == KAuth::ActionReply::UserCancelledError to not show a pointless error message when user canceled

233

This isn't a signal, also why do you need to reload when saving? It's not like any themes could magically appear here?

src/kcm.h
77

I think this should be a property Q_PROPERTY(bool saving...) or more generic busy like in the other KCMs and then proper bindings in the UI

src/package/contents/ui/main.qml
69–74

In all the other KCMs the InlineMessage is above the controls to not push them around as it comes and goes

76–84

I don't think this "progress" bar adds much value

broulik requested changes to this revision.Apr 17 2019, 3:34 PM
This revision now requires changes to proceed.Apr 17 2019, 3:34 PM
ngraham added inline comments.Apr 17 2019, 3:35 PM
src/package/contents/ui/main.qml
76–84

I think it really does. Otherwise the whole UI freezes for 20 seconds and you think it's died. At least with the progress bar, you know something's happening.

broulik added inline comments.Apr 17 2019, 3:37 PM
src/package/contents/ui/main.qml
76–84

Ah, it calls update-alternatives internally, alright, that might take some time.
Is there a way to detect in KAuth whether we're waiting for the user to enter their password so we only show it once it actually does something?

GB_2 added inline comments.Apr 17 2019, 3:44 PM
src/kcm.cpp
233

It resets it if you cancel the action, so the apply button is enabled again and the selection is reset.

GB_2 updated this revision to Diff 56446.Apr 17 2019, 4:25 PM
GB_2 marked 8 inline comments as done.

Address some comments

GB_2 added a comment.Apr 17 2019, 4:26 PM

@broulik Do you want to commandeer this revision and do the rest?

@broulik Do you want to commandeer this revision and do the rest?

What is "the rest" ?

GB_2 added a comment.May 17 2019, 5:02 AM

What is "the rest" ?

The open inline comments.

GB_2 updated this revision to Diff 60944.Jul 1 2019, 5:13 PM
GB_2 marked 2 inline comments as done.
  • Split model reloading from the load() function
  • Emit selectedPluginIndexChanged(); after reloading the model
GB_2 added inline comments.Jul 1 2019, 5:13 PM
src/kcm.cpp
135–138

I think it should not select the newest installed theme instead of still having the current one selected, because in the "Get New..." dialog you can install multiple themes and it can't know which of them you actually want.

GB_2 marked an inline comment as done.Jul 1 2019, 5:13 PM
GB_2 updated this revision to Diff 61991.Jul 18 2019, 5:18 PM
  • use more bindings
  • set sourceSize on the thumbnail
GB_2 updated this revision to Diff 62043.Jul 19 2019, 10:26 AM

Move progress bar above GHNS button

GB_2 updated this revision to Diff 62050.Jul 19 2019, 10:55 AM

Make sure Apply button gets enabled when selecting a theme for the first time

GB_2 added a comment.EditedJul 28 2019, 4:34 PM

Ping @broulik
I think we should finally land this and make more changes later in case there are only any small issues left.

GB_2 edited the summary of this revision. (Show Details)Aug 1 2019, 1:42 PM
GB_2 edited the summary of this revision. (Show Details)Aug 1 2019, 1:47 PM
broulik accepted this revision.Aug 1 2019, 2:24 PM
This revision is now accepted and ready to land.Aug 1 2019, 2:24 PM
GB_2 updated this revision to Diff 62899.Aug 1 2019, 2:51 PM

Clean up debug messages

This revision was automatically updated to reflect the committed changes.