Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix GUI inputhook for qt 5.15.0 #12355

Merged
merged 1 commit into from Jun 5, 2020

Conversation

tlambert03
Copy link
Contributor

@tlambert03 tlambert03 commented May 31, 2020

this fixes #12350

minimal code that reproduces the error:

import asyncio
from prompt_toolkit.eventloop import new_eventloop_with_inputhook
from IPython.terminal.pt_inputhooks.qt import inputhook

async def simple():
    await asyncio.sleep(0.1)

pt_loop = new_eventloop_with_inputhook(inputhook)
asyncio.set_event_loop(pt_loop)
for i in range(10):
    pt_loop.run_until_complete(simple())

If you run that script on it's own, you get to see the error that PySide is throwing, which is:

TypeError: Can't call meta function because I have no idea how to handle QSocketDescriptor

(in a normal ipython --gui qt session, the error is not invisible... the interpreter just doesn't accept any input)

changing the way notifier.activated is connected to event_loop.exit fixes the issue. I can't say I completely understand why event_loop.exit is no longer a valid callback for the connection in 5.15 ... (though I have seen this seemingly unnecessary lambda help in the past with Qt)

I'm not sure how to add a test for this, as it seems inputhooks in general aren't being tested?

@tacaswell
Copy link
Contributor

tacaswell commented May 31, 2020

My guess is that:

  • the signature of the activated signal actually provides the notifier and a type to the slot (https://doc.qt.io/qt-5/qsocketnotifier.html#activated)
  • the python layer is doing some inspection on the connected signal to wrap it in a shim if needed
  • Functions defined via the c-api may not have signature that can be introspected (ex inspect.signature('bob'.format) fails)
  • There was a change from defaulting to wrapping to not wrapping or something about the way the wrapping was done.

Might be better to register lambda notifier, ntype: event_loop.exit() instead?

@tlambert03
Copy link
Contributor Author

tlambert03 commented May 31, 2020

ahh, that makes a lot of sense. I haven't tested it yet, but I wonder whether adding notifier, ntype to the lambda might break previous versions? Looks like the signal is now overloaded and type was added to the signature in 5.15... not there in 5.14

@tlambert03
Copy link
Contributor Author

tlambert03 commented Jun 1, 2020

ok, looks like lambda notifier, ntype: event_loop.exit() will not work on 5.15.
In fact, unlike what the docs seem to specify, the slot attached to notifier.activated is not passed any parameters (i.e. the socket identifier is not passed), so connecting a function that takes any parameters fails:

# these all fail
notifier.activated.connect(lambda socket, ntype: event_loop.exit())
notifier.activated.connect(lambda socket: event_loop.exit())
def _exit(*args):
    event_loop.exit()
notifier.activated.connect(_exit)

in qt 5.14.x, the slot was passed the socket identifier as a positional argument, so this was legal:

notifier.activated.connect(lambda socket: print(socket))

because notifier.activated is now overloaded... it seems one might conceivably be able to use syntax like notifier.activated[socket_descriptor].connect ... but that does NOT work... and indeed QSocketDescriptor is not public anyway... so this seems like an overload that might be inaccessible in the python API?

I think the fact that event_loop.exit takes an optional positional argument is the reason the error is now being thrown on 5.15... (and as a sidenote: this means that the socket identifier is actually being used as the returnCode for the event_loop.exit() call in the current master).

so the only syntax that works in both 5.15 and earlier versions is how this PR currently has it:

notifier.activated.connect(function_that_takes_no_arguments)

@tacaswell
Copy link
Contributor

tacaswell commented Jun 1, 2020

Yikes, glad my speculation at least pointed you in the right direction!

If I'm understanding this right, the issue is that pyside is failing inside of its dispatch logic? This probably should be reported upstream as a regression.

@tlambert03
Copy link
Contributor Author

tlambert03 commented Jun 1, 2020

This probably should be reported upstream as a regression.

Thanks! I'll see what I can do

@tlambert03
Copy link
Contributor Author

tlambert03 commented Jun 1, 2020

https://bugreports.qt.io/browse/PYSIDE-1317

@Carreau
Copy link
Member

Carreau commented Jun 3, 2020

Do you think we needs this kind of workaround in IPython still ?
If so it would be nice to have either a pyqt version check just before and/or a comment to indicate that this is a workaround or it may get refactor later.

@tlambert03
Copy link
Contributor Author

tlambert03 commented Jun 3, 2020

looks like it might be fixed in 5.15.1... so it may not be permanently required.

that said, it's worth pointing out that while the previous code (notifier.activated.connect(event_loop.exit)) did work just fine ... it was technically using the socket descriptor as the exit return code, which is probably not strictly what was intended, since a returnCode of 0 means success, and any non-zero value indicates an error. so in a way, this isn't entirely a workaround... it's also about discarding the argument to the exit() call.

@tacaswell
Copy link
Contributor

tacaswell commented Jun 4, 2020

Awesome, thanks for working with upstream here @tlambert03 !

I think this should go in anyway, passing the notifier to loop.exit is a bit silly....

@Carreau Carreau added this to the 7.16 milestone Jun 5, 2020
@Carreau Carreau merged commit 35c17e9 into ipython:master Jun 5, 2020
2 checks passed
meeseeksmachine pushed a commit to meeseeksmachine/ipython that referenced this issue Jun 5, 2020
Carreau added a commit to Carreau/ipython that referenced this issue Jun 5, 2020
Carreau added a commit that referenced this issue Jun 7, 2020
…355-on-7.x

Backport PR #12355 on branch 7.x (Fix GUI inputhook for qt 5.15.0)
@tlambert03 tlambert03 deleted the fix/pyside5-15 branch Aug 31, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants