Set focus on the exit confirmation dialog buttons

[Jacobo Aragunde Pérez]

Application: Eeschema, CvPcb, Pcbnew
Version: (2012-12-20 BZR 3860)-testing
Build: wxWidgets 2.8.12 (no debug,Unicode,compiler with C++ ABI 1002,GCC 4.7.2,wx containers,compatible with 2.4,compatible with 2.6)
Platform: Linux 3.6.10-2.fc17.x86_64 x86_64, 64 bit, Little endian, wxGTK
Distro: Fedora 17
Boost version: 1.49.0
Options: USE_PCBNEW_NANOMETRES=ON
         KICAD_GOST=OFF
         USE_WX_GRAPHICS_CONTEXT=OFF
         USE_WX_OVERLAY=OFF
         USE_BOOST_POLYGON_LIBRARY=OFF
         KICAD_SCRIPTING=OFF
         KICAD_SCRIPTING_MODULES=OFF
         KICAD_SCRIPTING_WXPYTHON=OFF

When you close one KiCad application and there are some unsaved changes a confirmation dialog appears ("Save the changes before closing?"). It would be a good idea setting the focus on one of the buttons inside the dialog, so the user can confirm without using the mouse.

This behaviour is consistent with other application dialogs; for example, when you try to open another document and there are some unsaved changes, another confirmation dialog appears ("Discard changes?") and the focus is set on the "yes" button.

[Jacobo Aragunde Pérez (jaragunde)]

If you don't mind, I'll try to do it myself ;)

[Jacobo Aragunde Pérez (jaragunde)]

Patch to set the "save and exit" button as default and focus the dialog to receive keyboard input.

[jean-pierre charras (jp-charras)]

Be prudent with dialog files: dialog_exit_base.cpp is automatically generated by wxFormBuilder from dialog_exit_base.fbp
(in fact files named dialog_xxx_base.cpp or named dialog_xxx_fbp.cpp and .h are always generated from dialog_xxx_base.fbp).
Therefore dialog_exit_base.fbp should be modified using wxFormBuilder, and the corresponfing files .cpp and .h should be recreated by wxFormBuilder.
And the patch should contain the *.fbp file.
Otherwise changes will be lost the next time the main file is modified by wxFormBuilder.
Also notice all dialogs created via wxFormBuilder *should* be derived from DIALOG_SHIM with dialog_shim.h included.
(seen some other dialogs for samples)
if dialog_exit_base.fbp does not derive dialog_exit_base from DIALOG_SHIM, please add it.

As a consequence adding this->SetFocus(); is source is useless, this is made by DIALOG_SHIM (which also allows remenber the size and position of the dialog during the session.)

[Jacobo Aragunde Pérez (jaragunde)]

Thanks for your comments, I'll work on another patch.

[Jacobo Aragunde Pérez (jaragunde)]

