D24821: Add ManagedConfigModule

2019-10-22 Thread Kevin Ottens 
This revision was automatically updated to reflect the committed changes.
Closed by commit R296:c8b95b07c53b: Add ManagedConfigModule (authored by ervin).

REPOSITORY
  R296 KDeclarative

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D24821?vs=68530=68545

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

AFFECTED FILES
  src/quickaddons/CMakeLists.txt
  src/quickaddons/managedconfigmodule.cpp
  src/quickaddons/managedconfigmodule.h

To: ervin, #plasma, #frameworks, mart, bport, davidedmundson
Cc: davidedmundson, bport, kde-frameworks-devel, LeGast00n, GB_2, michaelh, 
ngraham, bruns

D24821: Add ManagedConfigModule

2019-10-22 Thread David Edmundson 
davidedmundson accepted this revision.

REPOSITORY
  R296 KDeclarative

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

To: ervin, #plasma, #frameworks, mart, bport, davidedmundson
Cc: davidedmundson, bport, kde-frameworks-devel, LeGast00n, GB_2, michaelh, 
ngraham, bruns

D24821: Add ManagedConfigModule

2019-10-22 Thread Kevin Ottens 
ervin updated this revision to Diff 68530.
ervin added a comment.


  Address David's comments

REPOSITORY
  R296 KDeclarative

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D24821?vs=68411=68530

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

AFFECTED FILES
  src/quickaddons/CMakeLists.txt
  src/quickaddons/managedconfigmodule.cpp
  src/quickaddons/managedconfigmodule.h

To: ervin, #plasma, #frameworks, mart, bport, davidedmundson
Cc: davidedmundson, bport, kde-frameworks-devel, LeGast00n, GB_2, michaelh, 
ngraham, bruns

D24821: Add ManagedConfigModule

2019-10-22 Thread Kevin Ottens 
ervin marked an inline comment as done.

REPOSITORY
  R296 KDeclarative

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

To: ervin, #plasma, #frameworks, mart, bport, davidedmundson
Cc: davidedmundson, bport, kde-frameworks-devel, LeGast00n, GB_2, michaelh, 
ngraham, bruns

D24821: Add ManagedConfigModule

2019-10-22 Thread Kevin Ottens 
ervin marked 4 inline comments as done.

REPOSITORY
  R296 KDeclarative

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

To: ervin, #plasma, #frameworks, mart, bport, davidedmundson
Cc: davidedmundson, bport, kde-frameworks-devel, LeGast00n, GB_2, michaelh, 
ngraham, bruns

D24821: Add ManagedConfigModule

2019-10-21 Thread Nathaniel Graham 
ngraham added a dependent revision: D24822: Port the desktoptheme kcm to 
ManagedConfigModule.

REPOSITORY
  R296 KDeclarative

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

To: ervin, #plasma, #frameworks, mart, bport, davidedmundson
Cc: davidedmundson, bport, kde-frameworks-devel, LeGast00n, GB_2, michaelh, 
ngraham, bruns

D24821: Add ManagedConfigModule

2019-10-21 Thread David Edmundson 
davidedmundson accepted this revision.
davidedmundson added a comment.


  > Indeed the alternative is registerSettings(), I went for the less explicit 
"children settings" mechanism mostly because it was closer to the older system 
around KCModule which was very much working by convention.
  
  Having similarity to the KCModule is a perfectly good reason. Ship it.

REPOSITORY
  R296 KDeclarative

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

To: ervin, #plasma, #frameworks, mart, bport, davidedmundson
Cc: davidedmundson, bport, kde-frameworks-devel, LeGast00n, GB_2, michaelh, 
ngraham, bruns

D24821: Add ManagedConfigModule

2019-10-21 Thread Kevin Ottens 
ervin added a comment.


  In D24821#551179 , @davidedmundson 
wrote:
  
  > Nice!
  >
  > I'm not super sold on magically doing findChildren to get the config 
skeletons over an explicit
  >  registerSettings(KCoreConfigSkeleton*).
  >
  > I'm not against it either, but could you expand on the rationale.
  
  
  You mean here to sell it to you or in the doc or something else?
  
  Indeed the alternative is registerSettings(), I went for the less explicit 
"children settings" mechanism mostly because it was closer to the older system 
around KCModule which was very much working by convention.

INLINE COMMENTS

