D25539: feat(kcm): add revert timer

[Zixing Liu]

liushuyu abandoned this revision.
liushuyu added a comment.


  This patch has been migrated to 
https://invent.kde.org/plasma/kscreen/-/merge_requests/1

REPOSITORY
  R104 KScreen

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

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

D25539: feat(kcm): add revert timer

[Nathaniel Graham]

ngraham added a comment.


  In D25539#579182 , @broulik wrote:
  
  > > With direct-apply having a revert window/overlay/inline message only 
shown when certain critical options were changed could be a good compromise.
  >
  > It needs to be made absolute sure, though, that the keyboard focus is 
correct, so hitting e.g. Return will undo it, which iirc is the main point of 
having a modal dialog instead of an inline thing.
  
  
  Yes.

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, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, alexeymin, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D25539: feat(kcm): add revert timer

[Kai Uwe Broulik]

broulik added a comment.


  > With direct-apply having a revert window/overlay/inline message only shown 
when certain critical options were changed could be a good compromise.
  
  It needs to be made absolute sure, though, that the keyboard focus is 
correct, so hitting e.g. Return will undo it, which iirc is the main point of 
having a modal dialog instead of an inline thing.

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, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, alexeymin, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D25539: feat(kcm): add revert timer

[Roman Gilg]

romangg added a comment.


  With direct-apply having a revert window/overlay/inline message only shown 
when certain critical options were changed could be a good compromise. Though I 
would still like to see direct-apply land first in this case and then we can 
look into when and how the revert functionality should hook into.

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, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, alexeymin, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D25539: feat(kcm): add revert timer

[Noah Davis]

ndavis added a comment.


  In D25539#579057 , @ngraham wrote:
  
  > IIRC macOS's equivalent to this KCM uses instant apply (everything in macOS 
is instant apply) and they have a modal dialog with a revert timer in it. But I 
believe it's only shown when you change the screen; it isn't shown for anything 
else.
  
  
  MacOS also doesn't let you pick 320x180 as a screen resolution though. We 
should probably hide obviously bad settings as well.

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, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, alexeymin, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D25539: feat(kcm): add revert timer

[Nathaniel Graham]

ngraham added a comment.


  IIRC macOS's equivalent to this KCM uses instant apply (everything in macOS 
is instant apply) and they have a modal dialog with a revert timer in it. But I 
believe it's only shown when you change the screen; it isn't shown for anything 
else.

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, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, alexeymin, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D25539: feat(kcm): add revert timer

[Roman Gilg]

romangg added a comment.


  In D25539#579054 , @ngraham wrote:
  
  > I'm torn. In principle I agree that this is a great improvement to help 
people avoid blowing themselves up while testing settings. However I'm not 
convinced that the patch's current form strikes the right balance between 
achieving that goal and not annoying the user by reverting their intended 
changes when not needed.
  >
  > In general I love InlineMeessages but I don't know if they're the right UI 
element here because they aren't modal. For this warning here I think we want a 
modal dialog because then the user can't miss it and get their settings 
reverted accidentally, which is possible with non-modal InlineMessages.
  
  
  I agree with your argument in the current setting but a modal dialog won't 
work with instant-apply and that's where the KScreen KCM will be heading 
(reason for that again is primarily to make it work well in Plasma mobile).

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, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, alexeymin, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D25539: feat(kcm): add revert timer

[Nathaniel Graham]

ngraham added a comment.


  I'm torn. In principle I agree that this is a great improvement to help 
people avoid blowing themselves up while testing settings. However I'm not 
convinced that the patch's current form strikes the right balance between 
achieving that goal and not annoying the user by reverting their intended 
changes when not needed.
  
  In general I love InlineMeessages but I don't know if they're the right UI 
element here because they aren't modal. For this warning here I think we want a 
modal dialog because then the user can't miss it and get their settings 
reverted accidentally, which is possible with non-modal InlineMessages.

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, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, alexeymin, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D25539: feat(kcm): add revert timer

