D25539: feat(kcm): add revert timer

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,

D25539: feat(kcm): add revert timer

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

D25539: feat(kcm): add revert timer

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

D25539: feat(kcm): add revert timer

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

D25539: feat(kcm): add revert timer

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

D25539: feat(kcm): add revert timer

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

D25539: feat(kcm): add revert timer

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

D25539: feat(kcm): add revert timer

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

D25539: feat(kcm): add revert timer

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

D25539: feat(kcm): add revert timer

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

D25539: feat(kcm): add revert timer

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,

D25539: feat(kcm): add revert timer

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

D25539: feat(kcm): add revert timer

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

D25539: feat(kcm): add revert timer

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

D25539: feat(kcm): add revert timer

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

D25539: feat(kcm): add revert timer

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

D25539: feat(kcm): add revert timer

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

D25539: feat(kcm): add revert timer

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,

D25539: feat(kcm): add revert timer

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

D25539: feat(kcm): add revert timer

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