[WIP] Add waitSignals.

[The-Compiler]

After I was stuck with the other PR I tried adding something else I've been missing: The possibility to wait for multiple signals (until all have been emitted).

Here I also get a segfault:

pytestqt/_tests/test_wait_signal.py::test_signal_triggered[wait_function5-2000-500-False] PASSED
pytestqt/_tests/test_wait_signal.py::test_signal_triggered_multiple[wait_function0-5000-10000-20000-True] Fatal Python error: Segmentation fault

Current thread 0x00007f827e961700 (most recent call first):
  File "/home/florian/proj/pytest-qt/pytestqt/plugin.py", line 418 in wait
  File "/home/florian/proj/pytest-qt/pytestqt/_tests/test_wait_signal.py", line 32 in explicit_wait
  File "/home/florian/proj/pytest-qt/pytestqt/_tests/test_wait_signal.py", line 112 in test_signal_triggered_multiple
[...]

#0  0x0000555555d824a0 in ?? ()
#1  0x00007ffff3062921 in pyqtBoundSignal_emit (self=0x7ffff3371878, args=0x7ffff7fb8048) at ../qpy/QtCore/qpycore_pyqtboundsignal.cpp:633
#2  0x00007ffff79a9528 in PyObject_Call (func=func@entry=0x7fffe8f3fb08, arg=arg@entry=0x7ffff7fb8048, kw=<optimized out>) at Objects/abstract.c:2040
#3  0x00007ffff7a56867 in PyEval_CallObjectWithKeywords (func=0x7fffe8f3fb08, arg=0x7ffff7fb8048, kw=<optimized out>) at Python/ceval.c:4114
#4  0x00007ffff304e610 in PyQtSlot::call (this=0x555555e4b2e0, callable=0x7fffe8f3fb08, args=0x7ffff7fb8048) at ../qpy/QtCore/qpycore_pyqtslot.cpp:222
#5  0x00007ffff304e390 in PyQtSlot::invoke (this=0x555555e4b2e0, qargs=0x7fffffff73f0, self=0x0, result=0x0, no_receiver_check=false) at ../qpy/QtCore/qpycore_pyqtslot.cpp:147
#6  0x00007ffff304e0b6 in PyQtSlot::invoke (this=0x555555e4b2e0, qargs=0x7fffffff73f0, no_receiver_check=false) at ../qpy/QtCore/qpycore_pyqtslot.cpp:76
#7  0x00007ffff304fa2d in PyQtSlotProxy::unislot (this=0x555555e6a330, qargs=0x7fffffff73f0) at ../qpy/QtCore/qpycore_pyqtslotproxy.cpp:205
#8  0x00007ffff304f981 in PyQtSlotProxy::qt_metacall (this=0x555555e6a330, _c=QMetaObject::InvokeMetaMethod, _id=0, _a=0x7fffffff73f0) at ../qpy/QtCore/qpycore_pyqtslotproxy.cpp:170
#9  0x00007ffff2bbccea in QMetaObject::activate (sender=sender@entry=0x555555e50eb0, signalOffset=<optimized out>, local_signal_index=local_signal_index@entry=0, argv=argv@entry=0x0) at kernel/qobject.cpp:3733
#10 0x00007ffff2bbd257 in QMetaObject::activate (sender=sender@entry=0x555555e50eb0, m=m@entry=0x7ffff2dd90c0 <QSingleShotTimer::staticMetaObject>, local_signal_index=local_signal_index@entry=0, argv=argv@entry=0x0)
    at kernel/qobject.cpp:3583
