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)

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

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/85223
Your team OpenLP Core is requested to review the proposed merge of lp:~googol/openlp/bug-768495 into lp:openlp.

=== modified file 'openlp/core/ui/shortcutlistform.py'
--- openlp/core/ui/shortcutlistform.py	2011-07-07 21:22:41 +0000
+++ openlp/core/ui/shortcutlistform.py	2011-12-10 14:12:24 +0000
@@ -344,8 +344,11 @@
             if category.name is None:
                 continue
             for action in category.actions:
-                if self.changedActions .has_key(action):
+                if action in self.changedActions:
+                    old_shortcuts = map(unicode,
+                        map(QtGui.QKeySequence.toString, action.shortcuts()))
                     action.setShortcuts(self.changedActions[action])
+                    self.action_list.update_shortcut_map(action, old_shortcuts)
                 settings.setValue(
                     action.objectName(), QtCore.QVariant(action.shortcuts()))
         settings.endGroup()
@@ -452,7 +455,7 @@
         those shortcuts which are not saved yet but already assigned (as changes
         are applied when closing the dialog).
         """
-        if self.changedActions.has_key(action):
+        if action in self.changedActions:
             return self.changedActions[action]
         return action.shortcuts()
 

=== modified file 'openlp/core/utils/actions.py'
--- openlp/core/utils/actions.py	2011-12-07 20:28:26 +0000
+++ openlp/core/utils/actions.py	2011-12-10 14:12:24 +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._is_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._is_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,62 @@
             return
         self.categories.add(name, weight)
 
+    def update_shortcut_map(self, action, old_shortcuts):
+        """
+        Remove the action for the given ``old_shortcuts`` from the
+        ``shortcut_map`` to ensure its up-to-dateness.
+
+        **Note**: The new action's shortcuts **must** be assigned to the given
+        ``action`` **before** calling this method.
+
+        ``action``
+            The action whose shortcuts are supposed to be updated in the
+            ``shortcut_map``.
+
+        ``old_shortcuts``
+            A list of unicode keysequences.
+        """
+        for old_shortcut in old_shortcuts:
+            # Should always be the case.
+            if old_shortcut in ActionList.shortcut_map:
+                # Remove action from the list of actions which are using this
+                # shortcut.
+                ActionList.shortcut_map[old_shortcut].remove(action)
+                # Remove empty entries.
+                if not ActionList.shortcut_map[old_shortcut]:
+                    del ActionList.shortcut_map[old_shortcut]
+        new_shortcuts = map(unicode,
+            map(QtGui.QKeySequence.toString, action.shortcuts()))
+        # Add the new shortcut to the map.
+        for new_shortcut in new_shortcuts:
+            existing_actions = ActionList.shortcut_map.get(new_shortcut, [])
+            existing_actions.append(action)
+            ActionList.shortcut_map[new_shortcut] = existing_actions
+
+    def _is_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):
     """