Merge branch 'shortcuts' (!41)

11 files changed, 115 insertions, 97 deletions

diff --git a/src/helper/window.cpp b/src/helper/window.cpp
index 270f9a3c8..28fc25606 100644
--- a/src/helper/window.cpp
+++ b/src/helper/window.cpp
@@ -13,26 +13,12 @@
 #include "desktop.h"
 #include "inkscape.h"
 #include "shortcuts.h"
-#include "ui/tools/tool-base.h"
 #include "window.h"
 
 static bool on_window_key_press(GdkEventKey* event)
 {
     unsigned shortcut = 0;
-    // FIXME why?
-    shortcut = Inkscape::UI::Tools::get_group0_keyval (event) |
-	           ( event->state & GDK_SHIFT_MASK ?
-	             SP_SHORTCUT_SHIFT_MASK : 0 ) |
-	           ( event->state & GDK_SUPER_MASK ?
-	             SP_SHORTCUT_SUPER_MASK : 0 ) |
-	           ( event->state & GDK_HYPER_MASK ?
-	             SP_SHORTCUT_HYPER_MASK : 0 ) |
-	           ( event->state & GDK_META_MASK ?
-	             SP_SHORTCUT_META_MASK : 0 ) |
-	           ( event->state & GDK_CONTROL_MASK ?
-	             SP_SHORTCUT_CONTROL_MASK : 0 ) |
-	           ( event->state & GDK_MOD1_MASK ?
-	             SP_SHORTCUT_ALT_MASK : 0 );
+    shortcut = sp_shortcut_get_for_event(event);
     return sp_shortcut_invoke (shortcut, SP_ACTIVE_DESKTOP);
 }
 