[Noah Davis]

ndavis added a comment.


  In D25539#578688 , @romangg wrote:
  
  > In D25539#578624 , @liushuyu 
wrote:
  >
  > > Any other suggestions for this patch?
  >
  >
  > From a UX perspective the general question is if a revert timer makes sense 
when we have already the "Apply" action. As previously said I think it makes 
sense if we had instant-apply. I am currently going into this direction with 
D26038  but it does not yet do 
instant-apply. So my current leaning would be to wait until we have that and 
then revisit the revert timer.
  >
  > Could you look in a separate patch into how instant-apply would work with 
the current KScreen KCM (or on top of D26038 
)? That would be great.
  
  
  I think it makes sense with or without instant apply. You can accidentally 
make the UI unusable by messing with your screen settings, so having a timer to 
automatically revert isn't useless. An apply button doesn't save you when 
you've accidentally applied unusable settings.

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, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, alexeymin, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D25539: feat(kcm): add revert timer

[Roman Gilg]

romangg added a comment.


  In D25539#578624 , @liushuyu wrote:
  
  > Any other suggestions for this patch?
  
  
  From a UX perspective the general question is if a revert timer makes sense 
when we have already the "Apply" action. As previously said I think it makes 
sense if we had instant-apply. I am currently going into this direction with 
D26038  but it does not yet do 
instant-apply. So my current leaning would be to wait until we have that and 
then revisit the revert timer.
  
  Could you look in a separate patch into how instant-apply would work with the 
current KScreen KCM (or on top of D26038 )? 
That would be great.

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: feat(kcm): add revert timer

[Zixing Liu]

liushuyu added a comment.


  Any other suggestions for this patch?

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: feat(kcm): add revert timer

[Zixing Liu]

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


  Address ndavis' comments

REPOSITORY
  R104 KScreen

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D25539?vs=70451=70474

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, 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: feat(kcm): add revert timer

[Zixing Liu]

liushuyu added a comment.


  In D25539#568678 , @ndavis wrote:
  
  > 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.

INLINE COMMENTS

> ndavis wrote in 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.

Will do

> ndavis wrote in main.qml: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.

I will address this

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: feat(kcm): add revert timer

[Noah Davis]

ndavis added a comment.


  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.

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: feat(kcm): add revert timer

[Noah Davis]

ndavis added inline comments.

INLINE COMMENTS

> main.qml:109
> +iconName: "document-save"
> +text: i18n("Apply")
> +onTriggered: {

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.

> main.qml:115
> +},
> +Kirigami.Action {
> +iconName: "edit-undo"

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.

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: feat(kcm): add revert timer

[Zixing Liu]

liushuyu added a comment.


  After applying the patches from 
https://cgit.kde.org/kirigami.git/commit/?id=8c1e5b1336e6dcae0a1b7756977fcd9368f853a2,
  it now looks like this:
  
  F7787159: image.png 

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: feat(kcm): add revert timer

[Zixing Liu]

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


  Update icons and stop the timer on actions pressed

REPOSITORY
  R104 KScreen

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D25539?vs=70390=70451

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, 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: feat(kcm): add revert timer

[Zixing Liu]

liushuyu marked 2 inline comments as done.
liushuyu added a comment.


  Now it looks like this
  F7785973: image.png 

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: feat(kcm): add revert timer

[Zixing Liu]

liushuyu marked 2 inline comments as done.
liushuyu added inline comments.

INLINE COMMENTS

> ngraham wrote in main.qml:32
> Doesn't seem to be used; the timer duration is hardcoded on the C++ side

Now, it's used by the Timer

> ngraham wrote in main.qml:110
> I would change these to `StandardButton.Apply` and `StandardButton.Discard`

Now InlineMessage is used

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: feat(kcm): add revert timer

[Zixing Liu]

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


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

REPOSITORY
  R104 KScreen

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

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, 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