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

[Nathaniel Graham]

ngraham added a comment.


  T13008: Figure out a good UI for the "show which settings have been changed" 
feature 

REPOSITORY
  R265 KConfigWidgets

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

To: ervin, ngraham, davidedmundson, meven, crossi, bport, #vdg, ndavis, broulik
Cc: abetts, bam, GB_2, alexde, ndavis, iasensio, davidre, kde-frameworks-devel, 
LeGast00n, cblack, michaelh, ngraham, bruns

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

[Nathaniel Graham]

ngraham added a comment.


  Let's have the rest of the discussion in a central phab patch, which I 
probably should have pushed for at the start. I'll make one...

REPOSITORY
  R265 KConfigWidgets

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

To: ervin, ngraham, davidedmundson, meven, crossi, bport, #vdg, ndavis, broulik
Cc: abetts, bam, GB_2, alexde, ndavis, iasensio, davidre, kde-frameworks-devel, 
LeGast00n, cblack, michaelh, ngraham, bruns

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

[Noah Davis]

ndavis added a comment.


  I'm sorry this happened. I was working with what I could see. For what it's 
worth, I would have accepted it if you said it could not be done well. I know I 
don't know as much about the technical side of this as you do.

REPOSITORY
  R265 KConfigWidgets

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

To: ervin, ngraham, davidedmundson, meven, crossi, bport, #vdg, ndavis, broulik
Cc: abetts, bam, GB_2, alexde, ndavis, iasensio, davidre, kde-frameworks-devel, 
LeGast00n, cblack, michaelh, ngraham, bruns

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

[Andres Betts]

abetts added a comment.


  My opinion on this patch from the beginning has been that it really doesn't 
add much more than was there before. Visually, it clutters the UI. The icon 
selected for it might also not be visually appealing or meaningful enough. I 
believe there should be a different approach to this request. However, I am 
also open to just dropping the idea completely.
  
  Some ideas for better UX on this:
  
  1. Hover over the "Defaults" button and the UI shows highlighted controls and 
labels that have changed. If you want to return things to default, click the 
button.
  2. Color controls and labels differently once they have changed to a 
non-default state. From Breeze Blue to a lighter blue, for example. (This could 
be confusing to some users)
  3. Keyboard shortcut. Use a keyboard shortcut to return settings back to 
their default state. Control + Z comes to mind.

REPOSITORY
  R265 KConfigWidgets

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

To: ervin, ngraham, davidedmundson, meven, crossi, bport, #vdg, ndavis, broulik
Cc: abetts, bam, GB_2, alexde, ndavis, iasensio, davidre, kde-frameworks-devel, 
LeGast00n, cblack, michaelh, ngraham, bruns

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

[Nathaniel Graham]

ngraham added a comment.


  Yeah, I'm sorry about that.
  
  If VDG people ask for something that's technically impossible, you've gotta 
push back on that. They often don't know what is and isn't possible, or 
reasonable. We've been trying to help VDG people be more technical so they 
don't propose impossible things, but it's not perfect. The whole process needs 
to be a push-and-pull compromise where the design people accept when a design 
isn't technically feasible, and the tech people faithfully implement the design 
without diverging too far from it due to minor technical limitations, or 
letting the design people push them into something impossible due to major 
technical limitations.
  
  The basic problem with this feature is that I think we never did the initial 
design work to figure out who the target audience was, what their needs were, 
and why they would use and benefit from this feature. Even with an inline 
indicator like a glowing outline around the widget, we'd have the same problem 
that we do with the dot proposal in the sidebar view that it would be look like 
non-obvious visual noise to people. And with that, we'd lose the functionality 
of being able to revert individual settings. But is that needed? Who benefits 
from it? And so on. Such a complex and all-encompassing feature needs to have 
these kinds of questions answered first before implementation begins. We've 
found that Phabricator tasks are perfect for this, and we use them extensively 
to plan out work before coding begins for many projects.
  
  I know we're all exhausted and frustrated at this point, but maybe we can do 
that so we can push this forward in a way that makes everyone happy in the end?

REPOSITORY
  R265 KConfigWidgets

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

To: ervin, ngraham, davidedmundson, meven, crossi, bport, #vdg, ndavis, broulik
Cc: bam, GB_2, alexde, ndavis, iasensio, davidre, kde-frameworks-devel, 
LeGast00n, cblack, michaelh, ngraham, bruns

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

[Kevin Ottens]

ervin added a comment.


  You know it started as a proper painted indicator within the widget area, 
