Surprising behaviour when using validators with wxTextEntryDialog

[Vadim Zeitlin]

Hello,

wxTextEntryDialog helpfully exposes (undocumented) SetTextValidator()
added way back in 2002 by this patch: http://trac.wxwidgets.org/ticket/5420
Unfortunately calling it breaks the behaviour of GetValue() method of the
dialog: if a custom validator is used, m_value is not modified at all any
longer. This looks like a bug to me but because I have relatively little
experience with validators and because there is no documentation, I'm not
totally sure about this. So the question is: should I

1. Document the existing behaviour and tell people to provide their own
wxString if they call SetTextValidator()?

2. Fix it and always fill m_value even if a custom validator is used?

Personally I'd prefer (2). Any objections?
VZ

[Manolo]

I think the problem arises only when
wxTextEntryDialog::SetTextValidator(const wxTextValidator& validator)
version is used, because the parameter 'validator' knows nothing about the
address of m_value.

I don't see a quick workaround.
Perhaps adding a GetValueAddress(). People creating a custom
wxTextValidator would use this address at its ctor, so 'validator' does
know how to access m_value.

> 1. Document the existing behaviour and tell people to provide their own
> wxString if they call SetTextValidator()?
>
> 2. Fix it and always fill m_value even if a custom validator is used?

In the case of the fix I suggest, I prefer (1).

Regards
Manolo

[Vadim Zeitlin]

On Tue, 02 Apr 2013 01:35:04 +0200 Manolo wrote:

M> I think the problem arises only when
M> wxTextEntryDialog::SetTextValidator(const wxTextValidator& validator)
M> version is used, because the parameter 'validator' knows nothing about the
M> address of m_value.

Yes.

M> I don't see a quick workaround.

The fix would be to just always set m_value to the control contents in
OnOK(), it's not exactly difficult. In fact, we could stop using validator
for just transferring the value and simply do it manually, simplifying the
code as we'd get rid of all those "#if wxUSE_VALIDATORS" in it.

M> Perhaps adding a GetValueAddress(). People creating a custom
M> wxTextValidator would use this address at its ctor, so 'validator' does
M> know how to access m_value.

The bad ("surprising") thing is that you don't know you need to do it.
Even if we document that calling SetTextValidator() means that GetValue()
doesn't work, half of the people (whom am I kidding... let's say 90% of
them) won't read this documentation before they hit the bug. If you know
about it, providing the address of your own wxString is a good enough
workaround, no need for ugly GetValueAddress(). The real question is why do
we surprise people by default.

More I think about it, less excuses can I find for this behaviour.

Regards,
VZ

[Manolo]

> The fix would be to just always set m_value to the control contents in
> OnOK(), it's not exactly difficult. In fact, we could stop using validator
> for just transferring the value and simply do it manually, simplifying the
> code as we'd get rid of all those "#if wxUSE_VALIDATORS" in it.
>

But then Validate() won't be called at OnOK(). What would be the reason for
using a custom validator if it can't validate, not just filter keys?

> M> Perhaps adding a GetValueAddress(). People creating a custom
> M> wxTextValidator would use this address at its ctor, so 'validator' does
> M> know how to access m_value.
>
> The bad ("surprising") thing is that you don't know you need to do it.
> Even if we document that calling SetTextValidator() means that GetValue()
> doesn't work, half of the people (whom am I kidding... let's say 90% of
> them) won't read this documentation before they hit the bug.

I fully agree.

> If you know
> about it, providing the address of your own wxString is a good enough
> workaround, no need for ugly GetValueAddress().

If the custom validator calls textctrl->ChangeValue(), GetValue() should work.

> The real question is why do we surprise people by default.

I think surprised people have not been a lot, otherwise they had complained ;).

Regards,
Manolo

[Vadim Zeitlin]

On Tue, 02 Apr 2013 02:07:25 +0200 Manolo wrote:

M> But then Validate() won't be called at OnOK().

Why wouldn't it? See the patch below, IMO it's fully backwards compatible
except for actually fixing the bug.

M> What would be the reason for using a custom validator if it can't
M> validate, not just filter keys?

Of course it should be able to validate...

M> > The real question is why do we surprise people by default.
M>
M> I think surprised people have not been a lot, otherwise they had complained ;).

