Merge lp:~thelinuxguy/openlp/change-dropdown-to-checkbox into lp:openlp

[Simon Hanna]

* Change the Combobox used for the state of plugins to a checkbox.
* Do not show the plugins version numbers as they provide no additional information
* Show the plugin details (about text) even if the plugin is disabled

[Tim Bentley (trb143)]

Disable means the plugin cannot work due to missing dependencies like Impress or Powerpoint.
Inactive means that the plugin is not to be used i.e you do not want it running in most cases this would be remote.

Looking at the code the combo box needs to support three states not two so this change is not valid as a check box can only have 2 states so this is not a valid change.

The change to about looks valid but why only one plugin why not all of them?

Wew have a test, good but it seems a bit lite!

review: Disapprove

[Simon Hanna (thelinuxguy)]

But the third state doesn't allow the user to change anything.
I guess the third state could be emulated by either disabling the checkbox or replacing it with a label saying it's missing a dependency.

About the change to about:
I only changed about because I was looking for something to test and came across this function.
I can go through the others and change them as well...

about the lite test:
I know it's lite. But I'm not sure what else to test for the about function

[Tim Bentley (trb143)]

No the third state is there because the machine cannot support the plugin, just disabling the check box is not a good indication ofthat state.

[Simon Hanna (thelinuxguy)]

and replacing it with a label?

[Tim Bentley (trb143)]

Not in my opinion.

[Simon Hanna (thelinuxguy)]

I created a MR for the static methods here: https://code.launchpad.net/~thelinuxguy/openlp/make-methods-static/+merge/281488

[Simon Hanna (thelinuxguy)]

I just checked, when a plugin is disabled (you can just replace the songs.sqlite with a normal file)
The whole entry becomes disabled. So there really is no reason for the dropdown. Currently the branch throws an Exception when a plugin is disabled, but that can be fixed. What do you think?

[Raoul Snyman (raoul-snyman)]

Hey Simon, do you want to update your branch and re-propose?

[Raoul Snyman (raoul-snyman)]

(I mean, merge from trunk)

[Tim Bentley (trb143)]

Great code but we do need a test as all merges require tests.

Sorry.

review: Needs Fixing

[lp:~thelinuxguy/openlp/change-dropdown-to-checkbox]

2598. By Simon Hanna on 2016-05-17

Add comments to test

[Raoul Snyman (raoul-snyman)]

review: Approve

[Tim Bentley (trb143)]

review: Approve

=== modified file 'openlp/core/ui/plugindialog.py'
--- openlp/core/ui/plugindialog.py	2015-12-31 22:46:06 +0000
+++ openlp/core/ui/plugindialog.py	2016-05-17 19:35:55 +0000
@@ -53,15 +53,9 @@
         self.plugin_info_layout.setObjectName('plugin_info_layout')
         self.status_label = QtWidgets.QLabel(self.plugin_info_group_box)
         self.status_label.setObjectName('status_label')
-        self.status_combo_box = QtWidgets.QComboBox(self.plugin_info_group_box)
-        self.status_combo_box.addItems(('', ''))
-        self.status_combo_box.setObjectName('status_combo_box')
-        self.plugin_info_layout.addRow(self.status_label, self.status_combo_box)
-        self.version_label = QtWidgets.QLabel(self.plugin_info_group_box)
-        self.version_label.setObjectName('version_label')
-        self.version_number_label = QtWidgets.QLabel(self.plugin_info_group_box)
-        self.version_number_label.setObjectName('version_number_label')
-        self.plugin_info_layout.addRow(self.version_label, self.version_number_label)
+        self.status_checkbox = QtWidgets.QCheckBox(self.plugin_info_group_box)
+        self.status_checkbox.setObjectName('status_checkbox')
+        self.plugin_info_layout.addRow(self.status_label, self.status_checkbox)
         self.about_label = QtWidgets.QLabel(self.plugin_info_group_box)
         self.about_label.setObjectName('about_label')
         self.about_text_browser = QtWidgets.QTextBrowser(self.plugin_info_group_box)
