Re: [lmi] wxProgressDialog resists interruption with Cancel

[Vadim Zeitlin]

On Thu, 14 Nov 2013 16:54:38 +0000 Greg Chicares <address@hidden> wrote:

GC> Using HEAD revision 5830:
GC> 
GC> File | New | Census
GC> Census | Add cell [repeat several times]
GC> Census | Print case to disk
GC> 
GC> If you hit Esc while the busy cursor is not shown, it cancels.
GC> 
GC> OTOH, with older revisions, the progress dialog cannot be cancelled,
GC> even if you hold the Esc key down throughout. In this diff to 5830:
GC>   
http://svn.savannah.nongnu.org/viewvc/lmi/trunk/system_command_wx.cpp?root=lmi&r1=5634&r2=5830
GC> you can just comment out this line:
GC>   e && e->YieldFor(wxEVT_CATEGORY_UI);
GC> if that's easier.

 Hello again,

 Thanks for the precise instructions, I could reproduce the problem and
debug it easily with them. First of all, let me say that you were
absolutely right and I was completely wrong: I do see exactly the behaviour
you describe and which I claimed to be impossible. In my defense, this
behaviour is the result of several bugs overlapping in rather non-trivial
way, so it was quite difficult to foresee it and not that easy to debug it
neither...

 Anyhow, the first conclusion of my 2 hour long debugging session is that
Esc key works with your change only because of a bug in YieldFor() which is
not supposed to handle any input events at all when called with
wxEVT_CATEGORY_UI, normally you'd need wxEVT_CATEGORY_USER_INPUT for this.
But it so happens that in wxMSW keyboard input events are handled specially
and Esc events are handled even more specially and are generated from the
keyboard hook procedure, called before anything else takes place, instead
of during normal event dispatching. And the hook procedure code runs even
when YieldFor() only examines the pending messages, without dispatching
them. So this explains why Esc works, but doing anything else doesn't.

 The important thing, however, is the other bug or, if you want, a
peculiarity of wxMSW implementation. For a lot of complicated reasons,
wxMSW creates a dialog window, positioned off screen, so it's never visible
to the user, while looping in wxExecute(wxEXEC_SYNC). And so any events,
whether normal keyboard ones or mouse ones, such as are generated when
clicking on the button, are actually sent to this hidden dialog and not to
the progress dialog, where they are simply ignored. So this is, just as you
surmised, the case of wxExecute() "eating" all the events, although it does
it in the rather indirect way. And, because of the previous bug, lets Esc
key events through.


 Now that we understand what is going on, a more interesting question is
what can we do. The minimal change I'd recommend would be to put
reflect_progress() call before the call to wxExecute() and remove the
explicit calls to YieldFor() from system_command_wx(), please see the
minimal patch below [*]. This helps with not having mysteriously working
(this is even worse than mysteriously not working) code and also actually
does allow you to cancel the progress dialog by clicking the button or the
"x" title bar button, even though you still have to wait a bit for the
click to be taken into account because the call to run() itself takes quite
a long time, at least here (in the non-optimized debug build I should add).
And I don't see any drawbacks to doing this, so I'd definitely apply this
patch.


 Another thing which would help would be, perhaps surprisingly, upgrading
to a newer MinGW version and running the program under Windows Vista or
later. I had a momentary (at least I hope it was only momentary...) lapse
of memory and completely forgot to mention that normally a completely
different wxProgressDialog implementation is now used in wxMSW. It is based
on the native so called task dialog and, due to the peculiarities of the
native API, actually shows the dialog from a different thread. This creates
some problems (which I should really look into one of these days...) but it
helps a lot in situations like this one: as the dialog is shown by another
thread, it always reacts to the user input, i.e. it always repaints, even
while the main thread is busy, and you can always press the "Cancel" button
which immediately switches to the disabled state to indicate that your
action has been taken into account. Of course, it's still going to take
some time for the main thread to actually react to the dialog being
cancelled, i.e. it still has to call Update() or WasCancelled() to notice
it, but UI-wise the native dialog works much better (I tested it).

 The only reason LMI doesn't use it already is that you are using a too old
MinGW distribution which doesn't have the necessary native API declarations
in its headers. To be honest, I don't remember since when were the
necessary declarations (we check for TD_WARNING_ICON, i.e. if it's
available we suppose that all the rest of the task dialog APIs are there as
well) added, but the last few versions of MinGW definitely have them. And
MinGW-w64, which is actually a better version of g++ to use under Windows
nowadays IMHO, has had them since always.

 So if you updated to a more recent MinGW, this would improve the progress
dialog without doing anything else. Of course, this would only help users
under Windows Vista and later, so if LMI is still used under XP, this isn't
as interesting. But in any case I don't see what else could we do here,
short of using asynchronous wxExecute() -- which would definitely solve all
problems but would require a significant effort too.


 To summarize, my recommendations are:

1. To call wxProgressDialog::Update() before calling wxExecute(), this
   would take care of key/mouse presses during the run() execution which
   takes quite a long time, so in practice this helps a lot with usability.
   This is done by the patch below, which also removes the now unnecessary
   voodoo in system_command_wx() itself.

2. To update to the latest, or at least sufficiently recent, MinGW to get
   the native progress dialog for an even better usability under Vista+.

3. To rewrite the code using asynchronous wxExecute() if the behaviour is
   still unsatisfactory.

 Sorry but I don't think there is anything else to do here,
VZ


[*] ... minimal patch below:

diff --git a/group_values.cpp b/group_values.cpp
index 1da1619..c6c1c0b 100644
--- a/group_values.cpp
+++ b/group_values.cpp
@@ -137,6 +137,11 @@ census_run_result run_census_in_series::operator()
             IllusVal IV(serial_file_path(file, name, j, "hastur").string());
             IV.run(cells[j]);
             composite.PlusEq(*IV.ledger());
+            if(!meter->reflect_progress())
+                {
+                result.completed_normally_ = false;
+                goto done;
+                }
             result.seconds_for_output_ += emit_ledger
                 (serial_file_path(file, name, j, "hastur")
                 ,file
@@ -145,11 +150,6 @@ census_run_result run_census_in_series::operator()
                 );
             meter->dawdle(intermission_between_printouts(emission));
             }
-        if(!meter->reflect_progress())
-            {
-            result.completed_normally_ = false;
-            goto done;
-            }
         }
     meter->culminate();

diff --git a/system_command_wx.cpp b/system_command_wx.cpp
index 183747d..4d74399 100644
--- a/system_command_wx.cpp
+++ b/system_command_wx.cpp
@@ -94,9 +94,6 @@ void concrete_system_command(std::string const& command_line)
     std::ostream& statusbar_if_available = b ? status() : null_stream();
     statusbar_if_available << "Running..." << std::flush;

-    wxEventLoopBase* e = wxEventLoopBase::GetActive();
-    e && e->YieldFor(wxEVT_CATEGORY_UI);
-
     wxArrayString output;
     wxArrayString errors;
     long int exit_code = wxExecute(command_line, output, errors);