Fix registration of the enabled state of plugins
ClosedPublic

Authored by kossebau on Oct 6 2017, 4:03 AM.

Details

Summary

The old code was slightly broken:
At start-up plugin controller updated the config storage only for the
enabled state of the global-category plugins (and indirectly their
dependency plugins). Project-category plugins would only get an update
indirectly once loadProjectPlugins() was run, other plugins only once
the were requested by their interface.
Thus in a new session with no project yet loaded the config storage would
not yet cover the enabled state of the project-category plugins and those
other.

PluginPreferences code queried the initial enabled state of each plugin
from the plugin controller, which then has extra code in the isEnabled()
method to deal with that. Though that information is dropped and replaced
with the one fetched from the config storage, multiple times even:

  • the plugins are added to the selector using the flag KPluginSelector::ReadConfigFile, which results in the data manually set to each KPluginInfo instance with kpi.setPluginEnabled(...) being overwritten with the config storage data
  • selector->load() was called after plugins were added, which results in the same update from the config storage data
  • the PluginPreferences instance like each KDevelop::ConfigPage gets an initial call of the reset() method on setup, which again calls selector->load()

Which results in the plugin selector showing wrong enabled state at least
for new sessions, where no project has been loaded yet and not all plugins
at least been tried to load once.
While the first two times of data drop could be fixed, the third would be
more complicted to do.

So instead of keeping the current on-the-fly enabled state calculation for
non-global-category plugins, on start-up the config storage is updated for
any known plugin (and then consecutively updated if failing to load a
plugin when needed). This gives the plugin selector and any other code
consistent data and simplifies things.

Next to this, this patch also restores ShellExtension::defaultPlugins()
power to control which plugins should be loaded by default. Thus any
tests which (try to) make use of the AutoTestShell::init plugin list
feature can (and have to) now explicitly define all userselectable plugins
which should be used (thus also using the correct plugin id :) ).

Test Plan

Create a new session, open directly the plugin selection settings and see
that all build and projectmanager plugins but also the QML plugin are shown
as enabled, as they should be.

Diff Detail

Repository
R32 KDevelop
Branch
fixEstimatingDefaultEnabledPlugin_v1
Lint
No Linters Available
Unit
No Unit Test Coverage
kossebau created this revision.Oct 6 2017, 4:03 AM
Restricted Application added a subscriber: kdevelop-devel. · View Herald TranscriptOct 6 2017, 4:03 AM
mwolff added a subscriber: mwolff.Oct 6 2017, 12:13 PM

Could you write a unit test for the broken behavior? It seems to be somewhat related to the changes done to the AutoTestShell::init calls, so writing an explicit test for that which fails beforehand and passes now would be appreciated.

I also have to admit that I'm having a hard time reviewing the plugincontroller.cpp changes. Having concrete unit tests for that would be helpful to show that this doesn't break anything.

Am creating unit tests next, stay tuned today...

kossebau updated this revision to Diff 20402.Oct 6 2017, 8:15 PM

added some little unit test (well...)

mwolff requested changes to this revision.Oct 8 2017, 7:25 PM

awesome, thanks

kdevplatform/shell/tests/test_pluginenabling.cpp
45 ↗(On Diff #20402)

personally I'd prefer separate test functions instead of ifdef. afaik you can de-init the shell completely nowadays (or not?). so you could have one test method for the default scenario, and another one for the custom plugins

61 ↗(On Diff #20402)

if empty, remove those methods

This revision now requires changes to proceed.Oct 8 2017, 7:25 PM
kossebau added inline comments.Oct 8 2017, 7:39 PM
kdevplatform/shell/tests/test_pluginenabling.cpp
45 ↗(On Diff #20402)

Problem here is that we want to especially test the initial setup when plugincontroller is instantiated.
So it needs to be two different tests.

Would you prefer going for subclassing the test instead and doing the specialization there?
Personally would prefer the ifdef over that, as this way there is a one file to look at.
But as you prefer.

61 ↗(On Diff #20402)

Was copied over from some other test. Shall I then clean up the other tests as well from empty methods?

kossebau added inline comments.Oct 8 2017, 8:19 PM
kdevplatform/shell/tests/test_pluginenabling.cpp
45 ↗(On Diff #20402)

when plugincontroller is instantiated.

Which especially means a new session created, without any config yet stored.

Or did you mean to move the TestCore::initialize() & TestCore::shutdown() out of the init and cleanup methods and instead use them directly in the test functions, with different session arguments?
Hm... seems no other code does it. Though the API dox hints one might be able to use it more than once, and the current implementation on a first view does also not advise against it. So, shall I give that a try?

mwolff added inline comments.Oct 9 2017, 7:47 PM
kdevplatform/shell/tests/test_pluginenabling.cpp
45 ↗(On Diff #20402)

Yes, that was what I had in mind. Just init + shutdown the TestCore within the tests. What might be tricky is shutting down the AutoTestShell, not sure that's supported yet. That said, maybe you should push this as-is and we can clean it up in the future (i.e., probably never). But it's not too bad to hold up your overall change, I guess.

61 ↗(On Diff #20402)

Yes, in separate patches though.

kossebau updated this revision to Diff 20570.Oct 10 2017, 3:34 PM
kossebau marked 2 inline comments as done.

merge both tests variants into one test, doing core setup/shutdown in each
test run

kossebau added inline comments.Oct 10 2017, 3:37 PM
kdevplatform/shell/tests/test_pluginenabling.cpp
45 ↗(On Diff #20402)

Uploaded now a variant with init + shutdown of the TestCore within the tests. Not sure this is a lot better, but your call :)

Please have a look at the two TODOs in TestPluginEnabling::loadPluginCustomDefaults(), any feedback on those?

@mwolff So what do you like more, the new/current test code approach (with TestCore::init/shutdowndone on each test) or the old one? Any comments on the TODOs in the test code?

Any other issues, or could this be pushed? (Follow-up work planned to sort-out other plugin-enabling reality distortions)

mwolff accepted this revision.Oct 16 2017, 6:02 PM

minor nits, otherwise lgtm

kdevplatform/shell/plugincontroller.cpp
748

yes, what about them? Sorry, but it's been a long time since I worked on this code. Can you elaborate the problems you are seeing?

kdevplatform/shell/tests/plugins/globaldefaultplugin.cpp
30

here and below, make ctors explicit

kdevplatform/shell/tests/plugins/nonguiinterfaceplugin.cpp
33

explicit

kdevplatform/shell/tests/plugins/projectdefaultplugin.cpp
30

explicit

kdevplatform/shell/tests/plugins/projectnondefaultplugin.cpp
30

explicit

kdevplatform/shell/tests/test_pluginenabling.cpp
100

I'd have to debug that myself, it should work... maybe add a signal spy on some signal to ensure the core gets destroyed or otherwise shutdown properly? I.e. maybe we need to run the event loop?

103

sure, why not. So far, the empty name == temporary rule worked fine afaik. If there's more needed, extend the API as needed.

This revision is now accepted and ready to land.Oct 16 2017, 6:02 PM
This revision was automatically updated to reflect the committed changes.
kossebau marked 4 inline comments as done.