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).
| No Linters Available |
| No Unit Test Coverage |
| Buildable 19225 | |
| Build 19243: arc lint + arc unit |
+1 for this safety feature
It would be nice to have a visible countdown timer though.
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:
After applying the patches from https://cgit.kde.org/kirigami.git/commit/?id=8c1e5b1336e6dcae0a1b7756977fcd9368f853a2,
it now looks like this:
| 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.
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 | |
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.
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).
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.
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.
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.
This patch has been migrated to https://invent.kde.org/plasma/kscreen/-/merge_requests/1
