KCModule: Indicate when a setting has been changed from the default or previous value
Needs ReviewPublic

Authored by ervin on Feb 21 2020, 11:03 AM.

Details

Summary

Extend KConfigDialogManager internals to insert a small
SettingsStatusIndicator widget next to widgets representing a setting
which is differs from default value.

Test Plan

Tested it with several widgets based KCMs, namely: qtquicksettings,
desktoppath and screenlocker. Those three have different situations
in term of layouts. Worst case scenario is desktoppath which leads
to the right hand side of the fields moving a bit to fit the indicators
but that's the price to pay for the feature I guess while keeping
things readable.

Here is a screenshot how of it looks on the qtquicksettings KCM:

Note that the indicators are clickable and reset the field to default value.

Diff Detail

Repository
R265 KConfigWidgets
Lint
Lint Skipped
Unit
Unit Tests Skipped
Build Status
Buildable 24355
Build 24373: arc lint + arc unit
ervin created this revision.Feb 21 2020, 11:03 AM
Restricted Application added a project: Frameworks. · View Herald TranscriptFeb 21 2020, 11:03 AM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
ervin requested review of this revision.Feb 21 2020, 11:03 AM
meven added inline comments.Feb 21 2020, 11:24 AM
src/kconfigdialogmanager_p.h
61

I would have named those with s : indicatorWidgets, knownWidgets, buddyWidgets

src/settingsstatusindicator_p.h
42

Make setChanged and setDefaulted as slots to ease the reuse of this class outside of KConfigDialogManager

How does it look? Will this be opt in for other users of KConfigDialogManager?

ervin added inline comments.Feb 21 2020, 12:07 PM
src/kconfigdialogmanager_p.h
61

Not gonna rename those though, I'm merely moving code around (apart from indicatorWidget I could at least rename that one indeed).

