Small refactoring of keyboard bindings (first part)

- 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).

3 files changed, 163 insertions, 187 deletions

diff --git a/gtk2_ardour/keyeditor.cc b/gtk2_ardour/keyeditor.cc
index ba360e9de7..60e8bf58f5 100644
--- a/gtk2_ardour/keyeditor.cc
+++ b/gtk2_ardour/keyeditor.cc
@@ -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);
 	}
 }
 
diff --git a/libs/gtkmm2ext/bindings.cc b/libs/gtkmm2ext/bindings.cc
index aec2486803..802e32dd1a 100644
--- a/libs/gtkmm2ext/bindings.cc
+++ b/libs/gtkmm2ext/bindings.cc
@@ -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)
diff --git a/libs/gtkmm2ext/gtkmm2ext/bindings.h b/libs/gtkmm2ext/gtkmm2ext/bindings.h
index 6b1f4d122c..45c030b4c6 100644
--- a/libs/gtkmm2ext/gtkmm2ext/bindings.h
+++ b/libs/gtkmm2ext/gtkmm2ext/bindings.h
@@ -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