I have updated the .fbp to derive from DIALOG_SHIM and select the "save & exit" button, and re-generated the .cpp and .h files. Unfortunately, the focus doesn't work as expected. It doesn't work either in other dialogs derived from DIALOG_SHIM, for example, in "Page settings" (e.g. you can't press ESC to close it). It looks like the problem is elsewhere, at least in my system.

BTW I can attach the patches, but I noticed that the existing files had windows-style line breaks, so my patches rewrote the files completely. Is that a problem for you?

[Wayne Stambaugh (stambaughw)]

On 1/3/2013 6:44 AM, Jacobo Aragunde Pérez wrote:
> I have updated the .fbp to derive from DIALOG_SHIM and select the "save
> & exit" button, and re-generated the .cpp and .h files. Unfortunately,
> the focus doesn't work as expected. It doesn't work either in other
> dialogs derived from DIALOG_SHIM, for example, in "Page settings" (e.g.
> you can't press ESC to close it). It looks like the problem is
> elsewhere, at least in my system.
>
> BTW I can attach the patches, but I noticed that the existing files had
> windows-style line breaks, so my patches rewrote the files completely.
> Is that a problem for you?
>
I finally figured out (remembered) what is going on here. You cannot
simply set the focus to the dialog window to get the the escape key to
work with wxGTK. You must set the focus to one of the child controls in
the dialog. This problem goes all the way back to when I first joined
project. I should have remembered this. Your assessment is correct
that dialogs derived from DIALOG_SHIM do not honor the escape key
correctly unless the focus is set to a child control elsewhere. We
could modify the DIALOG_SHIM constructor to set the focus to the first
child control using wxWindow::GetChildren() but this may yield some
unexpected behavior if the user expects a control other than the first
control in the list to have the focus. The other option is to update
the constructors of every dialog box derived from DIALOG_SHIM (even
dialogs that are not derived from DIALOG_SHIM can suffer from this
problem) to set the focus to the appropriate control. My preference is
the latter. I have updated DIALOG_EXIT_BASE and DIALOG_EXIT to confirm
this behavior. I can commit the changes if you would like.

Are the files with the windows style line breaks generated by
wxFormBuilder? If so, I wouldn't worry too much about it. I haven't
found way to force wxFormBuilder to use a given line break style. If
the files are normal source files that you modified with your editor,
then you need to configure your editor to honor the file's current line
break style.

[Dick Hollenbeck (dickelbeck)]

On 01/09/2013 09:20 AM, Wayne Stambaugh wrote:
> On 1/3/2013 6:44 AM, Jacobo Aragunde Pérez wrote:
>> I have updated the .fbp to derive from DIALOG_SHIM and select the "save
>> & exit" button, and re-generated the .cpp and .h files. Unfortunately,
>> the focus doesn't work as expected. It doesn't work either in other
>> dialogs derived from DIALOG_SHIM, for example, in "Page settings" (e.g.
>> you can't press ESC to close it). It looks like the problem is
>> elsewhere, at least in my system.
>>
>> BTW I can attach the patches, but I noticed that the existing files had
>> windows-style line breaks, so my patches rewrote the files completely.
>> Is that a problem for you?
>>
> I finally figured out (remembered) what is going on here. You cannot
> simply set the focus to the dialog window to get the the escape key to
> work with wxGTK. You must set the focus to one of the child controls in
> the dialog. This problem goes all the way back to when I first joined
> project. I should have remembered this. Your assessment is correct
> that dialogs derived from DIALOG_SHIM do not honor the escape key
> correctly unless the focus is set to a child control elsewhere. We
> could modify the DIALOG_SHIM constructor to set the focus to the first
> child control using wxWindow::GetChildren() but this may yield some
> unexpected behavior if the user expects a control other than the first
> control in the list to have the focus. The other option is to update
> the constructors of every dialog box derived from DIALOG_SHIM (even
> dialogs that are not derived from DIALOG_SHIM can suffer from this
> problem) to set the focus to the appropriate control. My preference is
> the latter. I have updated DIALOG_EXIT_BASE and DIALOG_EXIT to confirm
> this behavior. I can commit the changes if you would like.

I need time to digest this. The idea of DIALOG_SHIM is it is to do the work for us.

I am going to challenge some of your assertions here, but don't have time now.

I think it has to do with whether you are using standard window IDs and what type of
buttons are in play.

I will resist any solution which does not comprehend the problem.

Later.

>
> Are the files with the windows style line breaks generated by
> wxFormBuilder? If so, I wouldn't worry too much about it. I haven't
> found way to force wxFormBuilder to use a given line break style. If
> the files are normal source files that you modified with your editor,
> then you need to configure your editor to honor the file's current line
> break style.
>

[Jacobo Aragunde Pérez (jaragunde)]

El 09/01/13 16:20, Wayne Stambaugh escribió:
> ...
> The other option is to update
> the constructors of every dialog box derived from DIALOG_SHIM (even
> dialogs that are not derived from DIALOG_SHIM can suffer from this
> problem) to set the focus to the appropriate control. My preference is
> the latter. I have updated DIALOG_EXIT_BASE and DIALOG_EXIT to confirm
> this behavior. I can commit the changes if you would like.
>

You mean editing the auto-generated code for the dialog class constructors?

I agree with Dick that we should find a more general solution. I think
we can:
* Set a default component in every form using wxFormBuilder.
* Modify DIALOG_SHIM constructor to select the default component.

This steps could do the trick without having to edit every constructor.
I still have to check if it works, I'll tell you soon :) .

> Are the files with the windows style line breaks generated by
> wxFormBuilder? If so, I wouldn't worry too much about it. I haven't
> found way to force wxFormBuilder to use a given line break style. If
> the files are normal source files that you modified with your editor,
> then you need to configure your editor to honor the file's current line
> break style.
>

Yes, they are. I'm attaching the patches here. What they do:
* DIALOG_EXIT_BASE extends DIALOG_SHIM
* "Save and Exit" button is set as default.

These two changes are useful in any case. I'll try to provide a patch
for DIALOG_SHIM to fix the problem completely.

--
Jacobo

[jean-pierre charras (jp-charras)]

Le 10/01/2013 19:44, Jacobo Aragunde Pérez a écrit :
> El 09/01/13 16:20, Wayne Stambaugh escribió:
>> ...
>> The other option is to update
>> the constructors of every dialog box derived from DIALOG_SHIM (even
>> dialogs that are not derived from DIALOG_SHIM can suffer from this
>> problem) to set the focus to the appropriate control. My preference is
>> the latter. I have updated DIALOG_EXIT_BASE and DIALOG_EXIT to confirm
>> this behavior. I can commit the changes if you would like.
>>
> You mean editing the auto-generated code for the dialog class
> constructors?
>
> I agree with Dick that we should find a more general solution. I think
> we can:
> * Set a default component in every form using wxFormBuilder.
> * Modify DIALOG_SHIM constructor to select the default component.
>
> This steps could do the trick without having to edit every constructor.
> I still have to check if it works, I'll tell you soon :) .
>
>
>> Are the files with the windows style line breaks generated by
>> wxFormBuilder? If so, I wouldn't worry too much about it. I haven't
>> found way to force wxFormBuilder to use a given line break style. If
>> the files are normal source files that you modified with your editor,
>> then you need to configure your editor to honor the file's current line
>> break style.
>>
> Yes, they are. I'm attaching the patches here. What they do:
> * DIALOG_EXIT_BASE extends DIALOG_SHIM
> * "Save and Exit" button is set as default.
>
> These two changes are useful in any case. I'll try to provide a patch
> for DIALOG_SHIM to fix the problem completely.
>
Because I did not see this issue, I tested this this dialog under Ubuntu
10.10 / Gnome.
This is an issue only under wxWidgets 2.8. (I tested the 2.8.12 version)
Version 2.9 (which is the version I am using) does not have this issue:
  the ESC key demises the dialog without using SetFocus().

--
Jean-Pierre CHARRAS

[Wayne Stambaugh (stambaughw)]

On 1/10/2013 1:44 PM, Jacobo Aragunde Pérez wrote:
> El 09/01/13 16:20, Wayne Stambaugh escribió:
>> ...
>> The other option is to update
>> the constructors of every dialog box derived from DIALOG_SHIM (even
>> dialogs that are not derived from DIALOG_SHIM can suffer from this
>> problem) to set the focus to the appropriate control. My preference is
>> the latter. I have updated DIALOG_EXIT_BASE and DIALOG_EXIT to confirm
>> this behavior. I can commit the changes if you would like.
>>
>
> You mean editing the auto-generated code for the dialog class
> constructors?

No. I mean the constructor of the class derived from the auto-generated
code. As far as I know, we never use the wxFormBuilder auto-generated
dialog classes directly. For example, in the file common/confirm.cpp
there is an DIALOG_EXIT class that is derived from DIALOG_EXIT_BASE.

>
> I agree with Dick that we should find a more general solution. I think
> we can:
> * Set a default component in every form using wxFormBuilder.
> * Modify DIALOG_SHIM constructor to select the default component.

This is one possible solution. There is a caveat with this solution in
that the first child control returned by wxWindow::GetChildren() may
*not* be the one that you want to have the focus. It's not possible to
use wxFormBuilder to set which control has the focus as far as I can
tell. Therefore, the fall back to guarantee that the control that you
want to have the focus would have to be set in the derived dialog class
constructor.

Dick's proposal suggested that on wxGTK using standard dialog button IDs
may also solve the problem. I have never run across anything in the
wxWidgets documentation or any bug reports that mentioned stock dialog
button IDs as a solution to the wxGTK escape key issue. If Dick is
correct, then this would be a much cleaner solution than the one I proposed.

>
> This steps could do the trick without having to edit every constructor.
> I still have to check if it works, I'll tell you soon :) .
>
>
>> Are the files with the windows style line breaks generated by
>> wxFormBuilder? If so, I wouldn't worry too much about it. I haven't
>> found way to force wxFormBuilder to use a given line break style. If
>> the files are normal source files that you modified with your editor,
>> then you need to configure your editor to honor the file's current line
>> break style.
>>
>
> Yes, they are. I'm attaching the patches here. What they do:
> * DIALOG_EXIT_BASE extends DIALOG_SHIM
> * "Save and Exit" button is set as default.
>
> These two changes are useful in any case. I'll try to provide a patch
> for DIALOG_SHIM to fix the problem completely.
>

[Jacobo Aragunde Pérez (jaragunde)]

As JP said, this is only an issue with wxWidgets 2.8. I have been able to try it in Ubuntu with wx 2.9 and I couldn't reproduce the problem.

I wonder if it's worth fixing what seems to be a bug in the framework and that has already been fixed.

[Wayne Stambaugh (stambaughw)]

On 1/15/2013 4:30 AM, Jacobo Aragunde Pérez wrote:
> As JP said, this is only an issue with wxWidgets 2.8. I have been able
> to try it in Ubuntu with wx 2.9 and I couldn't reproduce the problem.
>
> I wonder if it's worth fixing what seems to be a bug in the framework
> and that has already been fixed.
>
I still think it is worth supporting 2.8. AFAIK a wxWidgets 3 release
is no where in sight. Until wxWidgets 3 is packaged for all the major
distros, we should continue to support 2.8. I don't think asking users
to install wxWidgets from PPAs or other external package repos is a good
idea. For developers this is fine.

[Dick Hollenbeck (dickelbeck)]

On 01/15/2013 03:30 AM, Jacobo Aragunde Pérez wrote:
> As JP said, this is only an issue with wxWidgets 2.8. I have been able
> to try it in Ubuntu with wx 2.9 and I couldn't reproduce the problem.
>
> I wonder if it's worth fixing what seems to be a bug in the framework
> and that has already been fixed.
>

From our project's file:

Documentation/guidelines/UIpolicies.txt

We read:

  Follow the recommendations here:
    http://library.gnome.org/devel/hig-book/2.20/windows-dialog.html.en
    paying particular attention to "initial focus", "sensible default values",
    "default buttons", ESC key termination. Please note that the escape key
    termination only works properly if there is a dialog button defined with
    an ID of wxID_CANCEL or SetEscapeID( MY_ESCAPE_BUTTON_ID ) is called during
    dialog initialization. The former is the preferred method for handling
    escape key dialog termination.

Sorry my memory is so bad. That's what folks with weak should memories do, write stuff down.

So indeed, the control ID is significant, as I merely guessedearlier. So perhaps some
dialogs can simply re-assigned a proper wxID_CANCEL or use made of the std button panels
which use this idon cancel button.

A third option is to use SetEscapeID() functionin dialog's constructor, per above.

[jean-pierre charras (jp-charras)]

Le 17/01/2013 07:58, Dick Hollenbeck a écrit :
> On 01/15/2013 03:30 AM, Jacobo Aragunde Pérez wrote:
>> As JP said, this is only an issue with wxWidgets 2.8. I have been able
>> to try it in Ubuntu with wx 2.9 and I couldn't reproduce the problem.
>>
>> I wonder if it's worth fixing what seems to be a bug in the framework
>> and that has already been fixed.
>>
> >From our project's file:
>
> Documentation/guidelines/UIpolicies.txt
>
> We read:
>
> Follow the recommendations here:
> http://library.gnome.org/devel/hig-book/2.20/windows-dialog.html.en
> paying particular attention to "initial focus", "sensible default values",
> "default buttons", ESC key termination. Please note that the escape key
> termination only works properly if there is a dialog button defined with
> an ID of wxID_CANCEL or SetEscapeID( MY_ESCAPE_BUTTON_ID ) is called during
> dialog initialization. The former is the preferred method for handling
> escape key dialog termination.
>
>
> Sorry my memory is so bad. That's what folks with weak should memories
> do, write stuff down.
>
> So indeed, the control ID is significant, as I merely guessedearlier. So perhaps some
> dialogs can simply re-assigned a proper wxID_CANCEL or use made of the std button panels
> which use this idon cancel button.
>
> A third option is to use SetEscapeID() functionin dialog's constructor,
> per above.
>

In fact, in Exit dialog, the Cancel button with wxID_CANCEL exists.
After investigation:
SetFocus() called from DIALOG_SHIM constructor does not work because it
is called before the button exists.
When SetFocus() is called after the creation of this button, it works.

Therefore the requirements are:
1 - An item with wxID_CANCEL (or, I am guessing, MY_ESCAPE_BUTTON_ID)
should exists.
2 - After creation of this control, SetFocus() should be called.

--
Jean-Pierre CHARRAS

[Jacobo Aragunde Pérez (jaragunde)]

El 17/01/13 09:06, jean-pierre charras escribió:
> ...
> In fact, in Exit dialog, the Cancel button with wxID_CANCEL exists.
> After investigation:
> SetFocus() called from DIALOG_SHIM constructor does not work because it
> is called before the button exists.
> When SetFocus() is called after the creation of this button, it works.
>

Ahh, I was wondering what was happening here. When I manually modified
DIALOG_EXIT_BASE class (in the first patch) it worked correctly. I was
calling SetFocus() after having created the button, while DIALOG_SHIM
does it before.

I'll try to get a fix with the information you provided soon, thanks :)

[Wayne Stambaugh (stambaughw)]

On 1/17/2013 1:58 AM, Dick Hollenbeck wrote:
> On 01/15/2013 03:30 AM, Jacobo Aragunde Pérez wrote:
>> As JP said, this is only an issue with wxWidgets 2.8. I have been able
>> to try it in Ubuntu with wx 2.9 and I couldn't reproduce the problem.
>>
>> I wonder if it's worth fixing what seems to be a bug in the framework
>> and that has already been fixed.
>>
>
>>From our project's file:
>
> Documentation/guidelines/UIpolicies.txt
>
> We read:
>
> Follow the recommendations here:
> http://library.gnome.org/devel/hig-book/2.20/windows-dialog.html.en
> paying particular attention to "initial focus", "sensible default values",
> "default buttons", ESC key termination. Please note that the escape key
> termination only works properly if there is a dialog button defined with
> an ID of wxID_CANCEL or SetEscapeID( MY_ESCAPE_BUTTON_ID ) is called during
> dialog initialization. The former is the preferred method for handling
> escape key dialog termination.
>
>
> Sorry my memory is so bad. That's what folks with weak should memories
> do, write stuff down.

In theory writing it down works great as long as you remember where you
wrote it down :). I feel bit silly forgetting about this since this was
one of the issues that we wrestled with early on in the project. I'm
pretty sure it was either you or I who added this note UIpolicies.txt.

>
> So indeed, the control ID is significant, as I merely guessedearlier. So perhaps some
> dialogs can simply re-assigned a proper wxID_CANCEL or use made of the std button panels
> which use this idon cancel button.
>
> A third option is to use SetEscapeID() functionin dialog's constructor,
> per above.
>

[Dick Hollenbeck (dickelbeck)]

> In fact, in Exit dialog, the Cancel button with wxID_CANCEL exists.
> After investigation:
> SetFocus() called from DIALOG_SHIM constructor does not work because it
> is called before the button exists.
> When SetFocus() is called after the creation of this button, it works.
>
> Therefore the requirements are:
> 1 - An item with wxID_CANCEL (or, I am guessing, MY_ESCAPE_BUTTON_ID)
> should exists.
> 2 - After creation of this control, SetFocus() should be called.
>

Good observation JP. We have gradually gathered probably enough data on this one.

From a policy/strategy control structure viewpoint, it is always superior to be able to
control policy from a single point, rather than sprinkling policy issues around
everywhere. Of course this is because it gives you an easier ability to change policy or
strategy in the future, from one location.

So just setting SetFocus() in every dialog constructor is not attractive to me for this
reason.

The purpose of DIALOG_SHIM was to control policy and offer assistance from a single point.

I have some time today to look into this, and see what can be done in DIALOG_SHIM to
address it.

[Dick Hollenbeck (dickelbeck)]

> In theory writing it down works great as long as you remember where you
> wrote it down :). I feel bit silly forgetting about this since this was
> one of the issues that we wrestled with early on in the project. I'm
> pretty sure it was either you or I who added this note UIpolicies.txt.

Exactly.

Wayne, Jean Pierre, we are getting old. As KiCad gets better.

[Dick Hollenbeck (dickelbeck)]

Revision 3908 was a commit that is supposed to fixed this, based on data collected from above remarks.
It certainly works for wx 2.8 on wxGTK, but other platforms may need to be included in the test for

DLGSHIM_USE_SETFOCUS

in include/dialog_shim.h

at or around line 31 of said file.

[Dick Hollenbeck (dickelbeck)]

3909 has a better chance, I goofed the preprocessor test which decided when to employ the fix.