Re: [lmi] wxDialog::OnOK() removed from wx

[Greg Chicares]

On 2006-9-25 17:48 UTC, Vadim Zeitlin wrote:
> On Sun, 24 Sep 2006 23:51:15 +0000 Greg Chicares <address@hidden> wrote:
> 
> GC> > 1. in about_dialog.cpp, but there it seems to have nothing to do with
> GC> >    wxID_OK button handler
> GC> 
> GC> Here, the "Read the GNU General Public License" button is mapped
> GC> to wxID_OK, and the other button to wxID_CANCEL. Is there a
> GC> less confusing way to accomplish the same thing?
> 
>  I don't think it makes much sense to use wxID_OK button id for a button
> opening the licence dialog. Either a custom id or, maybe, wxID_ABOUT, would
> be more appropriate IMHO. Using wxID_CANCEL for closing the dialog is more
> understandable, especially as you had to do it with older wxWidgets
> versions to ensure that the button is closed automatically when "Esc" key
> is pressed

I guess that was my original reason for using wxID_CANCEL here.

> but now you can use any id you like and just call wxDialog::
> SetEscapeId() to indicate that the button with this id should be activated
> when "Esc" is pressed. Also, wxID_OK is now mapped to "Esc" automatically
> if there is no wxID_CANCEL.

Thanks--I didn't realize that.

>  To summarize, I'd replace wxID_OK/CANCEL with wxID_ABOUT/OK or just used
> custom LMI_ReadLicence and LMI_Close ids.

Okay, I changed that on 20061002T0055Z. I deleted the reference to
wxDialog::OnOK(), and replaced wxID_OK with wxID_ABOUT. I didn't
replace the wxID_CANCEL with wxID_OK, though, because that broke
compatibility with wx-2.5.4 .

Whether we should maintain backward compatibility with wx-2.5.4 is
a separate question. Probably that's unnecessary. But we shouldn't
cut corners by rushing into any such decision.

> GC> > 2. in mvc_controller.cpp and there I think the same solution applies:
> GC> >    it's better to move the code from UponOk() to 
> TransferDataFromWindow()
> GC> >    instead of conflating closing the dialog and validating data in it in
> GC> >    the same handler. I can make a patch doing this if you'd like.
> GC> 
> GC> Here, UponOk()'s purpose is to close the dialog and display some
> GC> information. But I really do think Validate() should be called
> GC> here, to prevent closing the dialog when validation fails, just
> GC> as UponPageChanging() vetoes a book-control page change in the
> GC> same circumstance.
> 
>  This is exactly why I thought this should be in TransferDataFromWindow().
[...]
> How does the fact that you use Validate() imply that
> you must replace the event handler for wxID_OK button?

I didn't realize this:

> If this method returns false, the dialog is not closed.

However, I'm still not sure how to remove wxDialog::OnOK() in the
'skeleton' trunks's 'mvc_controller.cpp', so I think I'd better
accept your offer:

> > >    I can make a patch doing this if you'd like.

The exact behavior desired in MvcController::UponOK() is:
 - if validation fails, then do nothing;
 - else,
   - first, validate, transfer data, and close the dialog;
   - then, display the data after the dialog has closed.
Sorry, I just can't see how to do that without either calling
wxDialog::OnOK() or writing its (former) body inline.

[...I had written about calling Skip() here:]
> GC>   virtual bool Validate();
[...]
>  Err. it can't call Skip() because there are no events involved here.

Oh, quite right, my mistake. What I meant to say is that I'll
document the reason for not calling wxWindow::Validate() in
this override.