use KPluginSelector to enable runners
ClosedPublic

Authored by mart on Sep 27 2016, 1:40 PM.

Details

Summary

the configuration dialog for enabling runners by category
was a neat idea but had several problems, gave by it
reinventing the wheel too much.
when changing the locale, the enabled categories list was resetted
due to categories being localized. it was also pretty
difficult to disable a particular plugin as you would have to
know what categoriesyou have to disable.

this approach is more basic, but reusing a common implementation,
we avoid some bugs and misbehaviors.

BUG:350779

Test Plan

ran on an existing krunnerrc with different sets of categories enabled, tested krunner and its kcm with it, all expected runner plugins were used both in the kcm and while searching with krunner

Diff Detail

Repository
R119 Plasma Desktop
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
mart updated this revision to Diff 6951.Sep 27 2016, 1:40 PM
mart retitled this revision from to use KPluginSelector to enable runners.
mart updated this object.
mart edited the test plan for this revision. (Show Details)
Restricted Application added a project: Plasma. · View Herald TranscriptSep 27 2016, 1:40 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
mart updated this revision to Diff 6952.Sep 27 2016, 1:42 PM
  • remove dead code
davidedmundson requested changes to this revision.Sep 27 2016, 1:48 PM
davidedmundson added a reviewer: davidedmundson.
davidedmundson added a subscriber: davidedmundson.

Testing done section should not be blank.

Also there's no migration path. So instead of it breaking it on locale change we break it for everyone on upgrade.
Worst, krunner framework still reads enabledCategories, so if something was disabled there's no way to enable it.

Finally this leaves frameworks krunner documentation being wrong.

This revision now requires changes to proceed.Sep 27 2016, 1:48 PM
mart added a comment.EditedSep 27 2016, 2:13 PM

Worst, krunner framework still reads enabledCategories, so if something was disabled there's no way to enable it.

Finally this leaves frameworks krunner documentation being wrong.

i did test it and it appeared to work correctly.
I incorrectly assumed that this categories->plugin translation was done at kcm level, while in reality things are more broken than that.
It was appearing to work correctly because i'm seeing now into krunner framework code, it's actually using a quite weird mixup of looking at categories and the plugin based one, so if a category is enabled (or all categories are disabled), if i enable/disable the plugin, the plugin will get enabled/disabled correctly.

since instead changes in the framework will be needed, this makes things a bit more difficult, because it becomes difficult to see what would happen when a newer framework would be used on old workspace.

probably the only way things would work is:

  • framework keeping supporting both categories and plugins as is now
  • workspace (not the framework) gets a kconfigupdate that converts the list of categories to list of plugins, removing completely the categoriesenabled key, this (not sure if intended but already happens in the framework) makes the behavior revert completely to plugin-based config
mart edited the test plan for this revision. (Show Details)Sep 27 2016, 3:46 PM
mart edited edge metadata.

with the kconfig update script of https://phabricator.kde.org/D2873 the krunner configuration seems to have proper plugins enabled

davidedmundson accepted this revision.Sep 27 2016, 3:52 PM
davidedmundson edited edge metadata.
This revision is now accepted and ready to land.Sep 27 2016, 3:52 PM
broulik added inline comments.Sep 29 2016, 10:12 AM
kcms/runners/kcm.cpp
47 ↗(On Diff #6952)

Are you even using that still? All I see is a KSharedConfig::openConfig in load

76 ↗(On Diff #6952)

Sure you can, if you cast :)

This revision was automatically updated to reflect the committed changes.