> bport wrote in managedconfigmodule.h:210
> I think we need to set this value to true by default, because if we don't 
> override it we assume value are not the default one

Right, makes sense for the future code.

REPOSITORY
  R296 KDeclarative

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

To: ervin, #plasma, #frameworks, mart, bport
Cc: davidedmundson, bport, kde-frameworks-devel, LeGast00n, GB_2, michaelh, 
ngraham, bruns

D24821: Add ManagedConfigModule

2019-10-21 Thread David Edmundson 
davidedmundson added inline comments.

INLINE COMMENTS

> managedconfigmodule.h:116
> + *
> + * If you want to make the ConfigModule available only conditionally (i.e. 
> show in
> + * the list of available modules only if some test succeeds) then you can use

I know this is just copy paste, but can you delete this paragraph as per 
https://phabricator.kde.org/D24824

REPOSITORY
  R296 KDeclarative

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

To: ervin, #plasma, #frameworks, mart, bport
Cc: davidedmundson, bport, kde-frameworks-devel, LeGast00n, GB_2, michaelh, 
ngraham, bruns

D24821: Add ManagedConfigModule

2019-10-21 Thread Benjamin Port 
bport accepted this revision.
bport added a comment.
This revision is now accepted and ready to land.


  Ok for me, when David remarks are addressed

REPOSITORY
  R296 KDeclarative

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

To: ervin, #plasma, #frameworks, mart, bport
Cc: davidedmundson, bport, kde-frameworks-devel, LeGast00n, GB_2, michaelh, 
ngraham, bruns

D24821: Add ManagedConfigModule

2019-10-21 Thread David Edmundson 
davidedmundson added a comment.


  Nice!
  
  I'm not super sold on magically doing findChildren to get the config 
skeletons over an explicit
  registerSettings(KCoreConfigSkeleton*).
  
  I'm not against it either, but could you expand on the rationale.

INLINE COMMENTS

> managedconfigmodule.cpp:131
> +needsSave |= skeleton->isSaveNeeded();
> +if (needsSave)
> +break;

Coding style {} is needed for single lines

REPOSITORY
  R296 KDeclarative

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

To: ervin, #plasma, #frameworks, mart, bport
Cc: davidedmundson, bport, kde-frameworks-devel, LeGast00n, GB_2, michaelh, 
ngraham, bruns

D24821: Add ManagedConfigModule

2019-10-21 Thread Kevin Ottens 
ervin updated this revision to Diff 68411.
ervin added a comment.


  Addresses bport's comment

REPOSITORY
  R296 KDeclarative

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D24821?vs=68408=68411

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

AFFECTED FILES
  src/quickaddons/CMakeLists.txt
  src/quickaddons/managedconfigmodule.cpp
  src/quickaddons/managedconfigmodule.h

To: ervin, #plasma, #frameworks, mart, bport
Cc: bport, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D24821: Add ManagedConfigModule

2019-10-21 Thread Benjamin Port 
bport requested changes to this revision.
bport added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> managedconfigmodule.cpp:96
> +{
> +return false;
> +}

Like above I think we need to set it to true

> managedconfigmodule.h:210
> + *
> + * By default this returns false, it needs to be overriden only
> + * if the module has state outside of the settings declared in

I think we need to set this value to true by default, because if we don't 
override it we assume value are not the default one

REPOSITORY
  R296 KDeclarative

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

To: ervin, #plasma, #frameworks, mart, bport
Cc: bport, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D24821: Add ManagedConfigModule

2019-10-21 Thread Kevin Ottens 
ervin created this revision.
ervin added reviewers: Plasma, Frameworks, mart.
Herald added a project: Frameworks.
Herald added a subscriber: kde-frameworks-devel.
ervin requested review of this revision.

REVISION SUMMARY
  This is a new type of ConfigModule which will manage the state of
  "apply" and "defaults" by itself depending on the state of the KConfigXT
  objects used within the module.
  
  This is sligthly uncomplete on the "defaults" side due to missing
  facilities in ConfigModule, but at least it shows the approach works.

REPOSITORY
  R296 KDeclarative

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

AFFECTED FILES
  src/quickaddons/CMakeLists.txt
  src/quickaddons/managedconfigmodule.cpp
  src/quickaddons/managedconfigmodule.h

To: ervin, #plasma, #frameworks, mart
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns