View Issue Details

IDCategoryLast Update
0006986features2016-08-29 14:28
ReporterAdmiralBumbleBeeAssigned To 
Reproducibilityalways 
Status newResolutionopen 
PlatformAnyOSOS Version
Product Version 
Fixed in Version 
Summary0006986: Include more information in bindings_collision_dialog
DescriptionCurrently when there's a colliding binding made, the user is not told which binding it collides with or what keys they pressed.

Ideally the error dialog should show which binding is colliding with the new assignment, what assignment is made, and potentially an option to jump to the old binding to replace it.

Image attached of better behaviour.
Steps To ReproduceCreate a binding that already exists. Look at the error dialog that shows up.
TagsNo tags attached.

Activities

AdmiralBumbleBee

2016-08-27 17:20

reporter  

AdmiralBumbleBee

2016-08-28 16:00

reporter  

binding.patch (3,027 bytes)
From c321d82e2ad5c49aef55f1b847336026668badcf Mon Sep 17 00:00:00 2001
From: ArdourEnv <ardourenv@Roberts-iMac.home>
Date: Sun, 28 Aug 2016 11:53:48 -0400
Subject: [PATCH] Unfortunate little patch for more clear binding's error
 dialog

---
 gtk2_ardour/keyeditor.cc            | 9 ++++++---
 libs/gtkmm2ext/bindings.cc          | 7 +++++++
 libs/gtkmm2ext/gtkmm2ext/bindings.h | 1 +
 3 files changed, 14 insertions(+), 3 deletions(-)

diff --git a/gtk2_ardour/keyeditor.cc b/gtk2_ardour/keyeditor.cc
index aea8185..18d8a3d 100644
--- a/gtk2_ardour/keyeditor.cc
+++ b/gtk2_ardour/keyeditor.cc
@@ -62,10 +62,13 @@ using Gtkmm2ext::Bindings;
 
 sigc::signal<void> KeyEditor::UpdateBindings;
 
-void bindings_collision_dialog (Gtk::Window& parent)
+void bindings_collision_dialog (Gtk::Window& parent, const string &missing_binding, Gtkmm2ext::KeyboardKey& key)
 {
 	ArdourDialog dialog (parent, _("Colliding keybindings"), true);
-	Label label (_("The key sequence is already bound. Please remove the other binding first."));
+    // Clearly _exceptionally_ ugly. I didn't want to mess with localization stuff.
+    string const label_text(string("The key sequence is already bound. Please remove the other binding first.").insert(33, " to " + key.display_label()).insert(17, missing_binding + " "));
+
+	Label label (label_text);
 
 	dialog.get_vbox()->pack_start (label, true, true);
 	dialog.add_button (_("Ok"), Gtk::RESPONSE_ACCEPT);
@@ -316,7 +319,7 @@ KeyEditor::Tab::bind (GdkEventKey* release_event, guint pressed_key)
 	Gtkmm2ext::KeyboardKey new_binding (mod, pressed_key);
 
 	if (bindings->is_bound (new_binding, Gtkmm2ext::Bindings::Press)) {
-		bindings_collision_dialog (owner);
+		bindings_collision_dialog (owner, bindings->get_action_name_by_key(new_binding, Gtkmm2ext::Bindings::Press), new_binding);
 		return;
 	}
 
diff --git a/libs/gtkmm2ext/bindings.cc b/libs/gtkmm2ext/bindings.cc
index e25ed20..de45495 100644
--- a/libs/gtkmm2ext/bindings.cc
+++ b/libs/gtkmm2ext/bindings.cc
@@ -1066,6 +1066,13 @@ Bindings::is_bound (KeyboardKey const& kb, Operation op) const
 	return km.find(kb) != km.end();
 }
 
+string const&
+Bindings::get_action_name_by_key (KeyboardKey const& kb, Operation op) const
+{
+	const KeybindingMap& km = get_keymap(op);
+	return km.find(kb)->second.action_name;
+}
+
 bool
 Bindings::is_registered (Operation op, std::string const& action_name) const
 {
diff --git a/libs/gtkmm2ext/gtkmm2ext/bindings.h b/libs/gtkmm2ext/gtkmm2ext/bindings.h
index d216e04..05d44c3 100644
--- a/libs/gtkmm2ext/gtkmm2ext/bindings.h
+++ b/libs/gtkmm2ext/gtkmm2ext/bindings.h
@@ -178,6 +178,7 @@ class LIBGTKMM2EXT_API Bindings {
 	bool activate (MouseButton, Operation);
 
 	bool is_bound (KeyboardKey const&, Operation) const;
+    const std::string &get_action_name_by_key(KeyboardKey const&, Operation op) const; 
 	bool is_registered (Operation op, std::string const& action_name) const;
 
 	KeyboardKey get_binding_for_action (Glib::RefPtr<Gtk::Action>, Operation& op);
-- 
2.7.4 (Apple Git-66)

binding.patch (3,027 bytes)

AdmiralBumbleBee

2016-08-28 16:00

reporter   ~0018503

Small patch added, not sure how you folks handle changing localized strings, so it was just hacked in there.

paul

2016-08-28 16:25

administrator   ~0018504

_("text to be localized")

string_compose (_("text to be localized with %1 placeholders %2"), first, second)

there are plural and context-sensitive variants in libs/pbd/pbd/i18n.h

AdmiralBumbleBee

2016-08-28 17:47

reporter  

better_binding.patch (3,122 bytes)
From 35238cd4f14f00509d1dfaf6d01566ec04eb9768 Mon Sep 17 00:00:00 2001
From: Robert Randolph <audiolabs@gmail.com>
Date: Sun, 28 Aug 2016 13:42:24 -0400
Subject: [PATCH] Bindings dialog change

---
 gtk2_ardour/keyeditor.cc            | 13 ++++++++-----
 libs/gtkmm2ext/bindings.cc          |  7 +++++++
 libs/gtkmm2ext/gtkmm2ext/bindings.h |  1 +
 3 files changed, 16 insertions(+), 5 deletions(-)

diff --git a/gtk2_ardour/keyeditor.cc b/gtk2_ardour/keyeditor.cc
index aea8185..f971879 100644
--- a/gtk2_ardour/keyeditor.cc
+++ b/gtk2_ardour/keyeditor.cc
@@ -62,12 +62,15 @@ using Gtkmm2ext::Bindings;
 
 sigc::signal<void> KeyEditor::UpdateBindings;
 
-void bindings_collision_dialog (Gtk::Window& parent)
+void bindings_collision_dialog (Gtk::Window& parent, const string &missing_binding, Gtkmm2ext::KeyboardKey& key)
 {
 	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);
+    Label label_error (_("The key sequence is already bound. Please remove the other binding first."));
+    Label label_missing (string_compose("%1: %2\n%3: %4", _("Shortcut"), key.display_label(), _("Action"), missing_binding));
+    
+    label_missing.set_padding(0, 10);
+	dialog.get_vbox()->pack_start (label_error, true, true);
+    dialog.get_vbox()->pack_end (label_missing, true, true);
 	dialog.add_button (_("Ok"), Gtk::RESPONSE_ACCEPT);
 	dialog.show_all ();
 	dialog.run();
@@ -316,7 +319,7 @@ KeyEditor::Tab::bind (GdkEventKey* release_event, guint pressed_key)
 	Gtkmm2ext::KeyboardKey new_binding (mod, pressed_key);
 
 	if (bindings->is_bound (new_binding, Gtkmm2ext::Bindings::Press)) {
-		bindings_collision_dialog (owner);
+		bindings_collision_dialog (owner, bindings->get_action_name_by_key(new_binding, Gtkmm2ext::Bindings::Press), new_binding);
 		return;
 	}
 
diff --git a/libs/gtkmm2ext/bindings.cc b/libs/gtkmm2ext/bindings.cc
index e25ed20..de45495 100644
--- a/libs/gtkmm2ext/bindings.cc
+++ b/libs/gtkmm2ext/bindings.cc
@@ -1066,6 +1066,13 @@ Bindings::is_bound (KeyboardKey const& kb, Operation op) const
 	return km.find(kb) != km.end();
 }
 
+string const&
+Bindings::get_action_name_by_key (KeyboardKey const& kb, Operation op) const
+{
+	const KeybindingMap& km = get_keymap(op);
+	return km.find(kb)->second.action_name;
+}
+
 bool
 Bindings::is_registered (Operation op, std::string const& action_name) const
 {
diff --git a/libs/gtkmm2ext/gtkmm2ext/bindings.h b/libs/gtkmm2ext/gtkmm2ext/bindings.h
index d216e04..05d44c3 100644
--- a/libs/gtkmm2ext/gtkmm2ext/bindings.h
+++ b/libs/gtkmm2ext/gtkmm2ext/bindings.h
@@ -178,6 +178,7 @@ class LIBGTKMM2EXT_API Bindings {
 	bool activate (MouseButton, Operation);
 
 	bool is_bound (KeyboardKey const&, Operation) const;
+    const std::string &get_action_name_by_key(KeyboardKey const&, Operation op) const; 
 	bool is_registered (Operation op, std::string const& action_name) const;
 
 	KeyboardKey get_binding_for_action (Glib::RefPtr<Gtk::Action>, Operation& op);
-- 
2.7.4 (Apple Git-66)

better_binding.patch (3,122 bytes)

AdmiralBumbleBee

2016-08-28 17:47

reporter   ~0018505

Less idiotic patch for bindings added. Behaves properly with localizations.

paul

2016-08-29 01:21

administrator   ~0018507

sorry to say, but indentation looks wonky.

also, get_action_by_key() will crash the application if called for an unbound key. either note that clearly in the code, or have it return some other value if the key is not found.

otherwise, great stuff.

AdmiralBumbleBee

2016-08-29 14:28

reporter  

bindings.patch (6,007 bytes)
From 35238cd4f14f00509d1dfaf6d01566ec04eb9768 Mon Sep 17 00:00:00 2001
From: ArdourEnv <ardourenv@Roberts-iMac.home>
Date: Sun, 28 Aug 2016 13:42:24 -0400
Subject: [PATCH 1/2] Bindings dialog change

---
 gtk2_ardour/keyeditor.cc            | 13 ++++++++-----
 libs/gtkmm2ext/bindings.cc          |  7 +++++++
 libs/gtkmm2ext/gtkmm2ext/bindings.h |  1 +
 3 files changed, 16 insertions(+), 5 deletions(-)

diff --git a/gtk2_ardour/keyeditor.cc b/gtk2_ardour/keyeditor.cc
index aea8185..f971879 100644
--- a/gtk2_ardour/keyeditor.cc
+++ b/gtk2_ardour/keyeditor.cc
@@ -62,12 +62,15 @@ using Gtkmm2ext::Bindings;
 
 sigc::signal<void> KeyEditor::UpdateBindings;
 
-void bindings_collision_dialog (Gtk::Window& parent)
+void bindings_collision_dialog (Gtk::Window& parent, const string &missing_binding, Gtkmm2ext::KeyboardKey& key)
 {
 	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);
+    Label label_error (_("The key sequence is already bound. Please remove the other binding first."));
+    Label label_missing (string_compose("%1: %2\n%3: %4", _("Shortcut"), key.display_label(), _("Action"), missing_binding));
+    
+    label_missing.set_padding(0, 10);
+	dialog.get_vbox()->pack_start (label_error, true, true);
+    dialog.get_vbox()->pack_end (label_missing, true, true);
 	dialog.add_button (_("Ok"), Gtk::RESPONSE_ACCEPT);
 	dialog.show_all ();
 	dialog.run();
@@ -316,7 +319,7 @@ KeyEditor::Tab::bind (GdkEventKey* release_event, guint pressed_key)
 	Gtkmm2ext::KeyboardKey new_binding (mod, pressed_key);
 
 	if (bindings->is_bound (new_binding, Gtkmm2ext::Bindings::Press)) {
-		bindings_collision_dialog (owner);
+		bindings_collision_dialog (owner, bindings->get_action_name_by_key(new_binding, Gtkmm2ext::Bindings::Press), new_binding);
 		return;
 	}
 
diff --git a/libs/gtkmm2ext/bindings.cc b/libs/gtkmm2ext/bindings.cc
index e25ed20..de45495 100644
--- a/libs/gtkmm2ext/bindings.cc
+++ b/libs/gtkmm2ext/bindings.cc
@@ -1066,6 +1066,13 @@ Bindings::is_bound (KeyboardKey const& kb, Operation op) const
 	return km.find(kb) != km.end();
 }
 
+string const&
+Bindings::get_action_name_by_key (KeyboardKey const& kb, Operation op) const
+{
+	const KeybindingMap& km = get_keymap(op);
+	return km.find(kb)->second.action_name;
+}
+
 bool
 Bindings::is_registered (Operation op, std::string const& action_name) const
 {
diff --git a/libs/gtkmm2ext/gtkmm2ext/bindings.h b/libs/gtkmm2ext/gtkmm2ext/bindings.h
index d216e04..05d44c3 100644
--- a/libs/gtkmm2ext/gtkmm2ext/bindings.h
+++ b/libs/gtkmm2ext/gtkmm2ext/bindings.h
@@ -178,6 +178,7 @@ class LIBGTKMM2EXT_API Bindings {
 	bool activate (MouseButton, Operation);
 
 	bool is_bound (KeyboardKey const&, Operation) const;
+    const std::string &get_action_name_by_key(KeyboardKey const&, Operation op) const; 
 	bool is_registered (Operation op, std::string const& action_name) const;
 
 	KeyboardKey get_binding_for_action (Glib::RefPtr<Gtk::Action>, Operation& op);
-- 
2.7.4 (Apple Git-66)


From 944d62e80058cb938731d1a7be9ae9f61d58eb63 Mon Sep 17 00:00:00 2001
From: Robert Randolph <audiolabs@gmail.com>
Date: Mon, 29 Aug 2016 10:19:04 -0400
Subject: [PATCH 2/2] Resolved potential crash in get_action_by_name() and
 fixed indentation.

---
 gtk2_ardour/keyeditor.cc            | 10 +++++-----
 libs/gtkmm2ext/bindings.cc          |  6 +++++-
 libs/gtkmm2ext/gtkmm2ext/bindings.h |  2 +-
 3 files changed, 11 insertions(+), 7 deletions(-)

diff --git a/gtk2_ardour/keyeditor.cc b/gtk2_ardour/keyeditor.cc
index f971879..302606b 100644
--- a/gtk2_ardour/keyeditor.cc
+++ b/gtk2_ardour/keyeditor.cc
@@ -65,12 +65,12 @@ sigc::signal<void> KeyEditor::UpdateBindings;
 void bindings_collision_dialog (Gtk::Window& parent, const string &missing_binding, Gtkmm2ext::KeyboardKey& key)
 {
 	ArdourDialog dialog (parent, _("Colliding keybindings"), true);
-    Label label_error (_("The key sequence is already bound. Please remove the other binding first."));
-    Label label_missing (string_compose("%1: %2\n%3: %4", _("Shortcut"), key.display_label(), _("Action"), missing_binding));
-    
-    label_missing.set_padding(0, 10);
+	Label label_error (_("The key sequence is already bound. Please remove the other binding first."));
+	Label label_missing (string_compose("%1: %2\n%3: %4", _("Shortcut"), key.display_label(), _("Action"), missing_binding));
+
+	label_missing.set_padding(0, 10);
 	dialog.get_vbox()->pack_start (label_error, true, true);
-    dialog.get_vbox()->pack_end (label_missing, true, true);
+	dialog.get_vbox()->pack_end (label_missing, true, true);
 	dialog.add_button (_("Ok"), Gtk::RESPONSE_ACCEPT);
 	dialog.show_all ();
 	dialog.run();
diff --git a/libs/gtkmm2ext/bindings.cc b/libs/gtkmm2ext/bindings.cc
index de45495..52834cd 100644
--- a/libs/gtkmm2ext/bindings.cc
+++ b/libs/gtkmm2ext/bindings.cc
@@ -1070,7 +1070,11 @@ string const&
 Bindings::get_action_name_by_key (KeyboardKey const& kb, Operation op) const
 {
 	const KeybindingMap& km = get_keymap(op);
-	return km.find(kb)->second.action_name;
+	if (is_bound(kb, op)) {
+		return km.find(kb)->second.action_name;
+	} else {
+		return _("no action bound");
+	}
 }
 
 bool
diff --git a/libs/gtkmm2ext/gtkmm2ext/bindings.h b/libs/gtkmm2ext/gtkmm2ext/bindings.h
index 05d44c3..9b5a923 100644
--- a/libs/gtkmm2ext/gtkmm2ext/bindings.h
+++ b/libs/gtkmm2ext/gtkmm2ext/bindings.h
@@ -178,7 +178,7 @@ class LIBGTKMM2EXT_API Bindings {
 	bool activate (MouseButton, Operation);
 
 	bool is_bound (KeyboardKey const&, Operation) const;
-    const std::string &get_action_name_by_key(KeyboardKey const&, Operation op) const; 
+	const std::string &get_action_name_by_key(KeyboardKey const&, Operation op) const; 
 	bool is_registered (Operation op, std::string const& action_name) const;
 
 	KeyboardKey get_binding_for_action (Glib::RefPtr<Gtk::Action>, Operation& op);
-- 
2.7.4 (Apple Git-66)

bindings.patch (6,007 bytes)

AdmiralBumbleBee

2016-08-29 14:28

reporter   ~0018511

crash and indentation fixed hopefully.

D'oh.

Issue History

Date Modified Username Field Change
2016-08-27 17:20 AdmiralBumbleBee New Issue
2016-08-27 17:20 AdmiralBumbleBee File Added: Screen Shot 2016-08-27 at 13.16.50.png
2016-08-28 16:00 AdmiralBumbleBee File Added: binding.patch
2016-08-28 16:00 AdmiralBumbleBee Note Added: 0018503
2016-08-28 16:25 paul Note Added: 0018504
2016-08-28 17:47 AdmiralBumbleBee File Added: better_binding.patch
2016-08-28 17:47 AdmiralBumbleBee Note Added: 0018505
2016-08-29 01:21 paul Note Added: 0018507
2016-08-29 14:28 AdmiralBumbleBee File Added: bindings.patch
2016-08-29 14:28 AdmiralBumbleBee Note Added: 0018511