inputhook_qt4: Use QEventLoop instead of starting up the QCoreApplication #2294

Merged
merged 1 commit into from Nov 3, 2012

Projects

None yet

7 participants

@bfroehle
Contributor

I referenced this branch in #2080 and was letting it sit for a little while, but I have decided to make it a full pull request to get some additional visibility.

Essentially our Qt event loop mechanism repeatedly starts and quits a QCoreApplication object. Unfortunately the QCoreApplication::quit slot has a lot of unintended side effects (like emitting an aboutToQuit signal which closes all open file dialogs).

For our input hook, we might be able to get by with just using a QEventLoop whose quit slot is much simpler and less destructive.

For a little bit of background on why one might want to just use QEventLoop::exec, let's examine what QCoreApplication::exec does:

int QCoreApplication::exec()
{
    if (!QCoreApplicationPrivate::checkInstance("exec"))
        return -1;

    // ... [some assertions]

    threadData->quitNow = false;
    QEventLoop eventLoop;
    self->d_func()->in_exec = true;
    self->d_func()->aboutToQuitEmitted = false;
    int returnCode = eventLoop.exec();
    threadData->quitNow = false;
    if (self) {
        self->d_func()->in_exec = false;
        if (!self->d_func()->aboutToQuitEmitted)
            emit self->aboutToQuit();
        self->d_func()->aboutToQuitEmitted = true;
        sendPostedEvents(0, QEvent::DeferredDelete);
    }

    return returnCode;
}

