Re: [lmi] wxProgressDialog resists interruption with Cancel

[Greg Chicares]

On 2013-11-13 21:01Z, Vadim Zeitlin wrote:
> On Wed, 13 Nov 2013 20:35:00 +0000 Greg Chicares <address@hidden> wrote:
> 
> GC> With lmi HEAD revision 5827, a progress dialog cannot be interrupted
> ...
> GC> What's so special about this command? My guess is that it uses
> GC> wxExecute() to run a command line like this:
> GC>   CMD /c C:/path/to/fop -fo file1 -pdf file2
> GC> and wx doesn't detect the Cancel event due to the external process.
> 
>  Yes, this looks like the most likely explanation. However isn't it the
> right thing to do in this case? After all, even if "Cancel" presses were
> taken into account visually, this still wouldn't affect the running child
> command, so the program still wouldn't be able to continue. I.e. while the
> situation is clearly not ideal from the UI point of view, at least it is
> consistent: "Cancel" doesn't work because this operation can't be
> cancelled.

I should more clearly explain the scope and limits of my objective.

I don't dream of reaching through this chain:
  wxExecute() --> CMD.EXE --> java --> fop
to abort an external process. Once wxExecute() has been called to
transform lmi data into a PDF file, I don't hope to interfere:
that file is going to be written.

But I do want Cancel to mean "Don't call wxExecute() again".
IOW: let any already-launched external process complete, but stop
the loop so that no more are launched.

> GC> Inserting this code from 'wx/src/generic/progdlgg.cpp':
> GC>   wxEventLoopBase::GetActive()->YieldFor(wxEVT_CATEGORY_UI);
> GC> into the main loop that uses the progress dialog didn't help much.
> 
>  It wouldn't, because this is the loop that already calls
> wxProgressDialog::Update() which does exactly this internally, as you've
> probably seen.

Yes. I hoped that calling it elsewhere in the loop might help....

> GC> Inserting it into the code that calls wxExecute() (patch below[0])
> GC> is much more effective:
> 
>  I don't see how could this be possible.

And yet it does work.

> Maybe it seems effective just
> because the child process terminates quickly in this case anyhow? If it
> really printed 5000 pages instead of 5, this wouldn't be the case, of
> course.

In this case, we aren't sending anything to a printer: we're just
writing PDF files to disk. Each one takes about 1.75 seconds:
  0.75 seconds for everything up to wxExecute(), and
  1.00 seconds for wxExecute to handle the external command.
For a thousand-life census, though, I don't want to wait 1750 seconds
for Cancel to take effect.

> GC> I can break out of the loop with a single press of the Cancel key, as
> GC> long as I press it when the busy cursor is not displayed. Questions:
> GC> 
> GC> - Is there any hidden danger in this patch? (I suspect not, based
> GC> on our earlier discussion.)
> 
>  Not danger per se, but I think this patch is rather pointless.

Except that it actually solves the problem--so I've committed it, at
least experimentally, so that others can easily test it: revision 5830.

> GC> - Is there a better way to do this? (A commented-out line in the
> GC> patch shows an unsuccessful SafeYieldFor() idea that I tried.)
> 
>  Just to explain the parenthesized remark: wxApp::SafeYieldFor(NULL)
> doesn't work because it disables the progress dialog, to avoid this you'd
> have to pass the pointer to the dialog to it instead of NULL. However in
> this case there is not any difference between wxApp::SafeYieldFor() and
> directly calling wxEventLoop::YieldFor() anyhow because the only safety the
> former function adds is the disabling of all the windows (other than the
> one specified as its argument) and in this case all the windows had been
> already disabled by wxProgressDialog.

Okay, thanks, now I understand that.

>  But the question about how to do it better is more interesting, of course.
> Unfortunately the answer to it is that it requires more work
[snip details concerning wxExecute(wxString, wxEXEC_ASYNC, wxProcess*)]
> [...] Only this will allow you to really cancel the child
> execution (either by killing it when "Cancel" is pressed or maybe just
> letting it run to its peaceful end but ignoring its output) and return to
> the normal processing in the main event loop.

It's okay for a (single) pending one-second external process to continue
running to completion.

>  IMO this would be the right thing to do here, synchronous wxExecute()
> should only be used for (very) short-running child processes

Is a one-second process longer than you have in mind?

Anyway, we're left with a puzzle: why does revision 5830 work?
The loop looks like this:

  wxProgressDialog progress(...);
  for(i = 0; ...)
    {
    // calling YieldFor() on this line here is ineffective
    do_something_within_lmi();       // step 1: takes ~0.75 seconds
    call YieldFor(wxEVT_CATEGORY_UI) // step 2  (this works)
    wxExecute(...);                  // step 3: takes ~1.00 seconds
    progress.Update(...);            // step 4
    }

It seems clear that a Cancel event issued during step 1 gets recognized
in step 2; but without step 2, Cancel gets lost--it is not recognized in
step 4. My hypothesis is that wxExecute() eats pending Cancel events.

I'm not really concerned that a Cancel event issued during step 3 is
ignored, especially because a busy cursor is displayed throughout that
step. If one press of the Esc key doesn't seem to work, I'll press it
again when the busy cursor reverts to normal after one second.