feat(kcm): add revert timer
Needs ReviewPublic

Authored by liushuyu on Tue, Nov 26, 4:01 AM.

Details

Reviewers
romangg
broulik
Group Reviewers
VDG
Plasma
Summary

Add a revert timer and an option for the user to revert the settings when they accidentally messed up the settings and unable to see the screen.

The current implementation is very rudimentary, a message box is shown and the text is static (no countdown in the dialog box).

Test Plan
  1. Open the System Settings and navigate to the "Display and Monitor" -> "Display Configuration."
  2. Change any settings and hit Apply.
  3. Don't click on the pop-up, wait for 10 seconds and see if the changes will be reverted and the dialog box closed.
  4. Repeat step 1 to step 2 and this time, click "cancel" and see if the changes will be reverted and the dialog box closed.
  5. Repeat step 1 to step 2 but this time click "save" and see if the changes will be saved and the dialog box closed, wait for 10 seconds to see if the changes are still retained.

Diff Detail

Repository
R104 KScreen
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 19237
Build 19255: arc lint + arc unit
liushuyu created this revision.Tue, Nov 26, 4:01 AM
Restricted Application added a project: Plasma. · View Herald TranscriptTue, Nov 26, 4:01 AM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
liushuyu requested review of this revision.Tue, Nov 26, 4:01 AM
ndavis added a subscriber: ndavis.Tue, Nov 26, 4:38 AM

+1 for this safety feature
It would be nice to have a visible countdown timer though.

liushuyu updated this revision to Diff 70329.Tue, Nov 26, 4:59 AM

Fix the diff

One of the key ideas of KScreen was that it will *not* have a revert timer.

One of the key ideas of KScreen was that it will *not* have a revert timer.

Could you expand on that?

broulik resigned from this revision.Tue, Nov 26, 8:43 AM
ngraham added a subscriber: ngraham.

Yeah, it would be good to know the history on that.

Also, please change the title to "feat(kcm): add a revert timer to the settings page" to comply with KScreen's new commit message guidelines.

kcm/package/contents/ui/main.qml
32

Doesn't seem to be used; the timer duration is hardcoded on the C++ side

110

I would change these to StandardButton.Apply and StandardButton.Discard

Do we want a revert timer or not? I thought about this in the past and have not yet come to a definite conclusion. Other question is how this relates to "instant-apply". Do we want this as well in the future?

Interesting would be the reasoning in the past to have neither.

This can be changed later, but inline message would be preferable to dialog popup.

What I could imagine here is the following:

  • Have instant-apply
  • Have 3 to 5 seconds change-timer after something changed (and was instantly applied)
  • If user changed something else before change-timer triggers restart change-timer
  • When change-timer triggers show warning and start revert-timer
  • When revert-timer triggers reset to the original configuration before any instantly-applied changes
  • When KCM closed by user assume user is content with all current changes and stop all timers
romangg retitled this revision from KScreen KCM: Add a revert timer to the settings page to feat(kcm): add revert timer.Tue, Nov 26, 7:30 PM
liushuyu updated this revision to Diff 70390.Wed, Nov 27, 2:05 AM

Use InlineMessage instead of MessageBox and move the timer to the QML portion

liushuyu marked 2 inline comments as done.Wed, Nov 27, 2:07 AM
liushuyu added inline comments.
kcm/package/contents/ui/main.qml
32

Now, it's used by the Timer

110

Now InlineMessage is used

liushuyu marked 2 inline comments as done.Wed, Nov 27, 2:08 AM

Now it looks like this

liushuyu updated this revision to Diff 70451.Wed, Nov 27, 6:50 PM

Update icons and stop the timer on actions pressed

ndavis added inline comments.Thu, Nov 28, 3:39 AM
kcm/package/contents/ui/main.qml
109

Since you are keeping the recently applied configuration rather than applying a new one, wouldn't it be better to say something like "Keep"? It would also be better to use dialog-ok-apply as the icon for consistency with similar controls.

115

I would put Undo to the left of Keep/Apply. Normally, Undo/Reset is to the far left of Apply. That can't be the case here, but I think preserving the order is better for muscle memory.

Is there a way to tell the revert timer not to be used if the user only makes a change that requires a session restart? It wouldn't be very useful to ask a user to confirm if the screen is displaying correctly if the change can't be seen.

Is there a way to tell the revert timer not to be used if the user only makes a change that requires a session restart? It wouldn't be very useful to ask a user to confirm if the screen is displaying correctly if the change can't be seen.

Sorry, I don't know how to determine whether the changes only require a session restart properly. To my knowledge, the global scale option is the only setting that probably does not need the timer and my current idea is to set a boolean flag somewhere to indicate the global scale has changed and see if this is the only change user has made.

kcm/package/contents/ui/main.qml
109

Will do

115

I will address this

liushuyu updated this revision to Diff 70474.Thu, Nov 28, 4:15 AM

Address ndavis' comments