From 20a9c7a38184c10b95ed0fca7dfc6728425ea418 Mon Sep 17 00:00:00 2001
From: Guillaume Munch <gm@lyx.org>
Date: Mon, 22 Feb 2016 00:36:33 +0000
Subject: [PATCH] Fix SIGSEGV when introducing a new shortcut (#9869)

removeShortcut() restores default settings, therefore was used incorrectly. I
introduce deactivateShortcuts() which only removes assignments.

Clean up a bit the lack of view / model distinction (getting rid of the crashing
code at the same time).

Repair inconsistency of the selection in the "modify" case. (regression at
717d19d3c)

Make the test for existing bindings a bit more robust. (Not perfect yet.)

Focus on the item that has just been added/modified. (cosmetic)
---
 src/frontends/qt4/GuiPrefs.cpp | 64 +++++++++++++++++++++++++---------
 src/frontends/qt4/GuiPrefs.h   |  3 ++
 2 files changed, 50 insertions(+), 17 deletions(-)

diff --git a/src/frontends/qt4/GuiPrefs.cpp b/src/frontends/qt4/GuiPrefs.cpp
index c9ca024cc1..1e255013e4 100644
--- a/src/frontends/qt4/GuiPrefs.cpp
+++ b/src/frontends/qt4/GuiPrefs.cpp
@@ -3001,6 +3001,40 @@ void PrefShortcuts::removeShortcut()
 }
 
 
+void PrefShortcuts::deactivateShortcuts(QList<QTreeWidgetItem*> const & items)
+{
+	for (int i = 0; i < items.size(); ++i) {
+		if (!items[i])
+			continue;
+		string shortcut = fromqstr(items[i]->data(1, Qt::UserRole).toString());
+		string lfun = fromqstr(items[i]->text(0));
+		FuncRequest func = lyxaction.lookupFunc(lfun);
+		KeyMap::ItemType tag =
+			static_cast<KeyMap::ItemType>(items[i]->data(0, Qt::UserRole).toInt());
+
+		switch (tag) {
+		case KeyMap::System:
+			// for system bind, we do not touch the item
+			// but add an user unbind item
+			user_unbind_.bind(shortcut, func);
+			setItemType(items[i], KeyMap::UserUnbind);
+			break;
+
+		case KeyMap::UserBind: {
+			// for user_bind, we remove this bind
+			QTreeWidgetItem * parent = items[i]->parent();
+			int itemIdx = parent->indexOfChild(items[i]);
+			parent->takeChild(itemIdx);
+			user_bind_.unbind(shortcut, func);
+			break;
+		}
+		default:
+			break;
+		}
+	}
+}
+
+
 void PrefShortcuts::selectBind()
 {
 	QString file = form_->browsebind(internalPath(bindFileED->text()));
@@ -3098,9 +3132,12 @@ void PrefShortcuts::shortcutOkPressed()
 		return;
 
 	// make sure this key isn't already bound---and, if so, prompt user
+	FuncCode const old_action = oldBinding.action();
 	FuncCode const unbind = user_unbind_.getBinding(k).action();
 	docstring const action_string = makeCmdString(oldBinding);
-	if (oldBinding.action() > LFUN_NOACTION && unbind == LFUN_UNKNOWN_ACTION
+	// FIXME: this logic is wrong. Some bindings in the treewidget can (still)
+	// escape from this test.
+	if (old_action > LFUN_NOACTION && unbind != old_action
 		  && save_lfun_ != toqstr(action_string)) {
 		docstring const new_action_string = makeCmdString(func);
 		docstring const text = bformat(_("Shortcut `%1$s' is already bound to "
@@ -3116,31 +3153,24 @@ void PrefShortcuts::shortcutOkPressed()
 		QString const sequence_text = toqstr(k.print(KeySequence::ForGui));
 		QList<QTreeWidgetItem*> items = shortcutsTW->findItems(sequence_text,
 			Qt::MatchFlags(Qt::MatchExactly | Qt::MatchRecursive), 1);
-		if (items.size() > 0) {
-			// should always happen
-			bool expanded = items[0]->parent()->isExpanded();
-			shortcutsTW->setCurrentItem(items[0]);
-			removeShortcut();
-			shortcutsTW->setCurrentItem(0);
-			// make sure user doesn't see tree expansion if
-			// old binding wasn't in an expanded tree
-			if (!expanded)
-				items[0]->parent()->setExpanded(false);
-		}
+		deactivateShortcuts(items);
+	}
+
+	if (!save_lfun_.isEmpty()) {
+		// real modification of the lfun's shortcut,
+		// so remove the previous one
+		QList<QTreeWidgetItem*> to_modify = shortcutsTW->selectedItems();
+		deactivateShortcuts(to_modify);
 	}
 
 	shortcut_->accept();
 
-	if (!save_lfun_.isEmpty())
-		// real modification of the lfun's shortcut,
-		// so remove the previous one
-		removeShortcut();
-
 	QTreeWidgetItem * item = insertShortcutItem(func, k, KeyMap::UserBind);
 	if (item) {
 		user_bind_.bind(&k, func);
 		shortcutsTW->sortItems(0, Qt::AscendingOrder);
 		shortcutsTW->setItemExpanded(item->parent(), true);
+		shortcutsTW->setCurrentItem(item);
 		shortcutsTW->scrollToItem(item);
 	} else {
 		Alert::error(_("Failed to create shortcut"),
diff --git a/src/frontends/qt4/GuiPrefs.h b/src/frontends/qt4/GuiPrefs.h
index 4094d0137e..644fb733e5 100644
--- a/src/frontends/qt4/GuiPrefs.h
+++ b/src/frontends/qt4/GuiPrefs.h
@@ -457,7 +457,10 @@ public:
 	void updateRC(LyXRC const & rc);
 	void updateShortcutsTW();
 	void modifyShortcut();
+	/// remove selected binding, restore default value
 	void removeShortcut();
+	/// remove bindings, do not restore default values
+	void deactivateShortcuts(QList<QTreeWidgetItem*> const & items);
 	///
 	void setItemType(QTreeWidgetItem * item, KeyMap::ItemType tag);
 	QTreeWidgetItem * insertShortcutItem(FuncRequest const & lfun,