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

[Greg Chicares]

On 2011-07-13 16:55Z, Vaclav Slavik wrote:
> 
> On 2011-07-05 15:11, Vaclav Slavik wrote:
>> Please replace this with an updated version:
>> 
>> commit 2806dea06f88c781fcf304beb6e22e6f1b752074
>> Author: Vaclav Slavik <address@hidden>
>> Date:   Fri Apr 29 10:26:53 2011 +0200
>> 
>>     Make InputSequenceEntry friendly to embedding in wxDVC.
> 
> I'm sorry that I keep updating this, but here's another patch
> (to be applied on top of the previous one) that makes the code cleaner.
> It's functionally equivalent, but as Vadim pointed out,
> TranferDataFromWindow() is the right and official way to do this.

I think this patch introduces a defect; to reproduce:
  File | New | Illustration
  On "Payments" tab, paste this into "Individual payment":
    40000 2;99999;100000;100001;110000;120000;130000;140000;150000;0
  Click "..." [the "40000" field is highlighted]
  Press Enter [the "40000" is overwritten; an error is diagnosed]
    [notice that the "Individual payment" field changes on the parent]
  Cancel
Because the dialog was cancelled, I feel that the "Individual payment"
field should be left unchanged. Instead, it changes from
  40000 2;99999;100000;100001;110000;120000;130000;140000;150000;0
to
  2; 99999 3; 100000 4; 100001 5; 110000 6; 120000 7; 130000 8; 140000 9; 
150000 10; 0
which--arising as it does from diagnosed invalid input--is semantically
wrong, even though it happens to be syntactically valid.

Here's the heart of the patch...

> -void InputSequenceEditor::EndModal(int retCode)
> +bool InputSequenceEditor::TransferDataFromWindow()
>  {
> +    if(!wxDialog::TransferDataFromWindow())
> +        return false;
> +
>      // We need to set the value as soon as possible -- when used in 
> wxDataViewCtrl, the value
>      // is read from editor control as soon as focus changes, which is before 
> ShowModal() returns.
> -    if(associated_text_ctrl_ && retCode == wxID_OK)
> +    if(associated_text_ctrl_)
>          associated_text_ctrl_->SetValue(sequence_string());
>  
> -    wxDialog::EndModal(retCode);
> +    return true;
>  }

...and I think the comment (untouched by the patch) may be mistaken,
because it seems to describe the surprising behavior after the patch
is applied, rather than the unpatched behavior that I think is right.