Merge lp:~raoul-snyman/openlp/custom-shortcuts-2.4 into lp:openlp/2.4

[Raoul Snyman]

Disable the shortcut controls when an action has not been selected

Disable the shortcut controls when an action has not been selected

Add this to your merge proposal:
--------------------------------
lp:~raoul-snyman/openlp/custom-shortcuts-2.4 (revision 2681)
[SUCCESS] https://ci.openlp.io/job/Branch-01-Pull/1952/
[SUCCESS] https://ci.openlp.io/job/Branch-02-Functional-Tests/1863/
[SUCCESS] https://ci.openlp.io/job/Branch-03-Interface-Tests/1804/
[SUCCESS] https://ci.openlp.io/job/Branch-04a-Windows_Functional_Tests/1530/
[SUCCESS] https://ci.openlp.io/job/Branch-04b-Windows_Interface_Tests/1120/
[SUCCESS] https://ci.openlp.io/job/Branch-05a-Code_Analysis/1188/
[SUCCESS] https://ci.openlp.io/job/Branch-05b-Test_Coverage/1056/

[Tim Bentley (trb143)]

review: Approve

=== modified file 'CHANGELOG.rst'
--- CHANGELOG.rst	2017-03-27 23:25:12 +0000
+++ CHANGELOG.rst	2017-03-29 18:46:40 +0000
@@ -1,11 +1,12 @@
 OpenLP 2.4.6
 ============
 
-* Fixed a bug where the author type upgrade was being ignore because it was looking at the wrong table
-* Fixed a bug where the songs_songbooks table was not being created because the if expression was the wrong way round
-* Changed the songs_songbooks migration SQL slightly to take into account a bug that has (hopefully) been fixed
+* Fix a bug where the author type upgrade was being ignore because it was looking at the wrong table
+* Fix a bug where the songs_songbooks table was not being created because the if expression was the wrong way round
+* Change the songs_songbooks migration SQL slightly to take into account a bug that has (hopefully) been fixed
 * Sometimes the timer goes off as OpenLP is shutting down, and the application has already been deleted (reported via support system)
 * Fix opening the data folder (KDE thought the old way was an SMB share)
 * Fix a problem with the new QMediaPlayer not controlling the playlist anymore
-* Added importing of author types to the OpenLP 2 song importer
+* Add importing of author types to the OpenLP 2 song importer
 * Fix a problem with loading Qt's translation files, bug #1676163
+* Disable the controls in the shortcut dialog unless a shortcut is actually selected
 
=== modified file 'openlp/core/ui/shortcutlistform.py'
--- openlp/core/ui/shortcutlistform.py	2016-12-31 11:05:48 +0000
+++ openlp/core/ui/shortcutlistform.py	2017-03-29 18:46:40 +0000
@@ -49,6 +49,8 @@
         self.changed_actions = {}
         self.action_list = ActionList.get_instance()
         self.dialog_was_shown = False
+        self.default_radio_button.setEnabled(False)
+        self.custom_radio_button.setEnabled(False)
         self.primary_push_button.toggled.connect(self.on_primary_push_button_clicked)
         self.alternate_push_button.toggled.connect(self.on_alternate_push_button_clicked)
         self.tree_widget.currentItemChanged.connect(self.on_current_item_changed)
@@ -110,8 +112,89 @@
         self.reload_shortcut_list()
         self._adjust_button(self.primary_push_button, False, False, '')
         self._adjust_button(self.alternate_push_button, False, False, '')
+        self._set_controls_enabled(False)
         return QtWidgets.QDialog.exec(self)
 
