Re: [lmi] wx-2.9.5-rc1 regression with conditional controls enablement

[Greg Chicares]

On 2013-07-16 00:42Z, Vadim Zeitlin wrote:
> On Mon, 15 Jul 2013 21:54:13 +0000 Greg Chicares <address@hidden> wrote:
[...enablement stopped working for controls within groupboxes...]
> 
> [...] the code in MvcController::ConditionallyEnable()
> only iterates over the current page children. And this used to include all
> controls shown on the page. However since r71886 (done on 2012-06-30, we
> seem to be really getting more than a share fair of problems happening on
> June 30), the children of wxStaticBoxSizer are created as *children* of
> their containing wxStaticBox, not its siblings as before. [...]
> 
>  This analysis does explain this part, too:
> 
> GC> With "Elected" checked, press "OK", then "Ctrl-E": now the
> GC> suboptions are enabled, as they should be. Unchecking "Elected"
> GC> should disable them; it does not.
> 
>  Because the code executed on the dialog initialization doesn't iterate on
> the page children but (AFAICS) on all controls, so it's not affected.

>From this important point I conclude that use of Lineage() and lineage_,
which are recursive, immunized parts of the code against the wx change
above (r71886); and that direct, nonrecursive use of wxWindowList in
'mvc_controller.cpp' needs to be reevaluated in both places it occurs:

(1) in ConditionallyEnable(), as discussed in your later message; and also

(2) in EnsureOptimalFocus(). I see a regression there: on the "Solve" tab,
every control is in a groupbox, and none can be reached with the Tab key.

> GC> We've found a few other examples where conditional enablement fails.
[...snip description of a particular problem...]
> 
>  I am not sure if the problem I found explains this behaviour however. But
> it would probably be simplest to fix this problem first and then retest if
> the problem still remains.

I agree: that's the right strategy. And the particular problem snipped
above seems to have been resolved by recursing into groupbox children.

> GC> Whether it works or not in a particular case seems to vary from one
> GC> machine to another.
> 
>  I also can't explain this at all. The behaviour I see should be perfectly
> reproducible. But, again, maybe it would be best to fix the problem we know
> about first and then see if any gremlins remain.

Yes, again, that's the right strategy. One of the gremlins we found
along the way turned out to be due to a recent, misbegotten lmi change;
that's what I'm working on at the moment.

>  Which brings me back to fixing the problem as this is one thing I hadn't
> done yet. One solution would be to simply iterate over the children
> recursively. But this could result in even further slowdown (which is a
> concern in this code and I can confirm that it's not as fast as I'd like it
> to be, as a user) because we'd be iterating over many controls
> unnecessarily, i.e. most of the sub-elements of composite controls which
> don't have any validators associated with them.

At a quick glance, it appears that whenever we iterate over lineage_,
we check whether the iterand has a validator, and skip it if it doesn't.
I doubt this is very costly, but it's wasteful; perhaps Lineage() should
simply not return any wxWindow* pointers that have no validator (though
we still have to recurse through windows without validators, in case they
have children with validators).

>  We could avoid this by using a restricted recursion and only recurse
> inside wxStaticBoxes but not the other controls. I think this is the
> smallest change that would be necessary to make this code work again so I
> plan to do this tomorrow if I don't hear anything different from you but I
> would be the first to admit that it's not the most elegant solution.

I'll comment on the patch you sent in a later message, after I've killed
the lmi gremlin mentioned above.

>  It would be nicer to somehow reuse the existing "lineage_" as we could,
> AFAICS, put only the windows that do have a Transferor associated with them
> into it (although there is some code in TestModelViewConsistency which
> would need to be updated then, I think).

Yes. My own naive attempt to work around this reuses the Lineage() function
where we need a "lineage" for the current page only. But upon reflection
your idea here sounds better:

>  Ideal would be to replace a flat vector by a map of parent pages to the
> (interesting) controls inside them, then we could iterate over this instead
> of iterating over GetChildren().

At first I thought you were suggesting that iteration would be faster over a
std::map than over a std::vector. Rereading this more carefully, though, I
see the real point: sometimes we want to iterate over the current page only,
and sometimes over all pages; but controls aren't added or removed, so we
only need to generate a container of controls once (whereas, as mentioned
just above, I did it dynamically in my ConditionallyEnable() experiment).

>  What do you think of all these suggestions and would you by chance see a
> better one?

I tried it myself while awaiting the patch you later sent. Once I'd done
that, everything you've said became quite clear. The problem is that we
don't always recurse through groupboxes; the solution, plainly, is always
to recurse through them.

This question calls to mind an investigation I abandoned years ago, but
I'll mention it again in case it leads us to a better idea:

http://lists.nongnu.org/archive/html/lmi/2010-05/msg00015.html
| Actually, at one time I experimented with the idea of writing
| Input::DoHarmonize() in Prolog. The problem I though that might solve
| is that there are many interdependencies among enablement conditions.
| Using a logical inference engine to untangle them is more attractive
| than spending time to write imperative-language statements in exactly
| the right order. And we could even use this rejected boost library:
|   http://www.cc.gatech.edu/~yannis/fc++/boostpaper/fcpp.html
| to write Prolog in C++, as discussed here:
|   http://www.cc.gatech.edu/~yannis/lc++/paper.pdf
| But that would be a separate project.

That would work for lmi's MVC implementation in general. As quoted above,
Input::DoHarmonize() takes care to express interrelated dependencies in
a viable order. But in code like MvcController::ConditionallyEnable() we
disregard interdependencies and impose all conditions afresh--each time
any control's value changes, even though that change won't affect most
of the other controls.

Without a radical change like importing a Prolog interpreter, we could
still get some of the benefits. Form a vector C of all controls that have
validators. Create a square boolean matrix M mapping C onto itself, and
set M[i,j] true iff a change in C[i] could cause a change in C[j] (any
sort of change, including enablement, limits, and so on). Naively, that
captures only direct dependencies, which is insufficient; but by raising
M to an adequate power (defining, I guess, multiplication to be logical
OR), we'd fill in indirect dependencies. Then, when C[i] changes, we
only need to consider C[k] for which C[i,k] is true when applying
iterative functions like ConditionallyEnable(). And that should be much
faster.