#11 0x00007ffff2bca71b in timeout (this=0x555555e50eb0) at .moc/qtimer.moc:123
#12 QSingleShotTimer::timerEvent (this=0x555555e50eb0) at kernel/qtimer.cpp:318
#13 0x00007ffff2bbd8e3 in QObject::event (this=0x555555e50eb0, e=<optimized out>) at kernel/qobject.cpp:1268
#14 0x00007fffe9d3f646 in QApplicationPrivate::notify_helper (this=this@entry=0x555555d537f0, receiver=receiver@entry=0x555555e50eb0, e=e@entry=0x7fffffff7780) at kernel/qapplication.cpp:3717
#15 0x00007fffe9d44521 in QApplication::notify (this=0x555555d3a4c0, receiver=0x555555e50eb0, e=0x7fffffff7780) at kernel/qapplication.cpp:3161
#16 0x00007fffe92d9f18 in sipQApplication::notify (this=0x555555d3a4c0, a0=0x555555e50eb0, a1=0x7fffffff7780) at sipQtWidgetsQApplication.cpp:349
#17 0x00007ffff2b8c9fb in QCoreApplication::notifyInternal (this=0x555555d3a4c0, receiver=0x555555e50eb0, event=event@entry=0x7fffffff7780) at kernel/qcoreapplication.cpp:963
#18 0x00007ffff2be4aad in sendEvent (event=0x7fffffff7780, receiver=<optimized out>) at ../../include/QtCore/../../src/corelib/kernel/qcoreapplication.h:224
#19 QTimerInfoList::activateTimers (this=0x555555d79830) at kernel/qtimerinfo_unix.cpp:637
#20 0x00007ffff2be4ef1 in timerSourceDispatch (source=<optimized out>) at kernel/qeventdispatcher_glib.cpp:177
#21 0x00007ffff16b39fd in g_main_context_dispatch () from /usr/lib/libglib-2.0.so.0
#22 0x00007ffff16b3ce0 in ?? () from /usr/lib/libglib-2.0.so.0
#23 0x00007ffff16b3d8c in g_main_context_iteration () from /usr/lib/libglib-2.0.so.0
#24 0x00007ffff2be5bcc in QEventDispatcherGlib::processEvents (this=0x555555d79550, flags=...) at kernel/qeventdispatcher_glib.cpp:418
#25 0x00007fffe87eb94c in QPAEventDispatcherGlib::processEvents (this=<optimized out>, flags=...) at eventdispatchers/qeventdispatcher_glib.cpp:115
#26 0x00007ffff2b8a3f2 in QEventLoop::exec (this=0x555555b577a0, flags=...) at kernel/qeventloop.cpp:204
[...]

[nicoddemus]

Thanks @The-Compiler! As I said in #42, I'm a little short on time today, but will do my best to work on this tomorrow! 😄

Cheers 🍻

[The-Compiler]

Cleaned up stacktraces:

PyQt4/QtCore.so( 0x17a7b1)[0x7fd07325a7b1]
python2.7(PyObject_Call 0x36)[0x43a8b6]
python2.7(PyEval_CallObjectWithKeywords 0x36)[0x43b626]
sip.so(sip_api_invoke_slot 0x1a0)[0x7fd071fbaf80]
PyQt4/QtCore.so( 0x17d396)[0x7fd07325d396]
PyQt4/QtCore.so( 0x17d690)[0x7fd07325d690]
PyQt4/QtCore.so( 0x17d772)[0x7fd07325d772]
libQtCore.so.4(QMetaObject::activate(QObject*, QMetaObject const*, int, void**) 0x4b9)[0x7fd072d97489]
libQtCore.so.4( 0x198a4f)[0x7fd072da0a4f]
libQtCore.so.4(QObject::event(QEvent*) 0x99)[0x7fd072d9c179]
libQtGui.so.4(QApplicationPrivate::notify_helper(QObject*, QEvent*) 0xb4)[0x7fd070bb3894]
libQtGui.so.4(QApplication::notify(QObject*, QEvent*) 0x113)[0x7fd070bb8713]
PyQt4/QtGui.so( 0x4b3a99)[0x7fd071b6ba99]
libQtCore.so.4(QCoreApplication::notifyInternal(QObject*, QEvent*) 0x8c)[0x7fd072d82e9c]
libQtCore.so.4( 0x1ac1f2)[0x7fd072db41f2]
libQtCore.so.4( 0x1a9c0d)[0x7fd072db1c0d]
libQtCore.so.4( 0x1a9c31)[0x7fd072db1c31]
libglib-2.0.so.0(g_main_context_dispatch 0x133)[0x7fd07244fd13]
libglib-2.0.so.0( 0x48060)[0x7fd072450060]
libglib-2.0.so.0(g_main_context_iteration 0x34)[0x7fd072450124]
libQtCore.so.4(QEventDispatcherGlib::processEvents(QFlags<QEventLoop::ProcessEventsFlag>) 0x6f)[0x7fd072db23bf]
libQtGui.so.4( 0x273d9e)[0x7fd070c5bd9e]
libQtCore.so.4(QEventLoop::processEvents(QFlags<QEventLoop::ProcessEventsFlag>) 0x32)[0x7fd072d81c82]
libQtCore.so.4(QEventLoop::exec(QFlags<QEventLoop::ProcessEventsFlag>) 0xf7)[0x7fd072d81ed7]

