openlp-core team mailing list archive

[Andreas Preikschat <andreaspreikschat@xxxxxxxxxx>]

Andreas Preikschat has proposed merging lp:~googol/openlp/bug-768495 into lp:openlp.

Requested reviews:
  OpenLP Core (openlp-core)
Related bugs:
  Bug #768495 in OpenLP: "Shortcut can be assigned twice in certain circumstances"
  https://bugs.launchpad.net/openlp/+bug/768495

For more details, see:
https://code.launchpad.net/~googol/openlp/bug-768495/+merge/84372

Hello,

Fixed bug 768495 (Shortcut can be assigned twice in certain circumstances)

Description of the fix:
1) Create a dict with the shortcuts as keys and a list of actions using this shortcut as value.
2) When we add a new action/shortcut we check if the shortcut is valid. If this is the case we add it to the dict and if not we remove the shortcut.

Possible space for improvements:
- notify the user that a shortcut has been disabled
- make sure that shortcuts with lower priority are disabled if there should be a conflict (currently we remove the shortcut from the last actions)

How to test this?
1) Assign "R" to the "Re-index tool" action
2) Disable the songs plugin
3) Assign "R" to another action
4) Enable the songs plugin
-- 
https://code.launchpad.net/~googol/openlp/bug-768495/+merge/84372
Your team OpenLP Core is requested to review the proposed merge of lp:~googol/openlp/bug-768495 into lp:openlp.

=== modified file 'openlp/core/utils/actions.py'
--- openlp/core/utils/actions.py	2011-06-12 16:02:52 +0000
+++ openlp/core/utils/actions.py	2011-12-03 15:13:32 +0000
@@ -188,6 +188,7 @@
     actions or categories.
     """
     instance = None
+    shortcut_map = {}
 
     def __init__(self):
         self.categories = CategoryList()
@@ -226,17 +227,41 @@
             self.categories[category].actions.append(action)
         else:
             self.categories[category].actions.add(action, weight)
-        if category is None:
-            # Stop here, as this action is not configurable.
-            return
         # Load the shortcut from the config.
         settings = QtCore.QSettings()
         settings.beginGroup(u'shortcuts')
         shortcuts = settings.value(action.objectName(),
             QtCore.QVariant(action.shortcuts())).toStringList()
+        settings.endGroup()
+        if not shortcuts:
+            action.setShortcuts([])
+            return
+        shortcuts = map(unicode, shortcuts)
+        # Check the alternate shortcut first, to avoid problems when the
+        # alternate shortcut becomes the primary shortcut after removing the
+        # (initial) primary shortcut due to confllicts.
+        if len(shortcuts) == 2:
+            existing_actions = ActionList.shortcut_map.get(shortcuts[1], [])
+            # Check for conflicts with other actions considering the shortcut
+            # context.
+            if self._shortcut_available(existing_actions, action):
+                actions = ActionList.shortcut_map.get(shortcuts[1], [])
+                actions.append(action)
+                ActionList.shortcut_map[shortcuts[1]] = actions
+            else:
+                shortcuts.remove(shortcuts[1])
+        # Check the primary shortcut.
+        existing_actions = ActionList.shortcut_map.get(shortcuts[0], [])
+        # Check for conflicts with other actions considering the shortcut
+        # context.
+        if self._shortcut_available(existing_actions, action):
+            actions = ActionList.shortcut_map.get(shortcuts[0], [])
+            actions.append(action)
+            ActionList.shortcut_map[shortcuts[0]] = actions
+        else:
+            shortcuts.remove(shortcuts[0])
         action.setShortcuts(
             [QtGui.QKeySequence(shortcut) for shortcut in shortcuts])
-        settings.endGroup()
 
     def remove_action(self, action, category=None):
         """
@@ -244,7 +269,7 @@
         automatically removed.
 
         ``action``
-            The QAction object to be removed.
+            The ``QAction`` object to be removed.
 
         ``category``
             The name (unicode string) of the category, which contains the
@@ -279,6 +304,30 @@
             return
         self.categories.add(name, weight)
 
+    def _shortcut_available(self, existing_actions, action):
+        """
+        Checks if the given ``action`` may use its assigned shortcut(s) or not.
+        Returns ``True`` or ``False.
+
+        ``existing_actions``
+            A list of actions which already use a particular shortcut.
+
+        ``action``
+            The action which wants to use a particular shortcut.
+        """
+        for existing_action in existing_actions:
+            if action is existing_action:
+                continue
+            if existing_action.parent() is action.parent():
+               return False
+            if existing_action.shortcutContext() in [QtCore.Qt.WindowShortcut,
+                QtCore.Qt.ApplicationShortcut]:
+                return False
+            if action.shortcutContext() in [QtCore.Qt.WindowShortcut,
+                QtCore.Qt.ApplicationShortcut]:
+                return False
+        return True
+
 
 class CategoryOrder(object):
     """