Re: [lmi] Anomaly: empty messagebox

[Greg Chicares]

On 2017-01-28 17:01, Vadim Zeitlin wrote:
> On Sat, 28 Jan 2017 12:28:54 +0000 Greg Chicares <address@hidden> wrote:
> 
> GC> To reproduce:
> GC>   File | New | Illustration
> GC> Clear any input-sequence textcontrol (so that no text remains),
> GC> then click its ellipsis button. I see a messagebox with the
> GC> textcontrol's name in its titlebar, which contains "OK" and
> GC> "Cancel" buttons, but no message.
> 
>  Thanks for the instructions, I can indeed reproduce this perfectly well.
> 
> GC> I looked in 'input_sequence_entry.cpp', but I can't see how it
> GC> could produce this messagebox. Vadim, can you figure this out?
> 
>  Yes, it's a bug introduced in my layout optimization patch. The real
> problem is that layout_freeze_count_ is never decremented in case of a
> premature return from InputSequenceEditor::sequence(). The following patch
> fixes it and makes sure it can't happen in the future:
[...]
> I don't know what do you think of the ugly use of unique_ptr<> as poor
> C++11 programmer replacement of the well-known "scope guard" class.

If a scope guard needs to be added within an existing LayoutOnceGuard,
then doesn't that mean that LayoutOnceGuard isn't doing what we need
it to do, in which case we should modify LayoutOnceGuard rather than
the code that uses it?

> GC> IOW, is this the intended behavior of the following code path?
> GC> 
> GC>     if(intervals.empty())
> GC>         {
> GC>         // have single row (initial state)
> GC>         add_row();
> GC>         return;
> GC>         }
> GC> 
> GC> If that's the explanation, then maybe the right thing to do
> GC> is to establish !intervals.empty() as a ctor postcondition;
> GC> I was thinking of doing that anyway.
> 
>  I can't really comment on this, I don't know what would be the most
> reasonable interpretation of an empty control contents, but I think the
> current code works correctly, from the user point of view, once the bug
> above, preventing the (created, but invisible) first row from being laid
> out correctly, is fixed.

I'm trying to recover a design intention that was never carried out,
while removing hooks for speculative features that cannot reasonably
be implemented, in brittle code that hasn't been touched in fifteen
years. The observed anomaly may be attributed either to a defect that
I've discovered there, or to an early exit that skipped thawing.

Either way, the best fix is to remove that conditional block
    if(intervals.empty()) {...}
after establishing the invariant that the set of intervals cannot be
empty, because to me it reads:
    if(class InputSequence must be repaired)
        {
        ...it should never have been necessary to write anything here...
        }
Once that's done, I'd want to revert the patch anyway because I don't
really understand it, so I've committed an alternative that doesn't
prevent errors of this sort in the future but is simple and effective.

Later, we can devise a solution that is simple and robust. If it needs
to do some exotic stuff, let's hide the implementation in a library,
or in class LayoutOnceGuard, and keep the rest of the code simple.