D24965: KConfigWidgets: port away from KF5 deprecated API

[David Faure]

dfaure closed this revision.

REPOSITORY
  R265 KConfigWidgets

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

To: dfaure, kossebau, elvisangelaccio, vkrause
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D24965: KConfigWidgets: port away from KF5 deprecated API

[Friedrich W. H. Kossebau]

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


  Tired given the time, but let's see if I get things straight this time:
  Given KF_DISABLE_DEPRECATED_BEFORE_AND_AT=0x053f00, in the API of KConfig  we 
just see KStandardShortcut::SaveOptions_DEPRECATED_DO_NOT_USE. And as I just 
found, there also in no build option to completely get rid of it . Possibly the 
patch I did there I left it as I assumed a chance the enum value might be 
stored as number in config entries, so changing any enum values would break 
data. Too bad there is no related comment, I shall fix that with a follow-up 
patch, to be put into review the next day.
  So, with these considerations,
  
#if KCONFIGWIDGETS_BUILD_DEPRECATED_SINCE(5, 38)
​{ SaveOptions,   
KStandardShortcut::SaveOptions_DEPRECATED_DO_NOT_USE, "options_save_options", 
I18N_NOOP("&Save Settings"), nullptr, nullptr },
#endif
  
  should actually be always correct. We do not support people building the 
library to override any *_DISABLE_DEPRECATED_BEFORE_AND_AT settings for the 
libraries we build against.

REPOSITORY
  R265 KConfigWidgets

BRANCH
  master

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

To: dfaure, kossebau, elvisangelaccio, vkrause
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D24965: KConfigWidgets: port away from KF5 deprecated API

[David Faure]

dfaure added a comment.


  Funnily enough, the 5.39 is irrelevant, since we don't support building with 
older versions of KConfig. But it becomes relevant because we support enabling 
one and not the other, as you mention.
  We could cheat and just s/38/39/ here, it wouldn't harm anyone.
  
  If we limit *_BUILD_DEPRECATED_SINCE to latest, then my build (with that 
variable enabled for kwindowsystem) could break at any time when new things get 
deprecated. But yeah, I'm the 0.1% of people compiling the code, who sets 
EXCLUDE_DEPRECATED_BEFORE_AND_AT, so I can live with that.

REPOSITORY
  R265 KConfigWidgets

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

To: dfaure, kossebau, elvisangelaccio, vkrause
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D24965: KConfigWidgets: port away from KF5 deprecated API

[Friedrich W. H. Kossebau]

kossebau added inline comments.

INLINE COMMENTS

> kossebau wrote in kstandardaction_p.h:103
> Hm, KStandardShortcut::SaveOptions_DEPRECATED_DO_NOT_USE should not be needed 
> to be used here.
> Does the use of KF_DISABLE_DEPRECATED_BEFORE_AND_AT trigger this? If so, IMHO 
> ECMGenerateExportHeader needs to be fixed. Will have a look tomorrow latest.

Ah, KStandardShortcut is from KConfig. Though deprecated only at 5.39 as per 
API dox, so this would need another if/else... welcome to the deprecation 
jungle :)

I start to think that the *_BUILD_DEPRECATED_SINCE should just be limited to 
latest perhaps, given the size of KF modules and their interdependencies.

REPOSITORY
  R265 KConfigWidgets

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

To: dfaure, kossebau, elvisangelaccio, vkrause
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D24965: KConfigWidgets: port away from KF5 deprecated API

[David Faure]

dfaure added inline comments.

INLINE COMMENTS

> kossebau wrote in kstandardaction_p.h:103
> Hm, KStandardShortcut::SaveOptions_DEPRECATED_DO_NOT_USE should not be needed 
> to be used here.
> Does the use of KF_DISABLE_DEPRECATED_BEFORE_AND_AT trigger this? If so, IMHO 
> ECMGenerateExportHeader needs to be fixed. Will have a look tomorrow latest.

Yes this is triggered by KF_DISABLE*