The editmarks should probably respect the visibility of the associated widget. In this picture I use a invisble lineedit to manage the settings of the QButtonGroup beneath (QButtonGroup isn't supported by KCOnfigDialogManager)

How does it look? Will this be opt in for other users of KConfigDialogManager?

I'd say you should try it. ;-)
I really need wider feedback on how it behaves in different context. Currently the patch makes it mandatory for everyone.

The editmarks should probably respect the visibility of the associated widget. In this picture I use a invisble lineedit to manage the settings of the QButtonGroup beneath (QButtonGroup isn't supported by KCOnfigDialogManager)

Very good point!

How does it look? Will this be opt in for other users of KConfigDialogManager?

I'd say you should try it. ;-)
I really need wider feedback on how it behaves in different context. Currently the patch makes it mandatory for everyone.

I use KConfigDialogManager to manage the settings in Spectacle MainWindow which are instant apply by
connect(mConfigManager, &KConfigDialogManager::widgetModified, mConfigManager, &KConfigDialogManager::updateSettings); and I don't like it that they appear there too

a setting which is currently dirty or which differs from default value.

What is your idea behind this? After first seeing this patch my intuition was that it would show for unsaved changes. But then I was confused when I open a SettingsDialog that there were already marks even though I changed nothing! I would expect them only for unsaved changes, I don't know how other platforms handle this if they have something similiar?

ervin added a comment.Feb 21 2020, 1:11 PM

How does it look? Will this be opt in for other users of KConfigDialogManager?

I'd say you should try it. ;-)
I really need wider feedback on how it behaves in different context. Currently the patch makes it mandatory for everyone.

I use KConfigDialogManager to manage the settings in Spectacle MainWindow which are instant apply by
connect(mConfigManager, &KConfigDialogManager::widgetModified, mConfigManager, &KConfigDialogManager::updateSettings); and I don't like it that they appear there too

You mean they appear and stick around? Or they appear/disappear immediately?
If they stick around I'd indeed have to check why the state isn't recomputed on the updateSettings call...

a setting which is currently dirty or which differs from default value.

What is your idea behind this? After first seeing this patch my intuition was that it would show for unsaved changes. But then I was confused when I open a SettingsDialog that there were already marks even though I changed nothing! I would expect them only for unsaved changes, I don't know how other platforms handle this if they have something similiar?

Currently it helps the user find out the unsaved changes (the least useful in some way, or your immediate memory is not good) but it also helps the user find out which of her settings differ from the default values (and that's indeed something you will forget over time and wonder "why the hell is the defaults button enabled").

Hope the extra context helps.

How does it look? Will this be opt in for other users of KConfigDialogManager?

I'd say you should try it. ;-)
I really need wider feedback on how it behaves in different context. Currently the patch makes it mandatory for everyone.

I use KConfigDialogManager to manage the settings in Spectacle MainWindow which are instant apply by
connect(mConfigManager, &KConfigDialogManager::widgetModified, mConfigManager, &KConfigDialogManager::updateSettings); and I don't like it that they appear there too

You mean they appear and stick around? Or they appear/disappear immediately?
If they stick around I'd indeed have to check why the state isn't recomputed on the updateSettings call...

a setting which is currently dirty or which differs from default value.

What is your idea behind this? After first seeing this patch my intuition was that it would show for unsaved changes. But then I was confused when I open a SettingsDialog that there were already marks even though I changed nothing! I would expect them only for unsaved changes, I don't know how other platforms handle this if they have something similiar?

Currently it helps the user find out the unsaved changes (the least useful in some way, or your immediate memory is not good) but it also helps the user find out which of her settings differ from the default values (and that's indeed something you will forget over time and wonder "why the hell is the defaults button enabled").

Hope the extra context helps.

They stick around because as you said the settings differ from the defaults.

The pen for me conveys the meaning that something was edited that's the reason I would only show it for unsaved changes. But let's others weigh in what they think

Please add screenshots to the Test Plan section for patches that change the UI. :) Also it would be nice to indicate how someone can test this. Which apps? System Settings? Where? How?

ervin added a comment.Feb 21 2020, 2:22 PM

Please add screenshots to the Test Plan section for patches that change the UI. :) Also it would be nice to indicate how someone can test this. Which apps? System Settings? Where? How?

Well, second part I pointed out a few kcms you can test with. ;-)

ervin added a comment.Feb 21 2020, 2:24 PM

They stick around because as you said the settings differ from the defaults.

Right, so "not a bug" (as in that's what's the patch is intended to do so far).

The pen for me conveys the meaning that something was edited that's the reason I would only show it for unsaved changes. But let's others weigh in what they think

Right, I picked visual indicators which came to mind, I'm totally open to suggestions on which would be better suited.

ervin added a comment.Feb 25 2020, 1:43 PM

The editmarks should probably respect the visibility of the associated widget. In this picture I use a invisble lineedit to manage the settings of the QButtonGroup beneath (QButtonGroup isn't supported by KCOnfigDialogManager)

For the record, next iteration of that patch will honor the visibility of the associated widget (got some more changes to do locally before uploading though, stay tuned).

That being said, what you did is a very naughty hack you know. Why not fixing KConfigDialogManager or go through a QGroupBox (which is handled in some way). ;-)

ervin updated this revision to Diff 76362.Feb 25 2020, 1:57 PM

Handle visibility of the tracked widget, setters become slots, and add plural to "indicatorWidgets".

ervin edited the test plan for this revision. (Show Details)Feb 25 2020, 2:00 PM

Please add screenshots to the Test Plan section for patches that change the UI. :) Also it would be nice to indicate how someone can test this. Which apps? System Settings? Where? How?

Well, second part I pointed out a few kcms you can test with. ;-)

And now you got a screenshot as well. Waiting for further feedback now.

ngraham retitled this revision from Status indicator for individual widgets in KCModule to KCModule: Indicate when a setting has been changed from the default value.Feb 25 2020, 2:24 PM

That title is now not optimal isn't it? 1. It's not only in KCModule 2. It also shows unsaved settings

ngraham retitled this revision from KCModule: Indicate when a setting has been changed from the default value to KCModule: Indicate when a setting has been changed from the default or previous value.Feb 25 2020, 2:40 PM
ndavis added a subscriber: ndavis.EditedFeb 25 2020, 2:43 PM

