D24494: Add convenience for defaults/dirty states to KCoreConfigSkeleton

2019-10-11 Thread Kevin Ottens
This revision was automatically updated to reflect the committed changes.
Closed by commit R237:4c3d37519684: Add convenience for defaults/dirty states 
to KCoreConfigSkeleton (authored by ervin).

REPOSITORY
  R237 KConfig

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D24494?vs=67597=67692

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

AFFECTED FILES
  autotests/kconfigskeletontest.cpp
  src/core/kcoreconfigskeleton.cpp
  src/core/kcoreconfigskeleton.h
  src/core/kcoreconfigskeleton_p.h

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


D24494: Add convenience for defaults/dirty states to KCoreConfigSkeleton

2019-10-11 Thread Aleix Pol Gonzalez
apol accepted this revision.

REPOSITORY
  R237 KConfig

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

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


D24494: Add convenience for defaults/dirty states to KCoreConfigSkeleton

2019-10-11 Thread David Edmundson
davidedmundson accepted this revision.
This revision is now accepted and ready to land.

REPOSITORY
  R237 KConfig

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

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


D24494: Add convenience for defaults/dirty states to KCoreConfigSkeleton

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


  Add the KF6 comment

REPOSITORY
  R237 KConfig

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D24494?vs=67522=67597

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

AFFECTED FILES
  autotests/kconfigskeletontest.cpp
  src/core/kcoreconfigskeleton.cpp
  src/core/kcoreconfigskeleton.h
  src/core/kcoreconfigskeleton_p.h

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


D24494: Add convenience for defaults/dirty states to KCoreConfigSkeleton

2019-10-10 Thread Aleix Pol Gonzalez
apol added a comment.


  Looks better, it could make sense to add a KF6 TODO/Warning.

REPOSITORY
  R237 KConfig

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

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


D24494: Add convenience for defaults/dirty states to KCoreConfigSkeleton

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


  Any chance to get another round of reviews now that this patch changed quite 
a bit?

REPOSITORY
  R237 KConfig

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

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


D24494: Add convenience for defaults/dirty states to KCoreConfigSkeleton

2019-10-08 Thread Kevin Ottens
ervin added inline comments.

INLINE COMMENTS

> davidedmundson wrote in kcoreconfigskeleton.cpp:140
> Do we need to make this
> 
>   if (d->mIsDefaultImpl){
>  return d->mIsDefaultImpl();
>   }
>   return false;
> 
> and initialize mIsDefaultImpl to nullptr
> 
> so that it doesn't crash if someone subclasses KConfigSkeletonItem directly 
> and doesn't implement this?

Initializing to nullptr wouldn't help, you would get the same behavior than 
with the default constructor.

It means it doesn't crash but raises a std::bad_function_call exception, which 
I think is fine... it's the next best thing to a pure virtual, but it's caught 
at runtime. I don't think we can do better than this in the current situation.

> davidedmundson wrote in kcoreconfigskeleton.h:218
> Is this an ABI break?
> 
> :/

*gasp* you're right, how stupid of me... we can't merge that...

REPOSITORY
  R237 KConfig

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

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


D24494: Add convenience for defaults/dirty states to KCoreConfigSkeleton

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


  5.63 -> 5.64

REPOSITORY
  R237 KConfig

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D24494?vs=67507=67522

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

AFFECTED FILES
  autotests/kconfigskeletontest.cpp
  src/core/kcoreconfigskeleton.cpp
  src/core/kcoreconfigskeleton.h
  src/core/kcoreconfigskeleton_p.h

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


D24494: Add convenience for defaults/dirty states to KCoreConfigSkeleton

2019-10-08 Thread Friedrich W. H. Kossebau
kossebau added inline comments.

INLINE COMMENTS

> kcoreconfigskeleton.h:216
> + *
> + * @since 5.63
> + */

-> 5.64

REPOSITORY
  R237 KConfig

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

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


D24494: Add convenience for defaults/dirty states to KCoreConfigSkeleton

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


  Looks good to me

INLINE COMMENTS

> kcoreconfigskeleton.cpp:140
> +{
> +return d->mIsDefaultImpl();
> +}

Do we need to make this

  if (d->mIsDefaultImpl){
 return d->mIsDefaultImpl();
  }
  return false;

and initialize mIsDefaultImpl to nullptr

so that it doesn't crash if someone subclasses KConfigSkeletonItem directly and 
doesn't implement this?

REPOSITORY
  R237 KConfig

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

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


D24494: Add convenience for defaults/dirty states to KCoreConfigSkeleton

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


  Second try, without breaking ABI... it's the best workaround I found in the 
current situation, I admit I died a bit inside.

REPOSITORY
  R237 KConfig

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D24494?vs=67496=67507

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

AFFECTED FILES
  autotests/kconfigskeletontest.cpp
  src/core/kcoreconfigskeleton.cpp
  src/core/kcoreconfigskeleton.h
  src/core/kcoreconfigskeleton_p.h

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


D24494: Add convenience for defaults/dirty states to KCoreConfigSkeleton

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


  In principle +

INLINE COMMENTS

> kcoreconfigskeleton.h:218
> + */
> +virtual bool isDefault() const = 0;
> +

Is this an ABI break?

:/

REPOSITORY
  R237 KConfig

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

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


D24494: Add convenience for defaults/dirty states to KCoreConfigSkeleton

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

REVISION SUMMARY
  It allows to verify if all the items of the skeleton are in their
  default values or if they hold any value deviating from the latest
  loaded values from KConfig.
  
  We didn't really need this during the KCModule/QtWidgets time since we
  could write KConfigDialogManager just fine without it. But for use with
  QML and aiming at having similar magic in KQuickAddons::ConfigModule
  such convenience functions will be needed.

REPOSITORY
  R237 KConfig

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

AFFECTED FILES
  autotests/kconfigskeletontest.cpp
  src/core/kcoreconfigskeleton.cpp
  src/core/kcoreconfigskeleton.h

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