Re: [lmi] Input-sequence editor testing

[Vaclav Slavik]

On Fri, 2010-07-16 at 19:36 +0000, Greg Chicares wrote:
> > It doesn't validate the entry under the -- wrong -- assumption that
> this
> > editor won't let you enter invalid data. I can see two fixes to
> this:
> > 
> > (1) Don't allow invalid input. We could replace the numeric duration
> >     field with wxSpinCtrl with restricted range. This isn't entirely
> >     trivial, because a change in one row affects valid ranges in all
> >     subsequent rows as well as the preceding one. This can be
> extremely
> >     annoying to the user if not implemented perfectly.
> > 
> > (2) Allow invalid entry, but if it happens, show diagnostics message
> >     in the bottom part of the dialog (in red, perhaps) and disable
> the
> >     OK button until the input passes validation. This is easier to
> >     implement.
> > 
> > What do you prefer?
> 
> The first, because that's what lmi already strives to do elsewhere. 

As the first step, here's a patch that does (2) -- doing (1) properly
will take more time (I'm not even sure how to fully do it yet), and (2)
is better than nothing. (Moreover, most of this patch will be used by
(1) too (it still needs to show errors somewhere for invalid values) and
the (2)-specific part is tiny.)

As an example of the problems with (1), the value may be a keyword, and
while "ann" is an invalid keyword value, it happens when typing the
valid "annual" keyword. This case can be handled in the same way that
the main dialog does it, by disallowing focus change.

But I have my doubts about this UI being a good fit for input sequence
editor. It's acceptable when any incorrect input can be fixed by
changing a single value, the last one modified. But an input sequence
has a more complex structure, you may want to fix a mistake by e.g.
removing a row or changing duration mode altogether -- something that
the focus change prevention approach doesn't allow.

I'd welcome some ideas how to make it (more) impossible to enter invalid
input sequences. For now, this patch adds an error message for invalid
sequences and disables the OK button in such case.

One thing to do is to change the duration_num_field() into wxSpinCtrl
with limited range, and I will do that.

Regards,
Vaclav


Here's the patch:

diff --git a/input_sequence_entry.cpp b/input_sequence_entry.cpp
index e815aaa..a02d269 100644
--- a/input_sequence_entry.cpp
+++ b/input_sequence_entry.cpp
@@ -261,6 +261,11 @@ class InputSequenceEditor
     int compute_duration_scalar(int row);
     void adjust_duration_num(int row);
 
+    void update_diagnostics();
+    bool is_valid_value(wxString const& s);
+    wxString get_diagnostics_message();
+
+    void UponValueChange(wxCommandEvent& event);
     void UponDurationModeChange(wxCommandEvent& event);
     void UponDurationNumChange(wxCommandEvent& event);
     void UponRemoveRow(wxCommandEvent& event);
@@ -275,6 +280,7 @@ class InputSequenceEditor
     wxFlexGridSizer* sizer_;
     wxButton* ok_button_;
     wxButton* cancel_button_;
+    wxStaticText* diagnostics_;
     typedef std::map<wxWindowID, int> id_to_row_map;
     id_to_row_map id_to_row_;
 
@@ -301,6 +307,9 @@ InputSequenceEditor::InputSequenceEditor(wxWindow* parent, 
wxString const& title
     sizer_ = new(wx) wxFlexGridSizer(Col_Max, 5, 5);
     top->Add(sizer_, wxSizerFlags(1).Expand().DoubleBorder());
 
+    diagnostics_ = new(wx) wxStaticText(this, wxID_ANY, "");
+    top->Add(diagnostics_, 
wxSizerFlags().Expand().DoubleBorder(wxLEFT|wxRIGHT));
+
     wxStdDialogButtonSizer* buttons = new(wx) wxStdDialogButtonSizer();
     buttons->AddButton(ok_button_ = new(wx) wxButton(this, wxID_OK));
     buttons->AddButton(cancel_button_ = new(wx) wxButton(this, wxID_CANCEL));
@@ -313,17 +322,6 @@ InputSequenceEditor::InputSequenceEditor(wxWindow* parent, 
wxString const& title
     add_row();
 
     value_field_ctrl(0).SetFocus();
-
-    ::Connect
-        (this
-        ,wxEVT_COMMAND_CHOICE_SELECTED
-        ,&InputSequenceEditor::UponDurationModeChange
-        );
-    ::Connect
-        (this
-        ,wxEVT_COMMAND_TEXT_UPDATED
-        ,&InputSequenceEditor::UponDurationNumChange
-        );
 }
 
 void InputSequenceEditor::sequence(InputSequence const& s)
@@ -398,6 +396,8 @@ void InputSequenceEditor::sequence(InputSequence const& s)
 
     // move focus to a reasonable place
     value_field_ctrl(0).SetFocus();
+
+    update_diagnostics();
 }
 
 std::string InputSequenceEditor::sequence_string()
@@ -528,7 +528,8 @@ void InputSequenceEditor::insert_row(int new_row)
     wxStaticText* from_label = new(wx) wxStaticText(this, wxID_ANY, 
LARGEST_FROM_TEXT);
     SizeWinForText(from_label, LARGEST_FROM_TEXT);
     sizer_->wxSizer::Insert(insert_pos++, from_label, flags);
-    sizer_->wxSizer::Insert(insert_pos++, new(wx) DurationModeChoice(this), 
flags);
+    wxChoice *duration_mode = new(wx) DurationModeChoice(this);
+    sizer_->wxSizer::Insert(insert_pos++, duration_mode, flags);
     wxTextCtrl* duration_num = new(wx) wxTextCtrl(this, wxID_ANY, "", 
wxDefaultPosition, wxDefaultSize, wxTE_RIGHT);
     duration_num->SetValidator(wxTextValidator(wxFILTER_DIGITS));
     sizer_->wxSizer::Insert(insert_pos++, duration_num, flags);
@@ -613,6 +614,29 @@ void InputSequenceEditor::insert_row(int new_row)
 
     set_tab_order();
 
+    // connect event handlers
+    ::Connect
+        (value_ctrl
+        ,wxEVT_COMMAND_TEXT_UPDATED
+        ,&InputSequenceEditor::UponValueChange
+        ,wxID_ANY
+        ,this
+        );
+    ::Connect
+        (duration_mode
+        ,wxEVT_COMMAND_CHOICE_SELECTED
+        ,&InputSequenceEditor::UponDurationModeChange
+        ,wxID_ANY
+        ,this
+        );
+    ::Connect
+        (duration_num
+        ,wxEVT_COMMAND_TEXT_UPDATED
+        ,&InputSequenceEditor::UponDurationNumChange
+        ,wxID_ANY
+        ,this
+        );
+
     // update state of controls on the two rows affected by addition of
     // a new row
     if(prev_row != -1)
@@ -942,6 +966,74 @@ void InputSequenceEditor::adjust_duration_num(int row)
     duration_num_field(row).SetValue(wxString::Format("%d", num));
 }
 
