On Sat, Mar 8, 2014 at 7:54 PM, Richard Heck <rgh...@lyx.org> wrote:
> 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
>
>
Alright, made the changes. Thanks for the tips. I'll be sending out the GPL
e-mail momentarily.
Thanks,
Roy Xia
diff --git a/src/frontends/qt4/GuiPrefs.cpp b/src/frontends/qt4/GuiPrefs.cpp
index e910b0c..7cd2152 100644
--- 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,40 @@ 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, _("&Overwrite"),
_("&Cancel"));
+ if (ret != 0)
+ return;
+ 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);
+ }
}
+ shortcut_->accept();
+
if (!save_lfun_.isEmpty())
// real modification of the lfun's shortcut,
// so remove the previous one
