On 03/08/2014 08:35 PM, Roy Xia wrote:
Hey everyone,
I'm a student interested in working with LyX for Google Summer of Code
2014, mostly looking towards overhauling the LyX converter scheme as
my project of choice. I decided that I'd go ahead and try and
familiarize myself with the source code by attempting to fix bug
ticket #8939 (http://www.lyx.org/trac/ticket/8939). The diff file for
this fix should be attached to this e-mail.
Technically the ticket's title only addresses the shortcut dialog
closing after the error alert, which is just a matter of moving the
call to accept() around, however I chose to go a bit further and
implement the comment's suggestion of a user dialog on whether to
overwrite the shortcut or not. Not sure if that was the consensus on
the appropriate behavior or not, but if not then most of the diff
patch can be ignored. Sorry in advance if there are any mistakes in
the patch.
Hi, Roy,
Thanks for the patch. I haven't tested it, but it generally looks pretty
good. If you could please send an email to the list saying something
along the lines of "I hereby license my contributions to LyX under the
General Public License, Version 2 or any later version", we can commit a
version of the patch.
A few minor comments below.
--- a/src/frontends/qt4/GuiPrefs.cpp
+++ b/src/frontends/qt4/GuiPrefs.cpp
@@ -3182,8 +3182,6 @@ void PrefShortcuts::shortcutOkPressed()
return;
}
- shortcut_->accept();
-
// check to see if there's been any change
FuncRequest oldBinding = system_bind_.getBinding(k);
if (oldBinding.action() == LFUN_UNKNOWN_ACTION)
@@ -3192,20 +3190,42 @@ void PrefShortcuts::shortcutOkPressed()
// nothing has changed
return;
- // make sure this key isn't already bound---and, if so, not unbound
+ // make sure this key isn't already bound---and, if so, prompt user
FuncCode const unbind = user_unbind_.getBinding(k).action();
docstring const action_string = makeCmdString(oldBinding);
if (oldBinding.action() > LFUN_NOACTION && unbind == LFUN_UNKNOWN_ACTION
&& save_lfun_ != toqstr(action_string)) {
- // FIXME Perhaps we should offer to over-write the old shortcut?
- // If so, we'll need to remove it from our list, etc.
- Alert::error(_("Failed to create shortcut"),
- bformat(_("Shortcut `%1$s' is already bound to:\n%2$s\n"
- "You need to remove that binding before creating a new
one."),
- k.print(KeySequence::ForGui), action_string));
- return;
+ docstring const new_action_string = makeCmdString(func);
+ docstring const text = bformat(_("Shortcut `%1$s' is already bound
to "
+ "%2$s.\n"
+ "Are you sure you want to unbind
the "
+ "current shortcut and bind it to
%3$s?"),
+ k.print(KeySequence::ForGui),
action_string,
+ new_action_string);
+ int ret = Alert::prompt(_("Overwrite shortcut?"),
+ text, 0, 1, _("&Yes"), _("&No"));
Most UI specs tell you to avoid "Yes" and "No" as button labels, since
people can easily misread the question asked. So something like
"Overwrite" and "Cancel" would be better.
+ if (ret == 0) {
+ 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);
+ }
+ }
+ else
+ return;
Just as a sylistic matter, it's better to invert the cases, thus:
if (ret != 0)
return;
QString const sequence_text ....
Now the main line of code is less indented, and the structure is clearer.
So, if you could restructure the patch slightly, and send the GPL email,
that'd be great.
Richard