diff --git a/src/main.cpp b/src/main.cpp
index 6f7624cf7..27db9903b 100644
--- a/src/main.cpp
+++ b/src/main.cpp
@@ -931,7 +931,7 @@ namespace Inkscape {
 namespace UI {
 namespace Tools {
 
-guint get_group0_keyval(GdkEventKey const* event);
+guint get_group0_keyval(GdkEventKey const* event, guint *consumed_modifiers = NULL);
 
 }
 }
diff --git a/src/shortcuts.cpp b/src/shortcuts.cpp
index 52248b81f..dc6c885c0 100644
--- a/src/shortcuts.cpp
+++ b/src/shortcuts.cpp
@@ -124,35 +124,93 @@ static bool try_shortcuts_file(char const *filename, bool const is_user_set) {
 }
 
 /*
- *  Inkscape expects to add the Shift modifier to any accel_keys create with Shift
- *  For exmaple on en_US keyboard <Shift>+6 = "&" - in this case return <Shift>+&
- *  See get_group0_keyval() for explanation on why
+ * Return the keyval corresponding to the key event in group 0 and the effective modifiers.
+ *
+ * Usage of group 0 (i.e. the main, typically English layout) instead of simply event->keyval
+ * ensures that shortcuts work regardless of the active keyboard layouts (e.g. Cyrillic).
+ *
+ * The effective modifiers are the modifers that were not "consumed" by the translation and
+ * can be used by the application to define a shortcut, e.g.
+ *  - when pressing "Shift+9" the resulting character is "("
+ *    the shift key was "consumed" to make this character and should not be part of the shortcut
+ *  - when pressing "Ctrl+9" the resulting character is also "9"
+ *    the ctrl key was *not* consumed to make this character and must be included in the shortcut
+ *  - Exception: letter keys like [A-Z] always need the shift modifier,
+ *               otherwise lower case and uper case keys are treated as equivalent
+ * The modifier values are already transformed from the default GDK_*_MASK into the equivalent high-bit masks
+ * defined by SP_SHORTCUT_*_MASK to allow for subsequent packing of the whole shortcut into a single int
+ *
+ * Note: Don't call this function directly but use the wrappers
+ *        - sp_shortcut_get_from_event() - create a new shortcut from a key event
+ *        - sp_shortcut_get_for_event()  - get an existing shortcut for a key event
+ *       (they correctly handle the packing of modifier keys into the keyval)
+ */
+guint sp_shortcut_translate_event(GdkEventKey const *event, guint *effective_modifiers) {
+    guint keyval = 0;
+
+    guint initial_modifiers = event->state;
+    guint consumed_modifiers = 0;
+    guint remaining_modifiers = 0;
+    guint resulting_modifiers = 0;  // remaining modifiers encoded in high-bit mask
+
+    keyval = Inkscape::UI::Tools::get_group0_keyval(event, &consumed_modifiers);
+
+    remaining_modifiers = initial_modifiers & ~consumed_modifiers;
+    resulting_modifiers = ( remaining_modifiers & GDK_SHIFT_MASK ? SP_SHORTCUT_SHIFT_MASK : 0 ) |
+	                      ( remaining_modifiers & GDK_CONTROL_MASK ? SP_SHORTCUT_CONTROL_MASK : 0 ) |
+	                      ( remaining_modifiers & GDK_SUPER_MASK ? SP_SHORTCUT_SUPER_MASK : 0 ) |
+	                      ( remaining_modifiers & GDK_HYPER_MASK ? SP_SHORTCUT_HYPER_MASK : 0 ) |
+	                      ( remaining_modifiers & GDK_META_MASK ? SP_SHORTCUT_META_MASK : 0 ) |
+	                      ( remaining_modifiers & GDK_MOD1_MASK ? SP_SHORTCUT_ALT_MASK : 0 );              
+
+    // enforce the Shift modifier for uppercase letters (otherwise plain A and Shift+A are equivalent)
+    // for characters that are not letters both (is_upper and is_lower) return TRUE, so the condition is false
+    if (gdk_keyval_is_upper(keyval) && !gdk_keyval_is_lower(keyval)) {
+        resulting_modifiers |= SP_SHORTCUT_SHIFT_MASK;
+    }
+
+    *effective_modifiers = resulting_modifiers;
+    return keyval;
+}
+
+/*
+ * Returns a new Inkscape shortcut parsed from a key event.
  */
-unsigned int sp_gdkmodifier_to_shortcut(guint accel_key, Gdk::ModifierType gdkmodifier, guint hardware_keycode) {
+unsigned int sp_shortcut_get_from_event(GdkEventKey const *event) {
+    guint effective_modifiers;
 
+    sp_shortcut_translate_event(event, &effective_modifiers);
 
-    unsigned int shortcut = 0;
+    // return the actual keyval and the corresponding modifiers for creating the shortcut
+    // we must not return the translated keyval, otherwise we end up with illegal shortcuts like "Shift+9" instead of "("
+    return (event->keyval) | effective_modifiers;
+}
+
+/*
+ * Returns a new Inkscape shortcut parsed from a key event.
+ * (equivalent to sp_shortcut_get_from_event() but accepts the arguments of Gtk::CellRendererAccel::signal_accel_edited)
+ */
+unsigned int sp_shortcut_get_from_gdk_event(guint accel_key, Gdk::ModifierType accel_mods, guint hardware_keycode) {
     GdkEventKey event;
-    event.state = gdkmodifier;
     event.keyval = accel_key;
+    event.state = accel_mods;
     event.hardware_keycode = hardware_keycode;
-    guint keyval = Inkscape::UI::Tools::get_group0_keyval (&event);
-
-    shortcut = accel_key |
-               ( (gdkmodifier & GDK_SHIFT_MASK) || ( accel_key != keyval) ?
-                 SP_SHORTCUT_SHIFT_MASK : 0 ) |
-               ( gdkmodifier & GDK_CONTROL_MASK ?
-                 SP_SHORTCUT_CONTROL_MASK : 0 ) |
-               ( gdkmodifier & GDK_SUPER_MASK ?
-                 SP_SHORTCUT_SUPER_MASK : 0 ) |
-               ( gdkmodifier & GDK_HYPER_MASK ?
-                 SP_SHORTCUT_HYPER_MASK : 0 ) |
-               ( gdkmodifier & GDK_META_MASK ?
-                 SP_SHORTCUT_META_MASK : 0 ) |
-               ( gdkmodifier & GDK_MOD1_MASK ?
-                 SP_SHORTCUT_ALT_MASK : 0 );
-
-    return shortcut;
+
+    return sp_shortcut_get_from_event(&event);
+}
+
+/*
+ * Returns the Inkscape-internal integral shortcut representation for a key event.
+ * Use this to compare the received key event to known shortcuts.
+ */
+unsigned int sp_shortcut_get_for_event(GdkEventKey const *event) {
+    guint keyval;
+    guint effective_modifiers;
+
+    keyval = sp_shortcut_translate_event(event, &effective_modifiers);
+
+    // return the keyval translated to group 0 (English keyboard layout) and corresponding modifiers
+    return keyval | effective_modifiers;
 }
 
 Glib::ustring sp_shortcut_to_label(unsigned int const shortcut) {
@@ -335,13 +393,14 @@ void sp_shortcut_file_export()
        Inkscape::UI::Dialog::FileSaveDialog::create(
             *(desktop->getToplevel()),
             open_path,
-        Inkscape::UI::Dialog::CUSTOM_TYPE,
-        _("Select a filename for exporting"),
-        "",
-        "",
-        Inkscape::Extension::FILE_SAVE_METHOD_SAVE_AS
+            Inkscape::UI::Dialog::CUSTOM_TYPE,
+            _("Select a filename for exporting"),
+            "",
+            "",
+            Inkscape::Extension::FILE_SAVE_METHOD_SAVE_AS
         );
-    saveDialog->addFileType("All Files", "*");
+    saveDialog->addFileType(_("Inkscape shortcuts (*.xml)"), ".xml");
+    
 
     bool success = saveDialog->show();
     if (!success) {
diff --git a/src/shortcuts.h b/src/shortcuts.h
index fb36dfd11..2cfd4f495 100644
--- a/src/shortcuts.h
+++ b/src/shortcuts.h
@@ -51,7 +51,9 @@ void sp_shortcut_add_to_file(char const *action, unsigned int const shortcut);
 void sp_shortcut_delete_from_file(char const *action, unsigned int const shortcut);
 void sp_shortcuts_delete_all_from_file();
 Glib::ustring sp_shortcut_to_label(unsigned int const shortcut);
-unsigned int sp_gdkmodifier_to_shortcut(unsigned int accel_key, Gdk::ModifierType gdkmodifier, unsigned int hardware_keycode);
+unsigned int sp_shortcut_get_from_event(GdkEventKey const *event);
+unsigned int sp_shortcut_get_from_gdk_event(unsigned int accel_key, Gdk::ModifierType accel_mods, unsigned int hardware_keycode);
+unsigned int sp_shortcut_get_for_event(GdkEventKey const *event);
 void sp_shortcut_get_file_names(std::vector<Glib::ustring> *names, std::vector<Glib::ustring> *paths);
 bool sp_shortcut_is_user_set(Inkscape::Verb *verb);
 void sp_shortcut_file_export();
diff --git a/src/ui/dialog/dialog.cpp b/src/ui/dialog/dialog.cpp
index 0f4f30ac0..9037e8377 100644
--- a/src/ui/dialog/dialog.cpp
+++ b/src/ui/dialog/dialog.cpp
@@ -287,13 +287,7 @@ bool Dialog::_onEvent(GdkEvent *event)
 bool Dialog::_onKeyPress(GdkEventKey *event)
 {
     unsigned int shortcut;
-    shortcut = Inkscape::UI::Tools::get_group0_keyval(event) |
-        ( event->state & GDK_SHIFT_MASK ?
-          SP_SHORTCUT_SHIFT_MASK : 0 ) |
-        ( event->state & GDK_CONTROL_MASK ?
-          SP_SHORTCUT_CONTROL_MASK : 0 ) |
-        ( event->state & GDK_MOD1_MASK ?
-          SP_SHORTCUT_ALT_MASK : 0 );
+    shortcut = sp_shortcut_get_for_event((GdkEventKey*)event);
     return sp_shortcut_invoke(shortcut, SP_ACTIVE_DESKTOP);
 }
 
diff --git a/src/ui/dialog/inkscape-preferences.cpp b/src/ui/dialog/inkscape-preferences.cpp
index 5ebad0746..13729cf0f 100644
--- a/src/ui/dialog/inkscape-preferences.cpp
+++ b/src/ui/dialog/inkscape-preferences.cpp
@@ -1535,7 +1535,7 @@ void InkscapePreferences::initKeyboardShortcuts(Gtk::TreeModel::iterator iter_ui
     _page_keyshortcuts.add_line( false, _("Search:"), _kb_search, "", "", true);
 
     _kb_store = Gtk::TreeStore::create( _kb_columns );
-    _kb_store->set_sort_column (_kb_columns.id, Gtk::SORT_ASCENDING );
+    _kb_store->set_sort_column ( GTK_TREE_SORTABLE_UNSORTED_SORT_COLUMN_ID, Gtk::SORT_ASCENDING ); // only sort in onKBListKeyboardShortcuts()
 
     _kb_filter = Gtk::TreeModelFilter::create(_kb_store);
     _kb_filter->set_visible_func (sigc::mem_fun(*this, &InkscapePreferences::onKBSearchFilter));
@@ -1676,7 +1676,7 @@ void InkscapePreferences::onKBTreeEdited (const Glib::ustring& path, guint accel
         return;
     }
 
-    unsigned int const new_shortcut_id =  sp_gdkmodifier_to_shortcut(accel_key, accel_mods, hardware_keycode);
+    unsigned int const new_shortcut_id =  sp_shortcut_get_from_gdk_event(accel_key, accel_mods, hardware_keycode);
     if (new_shortcut_id) {
 
         // Delete current shortcut if it existed
@@ -1812,7 +1812,7 @@ void InkscapePreferences::onKBListKeyboardShortcuts()
         if (shortcut_id != GDK_KEY_VoidSymbol) {
             gchar* str = sp_shortcut_get_label(shortcut_id);
             if (str) {
-                shortcut_label = str;
+                shortcut_label = Glib::Markup::escape_text(str);
                 g_free(str);
                 str = 0;
             }
@@ -1833,6 +1833,10 @@ void InkscapePreferences::onKBListKeyboardShortcuts()
         }
     }
 
+    // re-order once after updating (then disable ordering again to increase performance)
+    _kb_store->set_sort_column (_kb_columns.id, Gtk::SORT_ASCENDING );
+    _kb_store->set_sort_column ( GTK_TREE_SORTABLE_UNSORTED_SORT_COLUMN_ID, Gtk::SORT_ASCENDING );
+
     if (selected_id.empty()) {
         _kb_tree.expand_to_path(_kb_store->get_path(_kb_store->get_iter("0:1")));
     }
diff --git a/src/ui/dialog/objects.cpp b/src/ui/dialog/objects.cpp
index a28c75514..dd8879dc1 100644
--- a/src/ui/dialog/objects.cpp
+++ b/src/ui/dialog/objects.cpp
@@ -704,13 +704,7 @@ bool ObjectsPanel::_handleKeyEvent(GdkEventKey *event)
         return false;
 
     unsigned int shortcut;
-    shortcut = Inkscape::UI::Tools::get_group0_keyval(event) |
-        ( event->state & GDK_SHIFT_MASK ?
-          SP_SHORTCUT_SHIFT_MASK : 0 ) |
-        ( event->state & GDK_CONTROL_MASK ?
-          SP_SHORTCUT_CONTROL_MASK : 0 ) |
-        ( event->state & GDK_MOD1_MASK ?
-          SP_SHORTCUT_ALT_MASK : 0 );
+    shortcut = sp_shortcut_get_for_event(event);
 
     switch (shortcut) {
         // how to get users key binding for the action “start-interactive-search” ??
diff --git a/src/ui/dialog/xml-tree.cpp b/src/ui/dialog/xml-tree.cpp
index d39a4cb8d..83c0de45b 100644
--- a/src/ui/dialog/xml-tree.cpp
+++ b/src/ui/dialog/xml-tree.cpp
@@ -347,13 +347,7 @@ void XmlTree::attr_reset_context(gint attr)
 
 bool XmlTree::sp_xml_tree_key_press(GdkEventKey *event)
 {
-    unsigned int shortcut = Inkscape::UI::Tools::get_group0_keyval (event) |
-        ( event->state & GDK_SHIFT_MASK ?
-          SP_SHORTCUT_SHIFT_MASK : 0 ) |
-        ( event->state & GDK_CONTROL_MASK ?
-          SP_SHORTCUT_CONTROL_MASK : 0 ) |
-        ( event->state & GDK_MOD1_MASK ?
-          SP_SHORTCUT_ALT_MASK : 0 );
+    unsigned int shortcut = sp_shortcut_get_for_event((GdkEventKey*)event);
 
     /* fixme: if you need to add more xml-tree-specific callbacks, you should probably upgrade
      * the sp_shortcut mechanism to take into account windows. */
diff --git a/src/ui/tools/pen-tool.cpp b/src/ui/tools/pen-tool.cpp
index 265ddf2ea..4f19b25c6 100644
--- a/src/ui/tools/pen-tool.cpp
+++ b/src/ui/tools/pen-tool.cpp
@@ -1050,13 +1050,7 @@ bool PenTool::_handleKeyPress(GdkEvent *event) {
 
     // Check for undo if we have started drawing a path.
     if (this->npoints > 0) {
-        unsigned int shortcut = Inkscape::UI::Tools::get_group0_keyval (&event->key) |
-                   ( event->key.state & GDK_SHIFT_MASK ?
-                     SP_SHORTCUT_SHIFT_MASK : 0 ) |
-                   ( event->key.state & GDK_CONTROL_MASK ?
-                     SP_SHORTCUT_CONTROL_MASK : 0 ) |
-                   ( event->key.state & GDK_MOD1_MASK ?
-                     SP_SHORTCUT_ALT_MASK : 0 );
+        unsigned int shortcut = sp_shortcut_get_for_event((GdkEventKey*)event);
         Inkscape::Verb* verb = sp_shortcut_get_verb(shortcut);
         if (verb) {
             unsigned int vcode = verb->get_code();
diff --git a/src/ui/tools/tool-base.cpp b/src/ui/tools/tool-base.cpp
index 59346b491..12c3a3675 100644
--- a/src/ui/tools/tool-base.cpp
+++ b/src/ui/tools/tool-base.cpp
@@ -606,20 +606,7 @@ bool ToolBase::root_handler(GdkEvent* event) {
         case GDK_KEY_Tab:
         case GDK_KEY_ISO_Left_Tab:
         case GDK_KEY_F1:
-            shortcut = get_group0_keyval(&event->key);
-
-            if (event->key.state & GDK_SHIFT_MASK) {
-                shortcut |= SP_SHORTCUT_SHIFT_MASK;
-            }
-
-            if (event->key.state & GDK_CONTROL_MASK) {
-                shortcut |= SP_SHORTCUT_CONTROL_MASK;
-            }
-
-            if (event->key.state & GDK_MOD1_MASK) {
-                shortcut |= SP_SHORTCUT_ALT_MASK;
-            }
-
+            shortcut = sp_shortcut_get_for_event((GdkEventKey*)event);
             ret = sp_shortcut_invoke(shortcut, desktop);
             break;
 
@@ -1178,14 +1165,18 @@ void sp_event_show_modifier_tip(Inkscape::MessageContext *message_context,
  * Use this instead of simply event->keyval, so that your keyboard shortcuts
  * work regardless of layouts (e.g., in Cyrillic).
  */
-guint get_group0_keyval(GdkEventKey const *event) {
+guint get_group0_keyval(GdkEventKey const *event, guint *consumed_modifiers /*= NULL*/) {
     guint keyval = 0;
+    GdkModifierType modifiers;
 
-    gdk_keymap_translate_keyboard_state(gdk_keymap_get_for_display(
-            gdk_display_get_default()), event->hardware_keycode,
-            (GdkModifierType) event->state, 0 /*event->key.group*/, &keyval,
-            NULL, NULL, NULL);
+    gdk_keymap_translate_keyboard_state(
+            gdk_keymap_get_for_display(gdk_display_get_default()),
+            event->hardware_keycode, (GdkModifierType) event->state, 0 /*event->group*/,
+            &keyval, NULL, NULL, &modifiers);
 
+    if (consumed_modifiers) {
+        *consumed_modifiers = modifiers;
+    }
     return keyval;
 }
 
diff --git a/src/ui/tools/tool-base.h b/src/ui/tools/tool-base.h
index 52f641ecb..09a9db660 100644
--- a/src/ui/tools/tool-base.h
+++ b/src/ui/tools/tool-base.h
@@ -251,7 +251,7 @@ gint gobble_motion_events(gint mask);
 void sp_event_show_modifier_tip(Inkscape::MessageContext *message_context, GdkEvent *event,
                                 gchar const *ctrl_tip, gchar const *shift_tip, gchar const *alt_tip);
 
-guint get_group0_keyval(GdkEventKey const *event);
+guint get_group0_keyval(GdkEventKey const *event, guint *consumed_modifiers = NULL);
 
 SPItem *sp_event_context_find_item (SPDesktop *desktop, Geom::Point const &p, bool select_under, bool into_groups);
 SPItem *sp_event_context_over_item (SPDesktop *desktop, SPItem *item, Geom::Point const &p);