Fix #588 - Refactor plugins.

[FranciscoCanas]

• Added settings.py with PluginConfig.
• Rewrote jackaudio's load_init()

[FranciscoCanas]

Should the Configs be persisted to plugin.conf even when the user doesn't change any of the default values for that config?

[mtomwing]

There are a lot more places where ConfigurablePluginManager is used. You'll have to remove anything that interacts with it or plugin.conf directly.

[FranciscoCanas]

@mtomwing: I found that PluginManager's set_plugin_option and get_plugin_option still use ConfigurablePluginManager, so I would like to reimplement them using the Config class.

However, I don't see of a way to read the values of an option from an instance of Config using only the name of the option. Config.get_value() requires the option instance itself as an argument. Is there a specific way I can map from an option's string name -> option's value?

[mtomwing]

It's going to take me another day or so to completely vet the changes. There appears to be a bug with the mixer plugin configs but I'm not entirely sure if it exists in master too.

[FranciscoCanas]

I can look into it this afternoon. If you have time, let me know how to reproduce it.

[mtomwing]

IIRC:

1. nuke freeseer configs
2. start freeseer, open config tool
3. change video input mixer to pip
4. press setup for pip
5. check ~/.freeseer/profiles/default/plugin.conf and see that there is only 1 video test input instance
6. change the secondary pip input to something else, and then back to the test input
7. plugin.conf now has both 0 and 1 instances

[FranciscoCanas]

On my branch, the behavior I see when following the steps described above is that the 2nd video test input instance is not being added to the plugin.conf file:

I spent some time debugging this, and to me it looks like the self.config instance for the two video test sources is actually shared. the 'storage_args' argument that Config uses to hold the name by which to save to file is the same for both: 'Video Test Source-0'. So when I change settings for the pip input, self.config.save() saves it to 'Video Test Source-0' still. Will update more later.

[mtomwing]

There's probably a missing plugin_object.set_instance(1) somewhere. -_-

[FranciscoCanas]

That's what I thought at first too, but the .set_instance(1) does get called when you update a setting on the pip input Test Video Source. So the plugin object has instance = 1. But when it gets to the config.save(), the config.save() still retains that 'Video Test Source-0' in its storage_args and so it just overwrites that entry.

[mtomwing]

Ah, then it's likely missing a plugin_object.load_config(...) after that set_instance(1). Try adding a self.load_config(...) to set_instance and see if that fixes it.

[FranciscoCanas]

That does fix it. :) I spent way too long stepping through code trying to find why it was loading the same config instance. :P

[mtomwing]

Would you mind submitting another PR for that fix since it also exists in master (or at least I think it does)?

[FranciscoCanas]

On master the behaviour was a bit different:

It creates a new instance, but only once you have changed some setting in the pip input's video test source so that it's different than the main input's test source. I'm not sure if that's by design or not. I can still open an PR, since this changes the behaviour.

[mtomwing]

Okay, leave it here then. Can you play around with the other mixers and make sure they're all good?

[FranciscoCanas]

I noticed another quirk:

For the pulsesrc plugin (and possibly others with similar drop-down widget and default values), I have noticed that we only call its set_source() handler when the drop-down option changes. If the user opens pulsesrc settings widget but closes it again without changing anything then we don't save the current selection. We save the default, which is currently an empty string.

So it's unintuitive: it looks like you have the right source selected, but it doesn't save it until you change it.

This behaviour is actually the same in master branch currently.

[mtomwing]

Okay, that definitely deserves an issue of its own. Would you mind creating one with your findings?

I'll look into merging this refactor tonight now that the set_instance thing is sorted. Maybe one day Freeseer will have a good manager that doesn't treat plugins as singletons.

[mtomwing]

Looks good to go now. I'll merge whenever you have time to squash your commits again.

[mtomwing]

How about, "Fix #588 Refactor plugins to use Config framework"?

[mtomwing]

Merged in 9a9f2a3fcebe62b6a272e3b2bbcfd86f70e885e3.

Thanks for your contribution!

[FranciscoCanas]

Thanks for all the feedback!