python2.7(PyObject_Call 0x9)[0x43a889]
python2.7(PyEval_CallObjectWithKeywords 0x36)[0x43b626]
libpyside-python2.7.so.1.1( 0x17c14)[0x7f5b0692fc14]
python2.7(PyObject_Call 0x36)[0x43a8b6]
python2.7(PyEval_CallObjectWithKeywords 0x36)[0x43b626]
libpyside-python2.7.so.1.1(PySide::SignalManager::callPythonMetaMethod(QMetaMethod const&, void**, _object*, bool) 0x4a)[0x7f5b069272da]
libpyside-python2.7.so.1.1( 0x14dd7)[0x7f5b0692cdd7]
libQtCore.so.4(QMetaObject::activate(QObject*, QMetaObject const*, int, void**) 0x4b9)[0x7f5b05e9f489]
PySide/QtCore.so( 0x192c03)[0x7f5b06cd2c03]
libQtCore.so.4(QObject::event(QEvent*) 0x99)[0x7f5b05ea4179]
PySide/QtCore.so( 0x191a6b)[0x7f5b06cd1a6b]
libQtGui.so.4(QApplicationPrivate::notify_helper(QObject*, QEvent*) 0xb4)[0x7f5b03a73894]
libQtGui.so.4(QApplication::notify(QObject*, QEvent*) 0x113)[0x7f5b03a78713]
PySide/QtGui.so( 0x1e8b26)[0x7f5b04760b26]
libQtCore.so.4(QCoreApplication::notifyInternal(QObject*, QEvent*) 0x8c)[0x7f5b05e8ae9c]
libQtCore.so.4( 0x1ac1f2)[0x7f5b05ebc1f2]
libQtCore.so.4( 0x1a9c0d)[0x7f5b05eb9c0d]
libQtCore.so.4( 0x1a9c31)[0x7f5b05eb9c31]
libglib-2.0.so.0(g_main_context_dispatch 0x133)[0x7f5b05557d13]
libglib-2.0.so.0( 0x48060)[0x7f5b05558060]
libglib-2.0.so.0(g_main_context_iteration 0x34)[0x7f5b05558124]
libQtCore.so.4(QEventDispatcherGlib::processEvents(QFlags<QEventLoop::ProcessEventsFlag>) 0x6f)[0x7f5b05eba3bf]
libQtGui.so.4( 0x273d9e)[0x7f5b03b1bd9e]
libQtCore.so.4(QEventLoop::processEvents(QFlags<QEventLoop::ProcessEventsFlag>) 0x32)[0x7f5b05e89c82]
libQtCore.so.4(QEventLoop::exec(QFlags<QEventLoop::ProcessEventsFlag>) 0xf7)[0x7f5b05e89ed7]

Unfortunately not too detailed...

[The-Compiler]

There we go, the tests pass with the QTimer fixes from #42 😄

