Re: [lmi] Anomaly: empty messagebox

[Vadim Zeitlin]

On Sat, 28 Jan 2017 22:40:43 +0000 Greg Chicares <address@hidden> wrote:

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

 No, I think LayoutOnceGuard does what it's meant to do, which is to
prevent multiple calls to Layout() during its lifetime to perform more than
at most one actual layout. It can be used naturally in all the other
functions performing actions affecting layout.

 The problem here is that we artificially disable layout initially by
incrementing the layout freeze count. This uses the same variable that
LayoutOnceGuard uses because it doesn't make sense to introduce another
one, but isn't part of this class responsibilities at all.

 The good news is that there is a much better and simpler way to do what we
need to do in this particular sequence() function where we both operate on
the layout freeze count manually and use LayoutOnceGuard, sorry for failing
to see it the first time and here is the patch doing the right thing:
---------------------------------- >8 --------------------------------------
diff --git a/input_sequence_entry.cpp b/input_sequence_entry.cpp
index e409b61..7c4595e 100644
--- a/input_sequence_entry.cpp
+++ b/input_sequence_entry.cpp
@@ -413,6 +413,12 @@ void InputSequenceEditor::sequence(InputSequence const& s)
 {
     LayoutOnceGuard guard(this);
 
+    // The layout was frozen initially, but it can be thawed as soon as this
+    // function returns because we will finally be able to determine our real
+    // size. Notice that thaw will only happen on function exit, in the dtor
+    // of the layout guard defined just above.
+    --layout_freeze_count_;
+
     while(0 < rows_count_)
         {
         remove_row(0);
@@ -425,7 +431,7 @@ void InputSequenceEditor::sequence(InputSequence const& s)
         {
         // have single row (initial state)
         add_row();
-        goto done;
+        return;
         }
 
     LMI_ASSERT(0 == intervals.front().begin_duration);
@@ -485,11 +491,6 @@ void InputSequenceEditor::sequence(InputSequence const& s)
     value_field_ctrl(0).SetFocus();
 
     update_diagnostics();
-
-  done:
-    // The layout was frozen initially, thaw it now, just once, as we can
-    // determine our really final size.
-    --layout_freeze_count_;
 }
 
 std::string InputSequenceEditor::sequence_string()
---------------------------------- >8 --------------------------------------

i.e. instead of using RAII to ensure that the layout freeze counter is
decremented at the end of the function, just decrement it directly in the
beginning because this is not going to do anything until LayoutOnceGuard
dtor is executed anyhow. Sorry again for failing to see this much simpler
solution originally, RAII is great but this examples shows that you can
have too much of a good thing...


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

 The early exit is without doubt a bug. There may be other problems or, at
least, advantages to further changing the code, but both the fact that the
code was not structured in a way making it impossible for the bug resulting
in the layout remaining forever frozen to happen, and the fact that this
bug did indeed happen are clear problems, so I'd very grateful if you could
please apply the patch above to fix them.

GC> Once that's done, I'd want to revert the patch anyway because I don't
GC> really understand it, so I've committed an alternative that doesn't
GC> prevent errors of this sort in the future but is simple and effective.

 I hope I could explain what the patch did and if you do apply the patch
above, you won't have to revert it later, as it will still remain correct
and will ensure that no similar problems can be introduced in this function
in the future.

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

 FWIW I still think scope_{exit,success,failure} helpers would be handy to
have. But there is no urgent need for them right now.

VZ