I can see the utility of indicating non-default values in some cases, particularly with software that has a lot of necessary complexity in the settings or where non-default values can cause problems (see SVG Cleaner GUI for an example of both). However, I don't think it's necessary for most software to indicate non-default values or even dirty values.

Here's a picture of SVG Cleaner GUI for reference:

Notice how the non-default values are indicated with a blue dot and categories that contain options with non-default values also have a blue dot.

alexde added a subscriber: alexde.Feb 25 2020, 3:06 PM

Does this patch also fix indirectly #274629?

ervin added a comment.Feb 25 2020, 3:16 PM

Does this patch also fix indirectly #274629?

I doubt it, this bug report seems confused BTW... it was never broken on the kdelibs side but *some* dialogs have bugs which break the feature for them. Unrelated to this patch we'd been doing a big sweep on the systemsettings module to address among other thing the problems they might have in that department.

ervin added a comment.Fri, Mar 13, 8:51 AM

And now you got a screenshot as well. Waiting for further feedback now.

Ping? This patch was first created three weeks ago now.

Note that look in D27840 and D27841 is pretty much the same than in here for which I provided screenshot

@ndavis I know you had some idea for which icon to use, and an idea to make it a clickable button, right?

ndavis added a comment.EditedSun, Mar 15, 2:21 AM

@ndavis I know you had some idea for which icon to use, and an idea to make it a clickable button, right?

Right, it's like MuseScore:

  • It's simple to understand with the right iconography.
  • It's convenient when you're working with a lot of settings that you don't necessarily fully understand.
  • By being enabled or disabled, it indicates whether or not a control is set to the default value or not.

Some extra rules I thought of:

  • With the checkable label example in the mockup above, it should reset both the label and the checkbox.
  • In cases where there is no default value and a user is expected/required to change the value (e.g., LineEdits for the user's first and last names), we should not show a reset button.
    • I don't want users to mistake it for a clear text or undo button.
  • When the reset button is disabled, it should be invisible to reduce visual clutter.
ervin added a comment.Tue, Mar 17, 3:44 PM

Some extra rules I thought of:

  • With the checkable label example in the mockup above, it should reset both the label and the checkbox.

Just for the record, this will unlikely to be enforceable on the framework side of the code, likely to be taken care of on a case by case basis when we encounter those in the modules themselves. Just managing expectations on what the code can easily do at that level of abstraction. So don't look for it in the patches I submit around the topic. ;-)

The rest should be doable I'll update my patches shortly.

ervin updated this revision to Diff 77847.Tue, Mar 17, 5:00 PM

Take feedback about the GUI into account

ervin edited the summary of this revision. (Show Details)Tue, Mar 17, 5:01 PM
ervin edited the test plan for this revision. (Show Details)
ervin edited the test plan for this revision. (Show Details)

Is it possible to align all of the reset buttons like a column?

ervin added a comment.Tue, Mar 17, 5:39 PM

Is it possible to align all of the reset buttons like a column?

Why I'm not surprised. ;-)

Honestly with enough code it might be possible, but that'd be expensive in term of effort... I'll need to keep track of all widgets in the page. I'll give it a shot but don't hold your breath.

ervin updated this revision to Diff 78468.Wed, Mar 25, 3:51 PM

Have the indicators vertically line up automatically

ervin edited the test plan for this revision. (Show Details)Wed, Mar 25, 3:52 PM
ervin added a comment.Wed, Mar 25, 3:55 PM

Is it possible to align all of the reset buttons like a column?

Why I'm not surprised. ;-)

Honestly with enough code it might be possible, but that'd be expensive in term of effort... I'll need to keep track of all widgets in the page. I'll give it a shot but don't hold your breath.

Alright, turns out I managed to do it both for QtWidgets and QtQuick cases, so you can breath again. ;-)

Reviews on all my patches very welcome.

ervin updated this revision to Diff 78703.Fri, Mar 27, 8:57 PM

As advised by Kai and David on D27840, switch to using tool buttons and fix RTL handling.