Re: [lmi] [PATCH 1/2] Make InputSequenceEntry friendly to embedding in wxDVC.

[Vadim Zeitlin]

On Tue, 9 Aug 2011 11:16:03 +0200 Václav Slavík <address@hidden> wrote:

VS> On 8 Aug 2011, at 20:26, Greg Chicares wrote:
VS> > I'd really like to hear the answer, too--AFAICT, TransferDataFromWindow()
VS> > shouldn't be called by wx (or at least by wxmsw) unless an Apply button
VS> > is pressed or AcceptAndClose() is called.
VS> 
VS> The answer lies in LMI's MvcController::UponUpdateUI(), which is called
VS> at idle time and calls TransferDataFromWindow():

 Indeed, I can now see it as well (FWIW I indeed just wasn't testing the
correct code version so while I tried with both Enter, Space and other keys
I didn't see anything wrong because there was nothing wrong in the version
I had).

VS> Here's the relevant part of the code:
VS> 
VS>     // Exit immediately if nothing changed. The library calls this
VS>     // function continually in idle time, and it's pointless to fret
VS>     // over inputs that didn't change on this update because they've
VS>     // already been handled. Complex processing of many inputs has
VS>     // been observed to consume excessive CPU time when a malloc
VS>     // debugger is running, so this optimization is significant.
VS>     //
VS>     // The early-exit condition cannot succeed until Assimilate() has
VS>     // been called: therefore, Assimilate() is guaranteed to be called
VS>     // here by the time the user can interact with the GUI.
VS>     TransferDataFromWindow();

 I understand the comment and it's a perfectly valid concern but I am not
sure if it's really ideal to call TransferDataFromWindow() from here. I'd
rather call some other GetWindowData() function which would reuse mostly
the same code that our implementation of TransferDataFromWindow()/Validate()
uses but would return the new data object instead of implicitly changing
the member transfer_data_. I.e. instead of current

    TransferDataFromWindow();
    if(cached_transfer_data_ == transfer_data_)
        {
        return;
        }

code I'd rather have

    std::map<std::string,std::string> data_new = GetWindowData();
    if(data_new == transfer_data_)
        {
        return;
        }

and get rid of cached_transfer_data_ all together. IMHO this would be
somewhat cleaner -- and would get rid of this bug as a nice side effect.


VS> I already mentioned one quick fix. Another would be to modify
VS> wxWindow::TransferDataFromWindow() so that it does not recursively
VS> descend into child toplevel windows, as it does now. That's the cause
VS> of this bug and I think it doesn't make much sense: I expect
VS> wxDialog::TransferDataFromWindow() to get everything from the dialog,
VS> but not from other toplevel windows that this one is transient for.

 I agree that neither of TransferDataFromWindow(), TransferDataToWindow()
nor Validate() should "descend" into their top level children. This is
trivial to change in wx (we basically just need to test for
child->IsTopLevel() inside all these functions) but it won't help if you
plan to use 2.9.2 for the August release and something like above would
still be required.

 Regards,
VZ