D26133: Enable Auto Save

[Aleix Pol Gonzalez]

apol added a comment.


  In D26133#603118 , @tcanabrava 
wrote:
  
  > > That's why personally I think it's fine to assume people need to opt-in 
for GenerateProperties if they want the feature, it's closely related after all.
  >
  > I disagree here.
  >  my application can be a simple QWidget based and have a KConfigXT enabled 
configuration dialog that I want to trigger a save on any edit.
  >  I don't need properties for that.
  
  
  What's the problem with having properties though?

REPOSITORY
  R237 KConfig

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

To: tcanabrava, ervin
Cc: ervin, GB_2, apol, kde-frameworks-devel, LeGast00n, michaelh, ngraham, bruns

D26133: Enable Auto Save

[Tomaz Canabrava]

tcanabrava added a comment.


  
  
  > That's why personally I think it's fine to assume people need to opt-in for 
GenerateProperties if they want the feature, it's closely related after all.
  
  I disagree here.
  my application can be a simple QWidget based and have a KConfigXT enabled 
configuration dialog that I want to trigger a save on any edit.
  I don't need properties for that.

REPOSITORY
  R237 KConfig

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

To: tcanabrava, ervin
Cc: ervin, GB_2, apol, kde-frameworks-devel, LeGast00n, michaelh, ngraham, bruns

D26133: Enable Auto Save

[Kevin Ottens]

ervin added a comment.


  Maybe look at KConfigCompilerSignallingItem? But I don't see another way of 
doing it than a wrapper approach like KConfigCompilerSignallingItem does... but 
then that's why ManagedConfigModule assumes GenerateProperties=true in the 
kcfgc, otherwise we'd have hacked the compiler to do the same thing anyway just 
inconditionnally...
  
  That's why personally I think it's fine to assume people need to opt-in for 
GenerateProperties if they want the feature, it's closely related after all.

REPOSITORY
  R237 KConfig

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

To: tcanabrava, ervin
Cc: ervin, GB_2, apol, kde-frameworks-devel, LeGast00n, michaelh, ngraham, bruns

D26133: Enable Auto Save

[Tomaz Canabrava]

tcanabrava added a comment.


  I agree with the comments, but I'm a bit lost on how to implement that in 
KCoreConfigSkeleton:
  the isSaveNeeded reads the value of the variable and return if it's different 
from the reference variable. (that I tougth it was a reference *value*, to find 
out that it's a *variable reference*.
  so, if I change my setting via:
  
  Setting::self()->setSomething(5)
  
  the code for this setSomething will not call any KCoreConfigSkeleton code, 
but will just do a:
  
  GeneratedConfig::setSomething(int value) {
  
mSomething = value;
  
  }
  
  this is then handled internally on the KCoreSkeletonItem but I don't really 
understand how.
  anyone has a hint on how I can trigger a signal on it?
  I'm a bit lost.

REPOSITORY
  R237 KConfig

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

To: tcanabrava, ervin
Cc: ervin, GB_2, apol, kde-frameworks-devel, LeGast00n, michaelh, ngraham, bruns

D26133: Enable Auto Save

[Kevin Ottens]

ervin requested changes to this revision.
ervin added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> apol wrote in test_signal.h.ref:137
> How about having `isSaveNeededChanged(bool)`? It could be in 
> KCoreConfigSkeleton.

Damn, and I was wondering which better name we could use... it was already 
there all along since KCoreConfigSkeleton has isSaveNeeded indeed... Thanks 
Aleix! :-)

Note this means the logic will need to be adjusted though since it'll also need 
being emitted only when the value changes. AFAICT currently it does emit always 
on property change while only the first change would be necessary... also it 
won't emit again on load() calls when it's most likely we're rolling back.

REPOSITORY
  R237 KConfig

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

To: tcanabrava, ervin
Cc: ervin, GB_2, apol, kde-frameworks-devel, LeGast00n, michaelh, ngraham, bruns

D26133: Enable Auto Save

[Aleix Pol Gonzalez]

apol added inline comments.

INLINE COMMENTS

> test_signal.h.ref:137
> +/** Triggered whenever a setting changes */
> +void configurationChanged();
> +

How about having `isSaveNeededChanged(bool)`? It could be in 
KCoreConfigSkeleton.

> kconfig_compiler.cpp:2121
>  }
> -h << ");" << endl;
> +h << ");" << endl << endl;
> +

Seems like this endl belongs outside the loop.

> kconfig_compiler.cpp:2125
> +h << "/** Triggered whenever a setting changes */" << endl;
> +h << "void configurationChanged();" << endl;
> +

If it's in every instance, it should be part of the parent class 
(KCoreConfigSkeleton) rather than the generated code.

REPOSITORY
  R237 KConfig

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

To: tcanabrava, ervin
Cc: ervin, GB_2, apol, kde-frameworks-devel, LeGast00n, michaelh, ngraham, bruns

D26133: Enable Auto Save

[Tomaz Canabrava]

