Fix #588 - Refactor plugins. #589

FranciscoCanas · 2014-09-20T18:43:29Z

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

mtomwing · 2014-09-20T19:18:54Z

src/freeseer/plugins/audioinput/jackaudiosrc/__init__.py

+        self.client = config.client
+        self.connect = config.connect
+        self.server = config.server
+        self.clientname = config.clientname


There's no need to store any of these values as they can be accessed directly from config.

I'll also add that there's a way to make Config objects automatically persist their values on change.

mtomwing · 2014-09-20T19:20:06Z

src/freeseer/plugins/audioinput/jackaudiosrc/__init__.py

-            self.plugman.set_plugin_option(self.CATEGORY, self.get_config_name(), "Server", self.server)
-            self.plugman.set_plugin_option(self.CATEGORY, self.get_config_name(), "ClientName", self.clientname)
+        profile = settings.profile_manager.get()
+        config = profile.get_config('plugin.conf', JackAudioConfig, storage_args=[self.get_config_name()])


A plugin shouldn't need to know about profiles or anything - all it cares about is its Config class.

Should the plugin be reading from the plugin.conf file here though? In which case, would it need to know the profile so that it can read the plugin.conf file from the correct file (ie ~/.freeseer/profiles//plugin.conf).

That's what I was getting at. A plugin shouldn't be loading its config here - it should be loaded as part of whatever is using the Plugin. This way there's less coupling between a plugin and where its config comes from.

FranciscoCanas · 2014-10-04T16:57:56Z

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

mtomwing · 2014-10-16T04:25:02Z

src/freeseer/plugins/audioinput/pulsesrc/__init__.py

+        try:
+            source = self.config.source
+        except OptionValueNotSetError:
+            source = None


It shouldn't be possible for self.config.source to be not set since it has a default value of ''.

mtomwing · 2014-10-19T01:21:56Z

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 · 2014-10-19T14:43:19Z

@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 · 2014-10-20T07:37:30Z

src/freeseer/framework/plugin.py

-        self.set_plugin_option("VideoMixer", "Video Passthrough-0", "Video Input", "Video Test Source")
-        self.save()
-        log.info("Default plugins enabled.")
-
    def get_plugin_option(self, category, name, option):


Are get_plugin_option and set_plugin_option actually used anywhere?

They're not used now, but I was thinking they may be used in the configuration endpoints.

Maybe it would make more sense to get/set config options directly on a plugin instance rather then through PluginManager's get/set_plugin_option?

Yeah, that would make more sense.

mtomwing · 2014-10-20T21:01:33Z

src/freeseer/plugins/audioinput/pulsesrc/__init__.py

-        self.plugman.save()
-        log.debug('Set pulseaudio source to %s' % self.source)
+        self.config.source = self.widget.source_combobox.itemData(index).toString()
+        log.debug('Set pulseaudio source to %s' % self.config.source)


log.debug('message with arg %s', arg1)

mtomwing · 2014-10-24T02:20:51Z

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 · 2014-10-24T11:40:57Z

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

mtomwing · 2014-10-24T18:06:47Z

IIRC:

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

FranciscoCanas · 2014-10-24T23:14:35Z

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 · 2014-10-24T23:31:58Z

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

FranciscoCanas · 2014-10-24T23:48:03Z

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 · 2014-10-24T23:53:15Z

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 · 2014-10-25T00:04:01Z

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 · 2014-10-25T00:08:36Z

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

FranciscoCanas · 2014-10-25T00:14:18Z

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 · 2014-10-25T00:16:10Z

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

FranciscoCanas · 2014-10-25T00:18:40Z

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 · 2014-10-25T02:08:41Z

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 · 2014-10-25T16:02:44Z

src/freeseer/framework/plugin.py

        # Get the path where the installed plugins are located on systems where
        # freeseer is installed.
-        pluginpath = "%s/../plugins" % os.path.dirname(os.path.abspath(__file__))
+        pluginpath = "{}/../plugins".format(os.path.dirname(os.path.abspath(__file__)))


I guess if this is going to be changed, it may as well also use os.path.join too.

mtomwing · 2014-10-25T23:38:37Z

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

mtomwing · 2014-10-26T00:08:02Z

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

Each plugin class that saves configuration settings to the plugin.conf file now has its own CONFIG_CLASS that extends the Config class from the freeseer.framework.config.core package Plugin options are loaded and saved from/to file via the plugin's Config class. Changes the YAPSY ConfigurablePluginManager to simple PluginManager. Refactored plugins: - Jackaudio - PulseSrcAudio - Multiaudio - DesktopLinuxSrc - Firewiresrc - Usbsrc - Videotestsrc - Picture-in-Picture (pip) - Videopassthrough - Audiofeedback - Ogg Icecast - Ogg output - Videopreview - RTMP Stream

mtomwing · 2014-10-26T01:00:22Z

Merged in 9a9f2a3.

Thanks for your contribution!

FranciscoCanas · 2014-10-26T01:05:03Z

Thanks for all the feedback!

mtomwing reviewed Sep 20, 2014
View reviewed changes

FranciscoCanas changed the title ~~WIP #588 - Refactor jackaudio plugins.~~ WIP #588 - Refactor plugins. Sep 20, 2014

mtomwing reviewed Sep 20, 2014
View reviewed changes

dideler added this to the 2014 Fall - UCOSP milestone Sep 22, 2014

FranciscoCanas force-pushed the 588-refactor-plugins branch 2 times, most recently from 9485607 to 08ced34 Compare October 4, 2014 20:12

FranciscoCanas force-pushed the 588-refactor-plugins branch from fd02b69 to 0a600ff Compare October 14, 2014 19:31

FranciscoCanas changed the title ~~WIP #588 - Refactor plugins.~~ Fix #588 - Refactor plugins. Oct 15, 2014

mtomwing reviewed Oct 16, 2014
View reviewed changes

mtomwing reviewed Oct 20, 2014
View reviewed changes

mtomwing mentioned this pull request Oct 20, 2014

Fixed #614 Change resolution for input Video #615

Closed

FranciscoCanas force-pushed the 588-refactor-plugins branch from 2841c72 to a90afe2 Compare October 22, 2014 00:19

mtomwing reviewed Oct 25, 2014
View reviewed changes

FranciscoCanas force-pushed the 588-refactor-plugins branch 2 times, most recently from b58b6f8 to 6ea813a Compare October 25, 2014 23:42

FranciscoCanas force-pushed the 588-refactor-plugins branch from 6ea813a to d13a0b2 Compare October 26, 2014 00:21

mtomwing closed this Oct 26, 2014

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #588 - Refactor plugins. #589

Fix #588 - Refactor plugins. #589

FranciscoCanas commented Sep 20, 2014

mtomwing Sep 20, 2014

mtomwing Sep 20, 2014

mtomwing Sep 20, 2014

FranciscoCanas Sep 20, 2014

mtomwing Sep 20, 2014

FranciscoCanas commented Oct 4, 2014

mtomwing Oct 16, 2014

mtomwing commented Oct 19, 2014

FranciscoCanas commented Oct 19, 2014

mtomwing Oct 20, 2014

FranciscoCanas Oct 20, 2014

mtomwing Oct 20, 2014

mtomwing Oct 20, 2014

mtomwing commented Oct 24, 2014

FranciscoCanas commented Oct 24, 2014

mtomwing commented Oct 24, 2014

FranciscoCanas commented Oct 24, 2014

mtomwing commented Oct 24, 2014

FranciscoCanas commented Oct 24, 2014

mtomwing commented Oct 24, 2014

FranciscoCanas commented Oct 25, 2014

mtomwing commented Oct 25, 2014

FranciscoCanas commented Oct 25, 2014

mtomwing commented Oct 25, 2014

FranciscoCanas commented Oct 25, 2014

mtomwing commented Oct 25, 2014

mtomwing Oct 25, 2014

mtomwing commented Oct 25, 2014

mtomwing commented Oct 26, 2014

mtomwing commented Oct 26, 2014

FranciscoCanas commented Oct 26, 2014

Fix #588 - Refactor plugins. #589

Fix #588 - Refactor plugins. #589

Conversation

FranciscoCanas commented Sep 20, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FranciscoCanas commented Oct 4, 2014

Choose a reason for hiding this comment

mtomwing commented Oct 19, 2014

FranciscoCanas commented Oct 19, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mtomwing commented Oct 24, 2014

FranciscoCanas commented Oct 24, 2014

mtomwing commented Oct 24, 2014

FranciscoCanas commented Oct 24, 2014

mtomwing commented Oct 24, 2014

FranciscoCanas commented Oct 24, 2014

mtomwing commented Oct 24, 2014

FranciscoCanas commented Oct 25, 2014

mtomwing commented Oct 25, 2014

FranciscoCanas commented Oct 25, 2014

mtomwing commented Oct 25, 2014

FranciscoCanas commented Oct 25, 2014

mtomwing commented Oct 25, 2014

Choose a reason for hiding this comment

mtomwing commented Oct 25, 2014

mtomwing commented Oct 26, 2014

mtomwing commented Oct 26, 2014

FranciscoCanas commented Oct 26, 2014