@@ -80,8 +74,6 @@
         """
         plugin_view_dialog.setWindowTitle(translate('OpenLP.PluginForm', 'Manage Plugins'))
         self.plugin_info_group_box.setTitle(translate('OpenLP.PluginForm', 'Plugin Details'))
-        self.version_label.setText('%s:' % UiStrings().Version)
         self.about_label.setText('%s:' % UiStrings().About)
         self.status_label.setText(translate('OpenLP.PluginForm', 'Status:'))
-        self.status_combo_box.setItemText(0, translate('OpenLP.PluginForm', 'Active'))
-        self.status_combo_box.setItemText(1, translate('OpenLP.PluginForm', 'Inactive'))
+        self.status_checkbox.setText(translate('OpenLP.PluginForm', 'Active'))

=== modified file 'openlp/core/ui/pluginform.py'
--- openlp/core/ui/pluginform.py	2016-01-09 16:26:14 +0000
+++ openlp/core/ui/pluginform.py	2016-05-17 19:35:55 +0000
@@ -49,7 +49,7 @@
         self._clear_details()
         # Right, now let's put some signals and slots together!
         self.plugin_list_widget.itemSelectionChanged.connect(self.on_plugin_list_widget_selection_changed)
-        self.status_combo_box.currentIndexChanged.connect(self.on_status_combo_box_changed)
+        self.status_checkbox.stateChanged.connect(self.on_status_checkbox_changed)
 
     def load(self):
         """
@@ -86,24 +86,23 @@
         """
         Clear the plugin details widgets
         """
-        self.status_combo_box.setCurrentIndex(-1)
-        self.version_number_label.setText('')
+        self.status_checkbox.setChecked(False)
         self.about_text_browser.setHtml('')
-        self.status_combo_box.setEnabled(False)
+        self.status_checkbox.setEnabled(False)
 
     def _set_details(self):
         """
         Set the details of the currently selected plugin
         """
         log.debug('PluginStatus: %s', str(self.active_plugin.status))
-        self.version_number_label.setText(self.active_plugin.version)
         self.about_text_browser.setHtml(self.active_plugin.about())
         self.programatic_change = True
-        status = PluginStatus.Active
-        if self.active_plugin.status == PluginStatus.Active:
-            status = PluginStatus.Inactive
-        self.status_combo_box.setCurrentIndex(status)
-        self.status_combo_box.setEnabled(True)
+        if self.active_plugin.status != PluginStatus.Disabled:
+            self.status_checkbox.setChecked(self.active_plugin.status == PluginStatus.Active)
+            self.status_checkbox.setEnabled(True)
+        else:
+            self.status_checkbox.setChecked(False)
+            self.status_checkbox.setEnabled(False)
         self.programatic_change = False
 
     def on_plugin_list_widget_selection_changed(self):
@@ -116,22 +115,21 @@
         plugin_name_singular = self.plugin_list_widget.currentItem().text().split('(')[0][:-1]
         self.active_plugin = None
         for plugin in self.plugin_manager.plugins:
-            if plugin.status != PluginStatus.Disabled:
-                if plugin.name_strings['singular'] == plugin_name_singular:
-                    self.active_plugin = plugin
-                    break
+            if plugin.name_strings['singular'] == plugin_name_singular:
+                self.active_plugin = plugin
+                break
         if self.active_plugin:
             self._set_details()
         else:
             self._clear_details()
 
-    def on_status_combo_box_changed(self, status):
+    def on_status_checkbox_changed(self, status):
         """
         If the status of a plugin is altered, apply the change
         """
-        if self.programatic_change or status == PluginStatus.Disabled:
+        if self.programatic_change or self.active_plugin is None:
             return
-        if status == PluginStatus.Inactive:
+        if status:
             self.application.set_busy_cursor()
             self.active_plugin.toggle_status(PluginStatus.Active)
             self.application.set_normal_cursor()

=== modified file 'tests/functional/openlp_core_common/test_actions.py'
--- tests/functional/openlp_core_common/test_actions.py	2016-03-31 16:34:22 +0000
+++ tests/functional/openlp_core_common/test_actions.py	2016-05-17 19:35:55 +0000
@@ -111,6 +111,21 @@
         self.assertEqual(self.list.actions[0], (41, self.action2))
         self.assertEqual(self.list.actions[1], (42, self.action1))
 
+    def iterator_test(self):
+        """
+        Test the __iter__ and __next__ methods
+        """
+        # GIVEN: The list including two actions
+        self.list.add(self.action1)
+        self.list.add(self.action2)
+        
+        # WHEN: Iterating over the list
+        l = [a for a in self.list]
+        # THEN: Make sure they are returned in correct order
+        self.assertEquals(len(self.list), 2)
+        self.assertIs(l[0], self.action1)
+        self.assertIs(l[1], self.action2)
+
     def remove_test(self):
         """
         Test the remove() method