-
-
Notifications
You must be signed in to change notification settings - Fork 375
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
ENH: add %gui
support for Qt6
#1054
Conversation
- Distinguish between specific version requests and the generic one. - Use a `QEventLoop` instance to properly keep windows open between event loop calls. This is "instrumented" with print statements to follow the flow
This way import errors show up in the client, not the kernel.
380d5ab
to
f772d80
Compare
for more information, see https://pre-commit.ci
Thanks for working on this @shaperilio! @ccordoba12, mind taking a look at this and/or testing against Spyder? |
In case it helps, I've published a notebook I used to test functionality. Each "case" should be run on a fresh kernel. It would be nice to turn these into unit tests, but I'm afraid setting those up is over my head. I'd be happy to take a stab at it if someone can point me in the right direction. |
I'm playing with this in Linux now and have become aware that Effectively, the logic here is not in |
https://github.com/matplotlib/matplotlib/blob/1d2abe7ab909ebca32908694fc3e3828c37cf11d/lib/matplotlib/tests/test_backends_interactive.py#L24-L68 this and |
I wrote a bit of a unit test, but it won't run unless we make the CI install a couple of versions of either PyQt or PySide. How do I do that / do we want that? |
Importing a second version of Qt is not allowed. `IPython` silently ignores requests for different versions; we want `enable_gui` to raise an exception so the user can see it.
We can create a coverage matrix: diff --git a/pyproject.toml b/pyproject.toml
index 9904b54..05be2c2 100644
--- a/pyproject.toml
+++ b/pyproject.toml
@@ -62,6 +62,7 @@ cov = [
"curio",
"trio",
]
+pyqt5 = ["pyqt5"]
[tool.hatch.version]
path = "ipykernel/_version.py"
@@ -92,6 +93,15 @@ features = ["test", "cov"]
test = "python -m pytest -vv --cov ipykernel --cov-branch --cov-report term-missing:skip-covered {args}"
nowarn = "test -W default {args}"
+[[tool.hatch.envs.cov.matrix]]
+qt = ["qt5", "qt6"]
+
+[tool.hatch.envs.cov.overrides]
+matrix.qt.features = [
+ { value = "pyqt5", if = ["qt5"] },
+ { value = "pyqt6", if = ["qt6"] },
+]
+
[tool.hatch.envs.typing]
features = ["test"]
dependencies = ["mypy>=0.990"] So when you call |
You can run just one variant locally as |
OK, I think this is ready! I've put code in this pull request to IPython which replicates the behavior here. The only difference is that, in IPython, 'qt4' is an alias to 'qt', so you can never explicitly get a Qt4 / PySide event loop there. That's not the case here. I left the behavior in IPython because I don't know the history of that alias. |
@blink1073 sorry about that - I had removed the windows skips because those tests passed fine on my windows machine. |
for more information, see https://pre-commit.ci
No worries! Not sure why they hang in CI then, but things like that happen for other platforms too. 😄 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great, thanks! I'll leave it open for a few days in case anyone else gets a chance to review.
@blink1073, this PR broke the |
Yanked |
Thanks @blink1073! And sorry for not reviewing it on time, I'm finishing a Spyder release right now. |
I'd love to add spyder as a downstream test, how feasible do you think that is? |
That'd be great! Thanks for the offer. I think adding our Spyder-kernels test suite shouldn't be that hard. As for the Spyder tests (which also exercise our kernel under different scenarios), we could only run a file called |
BTW, I found a simple way to solve the regressions introduced by this PR. I hope to submit a fix in the next couple of days, after testing that change against our two test suites. |
Great, thanks! |
Thanks y'all; @ccordoba12, let me know if I can be of any help. |
Addresses #13859 Changes here parallel the changes in [ipykernel](ipython/ipykernel#1054), i.e. prefer `PyQt` over `PySide` and allow requesting explicit versions (e.g. 'qt5') with one difference (see below). I believe that, eventually, the Qt importing logic should be all here, and `ipykernel` should defer to`ipython`. I chose not to do that at this time since it would mean the latest `ipykernel` would require the latest `IPython`; I'm happy to discuss further. The only difference between `IPython` and `ipykernel`, after these two pull requests, is that, in `IPython`, it's not possible to explicitly request an event loop for Qt4. This is because an alias exists [here](https://github.com/ipython/ipython/blob/5409de68d87ddd073a35111aca0cb8360ff63ca8/IPython/terminal/pt_inputhooks/__init__.py#L5) which effectively makes "qt4" be "the latest Qt available". I did not remove the alias because I don't know the history behind it.
Addresses #775
PyQt
orPySide
. Replicated logic fromIPython
(e.g. preferPyQt
overPySide
)enable_gui
so that any import errors show up in the client as opposed to in the kernel's console.