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

[Andreas Preikschat]

bzr commit -m "fixed bug #768495 (Shortcut can be assigned twice in certain circumstances)" --fixes lp:768495

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

[Andreas Preikschat (googol-deactivatedaccount)]

Resolved conflict

[Andreas Preikschat (googol-deactivatedaccount)]

- Fixed an i18n bug
- Remove shortcuts from the map if removed from the action list

[Raoul Snyman (raoul-snyman)]

shortcuts = map(unicode, map(QtGui.QKeySequence.toString, shortcuts))

I think you have two spaces in the line above... ^^

review: Needs Fixing

[Tim Bentley (trb143)]

review: Approve

[Raoul Snyman (raoul-snyman)]

review: Approve

=== 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-15 15:40:29 +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-10 20:31:16 +0000
+++ openlp/core/utils/actions.py	2011-12-15 15:40:29 +0000
@@ -188,6 +188,7 @@
     actions or categories.
     """
     instance = None
+    shortcut_map = {}
 
     def __init__(self):
         self.categories = CategoryList()
@@ -224,17 +225,45 @@
             self.categories[category].actions.append(action)
         else:
             self.categories[category].actions.add(action, weight)
-        if not category:
-            # 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
+        # We have to do this to ensure that the loaded shortcut list e. g.
+        # STRG+O (German) is converted to CTRL+O, which is only done when we
+        # convert the strings in this way (QKeySequence -> QString -> unicode).
+        shortcuts = map(QtGui.QKeySequence, shortcuts)
+        shortcuts = map(unicode, map(QtGui.QKeySequence.toString, 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 conflicts.
+        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):
         """
@@ -242,7 +271,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
@@ -254,6 +283,15 @@
         # Remove empty categories.
         if len(self.categories[category].actions) == 0:
             self.categories.remove(category)
+        shortcuts = map(unicode,
+            map(QtGui.QKeySequence.toString, action.shortcuts()))
+        for shortcut in shortcuts:
+            # Remove action from the list of actions which are using this
+            # shortcut.
+            ActionList.shortcut_map[shortcut].remove(action)
+            # Remove empty entries.
+            if not ActionList.shortcut_map[shortcut]:
+                del ActionList.shortcut_map[shortcut]
 
     def add_category(self, name, weight):
         """
@@ -275,6 +313,60 @@
             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:
+            # 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 shortcuts 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):
     """