Create Task
Maniphest T14491

Rethink/Deprecate rarely used methods in KPluginLoader
Open, Needs TriagePublic
Actions

Description

  • KPluginLoader::forEachPlugin is very rarely used. When it is used it displays debug information, like that a plugin got considered for loading and warns if the MetaData is invalid. This warning can not be emitted in findPlugins filter callback, because an early return is made in that case.

My suggestion would be to print out a debug message in KPluginLoader::findPlugins if the MetaData is invalid and marking this method as internal.

  • KPluginLoader::instantiatePlugins is very rarely used. It is supposed to be a utility method, but it lacks debug output and requires a manual qobject_cast after calling the method.

The usage (and comment) of this method in knotificationmanager.cpp does not make sense to me, because KPluginLoader::findPlugins is used internally in KPluginLoader::instantiatePlugins. In dolphin I could not found any providers for the KOverlayIconPlugin. in ksystemstats unsing the KPluginFactory directly would be simpler anyways.

My suggestion would be to clean up/port those usages and deprecate the method.

Related Objects

alex created this task.May 20 2021, 8:04 AM
davidedmundson added a subscriber: davidedmundson.May 20 2021, 10:32 AM

Something being rarely used is not reason alone to remove it.

forEachPlugin is used internally by findPlugins, so we don't reduce code maintenance on that one.
I'm not seeing the reason to change it.

With instantiatePlugins I can understand what you're saying, if you need to loop through casting things to do anything useful, you lose all the benefit of the utility. I don't think it's a priority, but I wouldn't object to dropping it.

alex updated the task description. (Show Details)May 20 2021, 10:51 AM
alex added a comment.EditedMay 20 2021, 10:56 AM

forEachPlugin is used internally by findPlugins, so we don't reduce code maintenance on that one. I'm not seeing the reason to change it.

But having that in the public API suggests that it should be used externally when it is in truth more of an implementation detail. Especially considering that it is kindof misused.

I don't think it's a priority, but I wouldn't object to dropping it.

As described there are not that many users anyways and KF6 would be the time to get rid of it :)

alex moved this task from Backlog to Done on the KF6 board.Aug 22 2021, 3:57 PM

Because of T14303 consumers will have to touch their code anyways.

The solution is quite simple then: the methods did not get imported in KPluginMetaData and got deprecated with the entire KPluginLoader class.
The deprecation message reference the alternatives I outlined in the task description.

https://invent.kde.org/frameworks/kcoreaddons/-/commit/660d8179486269e2e88e1175571cf6808e05164e