Deprecate defunct functions
ClosedPublic

Authored by alex on Apr 29 2020, 4:48 PM.

Details

Summary

As in T12163 explained KRunner should be ported away from QWidgets and the
fuctions are defuct anyway.

Test Plan

Compiles

Diff Detail

Repository
R308 KRunner
Branch
deprecations (branched from master)
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 27061
Build 27079: arc lint + arc unit
alex created this revision.Apr 29 2020, 4:48 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptApr 29 2020, 4:48 PM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
alex requested review of this revision.Apr 29 2020, 4:48 PM
alex updated this revision to Diff 81528.Apr 29 2020, 4:50 PM

Add missing KRUNNER_DEPRECATED

Be aware that just adding a plain KRUNNER_DEPRECATED is not up-to-modern KF standards :)
Please see cf5f7b4040a77ae69452d58bc189dcc3baaedd92 what macros there are now and how to apply them. Note especially the difference between the ENABLE (for declarations, visible both to library and 3rd-party API users) & BUILD (visible only to library)), as well as that 5.0 can be only used in the API dox text, the macros need the version where the deprecation tags are first used/released with, to be backward-compatible for library consumers which use the KF_DISABLE_DEPRECATED_BEFORE_AND_AT control macro.

Please see also https://community.kde.org/Policies/Library_Code_Policy#Deprecation_of_API for general info about API deprecation in KF.

alex updated this revision to Diff 81539.Apr 29 2020, 6:35 PM

Use mordern macros

Thanks :-)

Looks good, almost :) It's a bit complicated on first use (well, also still ater, but no better solution was found to get version-controlled API deprecation with C++ for now).

src/abstractrunner.cpp
226

Given EXCLUDE_DEPRECATED_BEFORE_AND_AT is used with KRunner, you want to also wrap all the implementations of the now deprecated API with the BUILD variant of the macro, so here for example:

#if KRUNNER_BUILD_DEPRECATED_SINCE(5, 70)
void AbstractRunner::createRunOptions(QWidget *parent)
{
    [...]
}
#endif

This allows to build KRunner itself without any of the deprecated API actually part of the build.
Test yourself by setting the cmake var EXCLUDE_DEPRECATED_BEFORE_AND_AT=CURRENT (e.g. edit the var in the cmake cache).

src/abstractrunner.h
154

The text in the API dox can stay "Since 5.0".

The only crititical thing where the number of an older, already released version should not be used is the KRUNNER_ENABLE_DEPRECATED_SINCE macro. Because that reacts to KF_DISABLE_DEPRECATED_BEFORE_AND_AT (or KRUNNER_DISABLE_DEPRECATED_BEFORE_AND_AT if set). And someone using, say KF_DISABLE_DEPRECATED_BEFORE_AND_AT=0x050000 in their projects because they checked their software and it has not used any deprecated API up to that version, and who still uses this newly deprecated API, would suddenly get no longer building software, which especially is annoying with already released code.

So: when retroactively tagging deprecated API, in the API dox text mention the correct version where actual deprecation happened, in the macros the version of the upcoming release where the deprecation macros are first existing for API users. Makes sense?

alex added inline comments.Apr 29 2020, 6:59 PM
src/abstractrunner.h
154

That makes sense, but I want to express that this method has been defunct since version 5.0
so that the maintainers of existing plugins can be sure that they can savely remove the method calls.
Whats the best way to express this?

alex updated this revision to Diff 81543.Apr 29 2020, 7:06 PM

Wrap implementations

kossebau added inline comments.Apr 29 2020, 7:13 PM
src/abstractrunner.cpp
226

And best practice seems to be not to do mass wrapping of many methods, but do explicitely per method. While more code/lines, has the advantage to be easier to read/understand, as usually begin/emd are in view, and also avoids that actually wrong code is wrapped (e.g. when methods get added later and by accident are placed amid the wrapped ones.

src/abstractrunner.h
154

I would just say in the docs "@deprecated Since 5,0, this feature has been defunct". This is what has been done elsewhere where API got retrroactively tagged as deprecated. This is just text in the docs, now with up-to-date information, so other than the macros will not affect anything.

alex updated this revision to Diff 81546.Apr 29 2020, 7:21 PM

Wrap macro about single method, change deprecation text

And thanks for the feedback and explanations :-).

Looks good to me now, just caring about the use of the deprecation macros (no clue about the actual code, has to be Kai or someone else with krunner knowledge (if there is) to review).

Discussing this I see a small flaw in the case of retrospective deprecation when it comes to the difference of the version number used in the API dox comment (@deprecated Since 5,0, this feature has been defunct) to the version number use with the code deprecation macro (KRUNNER_DEPRECATED_VERSION(5, 70, "No longer use, feature removed") will resolve to the compiler warning ",Since 5 70. No longer use, feature removed"). But we cannot also use the true version there as well, otherwise might run into people who (in theory at least ) have -Werror=deprecated_declarations set and could now suddently trigger over the new warning, depending on what they set KF_DISABLE_WARNINGS_SINCE to... have to think about that some more how that can be resolved elegantly without more complexity, your current patch matches what other code does, so nothing to change here for now IMHO. If you have comments given your experience here, happy to hear.

alex updated this revision to Diff 83110.May 22 2020, 11:03 AM

Change deprecation version from 5.70 to 5.71

alex added a reviewer: meven.May 22 2020, 11:16 AM
kossebau added inline comments.May 22 2020, 11:22 AM
src/abstractrunner.h
154

Should be "5.0", not "5,0" here in the API dox text and elsewhere, no? (dot instead of comma)

156

ECMGenerateExportHeader now (for 5.71) features support for using a different version number to be shown in the compiler warning: KRUNNER_DEPRECATED_VERSION_BELATED

So you could make this:

KRUNNER_DEPRECATED_VERSION_BELATED(5, 71,  5, 0, "No longer use, feature removed")
alex updated this revision to Diff 83115.May 22 2020, 3:02 PM

Fix comma, KRUNNER_DEPRECATED_VERSION_BELATED macro

alex added inline comments.May 22 2020, 3:06 PM
src/abstractrunner.h
154

Yes. I accidentally did it this way, because in German you use a comma :D

156

This is pretty cool, many thanks again.

alex marked 9 inline comments as done.May 22 2020, 3:07 PM
meven added inline comments.May 22 2020, 4:58 PM
src/querymatch.cpp
338

Did you really need to remove this code ?

343

#if KRUNNER_BUILD_DEPRECATED_SINCE(5, 71)

alex added inline comments.May 22 2020, 5:01 PM
src/querymatch.cpp
338

The question this method should answer is: Does this match have a configuration interface?
And the answer is no, because the functionality does not exist anymore.
And if it is not removed a deprecated function would be called.

alex updated this revision to Diff 83119.May 22 2020, 5:16 PM

Fix typo in version number

alex marked an inline comment as done.May 22 2020, 5:16 PM
meven accepted this revision as: meven.May 23 2020, 6:38 AM

Nice

This revision is now accepted and ready to land.May 23 2020, 6:38 AM
alex closed this revision.May 23 2020, 7:36 AM

Gah, my bad for not catching this in the review. virtual methods need to be wrapped by the BUILD variant in the header. See also https://api.kde.org/ecm/module/ECMGenerateExportHeader.html?highlight=virtual

alex added a comment.Tue, Jul 14, 11:37 AM

Ufff, never thought sth. simple like deprecating a few functions could cause such an issue!

@davidre Thanks a lot for fixing this!