+void InputSequenceEditor::update_diagnostics()
+{
+    // Validate the sequence and if it's not valid, show an error
+    // and disable the OK button.
+
+    wxString msg = get_diagnostics_message();
+
+    if(diagnostics_->GetLabel() != msg)
+        {
+        diagnostics_->SetLabel(msg);
+        redo_layout();
+        }
+
+    ok_button_->Enable(msg.empty());
+}
+
+bool InputSequenceEditor::is_valid_value(wxString const& s)
+{
+    for(std::vector<std::string>::const_iterator k = keywords_.begin()
+       ;k != keywords_.end()
+       ;++k)
+        {
+        if(s == *k)
+            return true;
+        }
+
+    if(!keywords_only_)
+        return s.IsNumber();
+
+    return false;
+}
+
+wxString InputSequenceEditor::get_diagnostics_message()
+{
+    // Check some common problems and issue nice error messages for them:
+    for(int row = 0; row < rows_count_; ++row)
+        {
+        wxString const value = value_field(row).GetValue();
+        if(value.empty())
+            return wxString::Format("Missing value on row %d.", row);
+
+        if(!is_valid_value(value))
+            return wxString::Format("Invalid keyword \"%s\" on row %d.", 
value.c_str(), row);
+
+        if(duration_mode_field(row).needs_number() && 
duration_num_field(row).GetValue().empty())
+            return wxString::Format("Duration not entered on row %d.", row);
+        }
+
+    // As fallback, parse the sequence and check the diagnostics. This may be
+    // less human-readable, but it's better than nothing at all:
+    InputSequence const sequence
+        (sequence_string()
+        ,input_.years_to_maturity()
+        ,input_.issue_age        ()
+        ,input_.retirement_age   ()
+        ,input_.inforce_year     ()
+        ,input_.effective_year   ()
+        ,0
+        ,keywords_
+        );
+    return sequence.formatted_diagnostics().c_str();
+}
+
+void InputSequenceEditor::UponValueChange(wxCommandEvent&)
+{
+    update_diagnostics();
+}
+
 void InputSequenceEditor::UponDurationModeChange(wxCommandEvent& event)
 {
     int row = id_to_row_[event.GetId()];
@@ -964,6 +1056,8 @@ void 
InputSequenceEditor::UponDurationModeChange(wxCommandEvent& event)
             update_row(i); // for "from ..." text
             }
         }
+
+    update_diagnostics();
 }
 
 void InputSequenceEditor::UponDurationNumChange(wxCommandEvent& event)
@@ -974,12 +1068,16 @@ void 
InputSequenceEditor::UponDurationNumChange(wxCommandEvent& event)
         {
         update_row(i); // for "from ..." text and duration_scalars_
         }
+
+    update_diagnostics();
 }
 
 void InputSequenceEditor::UponRemoveRow(wxCommandEvent& event)
 {
     int row = id_to_row_[event.GetId()];
     remove_row(row);
+
+    update_diagnostics();
 }
 
 void InputSequenceEditor::UponAddRow(wxCommandEvent& event)
@@ -1001,6 +1099,8 @@ void InputSequenceEditor::UponAddRow(wxCommandEvent& 
event)
         }
 
     duration_num_field(new_row).SetFocus();
+
+    update_diagnostics();
 }
 } // Unnamed namespace.
 
diff --git a/wx_utility.hpp b/wx_utility.hpp
index f5877c1..c02b6b7 100644
--- a/wx_utility.hpp
+++ b/wx_utility.hpp
@@ -79,6 +79,7 @@ void Connect
     ,wxEventType     event
     ,Return (Class::*handler)(Argument)
     ,int             id = wxID_ANY
+    ,wxEvtHandler*   eventSink = NULL
     )
 {
     // Double parentheses: don't parse comma as a macro parameter separator.
@@ -98,7 +99,13 @@ void Connect
 
     typedef void (wxEvtHandler::*t0)(Argument);
     typedef wxObjectEventFunction t1;
-    object->Connect(id, event, c_cast<t1>(static_cast<t0>(handler)));
+    object->Connect
+        (id
+        ,event
+        ,c_cast<t1>(static_cast<t0>(handler))
+        ,NULL
+        ,eventSink
+        );
 }
 
 calendar_date ConvertDateFromWx(wxDateTime const&);