This is exactly the same issue as the one we just talked about.
SaveOptions is not longer available when using KConfig without deprecated API.
I thought that was what the "DO_NOT_USE" alias was for: internal usage like the 
one here.
The alternative is to make SaveOptions available unconditionally again, as we 
just did with programIconName, but I thought this solution was actually better, 
since it will still prevent apps from using SaveOptions.

REPOSITORY
  R265 KConfigWidgets

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

To: dfaure, kossebau, elvisangelaccio, vkrause
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D24965: KConfigWidgets: port away from KF5 deprecated API

[Friedrich W. H. Kossebau]

kossebau added inline comments.

INLINE COMMENTS

> kstandardaction_p.h:103
>  #if KCONFIGWIDGETS_BUILD_DEPRECATED_SINCE(5, 38)
> -{ SaveOptions,   KStandardShortcut::SaveOptions, "options_save_options", 
> I18N_NOOP("&Save Settings"), nullptr, nullptr },
> +{ SaveOptions,   KStandardShortcut::SaveOptions_DEPRECATED_DO_NOT_USE, 
> "options_save_options", I18N_NOOP("&Save Settings"), nullptr, nullptr },
>  #endif

Hm, KStandardShortcut::SaveOptions_DEPRECATED_DO_NOT_USE should not be needed 
to be used here.
Does the use of KF_DISABLE_DEPRECATED_BEFORE_AND_AT trigger this? If so, IMHO 
ECMGenerateExportHeader needs to be fixed. Will have a look tomorrow latest.

REPOSITORY
  R265 KConfigWidgets

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

To: dfaure, kossebau, elvisangelaccio, vkrause
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D24965: KConfigWidgets: port away from KF5 deprecated API

[David Faure]

dfaure updated this revision to Diff 68797.
dfaure added a comment.


  Keep code for compat. Requires D24971 .

REPOSITORY
  R265 KConfigWidgets

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D24965?vs=68789&id=68797

BRANCH
  master

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

AFFECTED FILES
  CMakeLists.txt
  src/kstandardaction.cpp
  src/kstandardaction_p.h

To: dfaure, kossebau, elvisangelaccio, vkrause
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D24965: KConfigWidgets: port away from KF5 deprecated API

[David Faure]

dfaure added a comment.


  Sorry, you were very clear, I just forgot that the compiler warning comes 
from a different macro :-)

REPOSITORY
  R265 KConfigWidgets

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

To: dfaure, kossebau, elvisangelaccio, vkrause
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D24965: KConfigWidgets: port away from KF5 deprecated API

[Friedrich W. H. Kossebau]

kossebau added a comment.


  So seems I missed to make clear what I meant, next try :)
  
  The goal is: make clients (e.g. apps) which are still using this method to 
set the app icon aware by a compiler warning that they should port their code 
to use QApplication::setWindowIcon. For that we would keep 
KCOREADDONS_DEPRECATED_VERSION, as it is what sets the warning attribute, if 
asked by given settings.
  
  What we cannot do is to allow making the API invisible to the compiler in 
general, as infrastructure code support such legacy settings always needs to 
access it. So we remove the wrapping with #if/#endif and 
KCOREADDONS_ENABLE_DEPRECATED_SINCE, as that is the one which controls the 
visibility of the API.
  
  And for the very infrastructure code we also need to have a way to not get 
the deprecation warning, as for that using the API is fine still. So we need to 
tell the compiler somehow the deprecation warning in such usage needs to be 
ignored. One could do complicated logic to have the 
KCOREADDONS_DEPRECATED_VERSION care for another macro definition. But for now 
the most simply might be to just tell the compiler in the calling code to 
ignore for that line deprecation warnings.

REPOSITORY
  R265 KConfigWidgets

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

To: dfaure, kossebau, elvisangelaccio, vkrause
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D24965: KConfigWidgets: port away from KF5 deprecated API

[David Faure]

dfaure added a comment.


  Why would I need to push/pop warning, if the method call doesn't trigger any 
warning anymore?

REPOSITORY
  R265 KConfigWidgets

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

To: dfaure, kossebau, elvisangelaccio, vkrause
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D24965: KConfigWidgets: port away from KF5 deprecated API

[Friedrich W. H. Kossebau]