You'll be surprised how often people just work around the bug, decide to
not use the buggy feature (i.e. omit validating entirely) or patch their
version of wxWidgets completely without ever telling anybody about it (not
counting swearing loudly). We may believe that we're getting a lot of
feedback and bug reports but my highly unscientific yet intuitively not far
off the mark estimate would be that at most 10% of wx users ever have any
interaction with the mailing lists or the bug tracker.

Regards,
VZ

--- Proposed patch starts here: ---

diff --git a/include/wx/generic/textdlgg.h b/include/wx/generic/textdlgg.h
index 4d37d24..12c806e 100644
--- a/include/wx/generic/textdlgg.h
+++ b/include/wx/generic/textdlgg.h
@@ -72,6 +72,9 @@ class WXDLLIMPEXP_CORE wxTextEntryDialog : public wxDialog
#endif
// wxUSE_VALIDATORS

+ virtual bool TransferDataToWindow();
+ virtual bool TransferDataFromWindow();
+
// implementation only
void OnOK(wxCommandEvent& event);

diff --git a/src/generic/textdlgg.cpp b/src/generic/textdlgg.cpp
index b00d192..2ea03f0 100644
--- a/src/generic/textdlgg.cpp
+++ b/src/generic/textdlgg.cpp
@@ -105,11 +105,6 @@ bool wxTextEntryDialog::Create(wxWindow *parent,
Expand().
TripleBorder(wxLEFT | wxRIGHT));

-#if wxUSE_VALIDATORS
- wxTextValidator validator( wxFILTER_NONE, &m_value );
- m_textctrl->SetValidator( validator );
-#endif // wxUSE_VALIDATORS
-
// 3) buttons if any
wxSizer *buttonSizer = CreateSeparatedButtonSizer(style & (wxOK | wxCANCEL));
if ( buttonSizer )
@@ -134,19 +129,26 @@ bool wxTextEntryDialog::Create(wxWindow *parent,
return true;
}

+bool wxTextEntryDialog::TransferDataToWindow()
+{
+ m_textctrl->SetValue(m_value);
+
+ return wxDialog::TransferDataToWindow();
+}
+
+bool wxTextEntryDialog::TransferDataFromWindow()
+{
+ m_value = m_textctrl->GetValue();
+
+ return wxDialog::TransferDataFromWindow();
+}
+
void wxTextEntryDialog::OnOK(wxCommandEvent& WXUNUSED(event) )
{
-#if wxUSE_VALIDATORS
- if( Validate() && TransferDataFromWindow() )
+ if ( Validate() && TransferDataFromWindow() )
{
EndModal( wxID_OK );
}
-#else
- m_value = m_textctrl->GetValue();
-
- EndModal(wxID_OK);
-#endif
- // wxUSE_VALIDATORS
}

void wxTextEntryDialog::SetValue(const wxString& val)
@@ -167,8 +169,7 @@ void wxTextEntryDialog::SetTextValidator( long style )

void wxTextEntryDialog::SetTextValidator( wxTextValidatorStyle style )
{
- wxTextValidator validator( style, &m_value );
- m_textctrl->SetValidator( validator );
+ SetTextValidator(wxTextValidator(style));
}

void wxTextEntryDialog::SetTextValidator( const wxTextValidator& validator )

[Manolo]

>M> But then Validate() won't be called at OnOK().
>
> Why wouldn't it? See the patch below, IMO it's fully backwards compatible
> except for actually fixing the bug.

Looks good.

> You'll be surprised how often people just work around the bug, decide to
> not use the buggy feature (i.e. omit validating entirely) or patch their
> version of wxWidgets completely without ever telling anybody about it (not
> counting swearing loudly). We may believe that we're getting a lot of
> feedback and bug reports but my highly unscientific yet intuitively not far
> off the mark estimate would be that at most 10% of wx users ever have any
> interaction with the mailing lists or the bug tracker.

Experience improves intuition. I agree with yours. 10% is too optimistic.

Regards,
Manolo

[Vadim Zeitlin]

On Tue, 02 Apr 2013 14:25:23 +0200 Manolo wrote:

M> >M> But then Validate() won't be called at OnOK().
M> >
M> > Why wouldn't it? See the patch below, IMO it's fully backwards compatible
M> > except for actually fixing the bug.
M>
M> Looks good.

OK, I've committed it now, please let me know if anybody sees any new
problems.

Thanks,
VZ