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

[Greg Chicares]

On 2011-08-09 09:41Z, Vadim Zeitlin wrote:
> 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

Me too, now that you point it out. I was staring at it yesterday--it's the only
place lmi calls TransferDataFromWindow()--but, even though the tabbed-dialog
.xrc files specify wxWS_EX_VALIDATE_RECURSIVELY, I didn't imagine it would
recurse into a pop-up dialog.

[...MvcController::UponUpdateUI()...]
>  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.

OTOH, that would mean copying and pasting (much of) a library function
into the new MvcController::GetWindowData(), which over time would drift
away from the library's TransferDataFromWindow() implementation.

> VS> I already mentioned one quick fix.

"The fix is simple, just don't apply this patch." That's best for now.
I have little enough time available to finish other work I must do for
the month-end release.

> 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.

Yes, I was very surprised that this descent occurred.

> 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.

Let's defer the patch in Vaclav's 2011-07-13T16:55Z email until that
change is made in 2.9.3 .