D29281: Deprecate defunct functions

[Alexander Lohnau]

alex added a comment.


  Ufff, never thought sth. simple like deprecating a few functions could cause 
such an issue!
  
  @davidre Thanks a lot for fixing this!

REPOSITORY
  R308 KRunner

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

To: alex, #plasma, broulik, davidedmundson, vkrause, meven
Cc: davidre, kossebau, kde-frameworks-devel, LeGast00n, cblack, michaelh, 
ngraham, bruns

D29281: Deprecate defunct functions

[Friedrich W. H. Kossebau]

kossebau added a comment.


  In D29281#675955 , @davidre wrote:
  
  > 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

REPOSITORY
  R308 KRunner

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

To: alex, #plasma, broulik, davidedmundson, vkrause, meven
Cc: davidre, kossebau, kde-frameworks-devel, LeGast00n, cblack, michaelh, 
ngraham, bruns

D29281: Deprecate defunct functions

[David Redondo]

davidre added a comment.


  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

REPOSITORY
  R308 KRunner

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

To: alex, #plasma, broulik, davidedmundson, vkrause, meven
Cc: davidre, kossebau, kde-frameworks-devel, LeGast00n, cblack, michaelh, 
ngraham, bruns

D29281: Deprecate defunct functions

[Alexander Lohnau]

alex closed this revision.

REPOSITORY
  R308 KRunner

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

To: alex, #plasma, broulik, davidedmundson, vkrause, meven
Cc: kossebau, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29281: Deprecate defunct functions

[Méven Car]

meven accepted this revision as: meven.
meven added a comment.
This revision is now accepted and ready to land.


  Nice

REPOSITORY
  R308 KRunner

BRANCH
  deprecations (branched from master)

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

To: alex, #plasma, broulik, davidedmundson, vkrause, meven
Cc: kossebau, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29281: Deprecate defunct functions

[Alexander Lohnau]

alex marked an inline comment as done.

REPOSITORY
  R308 KRunner

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

To: alex, #plasma, broulik, davidedmundson, vkrause, meven
Cc: kossebau, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29281: Deprecate defunct functions

[Alexander Lohnau]

alex updated this revision to Diff 83119.
alex added a comment.


  Fix typo in version number

REPOSITORY
  R308 KRunner

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29281?vs=83115=83119

BRANCH
  deprecations (branched from master)

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

AFFECTED FILES
  src/CMakeLists.txt
  src/abstractrunner.cpp
  src/abstractrunner.h
  src/querymatch.cpp
  src/querymatch.h

To: alex, #plasma, broulik, davidedmundson, vkrause, meven
Cc: kossebau, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29281: Deprecate defunct functions

[Alexander Lohnau]

alex added inline comments.

INLINE COMMENTS

> meven wrote in querymatch.cpp:338
> Did you really need to remove this code ?

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.

REPOSITORY
  R308 KRunner

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

To: alex, #plasma, broulik, davidedmundson, vkrause, meven
Cc: kossebau, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29281: Deprecate defunct functions

[Méven Car]

meven added inline comments.

INLINE COMMENTS

> querymatch.cpp:338
>  {
> -return d->runner && d->runner.data()->hasRunOptions();
>  }

Did you really need to remove this code ?

> querymatch.cpp:343
>  
> +#if KRUNNER_BUILD_DEPRECATED_SINCE(5, 1)
>  void QueryMatch::createConfigurationInterface(QWidget *parent)

#if KRUNNER_BUILD_DEPRECATED_SINCE(5, 71)

REPOSITORY
  R308 KRunner

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

To: alex, #plasma, broulik, davidedmundson, vkrause, meven
Cc: kossebau, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29281: Deprecate defunct functions

[Alexander Lohnau]

alex added inline comments.

INLINE COMMENTS

> kossebau wrote in abstractrunner.h:154
> Should be "5.0", not "5,0" here in the API dox text and elsewhere, no? (dot 
> instead of comma)

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

> kossebau wrote in abstractrunner.h: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")

This is pretty cool, many thanks again.

REPOSITORY
  R308 KRunner

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

To: alex, #plasma, broulik, davidedmundson, vkrause, meven
Cc: kossebau, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29281: Deprecate defunct functions

[Alexander Lohnau]

alex marked 9 inline comments as done.

REPOSITORY
  R308 KRunner

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

To: alex, #plasma, broulik, davidedmundson, vkrause, meven
Cc: kossebau, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29281: Deprecate defunct functions

[Alexander Lohnau]

alex updated this revision to Diff 83115.
alex added a comment.


  Fix comma, KRUNNER_DEPRECATED_VERSION_BELATED macro

REPOSITORY
  R308 KRunner

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29281?vs=83110=83115

BRANCH
  deprecations (branched from master)

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

AFFECTED FILES
  src/CMakeLists.txt
  src/abstractrunner.cpp
  src/abstractrunner.h
  src/querymatch.cpp
  src/querymatch.h

To: alex, #plasma, broulik, davidedmundson, vkrause, meven
Cc: kossebau, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29281: Deprecate defunct functions

[Friedrich W. H. Kossebau]

kossebau added inline comments.

INLINE COMMENTS

> abstractrunner.h:154
>   * is called, the runner should return true
> + * @deprecated Since 5,0, this feature has been defunct
>   */

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

> abstractrunner.h:156
>   */
> +KRUNNER_DEPRECATED_VERSION(5, 71, "No longer use, feature removed")
>  bool hasRunOptions();

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

REPOSITORY
  R308 KRunner

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