right? As such it couldn't have any of the issues you're pointing out now... 
Who pushed me to have them at distance I wonder? Right, was people from the 
VDG. So I find grand that then it goes all to revert because after weeks of 
pushing me to add more weird constraints then disappearing letting me alone 
trying to figure out where to go (and it was a large struggle at every step), 
the conclusion is "let's revert because VDG wasn't involved". There were 
technical reasons for the very first iteration and they had to be disregarded 
for design license.
  
  Honestly I'm disgusted by the way it's been handled.

REPOSITORY
  R265 KConfigWidgets

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

To: ervin, ngraham, davidedmundson, meven, crossi, bport, #vdg, ndavis, broulik
Cc: bam, GB_2, alexde, ndavis, iasensio, davidre, kde-frameworks-devel, 
LeGast00n, cblack, michaelh, ngraham, bruns

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

[Nathaniel Graham]

ngraham added a subscriber: GB_2.
ngraham added a comment.


  In D27540#652692 , @ervin wrote:
  
  > In D27540#652664 , @ngraham 
wrote:
  >
  > > David asked for VDG to approve before this landed, which wasn't done.
  >
  >
  > Dude, I jumped through all the hoops for the past weeks. Also it got no 
further reply after I updated the screenshot almost two weeks ago so yes I 
assumed you guys had nothing else.
  
  
  Sorry I didn't have the time to test it again. It's not just about the UI 
design itself, but also avoid obvious visual glitches, like this: F8249569: 
Screenshot_20200420_104632.png 
  
  There are also numerous cases in other KCMs where the indicator gets cut off, 
positioned strangely, or isn't visible at all for a non-default setting:
  F8249574: Screenshot_20200420_105414.png 

  F8249578: Screenshot_20200420_105450.png 

  F8249582: Peek 2020-04-20 10-57.webm 
  
  How are we going to fix that? The diversity of user interfaces we have 
throughout KDE software makes me skeptical that any kind of auto-generated icon 
placement can ever work, let alone look good. Where will the icons go in list 
items? Grid views? And so on. To be completely honest, if nobody could come up 
with a good UI, it might be a sign that the feature itself needs to be 
re-thought. I remain unconvinced that this is the best way to show that there 
are changed settings. I think @gb_2's idea of displaying the original or 
previous state when hovering over the Defaults or Reset button made more sense. 
As is, I don't really understand who the target user is for this feature.
  
  IMO this feature would have benefited from being outlined first in a 