kossebau added a comment.


  In D24965#554393 , @dfaure wrote:
  
  > So basically we just use //KF6 TODO REMOVE for programIconName, but still 
use the macro around setProgramIconName? Works for me.
  
  
  Yes, remove the KCOREADDONS_ENABLE_DEPRECATED_SINCE(5, 2), or turn to 
KCOREADDONS_BUILD_DEPRECATED_SINCE(5, 2) for help those who already look into 
building without the feature (and would have to wrap API using code with the 
same condition).
  And wrap API using code like here, to support clients still usng that 
feature, with something along QT_WARNING_PUSH QT_WARNING_POP for the 
deprecations warning.

REPOSITORY
  R265 KConfigWidgets

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

To: dfaure, kossebau, elvisangelaccio, vkrause
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D24965: KConfigWidgets: port away from KF5 deprecated API

[David Faure]

dfaure added a comment.


  So basically we just use //KF6 TODO REMOVE for programIconName, but still use 
the macro around setProgramIconName? Works for me.

REPOSITORY
  R265 KConfigWidgets

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

To: dfaure, kossebau, elvisangelaccio, vkrause
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D24965: KConfigWidgets: port away from KF5 deprecated API

[Friedrich W. H. Kossebau]

kossebau added a comment.


  This falls into a category I also saw elsewhere doing all the patches, where 
the same API is used from two sides: the client side, like application code, 
and the serving side, like infrastructure.
  
  While we want to tell the client side, stop using this and for that also 
allow them to disable it, the serving side still needs to access it to support 
the legacy system.
  
  I had some ideas how to support this, though this would complicated all this 
fragile macro logic even more, so not sure this will work out.
  
  The other option is to remove the diabling wrappers around the 
programIconName API, so one can still use KF_DISABLE_DEPRECATED_BEFORE_AND_AT 
for the rests of the API. Actually I had done that in the other places where I 
came across that category. I would be in favour to do the same for 
programIconName. What do you think?

REPOSITORY
  R265 KConfigWidgets

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

To: dfaure, kossebau, elvisangelaccio, vkrause
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D24965: KConfigWidgets: port away from KF5 deprecated API

[David Faure]

dfaure added a comment.


  The goal is to set KF_DISABLE_DEPRECATED_BEFORE_AND_AT, so this isn't just 
about a warning, it's about API that no longer exists.
  But I guess we can't do that then, if we want to preserve compatible behavior 
until KF6...

REPOSITORY
  R265 KConfigWidgets

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

To: dfaure, kossebau, elvisangelaccio, vkrause
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D24965: KConfigWidgets: port away from KF5 deprecated API

[David Faure]

dfaure added a reviewer: vkrause.

REPOSITORY
  R265 KConfigWidgets

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

To: dfaure, kossebau, elvisangelaccio, vkrause
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D24965: KConfigWidgets: port away from KF5 deprecated API

[Friedrich W. H. Kossebau]

kossebau added a comment.


  While being deprecated, there might be code still using it, and you are 
killing support for that now, which is backwards-incompatibel.
  
  IMHO the support should be kept. And we want to rather have a generic way to 
disable the warning here (still TODO to have generic macros for this made 
available as well by the ECMGenerateExportHeader).

REPOSITORY
  R265 KConfigWidgets

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

To: dfaure, kossebau, elvisangelaccio
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D24965: KConfigWidgets: port away from KF5 deprecated API

[David Faure]

dfaure created this revision.
dfaure added reviewers: kossebau, elvisangelaccio.
Herald added a project: Frameworks.
Herald added a subscriber: kde-frameworks-devel.
dfaure requested review of this revision.

REVISION SUMMARY
  KAboutData::setProgramIconName is deprecated and says
  "Use QApplication::windowIcon", so I removed the code
  that reads from programIconName()

TEST PLAN
  Builds

REPOSITORY
  R265 KConfigWidgets

BRANCH
  master

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

AFFECTED FILES
  CMakeLists.txt
  src/kstandardaction.cpp
  src/kstandardaction_p.h

To: dfaure, kossebau, elvisangelaccio
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns