Re: [lmi] UI updates during long operations

[Vadim Zeitlin]

On Tue, 26 Jun 2018 22:33:39 +0000 Greg Chicares <address@hidden> wrote:

GC> On 2018-06-26 16:27, Vadim Zeitlin wrote:
GC> > On Tue, 26 Jun 2018 15:15:27 +0000 Greg Chicares <address@hidden> wrote:
GC> > 
GC> > GC> Might it therefore be the case that, when UponPasteCensus() calls
GC> > GC> wxSafeYield(), it pulls queued events out of the message loop, and
GC> > GC> then they are prevented from having any effect? because "it disables
GC> > GC> the user input", so in effect it's as though we had written
GC> > GC>   {
GC> > GC>   wxWindowDisabler xyzzy;
GC> > GC>   wxYield();
GC> > GC>   }
GC> > GC> ?
GC> > 
GC> >  Yes, this is exactly what happens. Knowing, now, that the keyboard/mouse
GC> > events are queued for later execution -- and not taken into account
GC> > immediately as I originally thought -- I don't have any trouble with
GC> > explaining this behaviour any longer: without any calls to wxYield(), the
GC> > keyboard messages synthesized by Windows itself for the application remain
GC> > in its message queue and are converted to wx events, which are then
GC> > dispatched as usual, when the program eventually gets back to the event
GC> > loop. With wxSafeYield(), these events are dispatched from inside it, 
while
GC> > the window is disabled, and so are just ignored. Of course, this just
GC> > spells out in more details the same conclusion that you've already arrived
GC> > to empirically, so I realize this is not really helpful
GC> 
GC> No, it's very helpful, thanks. I just have two questions.
GC> 
GC> >  The only thing I can add is that while we can rely on the latter
GC> > behaviour, I don't think we should really count on the former because the
GC> > message queue could overflow (it is relatively small) and if there were
GC> > sufficiently many unprocessed messages in it, the keyboard events could
GC> > just be lost.
GC> 
GC> Which behaviors do "former" and "latter" apply to? Quoting from above:

 Sorry, I should have really been more clear. I used "former" for "without
any calls to wxYield [...] then dispatched as usual" case and "latter" for
the "with wxSafeYield() [...] are just ignored" one.

GC> > With wxSafeYield(), these events are dispatched from inside it, while
GC> > the window is disabled, and so are just ignored.
GC> 
GC> Is it that
GC>   "former" = "events...dispatched...while...disabled", and
GC>   "latter" = "just ignored"?

 Yes to the second one, but no to the first one.

GC> If so, then
GC>  - we can rely on an event being ignored, if it's dispatched from
GC>    inside the wx event loop while the window is disabled; but

 Yes.

GC>  - we can't rely on all events being dispatched from inside the
GC>    wx event loop, even though we call wxSafeYield(), because wx
GC>    events are held in a small queue that could overflow.

 Yes.

GC> But I doubt I've interpreted that correctly, because it would lead
GC> to the conclusion that
GC>  - if the queue doesn't overflow, events are read from the queue and
GC>    dispatched, but without effect because all windows are disabled;
GC>  - if the queue does overflow, events are discarded, without effect;
GC> but either way the result is the same. The first way, we look at an
GC> event, do nothing with it, and then send it into a black hole; the
GC> second way, we send it into a black hole without even looking at it.

 The queue is small but not that small (I don't know its exact size, which,
AFAIR, depends on MSW version, just that it is possible to overflow it), so
the events that are stored inside it would be processed when wxSafeYield()
is called, which is an important difference. E.g. it would be useless to
show a progress dialog with a "Cancel" button and not yielding, as presses
on this button would never be handled.


GC> That was my easier question. The hard one is: should lmi ever use
GC> wxSafeYield(), and why, and how?

 The only valid reason for using wxSafeYield() is to handle user actions,
such as button clicks, without returning to the event loop. If this is not
desired, then calling wxSafeYield() should be unnecessary, but should be
mostly "safe" in the sense that it doesn't result in reentrancies, as the
window is disabled. I don't think there is any reason to call wxYield(),
ever, if you don't count "make your life more interesting because you enjoy
hunting down mysterious bugs due to impossible reentrancies" as a valid
reason.