+    def _validiate_shortcut(self, changing_action, key_sequence):
+        """
+        Checks if the given ``changing_action `` can use the given ``key_sequence``. Returns ``True`` if the
+        ``key_sequence`` can be used by the action, otherwise displays a dialog and returns ``False``.
+
+        :param changing_action: The action which wants to use the ``key_sequence``.
+        :param key_sequence: The key sequence which the action want so use.
+        """
+        is_valid = True
+        for category in self.action_list.categories:
+            for action in category.actions:
+                shortcuts = self._action_shortcuts(action)
+                if key_sequence not in shortcuts:
+                    continue
+                if action is changing_action:
+                    if self.primary_push_button.isChecked() and shortcuts.index(key_sequence) == 0:
+                        continue
+                    if self.alternate_push_button.isChecked() and shortcuts.index(key_sequence) == 1:
+                        continue
+                # Have the same parent, thus they cannot have the same shortcut.
+                if action.parent() is changing_action.parent():
+                    is_valid = False
+                # The new shortcut is already assigned, but if both shortcuts are only valid in a different widget the
+                # new shortcut is valid, because they will not interfere.
+                if action.shortcutContext() in [QtCore.Qt.WindowShortcut, QtCore.Qt.ApplicationShortcut]:
+                    is_valid = False
+                if changing_action.shortcutContext() in [QtCore.Qt.WindowShortcut, QtCore.Qt.ApplicationShortcut]:
+                    is_valid = False
+        if not is_valid:
+            self.main_window.warning_message(translate('OpenLP.ShortcutListDialog', 'Duplicate Shortcut'),
+                                             translate('OpenLP.ShortcutListDialog',
+                                                       'The shortcut "%s" is already assigned to another action, please'
+                                                       ' use a different shortcut.') %
+                                             self.get_shortcut_string(key_sequence, for_display=True))
+            self.dialog_was_shown = True
+        return is_valid
+
+    def _action_shortcuts(self, action):
+        """
+        This returns the shortcuts for the given ``action``, which also includes those shortcuts which are not saved
+        yet but already assigned (as changes are applied when closing the dialog).
+        """
+        if action in self.changed_actions:
+            return self.changed_actions[action]
+        return action.shortcuts()
+
+    def _current_item_action(self, item=None):
+        """
+        Returns the action of the given ``item``. If no item is given, we return the action of the current item of
+        the ``tree_widget``.
+        """
+        if item is None:
+            item = self.tree_widget.currentItem()
+            if item is None:
+                return
+        return item.data(0, QtCore.Qt.UserRole)
+
+    def _adjust_button(self, button, checked=None, enabled=None, text=None):
+        """
+        Can be called to adjust more properties of the given ``button`` at once.
+        """
+        # Set the text before checking the button, because this emits a signal.
+        if text is not None:
+            button.setText(text)
+        if checked is not None:
+            button.setChecked(checked)
+        if enabled is not None:
+            button.setEnabled(enabled)
+
+    def _set_controls_enabled(self, is_enabled=False):
+        """
+        Enable or disable the shortcut controls
+        """
+        self.default_radio_button.setEnabled(is_enabled)
+        self.custom_radio_button.setEnabled(is_enabled)
+        self.primary_push_button.setEnabled(is_enabled)
+        self.alternate_push_button.setEnabled(is_enabled)
+        self.clear_primary_button.setEnabled(is_enabled)
+        self.clear_alternate_button.setEnabled(is_enabled)
+
     def reload_shortcut_list(self):
         """
         Reload the ``tree_widget`` list to add new and remove old actions.
@@ -229,8 +312,7 @@
         A item has been pressed. We adjust the button's text to the action's shortcut which is encapsulate in the item.
         """
         action = self._current_item_action(item)
-        self.primary_push_button.setEnabled(action is not None)
-        self.alternate_push_button.setEnabled(action is not None)
+        self._set_controls_enabled(action is not None)
         primary_text = ''
         alternate_text = ''
         primary_label_text = ''
@@ -244,7 +326,7 @@
                 if len(action.default_shortcuts) == 2:
                     alternate_label_text = self.get_shortcut_string(action.default_shortcuts[1], for_display=True)
             shortcuts = self._action_shortcuts(action)
-            # We do not want to loose pending changes, that is why we have to keep the text when, this function has not
+            # We do not want to loose pending changes, that is why we have to keep the text when this function has not
             # been triggered by a signal.
             if item is None:
                 primary_text = self.primary_push_button.text()
@@ -254,7 +336,7 @@
             elif len(shortcuts) == 2:
                 primary_text = self.get_shortcut_string(shortcuts[0], for_display=True)
                 alternate_text = self.get_shortcut_string(shortcuts[1], for_display=True)
-        # When we are capturing a new shortcut, we do not want, the buttons to display the current shortcut.
+        # When we are capturing a new shortcut, we do not want the buttons to display the current shortcut.
         if self.primary_push_button.isChecked():
             primary_text = ''
         if self.alternate_push_button.isChecked():
@@ -396,75 +478,6 @@
         self.refresh_shortcut_list()
         self.on_current_item_changed(self.tree_widget.currentItem())
 