Phabricator task and soliciting #VDG  
feedback before coding began, so we didn't frustratingly go back and forth in 
the patches. Given the above visual regressions and broken behavior on multiple 
KCMs, I think this needs to be reverted and discussed and re-thought in a 
central location. Sorry. :(

REPOSITORY
  R265 KConfigWidgets

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

To: ervin, ngraham, davidedmundson, meven, crossi, bport, #vdg, ndavis, broulik
Cc: GB_2, alexde, ndavis, iasensio, davidre, kde-frameworks-devel, LeGast00n, 
cblack, michaelh, ngraham, bruns

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

[Kevin Ottens]

ervin added a comment.


  In D27540#652669 , @ngraham wrote:
  
  > Finally clicking on the revert button in Spectacle's settings page causes a 
segfault for me: P590 Spectacle crash backtrace 

  
  
  Couldn't quite reproduce it the way you described, but indeed managed to get 
it to crash. It's fixed with D29014 

REPOSITORY
  R265 KConfigWidgets

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

To: ervin, ngraham, davidedmundson, meven, crossi, bport, #vdg, ndavis, broulik
Cc: alexde, ndavis, iasensio, davidre, kde-frameworks-devel, LeGast00n, cblack, 
michaelh, ngraham, bruns

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

[Kevin Ottens]

ervin added a comment.


  In D27540#652664 , @ngraham wrote:
  
  > David asked for VDG to approve before this landed, which wasn't done.
  
  
  Dude, I jumped through all the hoops for the past weeks. Also it got no 
further reply after I updated the screenshot almost two weeks ago so yes I 
assumed you guys had nothing else.

REPOSITORY
  R265 KConfigWidgets

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

To: ervin, ngraham, davidedmundson, meven, crossi, bport, #vdg, ndavis, broulik
Cc: alexde, ndavis, iasensio, davidre, kde-frameworks-devel, LeGast00n, cblack, 
michaelh, ngraham, bruns

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

[Nathaniel Graham]

ngraham added a comment.


  I have concerns about this. The button has no tooltip so it's not obvious 
what will happen when clicked on. It should at least say "Revert to default 
setting" in a tooltip, and preferably even the name or text of the default 
setting that will be reverted to.
  
  Also, we now have a new inconsistency in that a small fraction of System 
Settings KCMs and app settings windows will display these indicators, but not 
others.
  
  Finally clicking on the revert button in Spectacle's settings page causes a 
segfault for me: P590 Spectacle crash backtrace 


REPOSITORY
  R265 KConfigWidgets

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

To: ervin, ngraham, davidedmundson, meven, crossi, bport, #vdg, ndavis, broulik
Cc: alexde, ndavis, iasensio, davidre, kde-frameworks-devel, LeGast00n, cblack, 
michaelh, ngraham, bruns

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

[Nathaniel Graham]

ngraham added a comment.


  David asked for VDG to approve before this landed, which wasn't done.

REPOSITORY
  R265 KConfigWidgets

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

To: ervin, ngraham, davidedmundson, meven, crossi, bport, #vdg, ndavis, broulik
Cc: alexde, ndavis, iasensio, davidre, kde-frameworks-devel, LeGast00n, cblack, 
michaelh, ngraham, bruns

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

[Kevin Ottens]

This revision was automatically updated to reflect the committed changes.
Closed by commit R265:11186c056198: KCModule: Indicate when a setting has been 
changed from the default or previous… (authored by ervin).

REPOSITORY
  R265 KConfigWidgets

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D27540?vs=79656=80652

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

AFFECTED FILES
  src/CMakeLists.txt
  src/kconfigdialogmanager.cpp
  src/kconfigdialogmanager.h
  src/kconfigdialogmanager_p.h
  src/settingsstatusindicator.cpp
  src/settingsstatusindicator_p.h

To: ervin, ngraham, davidedmundson, meven, crossi, bport, #vdg, ndavis, broulik
Cc: alexde, ndavis, iasensio, davidre, kde-frameworks-devel, LeGast00n, cblack, 
michaelh, ngraham, bruns

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

[David Edmundson]

davidedmundson accepted this revision.
davidedmundson added a comment.
This revision is now accepted and ready to land.


  Assuming VDG are happy, go for it.

REPOSITORY
  R265 KConfigWidgets

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

To: ervin, ngraham, davidedmundson, meven, crossi, bport, #vdg, ndavis, broulik
Cc: alexde, ndavis, iasensio, davidre, kde-frameworks-devel, LeGast00n, cblack, 
michaelh, ngraham, bruns

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

[Kevin Ottens]

ervin updated this revision to Diff 79656.
ervin marked 6 inline comments as done.
ervin added a comment.


  Addresses David's comments

REPOSITORY
  R265 KConfigWidgets

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D27540?vs=78703=79656

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

AFFECTED FILES
  src/CMakeLists.txt
  src/kconfigdialogmanager.cpp
  src/kconfigdialogmanager.h
  src/kconfigdialogmanager_p.h
  src/settingsstatusindicator.cpp
  src/settingsstatusindicator_p.h

To: ervin, ngraham, davidedmundson, meven, crossi, bport, #vdg, ndavis, broulik
Cc: alexde, ndavis, iasensio, davidre, kde-frameworks-devel, LeGast00n, cblack, 
GB_2, michaelh, ngraham, bruns

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

[Kevin Ottens]

ervin marked 6 inline comments as done.
ervin added inline comments.

INLINE COMMENTS

> davidedmundson wrote in kconfigdialogmanager.cpp:609
> Why not item->readDefault()?

Wouldn't do the same thing at all. readDefault() takes a KConfig object and 
updates the default value stored in the item with what it found in there.

Yes, I know... the item API is weird...

> davidedmundson wrote in kconfigdialogmanager.cpp:625
> won't it do it itself when the property changes?

Good point, was indeed unnecessary now, I removed the line.

> davidedmundson wrote in settingsstatusindicator.cpp:75
> > This is equivalent to calling showFullScreen(), showMaximized(), or 
> > setVisible(true), depending on the platform's default behavior for the 
> > window flags.
> 
> For X and wayland it's setVisible(true)
> 
> but we shouldn't count on it.

I don't think it matters for widgets which have a parent and not the Qt::Window 
window flag, but OK, switching to setVisible() instead of show/hide.

> davidedmundson wrote in settingsstatusindicator.cpp:175
> unused?

I'm not sure how you end up to this conclusion, it's written two below if we 
are at the window edge, and it's used in the move call at the end.

> davidedmundson wrote in settingsstatusindicator.cpp:184
> Can we be sure the tracked widget always has a parent widget?
> 
> If someone doesn't use layouts a widget might not have a parent.

Well that'd mean the tracked widget is a window... it's pretty much an 
impossibility IMO.

> davidedmundson wrote in settingsstatusindicator.cpp:192
> that's not true for the RTL case where the widget is expected to resize.
> 
> It would be   w->pos().x() + w->width() - widgetExpectedWidth(w)

Either I misunderstood what you meant or you got the math wrong on that one.

What you're proposing (or what I understood you're proposing) breaks the "RTL + 
widget at edge" case in my tests. The line I wrote is working perfectly fine 
for my tests with desktoppath and qtquicksettings both in LTR and RTL modes.