❗ Please don't merge this yet - I'll test #42 with that fix as well, then open a separate PR for the fix, rebase and clean everything up.

[nicoddemus]

Nice! 😄

While I like the idea of waiting for multiple signals, there is duplicated logic in MultiSignalBlocker and SignalBlocker, and I would like to avoid that if possible. Perhaps we can get away to just just extend SignalBlocker to support multiple signals instead?

  # wait a single signal
  qtbot.waitSignal(signal)
  # wait multiple signals
  qtbot.waitSignal([signal1, signal2])

[The-Compiler]

I could do that as well - I originally thought of that but went for the other approach for the following reasons:

• Some people consider having a function which behaves differently based on if you pass an iterable or not "unpythonic" and prefer having two different functions.
• It's more clear when reading the documentation which kind of waiting is a logical OR and which is a logical AND. Currently the docs say blocker.connect(app.worker.failed) # Can add other signals to blocker - I think it'd be unclear that passing multiple signals means all of them has been triggered, but adding them via blocker.connect() means any of them has to be triggered.
• I didn't want to change the original logic to use the dict approach to make sure it works exactly like before.

What do you think about adding a waitSignals on SignalBlocker, and if that's used, using blocker.connect raises an exception? Then there's a clear distinction but less duplication.

[nicoddemus]

You raise some very valid points, thanks.

What do you think about adding a waitSignals on SignalBlocker, and if that's used, using blocker.connect raises an exception? Then there's a clear distinction but less duplication.

IIUC, you mean a user either uses connect/wait or add_signal/wait_signals? Or did you mean adding waitSignals to QtBot?

[The-Compiler]

Sorry, yes - I meant adding waitSignals to QtBot. I'll just write down some thoughts:

• Add a _signals dict to SignalBlocker and use the MultiSignalBlocker logic.
• Add SignalBlocker.add_signal which adds a new signal to the dict (logical AND)
• Add QtBot.waitSignals which works like waitSignal but takes an iterable, and calls add_signal for each signal.

However, what would SignalBlocker.connect do then? It can't add the signal to the dict as that'd be AND instead of OR, so the dict and the list would be needed, with connect/add_signal making sure they're mutually exclusive. (Otherwise, what would add_signal then connect do?).

At this point, I think the details get too complicated, and it's better to make a clear distinction and have two kinds of blockers.

The best idea I can think of to reduce code duplication is to have an AbstractSignalBlocker which only provides the common part of __init__ (sans _signals), wait, __enter__ and __exit__, and then have a SignalBlocker with connect and _quit_loop_by_signal, and a MultiSignalBlocker with add_signal and _signal_emitted. What do you think?

[nicoddemus]

I agree, having a commom base implementation just to share code seems to be the best approach here.

Feel free if you want to share the work here though (letting me worry about the refactor, for example), my main concern is to nail down the public interface correctly... we can always refactor that later. 😄

[The-Compiler]

I cleaned the previous commit up and added ad507a4 where I refactored the code so a common AbstractSignalBlocker base is used by SignalBlocker and MultiSignalBlocker.

I still prefer the public interface like this, i.e. having two separate things:

• qtbot.waitSignal() which takes a single signal and uses a SignalBlocker which provides blocker.connect() to add more signals (OR)
• qtbot.waitSignals() which takes an iterable of signals and uses a MultiSignalBlocker which provides blocker.add_signal() to add another signal (AND).

This way, there's hopefully less user confusion (AND/OR) and less corner cases (connect with multiple signals), and we can be pretty sure SignalBlocker behaves like before.

But if you have other ideas, let me know and we can discuss!

Two things I'm not sure about:

• How will the docs look like? Will signal_triggered from AbstractSignalBlocker be documented in SignalBlocker/MultiSignalBlocker docs?
• Should test_signal_triggered and test_signal_triggered_multiple be merged? Though I think when adding raising-tests there as well, this will get rather messy.

