preferences: Add Plugin Manager
ClosedPublic

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

Diff Detail

Repository
rPTV Pitivi
Branch
T3193-pluginmanager
There are a very large number of changes, so older changes are hidden. Show Older Changes
cfoch added inline comments.Aug 4 2017, 8:11 PM
pitivi/dialogs/prefs.py
187

Do you mean just one line in the docstring?
Just """Adds a widget to the internal stack.""" ?

aleb added inline comments.Aug 4 2017, 8:21 PM
pitivi/dialogs/prefs.py
130–132
if section.startswith("_"):
    raise Exception("Cannot add preferences to reserved sections")
187

Yes. I think it's easy to see if section not in self.section_names and figure out you need to do something about that beforehand.

cfoch marked 9 inline comments as done.Aug 4 2017, 8:55 PM
cfoch updated this revision to Diff 6195.Aug 6 2017, 9:11 AM
cfoch marked 12 inline comments as done.
aleb added inline comments.Aug 6 2017, 2:43 PM
pitivi/dialogs/prefs.py
200

*Adds a page for the preferences in the specified section.

716

You could use`self`?

cfoch added inline comments.Aug 6 2017, 5:42 PM
pitivi/dialogs/prefs.py
716

what do you mean?

cfoch added inline comments.Aug 6 2017, 6:43 PM
pitivi/dialogs/prefs.py
113

Do you mean to remove the "try, except"?
To be congruent with https://phabricator.freedesktop.org/D1791 line 128.

aleb added inline comments.Aug 6 2017, 7:42 PM
pitivi/dialogs/prefs.py
113

It's fine like it is now.

716

Can you use self instead of self.app.gui.preferences_dialog

cfoch added inline comments.Aug 6 2017, 7:55 PM
pitivi/dialogs/prefs.py
716

I think I can't because PluginsBox is a ListBox and not a PreferencesDialog? I don't understand yet

aleb added inline comments.Aug 6 2017, 8:01 PM
pitivi/dialogs/prefs.py
716

yes, stupid question

717

Why is this needed?

aleb requested changes to this revision.Aug 14 2017, 4:10 AM
aleb added inline comments.
pitivi/dialogs/prefs.py
701

The preferences dialog should better not interact directly with the engine. It should call plugin_manager.load_plugin() instead, which would call engine.load_plugin() and emit a signal whether it succeeded or not! And the preferences dialog is connected to those signals and updates itself. The block below this line should be in PluginManager..

712

The dependant plugins logic should also be moved to the plugin manager. The preferences dialog should be connected to signals and update itself..

This revision now requires changes to proceed.Aug 14 2017, 4:10 AM
aleb requested changes to this revision.Aug 16 2017, 12:04 AM

only minor comments

pitivi/dialogs/prefs.py
587

newline above
"""A row in the plugins list allowing activating and deactivating a plugin."""

589

remove because unused?

628

newline above

636

newline above

680

This does a loop over the loaded plugins and for each item a loop over the rows. It could simply do a loop over the rows and check if the plugin is in loaded_plugins.. something like that. I'm only thinking it will get rid of this if, I don't really care about performance. :)

737

unused_engine
"""Handles the activation of one of the available plugins."""

742

"""Handles the deactivation of one of the available plugins."""

808

will we ever use auto_hide?

This revision now requires changes to proceed.Aug 16 2017, 12:04 AM
aleb added a comment.Aug 16 2017, 12:06 AM

commit message: "Add PluginManager" -> "prefs: Allow the user to control which plugins are loaded" ?

aleb added inline comments.Aug 16 2017, 11:06 PM
pitivi/dialogs/prefs.py
739

See http://lazka.github.io/pgi-docs/#GObject-2.0/classes/Object.html#GObject.Object.handler_block for how to use handler_block to block the signals emitted by switch when you set it active, because that would not be useful for us. A bit more efficient this way, important just for good practice.

Same change should be made in __unload_plugin_cb.

cfoch added inline comments.Aug 19 2017, 10:11 PM
pitivi/dialogs/prefs.py
808

The infobar is automatically auto hidden after 5 seconds.

cfoch added inline comments.Aug 20 2017, 12:13 AM
pitivi/dialogs/prefs.py
190

So, do I remove most of the docstring but keeping one line?

701

What should be the names of this/these signals?
"plugin-loaded-result" adding an argument True when loaded or False when not loaded?

cfoch added inline comments.Aug 20 2017, 12:18 AM
pitivi/dialogs/prefs.py
717

Without this, when you add a new page to the stack, the GtkStack sets the new page as visible instead of the PluginManager.

cfoch marked 14 inline comments as done.Aug 20 2017, 12:37 AM
cfoch added inline comments.
pitivi/dialogs/prefs.py
748

What was this comment about?

aleb added inline comments.Aug 20 2017, 9:31 AM
pitivi/dialogs/prefs.py
808

When?

aleb added inline comments.Aug 20 2017, 10:56 AM
pitivi/dialogs/prefs.py
190

You already addressed this

701

This is already done in __load_plugin_cb and __unload_plugin_cb

717

I wonder why

748

get_rows, I see it's gone now

cfoch marked 2 inline comments as done.Aug 20 2017, 3:52 PM
cfoch added inline comments.
pitivi/dialogs/prefs.py
808

If you try to turn on a switch, but for any reason the plugin can't be loaded, then the switch turns off and an infobar appears telling you that there was an error loading the plugin.

cfoch added inline comments.Aug 20 2017, 4:00 PM
pitivi/dialogs/prefs.py
701

So I don't add the method "load_plugin" to the PluginManager class?

717

Sorry, it is not necessary.

aleb added inline comments.Aug 20 2017, 6:05 PM
pitivi/dialogs/prefs.py
701

It's already done.

717

thanks for double checking

808

Sorry, I realize the question might have sounded stupid, but last time I checked it was not used.

Anyway, why do we want to auto hide that?

cfoch added inline comments.Aug 20 2017, 6:26 PM
pitivi/dialogs/prefs.py
808

Just for UX. I think that if the infobar keeps always there telling you that there was an error to load A and then you load B and you still see that infobar with the error of the previous plugin it may confuse you.

Anyway, other possibilities is to not care about that and keep the infobar can forever (at least until other message needs to be showed in the infobar). Or just hide the infobar when other plugin has been loaded successfully.

aleb requested changes to this revision.Aug 22 2017, 8:21 AM
aleb added inline comments.
pitivi/dialogs/prefs.py
193

This is a protected method. For the future, protected methods don't need this level of documentation. Look a bit: 3 lines of code and 5 lines of pydoc.

589

Remove show_builtins because it's unused? Also in PluginsBox

710

This can be gone by using self.list_store.preferences_dialog - you can rename it to dialog if you want to save some space

This revision now requires changes to proceed.Aug 22 2017, 8:21 AM
aleb accepted this revision.Aug 24 2017, 7:35 PM
This revision is now accepted and ready to land.Aug 24 2017, 7:35 PM
This revision was automatically updated to reflect the committed changes.