-    def _validiate_shortcut(self, changing_action, key_sequence):
-        """
-        Checks if the given ``changing_action `` can use the given ``key_sequence``. Returns ``True`` if the
-        ``key_sequence`` can be used by the action, otherwise displays a dialog and returns ``False``.
-
-        :param changing_action: The action which wants to use the ``key_sequence``.
-        :param key_sequence: The key sequence which the action want so use.
-        """
-        is_valid = True
-        for category in self.action_list.categories:
-            for action in category.actions:
-                shortcuts = self._action_shortcuts(action)
-                if key_sequence not in shortcuts:
-                    continue
-                if action is changing_action:
-                    if self.primary_push_button.isChecked() and shortcuts.index(key_sequence) == 0:
-                        continue
-                    if self.alternate_push_button.isChecked() and shortcuts.index(key_sequence) == 1:
-                        continue
-                # Have the same parent, thus they cannot have the same shortcut.
-                if action.parent() is changing_action.parent():
-                    is_valid = False
-                # The new shortcut is already assigned, but if both shortcuts are only valid in a different widget the
-                # new shortcut is valid, because they will not interfere.
-                if action.shortcutContext() in [QtCore.Qt.WindowShortcut, QtCore.Qt.ApplicationShortcut]:
-                    is_valid = False
-                if changing_action.shortcutContext() in [QtCore.Qt.WindowShortcut, QtCore.Qt.ApplicationShortcut]:
-                    is_valid = False
-        if not is_valid:
-            self.main_window.warning_message(translate('OpenLP.ShortcutListDialog', 'Duplicate Shortcut'),
-                                             translate('OpenLP.ShortcutListDialog',
-                                                       'The shortcut "%s" is already assigned to another action, please'
-                                                       ' use a different shortcut.') %
-                                             self.get_shortcut_string(key_sequence, for_display=True))
-            self.dialog_was_shown = True
-        return is_valid
-
-    def _action_shortcuts(self, action):
-        """
-        This returns the shortcuts for the given ``action``, which also includes those shortcuts which are not saved
-        yet but already assigned (as changes are applied when closing the dialog).
-        """
-        if action in self.changed_actions:
-            return self.changed_actions[action]
-        return action.shortcuts()
-
-    def _current_item_action(self, item=None):
-        """
-        Returns the action of the given ``item``. If no item is given, we return the action of the current item of
-        the ``tree_widget``.
-        """
-        if item is None:
-            item = self.tree_widget.currentItem()
-            if item is None:
-                return
-        return item.data(0, QtCore.Qt.UserRole)
-
-    def _adjust_button(self, button, checked=None, enabled=None, text=None):
-        """
-        Can be called to adjust more properties of the given ``button`` at once.
-        """
-        # Set the text before checking the button, because this emits a signal.
-        if text is not None:
-            button.setText(text)
-        if checked is not None:
-            button.setChecked(checked)
-        if enabled is not None:
-            button.setEnabled(enabled)
-
     @staticmethod
     def get_shortcut_string(shortcut, for_display=False):
         if for_display:
 
=== modified file 'tests/interfaces/openlp_core_ui/test_shortcutlistform.py'
--- tests/interfaces/openlp_core_ui/test_shortcutlistform.py	2016-12-31 11:05:48 +0000
+++ tests/interfaces/openlp_core_ui/test_shortcutlistform.py	2017-03-29 18:46:40 +0000
@@ -52,6 +52,24 @@
         del self.form
         del self.main_window
 
+    def test_set_controls_enabled(self):
+        """
+        Test that the _set_controls_enabled() method works correctly
+        """
+        # GIVEN: A shortcut form, and a value to set the controls
+        is_enabled = True
+
+        # WHEN: _set_controls_enabled() is called
+        self.form._set_controls_enabled(is_enabled)
+
+        # THEN: The controls should be enabled
+        assert self.form.default_radio_button.isEnabled() is True
+        assert self.form.custom_radio_button.isEnabled() is True
+        assert self.form.primary_push_button.isEnabled() is True
+        assert self.form.alternate_push_button.isEnabled() is True
+        assert self.form.clear_primary_button.isEnabled() is True
+        assert self.form.clear_alternate_button.isEnabled() is True
+
     def adjust_button_test(self):
         """
         Test the _adjust_button() method
@@ -71,6 +89,27 @@
             mocked_check_method.assert_called_once_with(True)
             self.assertEqual(button.isEnabled(), enabled, 'The button should be disabled.')
 
+    @patch('openlp.core.ui.shortcutlistform.QtWidgets.QDialog.exec')
+    def test_exec(self, mocked_exec):
+        """
+        Test the exec method
+        """
+        # GIVEN: A form and a mocked out base exec method
+        mocked_exec.return_value = True
+
+        # WHEN: exec is called
+        result = self.form.exec()
+
+        # THEN: The result should be True and the controls should be disabled
+        assert self.form.default_radio_button.isEnabled() is False
+        assert self.form.custom_radio_button.isEnabled() is False
+        assert self.form.primary_push_button.isEnabled() is False
+        assert self.form.alternate_push_button.isEnabled() is False
+        assert self.form.clear_primary_button.isEnabled() is False
+        assert self.form.clear_alternate_button.isEnabled() is False
+        mocked_exec.assert_called_once_with(self.form)
+        assert result is True
+
     def space_key_press_event_test(self):
         """
         Test the keyPressEvent when the spacebar was pressed