As far as I can tell, it's a small wrapper around QEventLoop::exec which also:

  • Sets some variables regarding the current status
  • Emits an aboutToQuit signal right before the function returns (which is the root cause of @denisri's problem in #2080).

Historically, our Qt event loop is a python implementation of the (win 32) input hook supplied with the PyQt4 source (see qtcore_input_hookinpython-qt4/sip/QtCore/qcoreapplication.sip`), which more or less dates to a mailing list post from July 2007.

@travisbot

This pull request passes (merged 3e30dfe9 into 31afb62).

@ellisonbg
Member

The event loop integration stuff is extremely subtle and brittle. To make
any change like this, we will have to test a wide range of applications in
a wide range of configurations. But, most importantly, IIRC, we agreed on
a model for event loops that always creates the following two things
together:

  • Event loop running.
  • An application object.

IOW, our model is that if one if this is present, the other has to be.
Based on this, my first reply to this idea is that is break our model of
event loop integration.

On Sun, Aug 12, 2012 at 2:16 PM, Bradley M. Froehle <
notifications@github.com> wrote:

I referenced this branch in #2080https://github.com/ipython/ipython/issues/2080and was letting it sit for a little while, but I have decided to make it a
full pull request to get some additional visibility.

Essentially our Qt event loop mechanism repeatedly starts and quits a
QCoreApplication object. Unfortunately the QCoreApplication::quit slot
has a lot of unintended side effects (like emitting an aboutToQuit signal
which closes all open file dialogs).

For our input hook, we might be able to get by with just using a
QEventLoop whose quit slot is much simpler and less destructive.

For a little bit of background on why one might want to just use
QEventLoop::exec, let's examine what QCoreApplication::exec does:

int QCoreApplication::exec(){
if (!QCoreApplicationPrivate::checkInstance("exec"))
return -1;

// ... [some assertions]

threadData->quitNow = false;
QEventLoop eventLoop;
self->d_func()->in_exec = true;
self->d_func()->aboutToQuitEmitted = false;
int returnCode = eventLoop.exec();
threadData->quitNow = false;
if (self) {
    self->d_func()->in_exec = false;
    if (!self->d_func()->aboutToQuitEmitted)
        emit self->aboutToQuit();
    self->d_func()->aboutToQuitEmitted = true;
    sendPostedEvents(0, QEvent::DeferredDelete);
}

return returnCode;}

As far as I can tell, it's a small wrapper around QEventLoop::exec which
also:

Historically, our Qt event loop is a python implementation of the (win 32)
input hook supplied with the PyQt4 source (see qtcore_input_hookinpython-qt4/sip/QtCore/qcoreapplication.sip`),
which more or less dates to a

http://www.riverbankcomputing.com/pipermail/pyqt/2007-July/016512.htmlhttp://mailing%20list%20postfrom July 2007.

You can merge this Pull Request by running:

git pull https://github.com/bfroehle/ipython inputhook_qt4_alt

Or view, comment on, or merge it at:

#2294
Commit Summary

  • inputhook_qt4: Use QEventLoop instead of starting up the
    QCoreApplica…

File Changes

  • M IPython/lib/inputhookqt4.py (5)

Patch Links

Brian E. Granger
Cal Poly State University, San Luis Obispo
bgranger@calpoly.edu and ellisonbg@gmail.com

@bfroehle
Contributor

@ellisonbg Yes, it is exceptionally subtle. You'll notice that a QCoreApplication is still instantiated and validated -- this is actually a requirement of QEventLoop::exec.

The main issue I see here are:

  1. aboutToQuit is never signaled. This is almost certainly better than signaling it multiple times per second, but maybe this should be sent when we unregister the event loop.
  2. I'm not confident that we ever process DeferredDelete events.
@hmeine
Contributor
hmeine commented Sep 6, 2012

I have just tested this PR, that is - I tested your bfroehle/inputhook_qt4_alt branch. I am not 100% sure whether that is the right way to test a PR, whether this always gives the right code, and (in this case) why there is an inputhook_qt4_alt and an inputhook_qt4 branch. (I also tried with https://github.com/ipython/ipython/pull/2294.patch but that patch looked too small?)

Anyhow, I tested a complex application of mine (which allows to be started from within IPython) with and without %gui qt, and with %pylab, and noticed no problems. I could also verify that it does fix a problem that I have with modal dialogs started from the menu (only) with %gui qt with ipython master.

@fperez
Member
fperez commented Sep 6, 2012

Thanks a lot @hmeine, that's very useful feedback. I'd also like to hear from @jdmarch, @epatters and other Enthought folks who are heavy-duty Qt users, whether this improves things or causes any trouble. Once we're a bit more confident with in-the-field testing of this, it would be great to have it go in.

@epatters
Contributor
epatters commented Sep 6, 2012

After looking at the QCoreApplication source code, I agree with @bfroehle that this change seems unlikely to break anything. Even so, I have not tested it and it makes me a bit nervous.

From what I undertand, the main (only?) goal of this pull request is to suppress the aboutToQuit signal, which closes modal dialogs and has other obnoxious effects. There is a more direct way to accomplish this. After the timer fires, call app.aboutToQuit.disconnect() with no arguments to disconnect all slots connected to the signal. Then call app.quit() as before. (Disclaimer: I have not tested this approach.)

@bfroehle
Contributor
bfroehle commented Sep 7, 2012

@epatters: You are correct that the goal is to avoid the aboutToQuit signal. I tried your suggestion, but can't get it working:

  File "/home/bfroehle/src/ipython/IPython/lib/inputhookqt4.py", line 92, in inputhook_qt4
    app.aboutToQuit.disconnect()
TypeError: disconnect() failed between 'aboutToQuit' and all its connections
Got exception from inputhook_qt4, unregistering.

I'm not really sure what that means...

@epatters
Contributor
epatters commented Sep 7, 2012

I'm not sure what that means either. I wouldn't be surprised if its a bug in PyQt or PySide.

Given that my approach evidently doesn't work, I'm in favor of using a QEventLoop directly, rather than calling QCoreApplication::exec. Other than setting some state variables that don't seem to be used anywhere, the only thing QCoreApplication adds is the aboutToQuit signal, which is precisely what we don't want.

@epatters
Contributor
epatters commented Sep 7, 2012

Oh, but I do I think it is important to add a detailed comment explaining the motivation for this change. This really is an obscure thing...

@epatters
Contributor
epatters commented Sep 7, 2012

OK, I finally got around to testing this myself. I am now convinced that this PR won't break anything---but equally convinced that it will not fix issues like #2380. I'm not sure why the reporter of that issue cannot reproduce the problem; for me, modal dialogs are consistently broken under the conditions described in #2080, when running under either master or this PR.

Modal dialogs are typically opened by a blocking call to QDialog.exec_. If this is called directly from the IPython prompt, it blocks as expected. If, however, it is called from a Qt event handler (e.g., when the user clicks on something), it is terminated by the input hook in at most 50 ms. Here is now this happens, as far as I can tell. When QDialog.exec_ is called, it starts own event loop, but uses the same event dispatcher as our event loop (there is only one of these per thread). So when we terminate our event loop, we also cause the event loop of the dialog to terminate, thereby closing the dialog.

From this it is clear that the logic of the present input hook is wrong. When a blocking call is made from a Qt event handler, IPython must respect this. This means that the terminal prompt will hang until the call completes, but that simply cannot be avoided in the single process model.

After several false starts, I hit upon what appears to be a very simple solution to this problem. Rather than starting and stopping an event loop every 50 ms, just call processEvents with a 50 ms timeout:

while not stdin_ready():
    app.processEvents(QtCore.QEventLoop.AllEvents, 50)

Under normal circumstances, this seems to work as before, but blocking calls started from event handlers are not interrupted. In particular, issue #2380 is resolved.

I think this approach is promising, but more testing is required.

@bfroehle
Contributor
bfroehle commented Sep 8, 2012

@epatters Funny how this all comes full circle. The original implementation of the PyQt4 input hook did nothing more than call QCoreApplication::processEvents().

@epatters
Contributor
epatters commented Sep 8, 2012

@bfroehle: Well, this is a nightmare. For reference, here is the current PyQt4 input hook implementation, which seems to include all the suggestions in the linked email:

static int qtcore_input_hook()
{
    QCoreApplication *app = QCoreApplication::instance();

    if (app && app->thread() == QThread::currentThread())
    {
#if defined(Q_OS_WIN32)
        QTimer timer;
        QObject::connect(&timer, SIGNAL(timeout()), app, SLOT(quit()));

        do
        {
            timer.start(100);
            QCoreApplication::exec();
            timer.stop();
        }
        while (!_kbhit());

        QObject::disconnect(&timer, SIGNAL(timeout()), app, SLOT(quit()));
#else
        QSocketNotifier notifier(0, QSocketNotifier::Read, 0);
        QObject::connect(&notifier, SIGNAL(activated(int)), app, SLOT(quit()));
        QCoreApplication::exec();
        QObject::disconnect(&notifier, SIGNAL(activated(int)), app, SLOT(quit()));
#endif
    }

    return 0;
}

More to the point, the linked email indicates that processEvents only failed to work in environments without readline. However, if I am understanding the email correctly, our current approach should fail just as badly because it also returns from the input hook very quickly. It seems we will have to investigate this further, particularly on Windows.

@hmeine
Contributor
hmeine commented Sep 10, 2012

@epatters wrote:

[I am] equally convinced that it will not fix issues like #2380. I'm not sure why the reporter of that issue cannot reproduce the problem; for me, modal dialogs are consistently broken under the conditions described in #2080, when running under either master or this PR.

I was quite surprised, because I tested @bfroehle's inputhook_qt4_alt branch, and it made a difference for me.
So I just re-tested with exactly the steps you mentioned above (--pylab=qt and the save dialog), and

  • I see the problem in ipython/master, but
  • it works with bfroehle/inputhook_qt4_alt and
  • it works with bfroehle/inputhook_qt4.

I tried it multiple times, and had no problems with bfroehles fix(es).
Now one last thing I can imagine to make a difference is that I am using KDE, because Qt has this special handling of "native file dialogs". Anyhow, last time I successfully tested with my own pure Qt dialogs (opened from a menu bar), and I think the KDE file dialog is not out-of-process either.

@epatters
Contributor

@hmeine, you're probably right, then, that the behavior is platform specific. Here is an illuminating comment from the QFileDialog implementation on OS X:

void QFileDialogPrivate::mac_nativeDialogModalHelp()
{
    // Do a queued meta-call to open the native modal dialog so it opens after the new
    // event loop has started to execute (in QDialog::exec). Using a timer rather than
    // a queued meta call is intentional to ensure that the call is only delivered when
    // [NSApp run] runs (timers are handeled special in cocoa). If NSApp is not
    // running (which is the case if e.g a top-most QEventLoop has been
    // interrupted, and the second-most event loop has not yet been reactivated (regardless
    // if [NSApp run] is still on the stack)), showing a native modal dialog will fail.
    if (nativeDialogInUse){
        Q_Q(QFileDialog);
        QTimer::singleShot(1, q, SLOT(_q_macRunNativeAppModalPanel()));
    }
}

So interrupting the top-level event loop is a no-no on OS X. In general, I think we should, if possible, move away from this approach of constantly starting and stopping the event loop.

@bfroehle
Contributor

@epatters It'd be nice if Qt provided some guidance here.

Perhaps we should move back to a processEvents approach if readline is present. IIRC, IPython already performs pretty poorly when readline is not present, so perhaps we could just keep the existing implementation in that situation.

@epatters
Contributor

@bfroehle, yes, my feeling is that we should move back to processEvents if readline is present. Of course, we will have to test this on all three platforms and with a variety of applications.

I couldn't tell you how the present implementation performs when readline is absent. If it's also pretty broken, we should consider just telling people that readline is a dependency for GUI integration. Don't most users have readline anyway, even on Windows?

@hmeine
Contributor
hmeine commented Sep 19, 2012

Should this PR be closed then?

@epatters
Contributor

@hmeine, this PR fixes a problem for you, right? Then my vote would actually be to merge this, since it won't break anything that wasn't already broken.

There are deeper problems that remain unsolved, but I can't afford to spend any more time on this right now.

@hmeine
Contributor
hmeine commented Sep 19, 2012

That's right. On Linux, it does improve the situation.

@fperez
Member
fperez commented Sep 21, 2012

Just confirming here what I said in #2380: for me, this does fix #2380, which is IMO a major, critical bug as we're breaking the use of the Qt backend for matplotlib, as reported by @mdboom.

@epatters
Contributor

It seems to be a fix on Linux. On OS X, modal dialogs are still broken.

But yes, this should definitely be merged.

@Carreau
Member
Carreau commented Sep 30, 2012

As this seem to improve most cases I vote for merging this in.
I'll do it in the next 2 days if no one opposes.

@Carreau Carreau merged commit 36a4acb into ipython:master Nov 3, 2012

1 check passed

default The Travis build passed
Details
@minrk minrk added a commit that referenced this pull request Mar 5, 2013
@minrk minrk Backport PR #2294: inputhook_qt4: Use QEventLoop instead of starting …
…up the QCoreApplication

I referenced this branch in #2080 and was letting it sit for a little while, but I have decided to make it a full pull request to get some additional visibility.

Essentially our Qt event loop mechanism repeatedly starts and quits a `QCoreApplication` object. Unfortunately the `QCoreApplication::quit` slot has a lot of unintended side effects (like emitting an `aboutToQuit` signal which closes all open file dialogs).

For our input hook, we _might_ be able to get by with just using a `QEventLoop` whose quit slot is much simpler and less destructive.

For a little bit of background on why one might want to just use `QEventLoop::exec`, let's examine what `QCoreApplication::exec` does:

```c++
int QCoreApplication::exec()
{
    if (!QCoreApplicationPrivate::checkInstance("exec"))
        return -1;

    // ... [some assertions]

    threadData->quitNow = false;
    QEventLoop eventLoop;
    self->d_func()->in_exec = true;
    self->d_func()->aboutToQuitEmitted = false;
    int returnCode = eventLoop.exec();
    threadData->quitNow = false;
    if (self) {
        self->d_func()->in_exec = false;
        if (!self->d_func()->aboutToQuitEmitted)
            emit self->aboutToQuit();
        self->d_func()->aboutToQuitEmitted = true;
        sendPostedEvents(0, QEvent::DeferredDelete);
    }

    return returnCode;
}
```

As far as I can tell, it's a small wrapper around `QEventLoop::exec` which also:
* Sets some variables regarding the current status
* Emits an `aboutToQuit` signal right before the function returns (which is the root cause of @denisri's problem in #2080).

Historically, our Qt event loop is a python implementation of the (win 32) input hook supplied with the PyQt4 source (see qtcore_input_hook` in `python-qt4/sip/QtCore/qcoreapplication.sip`), which more or less dates to a [mailing list post](http://www.riverbankcomputing.com/pipermail/pyqt/2007-July/016512.html) from July 2007.
7119683
@efiring efiring referenced this pull request in matplotlib/matplotlib Nov 30, 2013
Closed

Qt4 save file dialog fails to appear on OSX #2630

@janschulz janschulz referenced this pull request in matplotlib/matplotlib Oct 4, 2015
Open

plt.plot(...) makes python repl in cmd slow #5159

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment