Re: [lmi] System crashes with invalid input using input sequence GUI

[Vadim Zeitlin]

On Fri, 09 Oct 2015 17:49:04 +0000 Greg Chicares <address@hidden> wrote:

GC> [...trying to make sure deferred issues haven't been forgotten...]

[If you're interested, I have a dozen of items in my lmi backlog which I
 could bring back to your attention in November... Although judging by the
 number of items postponed until after the release, realistically it will
 be November 2017]

GC> On 2015-05-20 23:31, Vadim Zeitlin wrote:
GC> > On Wed, 20 May 2015 14:47:33 +0000 "Boutin, Wendy" <address@hidden> wrote:
GC> > 
GC> > BW> File | New | Illustration
GC> > BW> Payments tab
GC> > BW> Select ellipsis for 'Individual payment'
GC> > BW> Change 'until maturity' to 'until age'
GC> > BW> Select 'Add' button
GC> > BW> 
GC> > BW> There is clearly an input validation error occurring,
GC> > BW> but the system crashes when the diagnostic appears.
GC> > 
GC> >  I did end up by modifying wxWidgets to make sure such crashes do not
GC> > happen in the future, see commit[1].
GC> 
GC> We updated wx in August. Now I don't observe a crash.

 Yes, this is the only good thing here -- and the only one that could be
fixed at wx level. The rest [still] needs to be fixed at lmi level.

GC> > However lmi UI still behaves
GC> > surprisingly (at least I personally find the behaviour described in [2]
GC> [ 
https://github.com/wxWidgets/wxWidgets/commit/ba107a9a873abf7fefe003ef370e0f3c8c9239aa
 ]
GC> > quite unusual) even with that fix and I think this really needs to be 
fixed
GC> > at lmi level instead
GC> 
GC> Following those steps with lmi HEAD now, here's what I see. A new row
GC> is added in the middle of the dialog:
GC>   20000 from age 99 for a period of 0 years
GC> Below the bottom row this message appears:
GC>   Interval [54, 54) is improper: it ends before it begins. ...
GC> and the "OK" button is disabled.
GC> 
GC> The value in that spin control
GC>   for a period of 0 years
GC>                   ^
GC> can be changed from 0 to 1.

 This is the first problem. The second problem is that it can be changed
from 0 to 1 by pressing the "down" arrow and not the "up" one (this is the
"unusual behaviour" described above). The third problem is that the arrows
are enabled at all when the control can't be changed.

 All of those are IMO due to an off-by-1 error in lmi code: the minimum
value (1 here) should never be greater than the maximum one (0 here) and
fixing this would get rid of all three problems above.

GC> But if I change it to 1 and then tab away, it
GC> changes back to 0. AIUI, that's the anomaly that the lmi patch would fix,
GC> by graying the spin control and freezing its value at zero.

 Yes, the patch from 
http://lists.nongnu.org/archive/html/lmi/2015-05/msg00004.html
disabled the control entirely to avoid these problems. We may prefer to
just set both min and max to 0 instead: the control would remain enabled,
but at least its arrows would be disabled not allowing to change it in this
way any more. I still prefer the original patch because I don't see any
sense in allowing the user to enter (necessarily invalid) values into this
control even manually, but just setting the min and max might have the
advantage of making even fewer changes to the code and being even more
obviously correct: it's hard to argue against the change to the code that
prevents it from setting min to be strictly greater than max, isn't it?

GC> I would say that the real defect is that the "Add" button was enabled at
GC> all,

 Sorry, I still disagree with this. As I wrote in the message above:

VZ> A possible alternative would be to disable the "Add" button if there
VZ> are no valid values for the controls in a new row, but this could be
VZ> quite confusing as nothing would indicate to the user that the age must
VZ> be reduced in order to allow a new row to be added.

I think it's preferable to let the user use "Add", realize that the
duration is blocked at 0 and then adjust the age in the row above to enable
it. This seems to be much simpler and is definitely more discoverable than
your proposal, in which the user would have to be smart enough to realize
that the "Add" button is disabled because the age is at the maximum and
decrease the latter in order to enable the former. Maybe lmi users are much
smarter than average, but normally I would guarantee that people would be
stumped by such UI. So if you really insist on doing it like this, I think
we absolutely must implement the proposal also expressed in that May 2015
email:

VZ> So if you prefer such more pro-active approach, I think it would make
VZ> sense to at least show a message like "Sequence cannot be extended due
VZ> to age 99 running up to the maturity" (but preferably better worded).

 And I can also only repeat the following sentence from it:

VZ> Please let me know if you'd like me to make an alternative patch doing
VZ> it like this.


 Returning to the present:

GC> when clicking it produces these necessary consequences:
GC>   (a) an invalid interval,
GC>   (b) an error message to that effect, and
GC>   (c) a disabled "OK" button;
GC> and, incidentally, another consequence that could be avoided:
GC>   (d) the weird spin control symptoms described above,
GC> which formerly led to:
GC>   (e) a crash, which has now been prevented by a wx update.
GC> I don't think it's worth eighteen lines of code to avoid (d) now. Either
GC> we attack the root cause, or we decide to live with a set of anomalies;

 Sorry, but I disagree with this too. (d) is a relatively minor problem,
but fixing it is trivial too and it helps not only the users but the code
clarity and correctness. To be blunt, setting wxSpinCtrl minimum to 1 and
its maximum to 0 is obviously a bug. It just happens to work, for some
definition of "work", with wxMSW because of a quirk of the native control
(which also means that it won't be changed in wxWidgets because we don't
want to deviate from the native control behaviour gratuitously), but it's
still wrong.

 IMO even if the root cause is addressed and fixed, the code should still
check that it calls wxSpinCtrl::SetRange() with well-ordered parameters.

GC> The problem isn't where we cut the interval into two nonempty pieces.
GC> The problem is that we then offer to divide an indivisible piece.

 Perhaps, but if we want to disallow trying to do this, we must add an
explanation about why exactly it can't be done. IMNSHO it wouldn't be
acceptable to just refuse to do it without any hint, the program might be
in its right to do it but it would hardly endear it to the users.

GC> > A possible alternative would be to disable the "Add"
GC> > button if there are no valid values for the controls in a new row, but 
this
GC> > wouldn't could be quite confusing as nothing would indicate to the user
GC> > that the age must be reduced in order to allow a new row to be added.
GC> 
GC> Let's take a moment to think this through carefully.
GC> 
GC> When the ellipsis is clicked, we see a single row:
GC>   20000 from issue date until maturity
GC> There's no "Add" button because that interval is plenary and there's nothing
GC> to add.

 You're absolutely right that the absence of the "Add" button initially is
inconsistent with its presence in the "until age 99" case. This is the main
argument I see for doing something about it. I don't draw the same
conclusion about what this "something" is however.

GC> The "until maturity" control can be changed, e.g., to "until age";
GC> we have to allow that, because otherwise there's no way for the user ever
GC> to change the sequence. So far, everything seems correct.
GC> 
GC> Now change the endpoint to "until age". A limiting age of 99 appears:
GC>   20000 from issue date until age 99, then   [Remove]  [Add]
GC>                                               ^^^^^^^^^^^^^ both enabled
GC>   0 from age 99 until maturity
GC> This is where the problem arises; but what is actually wrong here?

 Let me give my theoretical answer: in theory, the wrong thing is actually
the _presence_ of the "Remove" and "Add" buttons and the second row.
Logically, it shouldn't be there at all and should only appear once the age
is reduced from 99.

 But in practice the UI implemented according to the above would be very
awkward to use as the user would have to order his actions carefully to
avoid creating an invalid sequence, even temporarily (as he wouldn't be
allowed to do it). As you say:

GC> (Of course, it is to be anticipated that those defaults will be
GC> modified immediately by users to fit their requirements.)

 This is a good practical argument for allowing putting the UI in an
invalid, or at least suboptimal (because "0 @99; 0" is valid, but clearly
should be simplified to "0", so the only reason to allow entering it is to
allow changing it to something else later), state and I completely agree
with it. But this is also an argument for keeping "Add" enabled: the user
should be able to add an interval first and make it valid, by adjusting the
ages later. If he thinks "I need 3 intervals with different payments" it
would be natural to create these intervals first, fill in the payments and
then adjust the ages, but disabling "Add" would make it impossible and
break the user's workflow.

GC> The natural-language description of this sequence is correct. Nothing
GC> can be wrong except the enablement of the buttons. And the "Add" button
GC> should be disabled because it cannot perform any correct action.

 Notice that if you use this logic, then it must go beyond the "Add" button
because you can also add a new row implicitly by changing "until maturity"
in the second row to "until age" -- this adds a new row as well. And it
also results in all the (a-d) problems above.

 To summarize, for me the only 2 consistent positions are:

1. Don't add new rows when the existing rows cover the entire interval,
   neither explicitly (using the "Add" button) nor implicitly. This is
   logical but inconvenient and would result in a lot of GUI updates as
   rows are added/removed whenever the year in a spin control is updated
   and so doesn't seem like a good solution to me.

2. Allow adding new rows even if they can't be non-empty, as we do now.
   This does put the dialog in an invalid state, but as long as it's
   clearly indicated (I would make the error message from (b) more visible
   by e.g. rendering it in red), it isn't a problem IMHO. The only problem
   is that the current UI doesn't handle this state well and this is fixed
   by my original patch.

GC> That problem is the root cause of all the ugly symptoms. Resolving it
GC> would justify making the code reasonably more complex. I don't know
GC> whether it's actually feasible.

 It is definitely feasible to implement (1) above. I just don't think it's
 a good idea.

GC> I'm sure we don't need to address this before Halloween, but I'd test a
GC> patch now and apply it later. Or, if it's not feasible, let's document
GC> the reason for deciding not to fix it.

 If you are absolutely sure that (1) is what you want, I'll work on a patch
implementing this in November [year redacted]. But could you please confirm
that this is really what you want, in spite of my objections above?

 And I'd like to make the one after last (see the second paragraph of
http://lists.nongnu.org/archive/html/lmi/2015-05/msg00007.html) request to
apply my patch fixing wxSpinCtrl range. IMO it's an obvious bug fix and it
also fixes a slight but real problem in the UI.

 Thanks in advance,
VZ