[nicoddemus]

I agree that having both waitSignal and waitSignals is the better approach, as we want to return different objects anyway (BTW, the fact that the only difference between the two method names is the s worries me a little... do you think maybe waitAllSignals or something like this would be too ugly? Not a big deal if you are fond of the name though).

Perhaps we can remove add_signal() altogether from the public API? I don't see any use-cases for it, other to have a method with similar purpose as SignalBlocker.connect. I'm trying to be cautious because it is OK to add this later if we want to, but once released we may not remove it without a 2.0 release. (In retrospect, I'm not sure having connect was a good idea, it would be better to have waitSignal which received a single signal or an iterable of signals with OR semantics, but alas).

• How will the docs look like? Will signal_triggered from AbstractSignalBlocker be documented in SignalBlocker/MultiSignalBlocker docs?

I think it is possible to document it in AbstractSignalBlocker but ask Sphinx to not generate docs for it (as it is an implementation detail anyway and I don't want to expose it) and instead let Sphinx duplicate it in both subclasses. But don't worry about it, I can research how to do this later. 😄

• Should test_signal_triggered and test_signal_triggered_multiple be merged? Though I think when adding raising-tests there as well, this will get rather messy.

I agree, it will probably get messy. Let's keep them separated, even at the cost of of some code duplication. I will refactor them later to share more code.

(Sorry if this is taking longer/more trouble than you expected, I want to sure to nail down a nice public API before this goes out 😄)

[The-Compiler]

BTW, the fact that the only difference between the two method names is the s worries me a little... do you think maybe waitAllSignals or something like this would be too ugly? Not a big deal if you are fond of the name though

Hmm, I have to say I don't really like waitAllSignals. On the other hand, the method name would make it perfectly clear it waits for all signals, rather than just any of them. Not sure what to pick really 😆

Perhaps we can remove add_signal() altogether from the public API? I don't see any use-cases for it, other to have a method with similar purpose as SignalBlocker.connect.

Same here - I only really added it because connect was there. Let's remove it.

I think it is possible to document it in AbstractSignalBlocker but ask Sphinx to not generate docs for it (as it is an implementation detail anyway and I don't want to expose it) and instead let Sphinx duplicate it in both subclasses. But don't worry about it, I can research how to do this later. 😄

Thanks! Would you mind also taking care of removing add_signal() and adding the raising parameter? The latter is trivial other than the tests I think, and since you're refactoring the tests anyways, I think it'd make more sense if you could do that as well.

(Sorry if this is taking longer/more trouble than you expected, I want to sure to nail down a nice public API before this goes out 😄)

No worries! It's always better to get public APIs right instead of rushing, and it's not like I desparately need it, I'm still busy with other tests in qutebrowser which don't need that functionality.

[nicoddemus]

Hmm, I have to say I don't really like waitAllSignals.

No problem, let's go with your original proposal then. But if you think of a name you like and makes it explicit about waiting all signals, let me know.

Would you mind also taking care of removing add_signal() and adding the raising parameter?

Sure, I will also take care of the docs. 😄

I will only be able to tackle this next week though, as I'm gonna travel this weekend. 😅

[nicoddemus]

Merged! Took a little work to refactor all the tests in a manner to cover all cases, but I think it was worth it. 😅

Thanks again @The-Compiler, your interest and contributions are greatly appreciated! 😄

[The-Compiler]

Thanks for taking over - looks good to me!

I just opened #51 with an additional small test - after that, what are your plans regarding releasing 1.4.0? I'd certainly appreciate it, and I think I'm done with PRs until there's something new I miss 😉

[nicoddemus]

As soon as I finish #40... Hopefully by the end of this week. It's nearly done, just need a little more time to finish it. 😄

[The-Compiler]

Awesome, that's going to be a great release 😄

Take your time, and let me know if you still need my help/opinion on anything - I lost track a bit 😊

[nicoddemus]

Sure thing, thank you! 😄