REPOSITORY
  R265 KConfigWidgets

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

To: ervin, ngraham, davidedmundson, meven, crossi, bport, #vdg, ndavis, broulik
Cc: alexde, ndavis, iasensio, davidre, kde-frameworks-devel, LeGast00n, cblack, 
GB_2, michaelh, ngraham, bruns

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

[Kevin Ottens]

ervin added a comment.


  In D27540#638985 , @ndavis wrote:
  
  > Somehow I missed the notification that this was updated.
  >
  > Thanks for the horizontal alignment. Could you also add a left margin to 
the column of reset buttons? It should be the same as the spacing between the 
labels and the controls, which is equivalent to `Kirigami.Units.smallSpacing`, 
IIRC.
  
  
  Actually was already the case, forgot to update the screenshot.

REPOSITORY
  R265 KConfigWidgets

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

To: ervin, ngraham, davidedmundson, meven, crossi, bport, #vdg, ndavis, broulik
Cc: alexde, ndavis, iasensio, davidre, kde-frameworks-devel, LeGast00n, cblack, 
GB_2, michaelh, ngraham, bruns

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

[Kevin Ottens]

ervin edited the test plan for this revision.

REPOSITORY
  R265 KConfigWidgets

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

To: ervin, ngraham, davidedmundson, meven, crossi, bport, #vdg, ndavis, broulik
Cc: alexde, ndavis, iasensio, davidre, kde-frameworks-devel, LeGast00n, cblack, 
GB_2, michaelh, ngraham, bruns

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

[Noah Davis]

ndavis added a comment.


  Somehow I missed the notification that this was updated.
  
  Thanks for the horizontal alignment. Could you also add a left margin to the 
column of reset buttons? It should be the same as the spacing between the 
labels and the controls, which is `Kirigami.Units.smallSpacing`, IIRC.

REPOSITORY
  R265 KConfigWidgets

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

To: ervin, ngraham, davidedmundson, meven, crossi, bport, #vdg, ndavis, broulik
Cc: alexde, ndavis, iasensio, davidre, kde-frameworks-devel, LeGast00n, cblack, 
GB_2, michaelh, ngraham, bruns

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

[David Edmundson]

davidedmundson added a comment.


  What do we want to happen for released code that gets a bugfix update?

INLINE COMMENTS

> kconfigdialogmanager.cpp:609
> +const auto defaultValue = [item] {
> +item->swapDefault();
> +const auto value = item->property();

Why not item->readDefault()?

> kconfigdialogmanager.cpp:625
> +q->setProperty(widget, defaultValue);
> +updateWidgetIndicator(configId, widget);
> +emit q->widgetModified();

won't it do it itself when the property changes?

> settingsstatusindicator.cpp:75
> +setFocusPolicy(Qt::NoFocus);
> +show();
> +}

> This is equivalent to calling showFullScreen(), showMaximized(), or 
> setVisible(true), depending on the platform's default behavior for the window 
> flags.

For X and wayland it's setVisible(true)

but we shouldn't count on it.

> settingsstatusindicator.cpp:175
> +const auto leftToRight = m_trackedWidget->isLeftToRight();
> +auto x = leftToRight ? m_trackedWidget->pos().x() + 
> m_trackedWidget->width()
> + : m_trackedWidget->pos().x() - width();

unused?

> settingsstatusindicator.cpp:184
> +const auto re = QRegularExpression("^kcfg_");
> +const auto children = 
> m_trackedWidget->parentWidget()->findChildren(re, 
> Qt::FindDirectChildrenOnly);
> +const auto xValues = [=] {

Can we be sure the tracked widget always has a parent widget?

If someone doesn't use layouts a widget might not have a parent.

> settingsstatusindicator.cpp:192
> +   const auto localX = leftToRight ? 
> widgetExpectedWidth(w) : -width();
> +   return w->pos().x() + localX;
> +   });