To: alex, #plasma, broulik, davidedmundson, vkrause, meven
Cc: kossebau, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29281: Deprecate defunct functions

[Alexander Lohnau]

alex added a reviewer: meven.

REPOSITORY
  R308 KRunner

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

To: alex, #plasma, broulik, davidedmundson, vkrause, meven
Cc: kossebau, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29281: Deprecate defunct functions

[Alexander Lohnau]

alex updated this revision to Diff 83110.
alex added a comment.


  Change deprecation version from 5.70 to 5.71

REPOSITORY
  R308 KRunner

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29281?vs=81546=83110

BRANCH
  deprecations (branched from master)

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

AFFECTED FILES
  src/CMakeLists.txt
  src/abstractrunner.cpp
  src/abstractrunner.h
  src/querymatch.cpp
  src/querymatch.h

To: alex, #plasma, broulik, davidedmundson, vkrause
Cc: kossebau, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29281: Deprecate defunct functions

[Friedrich W. H. Kossebau]

kossebau added a comment.


  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.

REPOSITORY
  R308 KRunner

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

To: alex, #plasma, broulik, davidedmundson, vkrause
Cc: kossebau, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29281: Deprecate defunct functions

[Alexander Lohnau]

alex updated this revision to Diff 81546.
alex added a comment.


  Wrap macro about single method, change deprecation text
  
  And thanks for the feedback and explanations :-).

REPOSITORY
  R308 KRunner

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29281?vs=81543=81546

BRANCH
  deprecations (branched from master)

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

AFFECTED FILES
  src/CMakeLists.txt
  src/abstractrunner.cpp
  src/abstractrunner.h
  src/querymatch.cpp
  src/querymatch.h

To: alex, #plasma, broulik, davidedmundson, vkrause
Cc: kossebau, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29281: Deprecate defunct functions

[Friedrich W. H. Kossebau]

kossebau added inline comments.

INLINE COMMENTS

> kossebau wrote in abstractrunner.cpp:221
> 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).

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.

> alex wrote in 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?

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.

REPOSITORY
  R308 KRunner

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

To: alex, #plasma, broulik, davidedmundson, vkrause
Cc: kossebau, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29281: Deprecate defunct functions

[Alexander Lohnau]

alex updated this revision to Diff 81543.
alex added a comment.


  Wrap implementations

REPOSITORY
  R308 KRunner

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29281?vs=81539=81543

BRANCH
  deprecations (branched from master)

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

AFFECTED FILES
  src/CMakeLists.txt
  src/abstractrunner.cpp
  src/abstractrunner.h
  src/querymatch.cpp
  src/querymatch.h

To: alex, #plasma, broulik, davidedmundson, vkrause
Cc: kossebau, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29281: Deprecate defunct functions

[Alexander Lohnau]

alex added inline comments.

INLINE COMMENTS

> kossebau wrote in 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=0x05 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?

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?

REPOSITORY
  R308 KRunner

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

To: alex, #plasma, broulik, davidedmundson, vkrause
Cc: kossebau, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29281: Deprecate defunct functions

[Friedrich W. H. Kossebau]

kossebau added a comment.


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

INLINE COMMENTS

> abstractrunner.cpp:221
>  
>  void AbstractRunner::createRunOptions(QWidget *parent)
>  {

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

> abstractrunner.h:154
>   * is called, the runner should return true
> + * @deprecated since 5.70, feature removed since version 5.0
>   */

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=0x05 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?

REPOSITORY
  R308 KRunner

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

To: alex, #plasma, broulik, davidedmundson, vkrause
Cc: kossebau, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29281: Deprecate defunct functions

[Alexander Lohnau]

alex updated this revision to Diff 81539.
alex added a comment.


  Use mordern macros
  
  Thanks :-)

REPOSITORY
  R308 KRunner

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29281?vs=81528=81539

BRANCH
  deprecations (branched from master)

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

AFFECTED FILES
  src/CMakeLists.txt
  src/abstractrunner.cpp
  src/abstractrunner.h
  src/querymatch.cpp
  src/querymatch.h

To: alex, #plasma, broulik, davidedmundson, vkrause
Cc: kossebau, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29281: Deprecate defunct functions

[Friedrich W. H. Kossebau]

kossebau added a comment.


  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.

REPOSITORY
  R308 KRunner

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

To: alex, #plasma, broulik, davidedmundson, vkrause
Cc: kossebau, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29281: Deprecate defunct functions

[Alexander Lohnau]

alex updated this revision to Diff 81528.
alex added a comment.


  Add missing KRUNNER_DEPRECATED

REPOSITORY
  R308 KRunner

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29281?vs=81527=81528

BRANCH
  deprecations (branched from master)

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

AFFECTED FILES
  src/abstractrunner.cpp
  src/abstractrunner.h
  src/querymatch.cpp
  src/querymatch.h

To: alex, #plasma, broulik, davidedmundson, vkrause
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29281: Deprecate defunct functions

[Alexander Lohnau]

alex created this revision.
alex added reviewers: Plasma, broulik, davidedmundson, vkrause.
Herald added a project: Frameworks.
Herald added a subscriber: kde-frameworks-devel.
alex requested review of this revision.

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

TEST PLAN
  Compiles

REPOSITORY
  R308 KRunner

BRANCH
  deprecations (branched from master)

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

AFFECTED FILES
  src/abstractrunner.cpp
  src/abstractrunner.h
  src/querymatch.cpp
  src/querymatch.h

To: alex, #plasma, broulik, davidedmundson, vkrause
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns