KConfigWidgets: port away from KF5 deprecated API
ClosedPublic

Authored by dfaure on Oct 26 2019, 2:39 PM.

Details

Summary

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

Test Plan

Builds

Diff Detail

Repository
R265 KConfigWidgets
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 18201
Build 18219: arc lint + arc unit
dfaure created this revision.Oct 26 2019, 2:39 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptOct 26 2019, 2:39 PM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
dfaure requested review of this revision.Oct 26 2019, 2:39 PM
kossebau added a comment.EditedOct 26 2019, 3:00 PM

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).

Deprecated != Disabled.

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...

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?

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

kossebau added a comment.EditedOct 26 2019, 4:17 PM

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, as the deprecation warning is not targetted for this legay-support code here.

This comment was removed by dfaure.

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.

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

dfaure updated this revision to Diff 68797.Oct 26 2019, 4:52 PM

Keep code for compat. Requires D24971.

kossebau added inline comments.Oct 26 2019, 5:07 PM
src/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.

dfaure added inline comments.Oct 26 2019, 5:12 PM
src/kstandardaction_p.h
103

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.

kossebau added inline comments.Oct 26 2019, 5:12 PM
src/kstandardaction_p.h
103

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.

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.

kossebau accepted this revision.Oct 30 2019, 1:21 AM

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.

This revision is now accepted and ready to land.Oct 30 2019, 1:21 AM
dfaure closed this revision.Wed, Nov 20, 10:12 PM