D27540: Status indicator for individual widgets in KCModule

2020-02-25 Thread Kevin Ottens 
ervin added a comment.


  In D27540#615160 , @ervin wrote:
  
  > In D27540#615156 , @ngraham 
wrote:
  >
  > > 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.

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

D27540: Status indicator for individual widgets in KCModule

2020-02-25 Thread 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
Cc: iasensio, davidre, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, 
ngraham, bruns

D27540: Status indicator for individual widgets in KCModule

2020-02-25 Thread Kevin Ottens 
ervin updated this revision to Diff 76362.
ervin added a comment.


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

REPOSITORY
  R265 KConfigWidgets

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

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
Cc: iasensio, davidre, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, 
ngraham, bruns

D27540: Status indicator for individual widgets in KCModule

2020-02-25 Thread Kevin Ottens 
ervin added a comment.


  In D27540#615091 , @davidre wrote:
  
  > 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)
  >  F8117874: grafik.png 
  
  
  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). 
;-)

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

D27540: Status indicator for individual widgets in KCModule

2020-02-21 Thread Kevin Ottens 
ervin added a comment.


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

REPOSITORY
  R265 KConfigWidgets

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

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

D27540: Status indicator for individual widgets in KCModule

2020-02-21 Thread Kevin Ottens 
ervin added a comment.


  In D27540#615156 , @ngraham wrote:
  
  > 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. ;-)

REPOSITORY
  R265 KConfigWidgets

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

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

D27540: Status indicator for individual widgets in KCModule

2020-02-21 Thread Nathaniel Graham 
ngraham added a comment.


  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?

REPOSITORY
  R265 KConfigWidgets

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

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

D27540: Status indicator for individual widgets in KCModule

2020-02-21 Thread David Redondo 
davidre added a comment.


  In D27540#615129 , @ervin wrote:
  
  > In D27540#615105 , @davidre 
wrote:
  >
  > > In D27540#615092 , @ervin 
wrote:
  > >
  > > > In D27540#615061 , @davidre 
wrote:
  > > >
  > > > > 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...
  >
  > > F8117881: grafik.png 
  > > 
  > >> 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

REPOSITORY
  R265 KConfigWidgets

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

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

D27540: Status indicator for individual widgets in KCModule

2020-02-21 Thread Kevin Ottens 
ervin added a comment.


  In D27540#615105 , @davidre wrote:
  
  > In D27540#615092 , @ervin wrote:
  >
  > > In D27540#615061 , @davidre 
wrote:
  > >
  > > > 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...
  
  > F8117881: grafik.png 
  > 
  >> 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.

REPOSITORY
  R265 KConfigWidgets

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

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

D27540: Status indicator for individual widgets in KCModule

2020-02-21 Thread David Redondo 
davidre added a reviewer: VDG.

REPOSITORY
  R265 KConfigWidgets

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

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

D27540: Status indicator for individual widgets in KCModule

2020-02-21 Thread David Redondo 
davidre added a comment.


  In D27540#615092 , @ervin wrote:
  
  > In D27540#615061 , @davidre 
wrote:
  >
  > > 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
  F8117881: grafik.png 
  
  > 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?

REPOSITORY
  R265 KConfigWidgets

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

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

D27540: Status indicator for individual widgets in KCModule

2020-02-21 Thread David Redondo 
davidre added a comment.


  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)
  F8117874: grafik.png 

REPOSITORY
  R265 KConfigWidgets

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

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

D27540: Status indicator for individual widgets in KCModule

2020-02-21 Thread Kevin Ottens 
ervin added a comment.


  In D27540#615091 , @davidre wrote:
  
  > 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)
  >  F8117874: grafik.png 
  
  
  Very good point!

REPOSITORY
  R265 KConfigWidgets

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

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

D27540: Status indicator for individual widgets in KCModule

2020-02-21 Thread Kevin Ottens 
ervin added a comment.


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

REPOSITORY
  R265 KConfigWidgets

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

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

D27540: Status indicator for individual widgets in KCModule

2020-02-21 Thread Kevin Ottens 
ervin added inline comments.

INLINE COMMENTS

> meven wrote in kconfigdialogmanager_p.h:60
> I would have named those with s : indicatorWidgets, knownWidgets, buddyWidgets

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

REPOSITORY
  R265 KConfigWidgets

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

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

D27540: Status indicator for individual widgets in KCModule

2020-02-21 Thread David Redondo 
davidre added a comment.


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

REPOSITORY
  R265 KConfigWidgets

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

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

D27540: Status indicator for individual widgets in KCModule

2020-02-21 Thread Méven Car 
meven added inline comments.

INLINE COMMENTS

> kconfigdialogmanager_p.h:60
> +QHash buddyWidget;
> +QHash indicatorWidget;
> +QSet allExclusiveGroupBoxes;

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

> settingsstatusindicator_p.h:41
> +bool isDefaulted() const;
> +void setDefaulted(bool defaulted);
> +

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

REPOSITORY
  R265 KConfigWidgets

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

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

D27540: Status indicator for individual widgets in KCModule

2020-02-21 Thread Kevin Ottens 
ervin created this revision.
ervin added reviewers: ngraham, davidedmundson, meven, crossi, bport.
Herald added a project: Frameworks.
Herald added a subscriber: kde-frameworks-devel.
ervin requested review of this revision.

REVISION SUMMARY
  Extend KConfigDialogManager internals to insert a small
  SettingsStatusIndicator widget next to widgets representing a setting
  which is currently dirty or which 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.

REPOSITORY
  R265 KConfigWidgets

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
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns