Small refactoring of keyboard bindings (first part)
authorMathias Buhr <napcode@apparatus.de>
Sun, 28 Feb 2016 20:54:08 +0000 (21:54 +0100)
committerRobin Gareus <robin@gareus.org>
Sat, 5 Mar 2016 15:27:48 +0000 (16:27 +0100)
- Adds collision detection for keybindings
- Fixes a bug that prevented newly created bindings to be deleted properly (reproduction: add a binding, remove it, restart ardour, binding is still there but can now be deleted).

gtk2_ardour/keyeditor.cc
libs/gtkmm2ext/bindings.cc
libs/gtkmm2ext/gtkmm2ext/bindings.h

index ba360e9de7885804de4453ecb4448526dc16c99f..60e8bf58f5feef73b6f84934f198a775f04b31be 100644 (file)
@@ -51,6 +51,19 @@ using namespace PBD;
 using Gtkmm2ext::Keyboard;
 using Gtkmm2ext::Bindings;
 
+/*========================================== HELPER =========================================*/
+void bindings_collision_dialog (Gtk::Window& parent)
+{
+       ArdourDialog dialog (parent, _("Colliding keybindings"), true);
+       Label label (_("The key sequence is already bound. Please remove the other binding first."));
+
+       dialog.get_vbox()->pack_start (label, true, true);
+       dialog.add_button (_("Ok"), Gtk::RESPONSE_ACCEPT);
+       dialog.show_all ();
+       dialog.run();
+}
+
+/*======================================== KEYEDITOR =========================================*/
 KeyEditor::KeyEditor ()
        : ArdourWindow (_("Key Bindings"))
        , unbind_button (_("Remove shortcut"))
@@ -211,13 +224,13 @@ KeyEditor::Tab::unbind ()
        owner.unbind_button.set_sensitive (false);
 
        if (i != model->children().end()) {
-               Glib::RefPtr<Action> action = (*i)[columns.action];
-
                if (!(*i)[columns.bindable]) {
                        return;
                }
 
-               bindings->remove (action, Gtkmm2ext::Bindings::Press, true);
+               const std::string& action_path = (*i)[columns.path];
+
+               bindings->remove (Gtkmm2ext::Bindings::Press,  action_path , true);
                (*i)[columns.binding] = string ();
        }
 }
@@ -227,23 +240,29 @@ KeyEditor::Tab::bind (GdkEventKey* release_event, guint pressed_key)
 {
        TreeModel::iterator i = view.get_selection()->get_selected();
 
-       if (i != model->children().end()) {
+       if (i == model->children().end()) {
+               return;
+       }
 
-               string action_name = (*i)[columns.path];
+       string action_path = (*i)[columns.path];
 
-               if (!(*i)[columns.bindable]) {
-                       return;
-               }
+       if (!(*i)[columns.bindable]) {
+               return;
+       }
 
-               GdkModifierType mod = (GdkModifierType)(Keyboard::RelevantModifierKeyMask & release_event->state);
-               Gtkmm2ext::KeyboardKey new_binding (mod, pressed_key);
+       GdkModifierType mod = (GdkModifierType)(Keyboard::RelevantModifierKeyMask & release_event->state);
+       Gtkmm2ext::KeyboardKey new_binding (mod, pressed_key);
 
-               bool result = bindings->replace (new_binding, Gtkmm2ext::Bindings::Press, action_name);
+       if (bindings->is_bound (new_binding, Gtkmm2ext::Bindings::Press)) {
+               bindings_collision_dialog (owner);
+               return;
+       }
 
-               if (result) {
-                       (*i)[columns.binding] = gtk_accelerator_get_label (new_binding.key(), (GdkModifierType) new_binding.state());
-                       owner.unbind_button.set_sensitive (true);
-               }
+       bool result = bindings->replace (new_binding, Gtkmm2ext::Bindings::Press, action_path);
+
+       if (result) {
+               (*i)[columns.binding] = gtk_accelerator_get_label (new_binding.key(), (GdkModifierType) new_binding.state());
+               owner.unbind_button.set_sensitive (true);
        }
 }
 
index aec248680341afa540fb778d9cbbd09d2e3e18cf..802e32dd1affc0204b6763eedfbcf6a20ac7e449 100644 (file)
@@ -46,6 +46,22 @@ list<Bindings*> Bindings::bindings; /* global. Gulp */
 list<ActionMap*> ActionMap::action_maps; /* global. Gulp */
 PBD::Signal1<void,Bindings*> Bindings::BindingsChanged;
 
+
+/*============================ ActionNameRegistered ===========================*/
+template <typename IteratorValueType>
+struct ActionNameRegistered
+{
+       ActionNameRegistered(std::string const& name)
+       : action_name(name)
+       {}
+
+       bool operator()(IteratorValueType elem) const {
+               return elem.second.action_name == action_name;
+       }
+       std::string const& action_name;
+};
+
+/*================================ MouseButton ================================*/
 MouseButton::MouseButton (uint32_t state, uint32_t keycode)
 {
         uint32_t ignore = ~Keyboard::RelevantModifierKeyMask;
@@ -140,6 +156,7 @@ MouseButton::name () const
         return str;
 }
 
+/*================================ KeyboardKey ================================*/
 KeyboardKey::KeyboardKey (uint32_t state, uint32_t keycode)
 {
         uint32_t ignore = ~Keyboard::RelevantModifierKeyMask;
@@ -270,6 +287,7 @@ KeyboardKey::make_key (const string& str, KeyboardKey& k)
         return true;
 }
 
+/*================================= Bindings =================================*/
 Bindings::Bindings (std::string const& name)
        : _name (name)
        , _action_map (0)
@@ -374,16 +392,8 @@ Bindings::empty() const
 bool
 Bindings::activate (KeyboardKey kb, Operation op)
 {
-        KeybindingMap* kbm = 0;
-
-        switch (op) {
-        case Press:
-                kbm = &press_bindings;
-                break;
-        case Release:
-                kbm = &release_bindings;
-                break;
-        }
+       KeybindingMap& kbm = get_keymap (op);
+
 
         /* if shift was pressed, GDK will send us (e.g) 'E' rather than 'e'.
            Our bindings all use the lower case character/keyname, so switch
@@ -392,9 +402,9 @@ Bindings::activate (KeyboardKey kb, Operation op)
 
         KeyboardKey unshifted (kb.state(), gdk_keyval_to_lower (kb.key()));
 
-        KeybindingMap::iterator k = kbm->find (unshifted);
+        KeybindingMap::iterator k = kbm.find (unshifted);
 
-        if (k == kbm->end()) {
+        if (k == kbm.end()) {
                 /* no entry for this key in the state map */
                DEBUG_TRACE (DEBUG::Bindings, string_compose ("no binding for %1\n", unshifted));
                return false;
@@ -506,156 +516,67 @@ Bindings::replace (KeyboardKey kb, Operation op, string const & action_name, boo
                return false;
        }
 
-       /* We have to search the existing binding map by both action and
-        * keybinding, because the following are possible:
-        *
-        *   - key is already used for a different action
-        *   - action has a different binding
-        *   - key is not used
-        *   - action is not bound
-        */
-
-       RefPtr<Action> action = _action_map->find_action (action_name);
-
-        if (!action) {
-               return false;
-        }
-
-        KeybindingMap* kbm = 0;
-
-        switch (op) {
-        case Press:
-                kbm = &press_bindings;
-                break;
-        case Release:
-                kbm = &release_bindings;
-                break;
-        }
-
-        KeybindingMap::iterator k = kbm->find (kb);
-
-        if (k != kbm->end()) {
-               kbm->erase (k);
-        }
-
-        /* now linear search by action */
-
-        for (k = kbm->begin(); k != kbm->end(); ++k) {
-               if (k->second.action_name == action_name) {
-                       kbm->erase (k);
-                       break;
-               }
-        }
-
-        add (kb, op, action_name, can_save);
-
-        /* for now, this never fails */
-
-        return true;
+       if (is_registered(op, action_name)) {
+               remove(op, action_name, can_save);
+       }
+       add (kb, op, action_name, can_save);
+       return true;
 }
 
-void
+bool
 Bindings::add (KeyboardKey kb, Operation op, string const& action_name, bool can_save)
 {
-        KeybindingMap* kbm = 0;
-
-        switch (op) {
-        case Press:
-               kbm = &press_bindings;
-                break;
-        case Release:
-                kbm = &release_bindings;
-                break;
-        }
+       if (is_registered(op, action_name)) {
+               return false;
+       }
 
-        KeybindingMap::iterator k = kbm->find (kb);
+       KeybindingMap& kbm = get_keymap (op);
 
-        if (k != kbm->end()) {
-               kbm->erase (k);
-        }
-        KeybindingMap::value_type new_pair (kb, ActionInfo (action_name));
+       KeybindingMap::value_type new_pair (kb, ActionInfo (action_name));
+       kbm.insert (new_pair).first;
 
-        kbm->insert (new_pair).first;
-
-        if (can_save) {
-               Keyboard::keybindings_changed ();
-        }
+       if (can_save) {
+               Keyboard::keybindings_changed ();
+       }
 
-        BindingsChanged (this); /* EMIT SIGNAL */
+       BindingsChanged (this); /* EMIT SIGNAL */
+       return true;
 }
 
-void
-Bindings::remove (KeyboardKey kb, Operation op, bool can_save)
+bool
+Bindings::remove (Operation op, std::string const& action_name, bool can_save)
 {
-        KeybindingMap* kbm = 0;
-
-        switch (op) {
-        case Press:
-                kbm = &press_bindings;
-                break;
-        case Release:
-                kbm = &release_bindings;
-                break;
-        }
-
-        KeybindingMap::iterator k = kbm->find (kb);
+       bool erased_action = false;
+       KeybindingMap& kbm = get_keymap (op);
+       for (KeybindingMap::iterator k = kbm.begin(); k != kbm.end(); ++k) {
+               if (k->second.action_name == action_name) {
+                       kbm.erase (k);
+                       erased_action = true;
+                       break;
+               }
+       }
 
-        if (k != kbm->end()) {
-                kbm->erase (k);
-        }
+       if (!erased_action) {
+               return erased_action;
+       }
 
-        if (can_save) {
-               Keyboard::keybindings_changed ();
-        }
+       if (can_save) {
+               Keyboard::keybindings_changed ();
+       }
 
-        BindingsChanged (this); /* EMIT SIGNAL */
+       BindingsChanged (this); /* EMIT SIGNAL */
+       return erased_action;
 }
 
-void
-Bindings::remove (RefPtr<Action> action, Operation op, bool can_save)
-{
-        KeybindingMap* kbm = 0;
-
-        switch (op) {
-        case Press:
-                kbm = &press_bindings;
-                break;
-        case Release:
-                kbm = &release_bindings;
-                break;
-        }
-
-        for (KeybindingMap::iterator k = kbm->begin(); k != kbm->end(); ++k) {
-               if (k->second.action == action) {
-                       kbm->erase (k);
-                       break;
-               }
-        }
-
-        if (can_save) {
-               Keyboard::keybindings_changed ();
-        }
-
-        BindingsChanged (this); /* EMIT SIGNAL */
-}
 
 bool
 Bindings::activate (MouseButton bb, Operation op)
 {
-        MouseButtonBindingMap* bbm = 0;
-
-        switch (op) {
-        case Press:
-                bbm = &button_press_bindings;
-                break;
-        case Release:
-                bbm = &button_release_bindings;
-                break;
-        }
+       MouseButtonBindingMap& bbm = get_mousemap(op);
 
-        MouseButtonBindingMap::iterator b = bbm->find (bb);
+        MouseButtonBindingMap::iterator b = bbm.find (bb);
 
-        if (b == bbm->end()) {
+        if (b == bbm.end()) {
                 /* no entry for this key in the state map */
                 return false;
         }
@@ -684,39 +605,20 @@ Bindings::activate (MouseButton bb, Operation op)
 void
 Bindings::add (MouseButton bb, Operation op, string const& action_name)
 {
-        MouseButtonBindingMap* bbm = 0;
-
-        switch (op) {
-        case Press:
-                bbm = &button_press_bindings;
-                break;
-        case Release:
-                bbm = &button_release_bindings;
-                break;
-        }
+       MouseButtonBindingMap& bbm = get_mousemap(op);
 
         MouseButtonBindingMap::value_type newpair (bb, ActionInfo (action_name));
-        bbm->insert (newpair);
+        bbm.insert (newpair);
 }
 
 void
 Bindings::remove (MouseButton bb, Operation op)
 {
-        MouseButtonBindingMap* bbm = 0;
-
-        switch (op) {
-        case Press:
-                bbm = &button_press_bindings;
-                break;
-        case Release:
-                bbm = &button_release_bindings;
-                break;
-        }
-
-        MouseButtonBindingMap::iterator b = bbm->find (bb);
+       MouseButtonBindingMap& bbm = get_mousemap(op);
+        MouseButtonBindingMap::iterator b = bbm.find (bb);
 
-        if (b != bbm->end()) {
-                bbm->erase (b);
+        if (b != bbm.end()) {
+                bbm.erase (b);
         }
 }
 
@@ -899,6 +801,56 @@ Bindings::associate_all ()
        }
 }
 
+bool
+Bindings::is_bound (KeyboardKey const& kb, Operation op) const
+{
+       const KeybindingMap& km = get_keymap(op);
+       return km.find(kb) != km.end();
+}
+
+bool
+Bindings::is_registered (Operation op, std::string const& action_name) const
+{
+       const KeybindingMap& km = get_keymap(op);
+       return std::find_if(km.begin(),  km.end(),  ActionNameRegistered<KeybindingMap::const_iterator::value_type>(action_name)) != km.end();
+}
+
+Bindings::KeybindingMap& 
+Bindings::get_keymap (Operation op)
+{
+       switch (op) {
+               case Press:
+                       return press_bindings;
+               case Release:
+               default:
+                       return release_bindings;
+       }
+}
+
+const Bindings::KeybindingMap& 
+Bindings::get_keymap (Operation op) const
+{
+       switch (op) {
+               case Press:
+                       return press_bindings;
+               case Release:
+               default:
+                       return release_bindings;
+       }
+}
+
+Bindings::MouseButtonBindingMap& 
+Bindings::get_mousemap (Operation op)
+{
+       switch (op) {
+               case Press:
+                       return button_press_bindings;
+               case Release:
+               default:
+                       return button_release_bindings;
+       }
+}
+
 /*==========================================ACTION MAP =========================================*/
 
 ActionMap::ActionMap (string const & name)
index 6b1f4d122c618359ba21c2bc53a1c3b7349f0e33..45c030b4c6c49d1ec97d57def8c8a14138ade4f1 100644 (file)
@@ -136,7 +136,7 @@ class LIBGTKMM2EXT_API ActionMap {
 };
 
 class LIBGTKMM2EXT_API Bindings {
-  public:
+       public:
         enum Operation {
                 Press,
                 Release
@@ -148,6 +148,7 @@ class LIBGTKMM2EXT_API Bindings {
                std::string action_name;
                Glib::RefPtr<Gtk::Action> action;
         };
+               typedef std::map<KeyboardKey,ActionInfo> KeybindingMap;
 
         Bindings (std::string const& name);
         ~Bindings ();
@@ -161,10 +162,9 @@ class LIBGTKMM2EXT_API Bindings {
         bool empty_keys () const;
         bool empty_mouse () const;
 
-        void add (KeyboardKey, Operation, std::string const&, bool can_save = false);
+        bool add (KeyboardKey, Operation, std::string const&, bool can_save = false);
         bool replace (KeyboardKey, Operation, std::string const& action_name, bool can_save = true);
-        void remove (KeyboardKey, Operation, bool can_save = false);
-        void remove (Glib::RefPtr<Gtk::Action>, Operation, bool can_save = false);
+        bool remove (Operation, std::string const& action_name, bool can_save = false);
 
         bool activate (KeyboardKey, Operation);
 
@@ -172,6 +172,9 @@ class LIBGTKMM2EXT_API Bindings {
         void remove (MouseButton, Operation);
         bool activate (MouseButton, Operation);
 
+               bool is_bound (KeyboardKey const&, Operation) const;
+               bool is_registered (Operation op, std::string const& action_name) const;
+
         KeyboardKey get_binding_for_action (Glib::RefPtr<Gtk::Action>, Operation& op);
 
         bool load (XMLNode const& node);
@@ -209,9 +212,7 @@ class LIBGTKMM2EXT_API Bindings {
 
        static PBD::Signal1<void,Bindings*> BindingsChanged;
 
-  private:
-        typedef std::map<KeyboardKey,ActionInfo> KeybindingMap;
-
+       private:
         std::string  _name;
         ActionMap*   _action_map;
         KeybindingMap press_bindings;
@@ -222,6 +223,10 @@ class LIBGTKMM2EXT_API Bindings {
         MouseButtonBindingMap button_release_bindings;
 
         void push_to_gtk (KeyboardKey, Glib::RefPtr<Gtk::Action>);
+
+               KeybindingMap& get_keymap (Operation op);
+               const KeybindingMap& get_keymap (Operation op) const;
+               MouseButtonBindingMap& get_mousemap (Operation op);
 };
 
 } // namespace