Re: [lmi] UI updates during long operations

[Greg Chicares]

On 2018-06-26 09:51, Vadim Zeitlin wrote:
> On Tue, 26 Jun 2018 00:36:27 +0000 Greg Chicares <address@hidden> wrote:
> 
[...reproducing this observation is the crucial next step...]
> 
> GC>  (2) It was possible to issue other commands (e.g., Census | Add cell) 
> while
> GC>      these operations were underway.
> 
>  This one is less so, but thinking more about it, this almost certainly
> happens due to the use of wx[Safe]Yield() in the code.
> 
>  I will probably need to reproduce this problem, if only in order to
> explain the strange behaviour discussed below, so I'd like to ask: how
> exactly do you "issue other commands" while these operations were in
> progress? Did you use keyboard or the mouse and, in the latter case, did
> you use the toolbar button or something else?

Let me step back now and document exactly how I can reproduce this.
Let's use 'clean1691.cns', a 1691-cell census I sent to your personal
email on 2018-02-28T22:56Z.

Load that census, and hit End so you can see the number of the last
cell. Then:
  hold down Ctrl
    press and release ']'
    press and release '=' [documented as Ctrl-+, but it's not shifted]
  release Ctrl.
Each of these two commands takes several seconds, so no great dexterity
is required. I observe that both are queued, even though I certainly
took my hands away from the keyboard before the first one completed.

[This is less awful that I had feared. Queuing two commands, and then
executing them separately in strict order, does no actual harm. I had
thought earlier that I was seeing two commands trying to execute at
the same time, and both trying to modify each other's data structures,
which would be very dangerous--but I cannot reproduce any such thing.]

I can also reproduce this using only the toolbar. Press the
  Varying column width
toolbar button. While that button is still visibly depressed, press the
  Add cell
button. The second button initially doesn't change, but it takes on the
"depressed" appearance after the first command has completed.

I can also reproduce this "extra command queuing" if I replace the first
command with "Census | Copy census", either from the keyboard:
  Ctrl-Shift-C
  Ctrl-=
or by clicking the toolbar buttons, but it's harder to accomplish this
because copying even a census this large takes only half a second. It's
easier if you repeat the first command several times, because reclicking
the same button doesn't require moving the mouse cursor; alternatively,
  hold down Ctrl and Shift; quickly press and release C several times
  Ctrl-=
On the statusbar I see something like:
  Copy: 611 milliseconds
  Copy: 542 milliseconds
  Copy: 565 milliseconds
  Add: 3381 milliseconds

Using "Census | Paste census" as the first command, I cannot reproduce
this issue at all. I tried
  Ctrl-Shift-C [to copy something that I could paste]
  Ctrl-Shift-S [to perform the paste command]
and then pressed Ctrl-= ten times; when the counter on the statusbar
passed 1000, I pressed Ctrl-= ten more times, before pasting finished.
Result: no cell was added--i.e., all twenty Ctrl-= keystrokes were
ignored. I repeated this experiment with the following patch:

---------8<--------8<--------8<--------8<--------8<--------8<--------8<-------
diff --git a/census_view.cpp b/census_view.cpp
index b41fe302..d02d25f0 100644
--- a/census_view.cpp
+++ b/census_view.cpp
@@ -1697,7 +1697,7 @@ void CensusView::UponRunCaseToGroupQuote(wxCommandEvent&)
 
 void CensusView::UponPasteCensus(wxCommandEvent&)
 {
-    wxBusyCursor reverie;
+//  wxBusyCursor reverie; // repress this part of commit 489f0a265
 
     std::string const census_data = ClipboardEx::GetText();
 
--------->8-------->8-------->8-------->8-------->8-------->8-------->8-------

and observed the same outcome, showing that the busy cursor doesn't
make any difference in the behavior under study.

Thus, I cannot reproduce the impossible behavior reported earlier,
when I added a...

> GC> wxBusyCursor instance to each. To my surprise, that seemed to prevent (2),
> GC> although I thought it couldn't.
> 
>  This is what I think too. I really don't know how to explain this and
> would need to debug the original (2) myself to be able to say more about
> it.

I withdraw the idea that a busy cursor "seemed to prevent (2)" if the
first command is census paste. And if the first command is census copy,
as noted above, the undesired behavior (2) is observed even though a
busy cursor is displayed. The busy cursor seems to make no difference.

> GC> OTOH, if (2) is prevented only by tight
> GC> loops that prevent events from being processed, I don't understand how I
> GC> was ever able to provoke (2) in the first place; I just didn't have enough
> GC> time to investigate that carefully.
> 
>  My current explanation of (2) is that the event handlers are called from
> inside wxSafeYield() in UponPasteCensus(). However this doesn't explain how
> does it happen when copying the census, as I don't see any wxYield() calls
> in DoCopyCensus(),

The new puzzle, based on careful tests that I hope you'll be able to
reproduce, is why (2) occurs during UponCopyCensus() (which does not
call wxSafeYield()) but not during UponPasteCensus() (which does call
wxSafeYield()). Is that actually contrary to expectation? Maybe I just
have the wrong expectation: the wx documentation says SafeYield()

| is similar to wxYield(), except that it disables the user input to
| all program windows before calling wxAppConsole::Yield and re-enables
| it again afterwards.

Might it therefore be the case that, when UponPasteCensus() calls
wxSafeYield(), it pulls queued events out of the message loop, and
then they are prevented from having any effect? because "it disables
the user input", so in effect it's as though we had written
  {
  wxWindowDisabler xyzzy;
  wxYield();
  }
?

> nor why does it stop happening when wxBusyCursor is
> instantiated

That phenomenon is not reproducible; my report was in error.

>, so this explanation must be wrong. If I could reproduce the
> behaviour myself, I'd just run the program under debugger and put a
> breakpoint on CensusView::UponAddCell() to see where is it called from:
> it's not nearly as elegant as finding the answer by examining the code and
> applying logical deduction, but should be pretty efficient in terms of
> actually explaining what goes on.

To a pure platonist, using a debugger does feel like cheating. But I'd be
tempted to use one here myself, if I had already updated the makefiles to
produce native x86_64-linux-gnu binaries.

> GC> Now that the release pressure is gone, let's consider what to do about
> GC> commands that may take a while to execute. At least until we rewrite lmi 
> to
> GC> use threading, it seems pretty important to prevent a new command from 
> being
> GC> issued while an old one is still executing;
> 
>  Actually, this would almost certainly remain the case even when/if lmi is
> rewritten to use threading as having a UI allowing to execute several
> different operations concurrently would be very confusing, even if we could
> implement it. The main UI improvement from using threads I can see is that
> we'd be able to show the status/progress of the current operation and, most
> importantly, allow cancelling it.

Oh...yes, you are quite right.

Let me send this now, and defer replying to the rest of your comments.