tcanabrava updated this revision to Diff 72110.
tcanabrava added a comment.


  - Use a timer of 1s to trigger the save

REPOSITORY
  R237 KConfig

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26133?vs=71920=72110

BRANCH
  enableAutoSave

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

AFFECTED FILES
  autotests/kconfig_compiler/test13.h.ref
  autotests/kconfig_compiler/test_signal.h.ref
  src/kconfig_compiler/kconfig_compiler.cpp

To: tcanabrava, ervin
Cc: ervin, GB_2, apol, kde-frameworks-devel, LeGast00n, michaelh, ngraham, bruns

D26133: Enable Auto Save

[Tomaz Canabrava]

tcanabrava updated this revision to Diff 72111.
tcanabrava added a comment.


  - Emit `configurationChanged()` when any configuration changes

REPOSITORY
  R237 KConfig

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26133?vs=72110=72111

BRANCH
  enableAutoSave

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

AFFECTED FILES
  autotests/kconfig_compiler/test13.h.ref
  autotests/kconfig_compiler/test_signal.h.ref
  src/kconfig_compiler/kconfig_compiler.cpp

To: tcanabrava, ervin
Cc: ervin, GB_2, apol, kde-frameworks-devel, LeGast00n, michaelh, ngraham, bruns

D26133: Enable Auto Save

[Kevin Ottens]

ervin requested changes to this revision.
ervin added a comment.
This revision now requires changes to proceed.


  In D26133#581832 , @tcanabrava 
wrote:
  
  > Because not all apps that use KConfigXT use a KCM, and I don't want to make
  >  it necessary to do so. This particular change is done with Kirogi in mind
  >  (that does not uses a KCM, and it's a pure QtQuick / Kirigami app, but uses
  >  KConfigXT)
  >  I'll add the testcase.
  
  
  Still this is the wrong layer of abstraction for that. KCM would need to 
adapt to the platform to autosave or not, if you're not using KCM then your own 
GUI need to adapt to the platform instead. By having this controlled by 
kconfig_compiler, the application gets no chance to adapt to the platform since 
it'll always be autosave as soon as you use the setting, whatever the platform.
  
  It needs to be solved higher up in the stack: KCM type of facilities. As you 
can tell, I'm very much against that change. ;-)

REPOSITORY
  R237 KConfig

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

To: tcanabrava, ervin
Cc: ervin, GB_2, apol, kde-frameworks-devel, LeGast00n, michaelh, ngraham, bruns

Re: D26133: Enable Auto Save

[Tomaz Canabrava]

Because not all apps that use KConfigXT use a KCM, and I don't want to make
it necessary to do so. This particular change is done with Kirogi in mind
(that does not uses a KCM, and it's a pure QtQuick / Kirigami app, but uses
KConfigXT)
I'll add the testcase.

On Mon, 23 Dec 2019 at 03:02 Aleix Pol Gonzalez 
wrote:

> apol added a comment. View Revision 
>
> It could make sense to add a test.
>
> Also for an application (system settings or kconfig dialogs) it's already
> possible to just trigger save when the kcm has changed (we already have
> signals for this). Why do you think it's needed?
>
> *REPOSITORY*
> R237 KConfig
>
> *REVISION DETAIL*
> https://phabricator.kde.org/D26133
>
> *To: *tcanabrava
> *Cc: *apol, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham,
> bruns
>

D26133: Enable Auto Save

[Aleix Pol Gonzalez]

apol added a comment.


  It could make sense to add a test.
  
  Also for an application (system settings or kconfig dialogs) it's already 
possible to just trigger save when the kcm has changed (we already have signals 
for this). Why do you think it's needed?

REPOSITORY
  R237 KConfig

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

To: tcanabrava
Cc: apol, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D26133: Enable Auto Save

[Tomaz Canabrava]

tcanabrava updated this revision to Diff 71920.
tcanabrava added a comment.


  - Use a timer of 1s to trigger the save

REPOSITORY
  R237 KConfig

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26133?vs=71916=71920

BRANCH
  enableAutoSave

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

AFFECTED FILES
  src/kconfig_compiler/kconfig_compiler.cpp

To: tcanabrava
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D26133: Enable Auto Save

[Nathaniel Graham]

ngraham edited the summary of this revision.

REPOSITORY
  R237 KConfig

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

To: tcanabrava
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D26133: Enable Auto Save

[Tomaz Canabrava]

tcanabrava created this revision.
Herald added a project: Frameworks.
Herald added a subscriber: kde-frameworks-devel.
tcanabrava requested review of this revision.

REVISION SUMMARY
  Some applications don't want the `wait for apply / ok / cancel`
  and prefer to automatically save the settings when the settings
  changes.
  
  This is used in Android, iOS and other systems.
  This is the first step to make KDE settings `instant`

REPOSITORY
  R237 KConfig

BRANCH
  enableAutoSave

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

AFFECTED FILES
  src/kconfig_compiler/kconfig_compiler.cpp

To: tcanabrava
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns