As in T12163 explained KRunner should be ported away from QWidgets and the
fuctions are defuct anyway.
Details
- Reviewers
broulik davidedmundson vkrause meven - Group Reviewers
Plasma - Commits
- R308:fc5738ab7c40: Deprecate defunct functions
Compiles
Diff Detail
- Repository
- R308 KRunner
- Branch
- deprecations (branched from master)
- Lint
No Linters Available - Unit
No Unit Test Coverage - Build Status
Buildable 27053 Build 27071: arc lint + arc unit
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.
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. | |
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? |
src/abstractrunner.h | ||
---|---|---|
154 | That makes sense, but I want to express that this method has been defunct since version 5.0 |
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. |
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.
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") |
src/querymatch.cpp | ||
---|---|---|
338 | The question this method should answer is: Does this match have a configuration interface? |
This caused https://bugs.kde.org/show_bug.cgi?id=423003. I removed excluding the virtual method from the build in
https://invent.kde.org/frameworks/krunner/commit/8f7ce559b84ee0c21de0256e6591793e4b95f411
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
Ufff, never thought sth. simple like deprecating a few functions could cause such an issue!
@davidre Thanks a lot for fixing this!