Re: [lmi] UI updates during long operations (was: master 489f0a2 1/2: Show busy cursor when cutting and pasting_

[Vadim Zeitlin]

On Tue, 26 Jun 2018 00:36:27 +0000 Greg Chicares <address@hidden> wrote:

GC> First, let me explain why I committed this change. I was testing some new
GC> copy-paste code with a large census, and noticed two problems:
GC> 
GC>  (1) These operations can take so long that a user might think lmi has
GC>      frozen and needs to be terminated.

 This is the expected (although bad, from the user point of view)
behaviour.

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?

GC> Experimentally, I instantiated each of these three classes:
GC>   wxBusyCursor
GC>   wxWindowDisabler
GC>   wxWindowUpdateLocker
GC> at the beginning of the copy and paste member functions, and a quick test
GC> suggested that I had prevented (2).

 Yes, wxWindowDisabler would do this.

GC> But this seemed like overkill, so I
GC> chose not to commit it without asking your advice; instead, I just 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.
 
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(), nor why does it stop happening when wxBusyCursor is
instantiated, 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.


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.

GC> is that what wxWindowDisabler is for?

 Yes.

GC> And it's a good idea to give some visual indication that the program is
GC> doing something: ideally, by showing a progress dialog or a running count on
GC> the statusbar,

 Using wxProgressDialog is simple, but it's also quite heavy, especially
for the relatively short operations. I rather like showing just a progress
bar (wxGauge) with a small "Cancel" button nearby directly in the main
window, without bring up any additional dialogs. This is simpler to do when
the work is being done in a background thread, but can also be done without
using threads.

GC> Can all this be done with a single class, which we might then instantiate at
GC> the beginning of every event handler?

 No, I don't think just instantiating it could be enough. We also need to
update it periodically, otherwise it would have no possibility to update
the UI after some delay.

GC> I'm not sure it's possible to make a
GC> class as simple as wxBusyCursor do that: it can't know when the threshold
GC> time (100msec, e.g.) has passed unless it either
GC>  - sets a wx timer...but that won't work because a tight loop would prevent
GC>    events from being processed; or

 Yes.

GC>  - creates a timing thread...but if we use multiple threads, this issue no
GC>    longer exists;

 Using another thread doesn't really help here because it's the main thread
that must show the busy cursor. And while it could do it in response to an
event sent from the worker thread, this is not going to work here, as the
events are not being dispatched. Unless we double down on using wxYield(),
but I don't think it's a good idea.

GC> so I guess we'd need to call a BusyLocker::Update() function to check a 
timer
GC> on every iteration,

 Yes. And if we wanted to handle cancelling, this function would return a
boolean indicating whether the current operation should be aborted or not.
I.e. we end up with basically wxProgressDialog API.

GC> which arguably makes this not worth doing everywhere.

 Yes, but still worth doing in some places IMHO.

GC> AFAICT, wxWindowUpdateLocker is not applicable here: we'd use it only when
GC> we want to prevent wx from repainting anything.

 Yes, exactly.

GC> BTW, when we use the statusbar to show progress thus:
GC>     status() << "Added cell number " << cells.size() << '.' << std::flush;
GC>     wxSafeYield();
GC> I see a "choppy" count: it counts up to 22 in a blur, then stalls for a
GC> decisecond, shows 57, stalls for another few centiseconds, then shows 67;
GC> on another run, the stalls occur at different times, and I see 12, 89, 125
GC> on the statusbar. Shouldn't the wxSafeYield() call ensure a smooth update,
GC> such as I see as a result of this code:
GC>     status() << "Read " << ++counter << " cells." << std::flush;
GC> (which doesn't call wxSafeYield()) elsewhere?

 The behaviour of wxYield() depends on the presence of the other events in
the event queue. If you call it a sufficient number of times, the window
should get repainted, but one call might not be enough. If possible -- and
I think it is indeed possible with wxStatusBar -- wxWindow::Update() should
be called instead, to update the window appearance immediately. Replacing
wxYield() with Update() would also solve the original problem (2) if my
explanation were correct (but, again, I must be missing something here, so
it probably isn't). Update() wouldn't allow handling clicks on the "Cancel"
button however, so if we want to let the user to cancel the operation (and
I think we do), we can't avoid wxYield(), unfortunately.

 Regards,
VZ