that's not true for the RTL case where the widget is expected to resize.

It would be   w->pos().x() + w->width() - widgetExpectedWidth(w)

REPOSITORY
  R265 KConfigWidgets

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

To: ervin, ngraham, davidedmundson, meven, crossi, bport, #vdg, ndavis, broulik
Cc: alexde, ndavis, iasensio, davidre, kde-frameworks-devel, LeGast00n, cblack, 
GB_2, michaelh, ngraham, bruns

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

[Kevin Ottens]

ervin updated this revision to Diff 78703.
ervin added a comment.


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

REPOSITORY
  R265 KConfigWidgets

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D27540?vs=78468=78703

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

AFFECTED FILES
  src/CMakeLists.txt
  src/kconfigdialogmanager.cpp
  src/kconfigdialogmanager.h
  src/kconfigdialogmanager_p.h
  src/settingsstatusindicator.cpp
  src/settingsstatusindicator_p.h

To: ervin, ngraham, davidedmundson, meven, crossi, bport, #vdg, ndavis
Cc: alexde, ndavis, iasensio, davidre, kde-frameworks-devel, LeGast00n, cblack, 
GB_2, michaelh, ngraham, bruns

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

[Kevin Ottens]

ervin added a reviewer: broulik.

REPOSITORY
  R265 KConfigWidgets

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

To: ervin, ngraham, davidedmundson, meven, crossi, bport, #vdg, ndavis, broulik
Cc: alexde, ndavis, iasensio, davidre, kde-frameworks-devel, LeGast00n, cblack, 
GB_2, michaelh, ngraham, bruns

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

[Kevin Ottens]

ervin added a comment.


  In D27540#629380 , @ervin wrote:
  
  > In D27540#629362 , @ndavis wrote:
  >
  > > 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.

REPOSITORY
  R265 KConfigWidgets

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

To: ervin, ngraham, davidedmundson, meven, crossi, bport, #vdg, ndavis
Cc: alexde, ndavis, iasensio, davidre, kde-frameworks-devel, LeGast00n, cblack, 
GB_2, michaelh, ngraham, bruns

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

[Kevin Ottens]

ervin edited the test plan for this revision.

REPOSITORY
  R265 KConfigWidgets

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

To: ervin, ngraham, davidedmundson, meven, crossi, bport, #vdg, ndavis
Cc: alexde, ndavis, iasensio, davidre, kde-frameworks-devel, LeGast00n, cblack, 
GB_2, michaelh, ngraham, bruns

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

[Kevin Ottens]

ervin updated this revision to Diff 78468.
ervin added a comment.


  Have the indicators vertically line up automatically

REPOSITORY
  R265 KConfigWidgets

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D27540?vs=77847=78468

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

AFFECTED FILES
  src/CMakeLists.txt
  src/kconfigdialogmanager.cpp
  src/kconfigdialogmanager.h
  src/kconfigdialogmanager_p.h
  src/settingsstatusindicator.cpp
  src/settingsstatusindicator_p.h

To: ervin, ngraham, davidedmundson, meven, crossi, bport, #vdg, ndavis
Cc: alexde, ndavis, iasensio, davidre, kde-frameworks-devel, LeGast00n, cblack, 
GB_2, michaelh, ngraham, bruns

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

[Kevin Ottens]

ervin added a comment.


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

REPOSITORY
  R265 KConfigWidgets

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

To: ervin, ngraham, davidedmundson, meven, crossi, bport, #vdg, ndavis
Cc: alexde, ndavis, iasensio, davidre, kde-frameworks-devel, LeGast00n, cblack, 
GB_2, michaelh, ngraham, bruns

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

[Noah Davis]

ndavis added a comment.


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

REPOSITORY
  R265 KConfigWidgets

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

To: ervin, ngraham, davidedmundson, meven, crossi, bport, #vdg, ndavis
Cc: alexde, ndavis, iasensio, davidre, kde-frameworks-devel, LeGast00n, cblack, 
GB_2, michaelh, ngraham, bruns

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

[Kevin Ottens]

ervin edited the test plan for this revision.

REPOSITORY
  R265 KConfigWidgets

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

To: ervin, ngraham, davidedmundson, meven, crossi, bport, #vdg, ndavis
Cc: alexde, ndavis, iasensio, davidre, kde-frameworks-devel, LeGast00n, cblack, 
GB_2, michaelh, ngraham, bruns

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

[Kevin Ottens]

