From 290d9e5e660ba1e8ae73fc6ff2a5e437174542a8 Mon Sep 17 00:00:00 2001 From: Mathias Buhr Date: Sun, 28 Feb 2016 21:54:08 +0100 Subject: [PATCH] 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). --- gtk2_ardour/keyeditor.cc | 49 +++-- libs/gtkmm2ext/bindings.cc | 282 ++++++++++++---------------- libs/gtkmm2ext/gtkmm2ext/bindings.h | 19 +- 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 = (*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; /* global. Gulp */ list ActionMap::action_maps; /* global. Gulp */ PBD::Signal1 Bindings::BindingsChanged; + +/*============================ ActionNameRegistered ===========================*/ +template +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_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, 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(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 action; }; + typedef std::map 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, 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, Operation& op); bool load (XMLNode const& node); @@ -209,9 +212,7 @@ class LIBGTKMM2EXT_API Bindings { static PBD::Signal1 BindingsChanged; - private: - typedef std::map 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); + + KeybindingMap& get_keymap (Operation op); + const KeybindingMap& get_keymap (Operation op) const; + MouseButtonBindingMap& get_mousemap (Operation op); }; } // namespace -- 2.30.2