pluginmanager: Write and read activated plugins from the configuration file
ClosedPublic

Authored by cfoch on Jul 28 2017, 8:29 PM.

Diff Detail

Repository
rPTV Pitivi
Branch
T3193-pluginmanager
cfoch updated this revision to Diff 6164.Jul 28 2017, 8:38 PM
cfoch retitled this revision from pluginmanager: Write and read activated plugins from the configuration file. to pluginmanager: Write and read activated plugins from the configuration file.
aleb requested changes to this revision.Aug 14 2017, 3:23 AM
aleb added inline comments.
pitivi/pluginmanager.py
82
if plugin_info not in self.plugins:
    self.warning("Plugin missing: %s", plugin_name)
    continue
self.engine.load_plugin(plugin_info)
116

"""Handles the addition of a window to the application."""
This assumes the first window is the main window, which is true currently. It could be very easy to check that isinstance(window, MainWindow) to make it more future proof and clarify the intention.

118

unused_pspec
"""Handles the changing of the loaded plugins list."""

pitivi/settings.py
141

fix duplication

154

quotes

162

quotes

166

The comment does not make sense. Storing an empty list is a valid use-case. The default value could be very well a non-empty list. Maybe extend the unit test with this case?

tests/test_plugin_manager.py
74

I would just call the signal handler directly from the test. It's too much noise to have a custom App class, and it does not add much. I would remove it, but your choice.

This revision now requires changes to proceed.Aug 14 2017, 3:23 AM
cfoch added inline comments.Aug 15 2017, 5:27 PM
pitivi/pluginmanager.py
116
if not isinstance(window, MainWindow):
    return
self._load_plugins()

?

aleb added inline comments.Aug 15 2017, 6:01 PM
pitivi/pluginmanager.py
116

Does it work? :D

cfoch added inline comments.Aug 15 2017, 7:52 PM
pitivi/pluginmanager.py
116

Yes, and without this it also work. Do I include

if not isinstance(window, MainWindow):
    return
self._load_plugins()

or not?

aleb requested changes to this revision.Aug 15 2017, 11:18 PM
aleb added inline comments.
pitivi/pluginmanager.py
64

Seems a bit more clean to connect this only after the plugins are loaded.

This revision now requires changes to proceed.Aug 15 2017, 11:18 PM
cfoch added inline comments.Aug 19 2017, 10:23 PM
pitivi/pluginmanager.py
64

connect_after?

aleb added inline comments.Aug 20 2017, 9:29 AM
pitivi/pluginmanager.py
64

No, I mean in __window_added_cb

cfoch added inline comments.Aug 20 2017, 4:25 PM
pitivi/pluginmanager.py
64

So I think it should be kept as it is. Because, what I actually want is that the MainWindow is created before loading plugins.

aleb requested changes to this revision.Aug 20 2017, 6:35 PM
aleb added inline comments.
pitivi/pluginmanager.py
63

Should we disconnect from this after the first window is loaded?

64

It does not make sense. This connect is done to keep track of the loaded plugins. This should be started only after the plugins are loaded.

This revision now requires changes to proceed.Aug 20 2017, 6:35 PM
cfoch added inline comments.Aug 21 2017, 2:46 AM
pitivi/pluginmanager.py
63

Yes. After that I suspect that signal will not be emitted any more.

64

I don't understand TBH if we are talking about "window-added" or "notify::loaded-plugins".

aleb added inline comments.Aug 21 2017, 6:39 AM
pitivi/pluginmanager.py
64

You can click the |<< icon to see the original line for which the comment was made. We're talking about self.engine.connect(...)

aleb requested changes to this revision.Aug 22 2017, 8:42 AM
aleb added inline comments.
tests/test_plugin_manager.py
4

2017

71

If you run this test, does it override ActivePlugins in ~/.config/pitivi/pitivi.conf? (I can check when I'm back at my computer)

This revision now requires changes to proceed.Aug 22 2017, 8:42 AM
cfoch added inline comments.Aug 22 2017, 1:36 PM
tests/test_plugin_manager.py
71

No, because I am not calling
_writeSettingsToConfigurationFile

aleb added inline comments.Aug 22 2017, 1:54 PM
tests/test_plugin_manager.py
71

πŸ‘

aleb accepted this revision.Aug 22 2017, 10:32 PM
This revision is now accepted and ready to land.Aug 22 2017, 10:32 PM
This revision was automatically updated to reflect the committed changes.