Merge lp:~googol-deactivatedaccount/openlp/bug-900399 into lp:openlp

[Andreas Preikschat]

bzr commit -m "-Fixed media item recreation (which caused bug #900399)
- Doc/method clean up
- Replaced continue with break
- Replaced method with one-liner"

Hello,

1) Fixed media item recreation (which caused bug #900399)
2) Doc/method clean up
3) Replaced continue with break (after "continuing" the first the x times follow where we continue, so we can just break the first time instead)
4) Replaced method with one-liner

[Tim Bentley (trb143)]

131 is an incorrect removal as when a plugin is disabled the settings dialog will fail as the value ins not defined.
Renaming fields is also iffy in a feature freeze. They are not a bug.
Code simplification is just Ok.

review: Needs Fixing

[Andreas Preikschat (googol-deactivatedaccount)]

> Renaming fields is also iffy in a feature freeze. They are not a bug.

I renamed getMediaManagerItem() and getSettingsTab() because their names caused this bug (line 190).

[Raoul Snyman (raoul-snyman)]

Andreas, my apologies, I didnt' explain myself well enough. What I meant by changing those "getMediaManagerItem" to "createMediaManagerItem" was not just renaming the methods, but actually doing the assignment inside the method, as opposed to externally.

i.e. instead of:

    if self.settings_tab_class:
        return self.settings_tab_class(parent, self.name, ...

do this:

    if settings_tab_class:
        self.settings_tab = self.settings_tab_class(parent, self.name, ...

[Raoul Snyman (raoul-snyman)]

Not that this is your problem, it's just some general code cleanup that should happen somewhere.

[Raoul Snyman (raoul-snyman)]

review: Approve

[Tim Bentley (trb143)]

review: Approve

=== modified file 'openlp/core/lib/plugin.py'
--- openlp/core/lib/plugin.py	2011-10-26 20:11:15 +0000
+++ openlp/core/lib/plugin.py	2011-12-08 17:56:27 +0000
@@ -91,8 +91,9 @@
     ``checkPreConditions()``
         Provides the Plugin with a handle to check if it can be loaded.
 
-    ``getMediaManagerItem()``
-        Returns an instance of MediaManagerItem to be used in the Media Manager.
+    ``createMediaManagerItem()``
+        Creates a new instance of MediaManagerItem to be used in the Media
+        Manager.
 
     ``addImportMenuItem(import_menu)``
         Add an item to the Import menu.
@@ -100,8 +101,8 @@
     ``addExportMenuItem(export_menu)``
         Add an item to the Export menu.
 
-    ``getSettingsTab()``
-        Returns an instance of SettingsTabItem to be used in the Settings
+    ``createSettingsTab()``
+        Creates a new instance of SettingsTabItem to be used in the Settings
         dialog.
 
     ``addToMenu(menubar)``
@@ -156,10 +157,10 @@
         self.icon = None
         self.media_item_class = media_item_class
         self.settings_tab_class = settings_tab_class
+        self.settings_tab = None
+        self.mediaItem = None
         self.weight = 0
         self.status = PluginStatus.Inactive
-        # Set up logging
-        self.log = logging.getLogger(self.name)
         self.previewController = plugin_helpers[u'preview']
         self.liveController = plugin_helpers[u'live']
         self.renderer = plugin_helpers[u'renderer']
@@ -178,7 +179,7 @@
         Provides the Plugin with a handle to check if it can be loaded.
         Failing Preconditions does not stop a settings Tab being created
 
-        Returns True or False.
+        Returns ``True`` or ``False``.
         """
         return True
 
@@ -210,15 +211,14 @@
         """
         return self.status == PluginStatus.Active
 
-    def getMediaManagerItem(self):
+    def createMediaManagerItem(self):
         """
         Construct a MediaManagerItem object with all the buttons and things
-        you need, and return it for integration into openlp.org.
+        you need, and return it for integration into OpenLP.
         """
         if self.media_item_class:
-            return self.media_item_class(self.mediadock.media_dock, self,
-                self.icon)
-        return None
+            self.mediaItem = self.media_item_class(self.mediadock.media_dock,
+                self, self.icon)
 
     def addImportMenuItem(self, importMenu):
         """
@@ -247,16 +247,15 @@
         """
         pass
 
-    def getSettingsTab(self, parent):
+    def createSettingsTab(self, parent):
         """
-        Create a tab for the settings window to display the configurable
-        options for this plugin to the user.
+        Create a tab for the settings window to display the configurable options
+        for this plugin to the user.
         """
         if self.settings_tab_class:
-            return self.settings_tab_class(parent, self.name,
+            self.settings_tab = self.settings_tab_class(parent, self.name,
                 self.getString(StringContent.VisibleName)[u'title'],
                 self.icon_path)
-        return None
 
     def addToMenu(self, menubar):
         """

=== modified file 'openlp/core/lib/pluginmanager.py'
--- openlp/core/lib/pluginmanager.py	2011-12-03 12:51:40 +0000
+++ openlp/core/lib/pluginmanager.py	2011-12-08 17:56:27 +0000
@@ -90,7 +90,7 @@
                     thisdepth = len(path.split(os.sep))
                     if thisdepth - startdepth > 2:
                         # skip anything lower down
-                        continue
+                        break
                     modulename = os.path.splitext(path)[0]
                     prefix = os.path.commonprefix([self.basepath, path])
                     # hack off the plugin base path
@@ -113,7 +113,7 @@
                 plugin_objects.append(plugin)
             except TypeError:
                 log.exception(u'Failed to load plugin %s', unicode(p))
-        plugins_list = sorted(plugin_objects, self.order_by_weight)
+        plugins_list = sorted(plugin_objects, key=lambda plugin: plugin.weight)
         for plugin in plugins_list:
             if plugin.checkPreConditions():
                 log.debug(u'Plugin %s active', unicode(plugin.name))
@@ -122,29 +122,13 @@
                 plugin.status = PluginStatus.Disabled
             self.plugins.append(plugin)
 
-    def order_by_weight(self, x, y):
-        """
-        Sort two plugins and order them by their weight.
-
-        ``x``
-            The first plugin.
-
-        ``y``
-            The second plugin.
-        """
-        return cmp(x.weight, y.weight)
-
-    def hook_media_manager(self, mediadock):
-        """
-        Loop through all the plugins. If a plugin has a valid media manager
-        item, add it to the media manager.
-
-        ``mediatoolbox``
-            The Media Manager itself.
+    def hook_media_manager(self):
+        """
+        Create the plugins' media manager items.
         """
         for plugin in self.plugins:
             if plugin.status is not PluginStatus.Disabled:
-                plugin.mediaItem = plugin.getMediaManagerItem()
+                plugin.createMediaManagerItem()
 
     def hook_settings_tabs(self, settings_form=None):
         """
@@ -152,14 +136,12 @@
         item, add it to the settings tab.
         Tabs are set for all plugins not just Active ones
 
-        ``settingsform``
+        ``settings_form``
             Defaults to *None*. The settings form to add tabs to.
         """
         for plugin in self.plugins:
             if plugin.status is not PluginStatus.Disabled:
-                plugin.settings_tab = plugin.getSettingsTab(settings_form)
-            else:
-                plugin.settings_tab = None
+                plugin.createSettingsTab(settings_form)
         settings_form.plugins = self.plugins
 
     def hook_import_menu(self, import_menu):
@@ -225,7 +207,7 @@
 
     def get_plugin_by_name(self, name):
         """
-        Return the plugin which has a name with value ``name``
+        Return the plugin which has a name with value ``name``.
         """
         for plugin in self.plugins:
             if plugin.name == name:

=== modified file 'openlp/core/ui/mainwindow.py'
--- openlp/core/ui/mainwindow.py	2011-12-05 21:40:56 +0000
+++ openlp/core/ui/mainwindow.py	2011-12-08 17:56:27 +0000
@@ -655,7 +655,7 @@
         self.pluginManager.hook_settings_tabs(self.settingsForm)
         # Find and insert media manager items
         log.info(u'hook media')
-        self.pluginManager.hook_media_manager(self.mediaDockManager)
+        self.pluginManager.hook_media_manager()
         # Call the hook method to pull in import menus.
         log.info(u'hook menus')
         self.pluginManager.hook_import_menu(self.fileImportMenu)

=== modified file 'openlp/plugins/media/mediaplugin.py'
--- openlp/plugins/media/mediaplugin.py	2011-12-02 21:13:05 +0000
+++ openlp/plugins/media/mediaplugin.py	2011-12-08 17:56:27 +0000
@@ -52,12 +52,12 @@
         for ext in self.video_extensions_list:
             self.serviceManager.supportedSuffixes(ext[2:])
 
-    def getSettingsTab(self, parent):
+    def createSettingsTab(self, parent):
         """
         Create the settings Tab
         """
         visible_name = self.getString(StringContent.VisibleName)
-        return MediaTab(parent, self.name, visible_name[u'title'],
+        self.settings_tab = MediaTab(parent, self.name, visible_name[u'title'],
             self.mediaController.mediaPlayers, self.icon_path)
 
     def about(self):

=== modified file 'openlp/plugins/presentations/presentationplugin.py'
--- openlp/plugins/presentations/presentationplugin.py	2011-10-03 20:12:57 +0000
+++ openlp/plugins/presentations/presentationplugin.py	2011-12-08 17:56:27 +0000
@@ -57,13 +57,13 @@
         self.icon_path = u':/plugins/plugin_presentations.png'
         self.icon = build_icon(self.icon_path)
 
-    def getSettingsTab(self, parent):
+    def createSettingsTab(self, parent):
         """
         Create the settings Tab
         """
         visible_name = self.getString(StringContent.VisibleName)
-        return PresentationTab(parent, self.name, visible_name[u'title'],
-            self.controllers, self.icon_path)
+        self.settings_tab = PresentationTab(parent, self.name,
+            visible_name[u'title'], self.controllers, self.icon_path)
 
     def initialise(self):
         """
@@ -94,11 +94,11 @@
                 controller.kill()
         Plugin.finalise(self)
 
-    def getMediaManagerItem(self):
+    def createMediaManagerItem(self):
         """
         Create the Media Manager List
         """
-        return PresentationMediaItem(
+        self.mediaItem = PresentationMediaItem(
             self.mediadock.media_dock, self, self.icon, self.controllers)
 
     def registerControllers(self, controller):

=== modified file 'openlp/plugins/songs/forms/editsongform.py'
--- openlp/plugins/songs/forms/editsongform.py	2011-12-02 20:17:57 +0000
+++ openlp/plugins/songs/forms/editsongform.py	2011-12-08 17:56:27 +0000
@@ -181,7 +181,7 @@
                 plugin.status == PluginStatus.Active:
                 self.audioAddFromMediaButton.setVisible(True)
                 self.mediaForm.populateFiles(
-                    plugin.getMediaManagerItem().getList(MediaType.Audio))
+                    plugin.mediaItem.getList(MediaType.Audio))
                 break
 
     def newSong(self):