Re: [lmi] [lmi-commits] master 9bed27ea 1/2: Revert "Remove unnecessary variable from IllustrationView::OnCreate()"

[Greg Chicares]

On 6/16/22 00:46, Vadim Zeitlin wrote:
> On Wed, 15 Jun 2022 21:27:50 +0000 Greg Chicares <gchicares@sbcglobal.net> 
> wrote:
> 
> GC> We hope to do our first 64-bit production release this month, and
> GC> there are only eight days until our normal code-freeze date,
> 
>  Oh, so I'm too late with wx upgrade patch I guess?

There's not enough time to test it. And a good case can be made for
upgrading wx separately from our first 64-bit production release,
as those are both enormous changes. If we combine them and test
only once, that may seem like less work, but it can be hard to tell
which change caused any regression we detect. Testing them separately
means testing twice, but is less risky. And now that we've removed
tools that were needed for i686 builds, we really do need to get that
64-bit code released soon.

> GC> It didn't seem to matter how the unwanted question was answered:
> GC> the dialog closed abruptly for all three answers. With msw
> GC> (though perhaps not with GTK), the file can be saved, and later
> GC> reopened, but if it's reopened, no view is created, and thus
> GC> there's no open document whose input the user can edit to fix.
> GC> Of course, if they remember the error message, they can re-reopen
> GC> it and fix it then, but they might not be that ingenious. IOW,
> GC> the problem isn't only that a stray question is asked, but that
> GC> the view isn't created. That makes sense to us, but to an end user
> GC> it may not.
> 
>  Sorry, I was confused here and thought that we did _not_ want to create
> any window in this case because I misunderstood the comment before
> IllustrationView::OnCreate() and thought that "a zombie view" referred to a
> blank view such as is shown currently, i.e. with the latest lmi master, if
> the parameters are incorrect.

/// Trap exceptions to ensure that this function returns 'false' on
/// failure, lest wx's doc-view framework create a zombie view. See:
///   https://lists.nongnu.org/archive/html/lmi/2008-12/msg00017.html

Following that mailing-list link:

|   Cannot dereference null pointer of type 'P8wxWindow'.
| pops up during ViewEx::OnClose(), which throws an exception and
| never returns--so the application cannot be closed at all by
| normal means.
...
| The ultimate cause is that IllustrationView::OnCreate(), an
| overridden virtual, can throw an exception (as in the situation
| above)--but it needs to return 'false' to prevent the doc-view
| framework from creating a zombie view.

The "zombie" was a never-created view that apparently was in wx's
view list: closing lmi caused wx to try to close that view,
leading to the weird outcome mentioned above. I should have read
the history carefully before cherry-picking these changes.

> GC> When you have time, could you add a case like that to the GUI test?
> 
>  Sure, but what exactly should the test check for? Just that a window is
> created even in case of invalid parameters entry?

  File | New | Illustration
  delete contents of "Current COI multiplier'
  OK

Expected behavior: a "view" window is created, and an "Error"
messagebox says:

| Input validation problems for '':
| COI multiplier entered is '', but it must contain at least one number other 
than zero.

Pressing the messagebox's "OK" button (its only pushbutton)
leaves the view open, with an asterisk on its titlebar name
indicating that its document contains nondefault user input.

> GC> >  There is a simple fix for this: we should ignore the view's modified
> GC> > status when destroying it unconditionally, and I'll make this change in 
> wx,
> GC> > but I'd understand if you didn't want to re-revert this again...

That may be the right thing to do in wx--I really don't know.
For lmi, we don't want to destroy this view unconditionally.

> GC> With an automated GUI test for this, we could reconsider later
> GC> after testing the behavior with that wx change. But the
> GC> behavior after this reversion is that a view is created, and
> GC> as explained above I think that's the least astonishing thing
> GC> we can do.
> 
>  I thought that failing to create a window with an error message and
> _without_ asking any questions about saving anything would be the best
> behaviour. But thinking more about this, I realize that this would mean
> losing all the entry in the parameters dialog, so this is probably not
> great neither.

Yes, end users may be furious if lmi discards data they entered.

>  In fact, ideal, from a purely UI point of view, IMO, would be to avoid
> accepting the dialog with invalid data in it.

That is the ideal that lmi's MVC framework was designed to achieve.
To see it working:

  File | New | Illustration
  "Payments" tab: change "Dumpin" to "-1000"

Now try to do almost anything outside that field, e.g.:
 - Tab to go to another field
 - Ctrl-PgUp to go to a different tab
 - click "OK"
and you can't--you're locked in "Dumpin" until you fix the problem
indicated in the static-text control at lower left:
  -1000 is too low: 0 is the lower limit.
Naturally, "Cancel" dismisses the dialog.

The same thing happens if you try other invalid input, such as
an empty string:
  '' is ill formed.
or "abc":
  'abc is ill formed.

In certain cases, the input-consistency rules aren't enforced
soon enough to achieve the ideal behavior above, but are only
caught by downstream assertions. One example is proposed for an
automated GUI test above, which occurs because the primary text
controls for sequence fields (as opposed to the popup dialog
opened by clicking the "..." button) are not validated while
the dialog is active. (Whether they should be is a separate
question not addressed here.) Here's another example...

  File | New | Illustration
  change "Issue age" to 85
  OK

...which has the same outcome except for the messagebox text, which is:

| Assertion 'yare_input_.RetireesCanEnroll || IssueAge <= RetAge' failed.

In 'input_harmonization.cpp' we see:

#if 0
// TODO ?? Temporarily suppress this while exploring automatic-
// enforcement options in the skeleton trunk. Certain limits are
// interdependent:
//    issue_age      [0, omega - 1] (taken as an independent variable)
//    attained_age   [x, omega - 1]
//    duration       [0, omega-x-1]
    IssueAge.minimum_and_maximum
        (database_->query<int>(DB_MinIssAge)
        ,database_->query<int>(DB_MaxIssAge)
        );
//    RetirementAge.minimum_and_maximum(...
#endif // 0

which begins to sketch out an idea, but stops short of grappling
with the complications. Here, the state of "RetireesCanEnroll"
would make a difference: if it's 'true', then this example would
be okay; if it's false, then the other condition in the failed-
assertion message above, i.e., "IssueAge <= RetAge", is not
fulfilled, and an ideal GUI would either block the input that
led to this failure (locking focus in its GUI field), or force
some input to change; or perhaps limit the spin controls for
issue and retirement ages so as to preclude violating input.
Whether such a change should be made may sound like an easy
question, but actually designing the change is really difficult,
and the difficulties are not discussed here.

> However if we have to accept
> it even in this case, then I think the view should show something
> indicating that it's invalid -- maybe even the error message itself? -- as
> showing just a blank window doesn't seem very user-friendly.

We don't just show a blank 'view' window: we also show a
messagebox. If you're suggesting we copy the messagebox text
into the view, I'd be firmly opposed: we shouldn't spend time
and effort duplicating information that's already provided
because that time would be better spent on solving the
underlying problem; any change can introduce a new defect;
and we have no time for any of this, anyway.