D25539: KScreen KCM: Add a revert timer to the settings page

[Roman Gilg]

romangg added a comment.


  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

REPOSITORY
  R104 KScreen

REVISION DETAIL
  https://phabricator.kde.org/D25539

To: liushuyu, #vdg, #plasma, romangg
Cc: ngraham, broulik, ndavis, plasma-devel, LeGast00n, The-Feren-OS-Dev, 
jraleigh, fbampaloukas, GB_2, ragreen, ZrenBot, alexeymin, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D25539: KScreen KCM: Add a revert timer to the settings page

[Nathaniel Graham]

ngraham added a reviewer: romangg.
ngraham added a comment.


  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.

INLINE COMMENTS

> main.qml:32
>  property int selectedOutput: 0
> +property int revertCountdown: 10
>  

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

> main.qml:110
> +}
> +standardButtons: StandardButton.Save | StandardButton.Cancel
> +}

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

REPOSITORY
  R104 KScreen

REVISION DETAIL
  https://phabricator.kde.org/D25539

To: liushuyu, #vdg, #plasma, romangg
Cc: ngraham, broulik, ndavis, plasma-devel, LeGast00n, The-Feren-OS-Dev, 
jraleigh, fbampaloukas, GB_2, ragreen, ZrenBot, alexeymin, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D25539: KScreen KCM: Add a revert timer to the settings page

[Noah Davis]

ndavis added a comment.


  In D25539#567576 , @broulik wrote:
  
  > One of the key ideas of KScreen was that it will *not* have a revert timer.
  
  
  Could you expand on that?

REPOSITORY
  R104 KScreen

REVISION DETAIL
  https://phabricator.kde.org/D25539

To: liushuyu, #vdg, #plasma
Cc: broulik, ndavis, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D25539: KScreen KCM: Add a revert timer to the settings page

[Kai Uwe Broulik]

broulik added a comment.


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

REPOSITORY
  R104 KScreen

REVISION DETAIL
  https://phabricator.kde.org/D25539

To: liushuyu, #vdg, #plasma
Cc: broulik, ndavis, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D25539: KScreen KCM: Add a revert timer to the settings page

[Zixing Liu]

liushuyu updated this revision to Diff 70329.
liushuyu added a comment.


  Fix the diff

REPOSITORY
  R104 KScreen

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D25539?vs=70328=70329

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D25539

AFFECTED FILES
  kcm/config_handler.cpp
  kcm/config_handler.h
  kcm/kcm.cpp
  kcm/kcm.h
  kcm/package/contents/ui/main.qml

To: liushuyu, #vdg, #plasma
Cc: ndavis, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, 
GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D25539: KScreen KCM: Add a revert timer to the settings page

[Noah Davis]

ndavis added a comment.


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

REPOSITORY
  R104 KScreen

REVISION DETAIL
  https://phabricator.kde.org/D25539

To: liushuyu, #vdg, #plasma
Cc: ndavis, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, 
GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D25539: KScreen KCM: Add a revert timer to the settings page

[Zixing Liu]

liushuyu created this revision.
liushuyu added a reviewer: VDG.
Herald added a project: Plasma.
Herald added a subscriber: plasma-devel.
liushuyu requested review of this revision.

REVISION 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).
  
  F7784367: 2019-11-25_20-46.png 
  
  F7784366: 2019-11-25_20-46_1.png 

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.

REPOSITORY
  R104 KScreen

REVISION DETAIL
  https://phabricator.kde.org/D25539

AFFECTED FILES
  kcm/config_handler.cpp
  kcm/config_handler.h
  kcm/kcm.cpp
  kcm/kcm.h
  kcm/package/contents/ui/main.qml

To: liushuyu, #vdg
Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, 
ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart