pluginmanager: Write and read activated plugins from the configuration file.
AbandonedPublic

Authored by cfoch on Jul 20 2017, 5:16 PM.

Details

Reviewers
thiblahute
aleb
Summary

Depends on D1748

Diff Detail

Repository
rPTV Pitivi
Branch
pluginmanager
cfoch created this revision.Jul 20 2017, 5:16 PM
aleb requested changes to this revision.Jul 21 2017, 3:01 PM

Lots of comments, most of them details and polishing, and me not understanding things.

This bindProperty mechanism .. is pretty complex, and I don't think we'll use it again very soon.
It's cool to see what you can do with Python, but please avoid doing complex things like this unless it's really necessary.
You could have very well connected manually to the self.engine's property to monitor when the setting should be updated.

pitivi/pluginmanager.py
29

Make sure you are always using quotes for strings, for consistency. Except when the string has quotes, in this case in other places we use '.

50

Add a comment to explain the reason why we load the plugins only after the window is added.
Also, which window is that exactly? the main window, or the startup wizard, should we double check?
Would it be better to create the plugin manager only after the main window is created? Or what's the point of connecting to this signal?

51

Add a comment to explain why this binding is done, something like:

# Bind the engine's loaded-plugins property so when a plugin
# is loaded or unloaded, the updated list of loaded plugins is stored
# in the PluginsActivePlugins setting.
101

Maybe rename the method to _load_plugins(names) and pass the argument here.

pitivi/settings.py
140

_read_value

158

_write_value

258

First line of a pydoc must be a complete sentence with dot at the end.
But this is used in a single place, better integrate it there, since it's two lines of code.. Same with setListStr

296

Maybe change the condition on line 224 from if value is not None to if value so you don't do it here.

298

bind_property

305

This should be in a "Raises:" section. run this for examples:
$ ag Raises pitivi -A 1

318

There is no point if using elif, use if

This revision now requires changes to proceed.Jul 21 2017, 3:01 PM
aleb added inline comments.Jul 21 2017, 7:35 PM
pitivi/pluginmanager.py
30

it's anyway in the "plugins" section, maybe use "ActivePlugins"

cfoch added inline comments.Jul 23 2017, 6:13 PM
pitivi/settings.py
258

Isn't it better to separate things in methods when possible?
Maybe instead of setListStr it should be a privated method renamed to _set_list_str.
I did this because in the future more types could be added and _readValue's (now _read_value) code would growth too much. Actually, that happened because some commits after I introduced get_rgba/set_rgba methods. Did I convince you to keep it as _set_list_str?

aleb added inline comments.Jul 23 2017, 6:21 PM
pitivi/settings.py
258

If the method is very thin, it does not make sense IMO.

cfoch added inline comments.Jul 23 2017, 6:22 PM
pitivi/settings.py
318

I understood that you prefer I do something more simple instead of using bindProperty. Right? Why are you commenting for changes in this method? It confuses me, sorry.

cfoch added inline comments.Jul 23 2017, 6:26 PM
pitivi/settings.py
258

ok, no problem

aleb added inline comments.Jul 23 2017, 6:33 PM
pitivi/settings.py
318

I might be wrong but I think it will never be used again, and being too sophisticated, it's better to keep it in the other place. I could have removed them but if you read them it won't harm. :)

cfoch added inline comments.Jul 23 2017, 6:33 PM
pitivi/settings.py
258

What should I do with the docstring? Do I put it as a big comment in the "elif" branch of _read_value/_write_value? Or I put it as part of the _read_value and _write_value's docstring? Or just I forget of it and I discard that documentation :) ?

cfoch added inline comments.Jul 23 2017, 6:38 PM
pitivi/settings.py
318

Ok, so I am replacing it with something more simple :)

aleb added inline comments.Jul 23 2017, 7:15 PM
pitivi/settings.py
258

It's pretty clear when you look at the "set" code, I'd discard it.

cfoch marked 2 inline comments as done.Jul 23 2017, 7:36 PM
cfoch added inline comments.
pitivi/pluginmanager.py
50

Yes, it is the main window. It is needed when plugins are loaded from settings. When you start Pitivi and you load plugins from settings, for example the developer console, it needs to access part of the main window, but by that time the main-window doesn't exist yet.

Actually, I think that it would be better to load plugins from settings when "everything" has loaded in Pitivi.

cfoch abandoned this revision.Jul 28 2017, 8:44 PM
cfoch marked 15 inline comments as done.