ervin edited the summary of this revision.
ervin edited the test plan for this revision.

REPOSITORY
  R265 KConfigWidgets

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

To: ervin, ngraham, davidedmundson, meven, crossi, bport, #vdg, ndavis
Cc: alexde, ndavis, iasensio, davidre, kde-frameworks-devel, LeGast00n, cblack, 
GB_2, michaelh, ngraham, bruns

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

[Kevin Ottens]

ervin updated this revision to Diff 77847.
ervin added a comment.


  Take feedback about the GUI into account

REPOSITORY
  R265 KConfigWidgets

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D27540?vs=76362=77847

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

AFFECTED FILES
  src/CMakeLists.txt
  src/kconfigdialogmanager.cpp
  src/kconfigdialogmanager.h
  src/kconfigdialogmanager_p.h
  src/settingsstatusindicator.cpp
  src/settingsstatusindicator_p.h

To: ervin, ngraham, davidedmundson, meven, crossi, bport, #vdg, ndavis
Cc: alexde, ndavis, iasensio, davidre, kde-frameworks-devel, LeGast00n, cblack, 
GB_2, michaelh, ngraham, bruns

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

[Kevin Ottens]

ervin added a comment.


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

REPOSITORY
  R265 KConfigWidgets

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

To: ervin, ngraham, davidedmundson, meven, crossi, bport, #vdg, ndavis
Cc: alexde, ndavis, iasensio, davidre, kde-frameworks-devel, LeGast00n, cblack, 
GB_2, michaelh, ngraham, bruns

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

[Noah Davis]

ndavis added a comment.


  In D27540#627615 , @ngraham wrote:
  
  > @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:
  
  F8176442: Screenshot_20200314_132437.PNG 

  
  - It's simple to understand with the right iconography.
  - It's convenient when you're working with a lot of settings.
  - 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.

REPOSITORY
  R265 KConfigWidgets

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

To: ervin, ngraham, davidedmundson, meven, crossi, bport, #vdg, ndavis
Cc: alexde, ndavis, iasensio, davidre, kde-frameworks-devel, LeGast00n, cblack, 
GB_2, michaelh, ngraham, bruns

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

[Nathaniel Graham]

ngraham added a reviewer: ndavis.
ngraham added a comment.


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

REPOSITORY
  R265 KConfigWidgets

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

To: ervin, ngraham, davidedmundson, meven, crossi, bport, #vdg, ndavis
Cc: alexde, ndavis, iasensio, davidre, kde-frameworks-devel, LeGast00n, cblack, 
GB_2, michaelh, ngraham, bruns

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

[Kevin Ottens]

ervin added a comment.


  In D27540#617485 , @ervin wrote:
  
  > 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

REPOSITORY
  R265 KConfigWidgets

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

To: ervin, ngraham, davidedmundson, meven, crossi, bport, #vdg
Cc: alexde, ndavis, iasensio, davidre, kde-frameworks-devel, LeGast00n, cblack, 
GB_2, michaelh, ngraham, bruns

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

[Kevin Ottens]

ervin added a comment.


  In D27540#617589 , @alexde wrote:
  
  > 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.

REPOSITORY
  R265 KConfigWidgets

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

To: ervin, ngraham, davidedmundson, meven, crossi, bport, #vdg
Cc: alexde, ndavis, iasensio, davidre, kde-frameworks-devel, LeGast00n, cblack, 
GB_2, michaelh, ngraham, bruns

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

[Alex Debus]

alexde added a comment.


  Does this patch also fix indirectly #274629?

REPOSITORY
  R265 KConfigWidgets

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

To: ervin, ngraham, davidedmundson, meven, crossi, bport, #vdg
Cc: alexde, ndavis, iasensio, davidre, kde-frameworks-devel, LeGast00n, cblack, 
GB_2, michaelh, ngraham, bruns

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

[Noah Davis]

ndavis added a comment.


  I can see the utility in 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:
  F8131329: Screenshot_20200225_094148.PNG 

  
  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.

REPOSITORY
  R265 KConfigWidgets

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

To: ervin, ngraham, davidedmundson, meven, crossi, bport, #vdg
Cc: ndavis, iasensio, davidre, kde-frameworks-devel, LeGast00n, cblack, GB_2, 
michaelh, ngraham, bruns

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

[Nathaniel Graham]

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".

REPOSITORY
  R265 KConfigWidgets

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

To: ervin, ngraham, davidedmundson, meven, crossi, bport, #vdg
Cc: iasensio, davidre, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, 
ngraham, bruns