GC> What's its intended purpose in wx? AFAICT, it's twofold:
GC> 
GC>  (A) wxSafeYield(SomeWindow) disables every other window in the
GC>      application, but processes events for SomeWindow. Maybe that
GC>      would be useful in a context like
GC>        GetStatusBar()->SetStatusText("Making progress...");
GC>        wxSafeYield(GetStatusBar());
GC>      (but I've never done quite that in lmi).

 Yes, this is the intended purpose. The window should be something with
"active" UI elements however and not a wxStatusBar (unless you put a
"Cancel" button in the status bar). I.e. something that you really want to
get the events from.

GC>  (B) wxSafeYield() with default argument NULL disables every window
GC>      in the application, and relinquishes the app's timeslice.

 No, I think this is confusing with Win16 (!) Yield() and its spiritual
successor wxThread::Yield(). wxSafeYield() doesn't relinquish the thread
timeslice.

GC>      How is
GC>        wxSafeYield();
GC>      really different from
GC>        {wxWindowDisabler foo;} // Disable events for zero nanoseconds.
GC>      ?

 It is different because wxSafeYield() could still handle events that don't
require the window to be enabled. This notably includes previously posted
events (from the main thread or other ones), timer events, socket events
and maybe some others I'm forgetting.

GC>      Is it just that it surrenders the app's timeslice? Does it even
GC>      make sense to speak of timeslices in a millennium where preemptive
GC>      multitasking is presumably universal even for refrigerators?

 It could be, but it's true that I don't remember when was it a concern for
the last time.

GC> Surveying lmi's current use of wxSafeYield():
GC>   /opt/lmi/src/lmi[0]$grep wxSafeYield *.?pp |less -S
GC> I see that it is never given an argument (it always defaults to NULL),
GC> and it's used almost exclusively in unit tests, the only exception
GC> being in CensusView::UponPasteCensus():
GC>     status() << "Added cell number " << cells.size() << '.' << std::flush;
GC>     wxSafeYield();
GC> The apparent purpose there is to let the statusbar update smoothly,
GC> but as recently reported it updates choppily, presumably because the
GC> invocation should be
GC>     wxSafeYield(pointer_to_statusbar_window);

 No, this wouldn't have changed anything.

GC> Now I wonder how that statusbar manages to get even choppy updates:
GC> it would seem that the CPU is constantly busy, preventing the wx
GC> event loop from doing anything...except during wxSafeYield(), which
GC> should disable the statusbar so that it can't be updated.

 Disabled windows can be updated. They just don't handle keyboard/mouse
events.

GC> I wouldn't be surprised if every call lmi makes to wxSafeYield() is
GC> actually a mistake. [I'm removing one that certainly was.]

 Yes, I agree that this one is.


GC> The only other concern that comes to mind right now is the one you raised,
GC> that wxBusyCursor ought to be avoided or somehow conditionalized when it
GC> might just flicker annoyingly for a single eyeblink. Paraphrasing our
GC> earlier discussion, delaying its onset means using a progress-meter API,
GC> which is so complicated that we'd rather not use it casually.

 I don't think it's _that_ complicated, but it's definitely more
complicated than not doing anything, so if you don't think it's a problem
for your users, doing nothing is a better solution.

GC> An intriguing
GC> alternative is to show something on the statusbar, because that's less
GC> obtrusive that changing the cursor for a fleeting instant. If it says only
GC> "Pasted cell 1", that's fine; if it counts up to "Pasted cell 1691", that's
GC> also fine. Writing such a message to the statusbar is probably a simple,
GC> self-contained, one-line change that we can drop in anywhere.

 Yes, I think so.

GC> In the changeset I'm just about to push, I've
GC>  - reverted the busy-cursor commit that started this thread;
GC>  - called wxWindow::Update() when writing to the statusbar as you suggested;

 This is the point where I should mention that I haven't actually tested
this. Update() is supposed to work, but there can sometimes be
complications with the native controls (of which wxStatusBar is one). Did
it actually solve the problem of "missing" updates?

GC>  - added a status-bar counter in CensusView::DoCopyCensus(), in lieu of
GC>    the former busy cursor.
GC> The last of those three changes is experimental, as it raises an interesting
GC> question: would you rather see chatter on the statusbar during an operation
GC> that takes an appreciable fraction of a second for a 1691-cell census (and
GC> presumably several seconds for one with more than 10000 cells), or would
GC> you rather have that operation take only half as long? Or--perhaps even
GC> better--in a case like this, since we're already using something like the
GC> progress-meter API, would we be better off with the 100-msec-delayed busy
GC> cursor previously discussed, assuming that its overhead is less than this
GC> rather hungry statusbar's? OTOH, because counting to 1691 on the statusbar
GC> adds the same amount of time to any 1691-fold operation, if it's negligible
GC> for one, should we deem it negligible for any operation?

 Typically, the status bar wouldn't be updated on every loop iteration, but
on every 10th, or maybe 100th, one (or, ideally, after every 100ms, i.e. a
dynamically determined number of iterations). This not only lowers the time
"wasted" on UI updates, but also avoids flicker that would surely happen if
each loop iteration is quick enough.

 So my API would actually look like this:

        void MyFrame::SomeLongRunningFunction()
        {
            StatusBarUpdater u{GetStatusBar()};
            for (auto const& x: many_xs)
                {
                u.Update(...);
                ...
                }
        }

and StatusBarUpdater would only actually update the status bar once some
initial timeout t0 expires and after this will update it after expiration
of (different, because I think it should be smaller) timeout t1.

 Implementing this seems straightforward and using it is really as simple
as it gets. I still believe that it'd be nice to allow cancelling the long
running operations and not just be able to follow their progress, but if
the former is deemed to be too complicated, it would still be worth doing
at least the latter IMHO.

 Regards,
VZ