Rewrite and greatly simplify qt_compat.py. #9993
Conversation
df8b780 to
333a453
Compare
|
attn @fariza |
|
I am 👍 on this in principle. I gave it a quick read and it seems on the right path but do not have time right now to test with the full matrix of qt5 / qt4 / all the bindings. Thoughts on using https://pypi.python.org/pypi/QtPy to simply this is well? Picks up an extra dependency, but I think will let us drop a bunch of code and integrate better with other libraries that also import Qt. |
|
spyder-ide/qtpy#84 added pyside2 support xref #6118 |
|
I don't think qtpy really helps, most of the logic in qt_compat is to decide how the already imported modules, the backend/backend.qt4/5 rcParams, the QT_API env var, and the actually importable modules interact to decide what binding to use; AFAICT QtPy only considers the last two points. QtPy also seems not to support setting the old v1 API for PyQt4 -- not that I really care about it :-) but if we were to drop support for it then qt_compat could be simplified accordingly. |
|
I am happy to merge this as-is (once we get the matrix tested) and punt on qtpy usage. I think the biggest selling point of switching to qtpy is (possibly) seamless integration with other libraries / tools that use qtpy. |
|
They can still use qtpy... More specifically, if they import first qtpy and then matplotlib, then mpl will use whatever binding got loaded by qtpy. If they first import matplotlib (or just anything that imports some qt binding first) and then qtpy, qtpy will not check what got loaded and we may end up with two different bindings: I would consider this an issue on qtpy's side. Switching to using qtpy ourselves only shifts the issue: in that case someone who first imports, say, PyQt5 but (mis)configured matplotlib to use PySide will run into the same issue as above (but this time we get the blame instead of qtpy). |
55a9d88 to
fdee996
Compare
|
Also removed qt4agg/qt5agg api docs as their shimming is insufficient with this new qt_compat. Note that the gtkagg api docs are in the same state. |
679db1f to
37ac201
Compare
05c4747 to
7b75f59
Compare
|
|
||
| # Available APIs. | ||
| QT_API_PYQT = 'PyQt4' # API is not set here; Python 2.x default is V 1 |
There was a problem hiding this comment.
The comments seem useful. I would keep them.
There was a problem hiding this comment.
Added back a comment re: v1 API as v2 is the default on Py3.
|
@anntzer Do you mind rebasing? There's currently a conflict. |
| dict.__setitem__(rcParams, "backend.qt5", QT_API_PYQT5) | ||
| elif QT_API_ENV == "pyside2": | ||
| dict.__setitem__(rcParams, "backend.qt5", QT_API_PYSIDE2) | ||
| QT_API = dict.__getitem__(rcParams, "backend.qt5") |
There was a problem hiding this comment.
Is this correct even if QT_API_ENV == 'foo'?
There was a problem hiding this comment.
then we just respect whatever rcParams["backend.qt5"] originally was.
lib/matplotlib/backends/qt_compat.py
Outdated
| # http://pyqt.sourceforge.net/Docs/PyQt4/incompatible_apis.html | ||
| _sip_apis = ["QDate", "QDateTime", "QString", "QTextStream", "QTime", | ||
| "QUrl", "QVariant"] | ||
| import sip |
There was a problem hiding this comment.
move import sip into the try-except as done in @tacaswell s commit to #11600.
There was a problem hiding this comment.
Not clear why this would be needed? The way this is currently written, if QT_API is set then we try that binding exactly (and fail with ImportError if not available, which includes sip not being available when PyQt4 is requested); if not (QT_API is None, which would only occur in a fully manual embedding while rcParams["backend"] is not even "qt4agg" or "qt5agg"), then we just try everything in the loop at the bottom where each iteration is protected by a try... except ImportError.
There was a problem hiding this comment.
Because riverbank (in the last planned pyqt4 release) made sip a 'private' library such that before you import pyqt import sip fails.
I am a bit unclear how to set the sip api in that case, but...
There was a problem hiding this comment.
I am not sure what you refer to; I can't repro with PyQt4 installed from conda (it's not available as a pypi wheel) or PyQt5 from either conda or pypi.
lib/matplotlib/backends/qt_compat.py
Outdated
| else: | ||
| raise ImportError("Failed to import any qt binding") | ||
| else: | ||
| raise AssertionError # We should not get there. |
There was a problem hiding this comment.
raise AssertionError("Invalid QT_API: '%s'" % QT_API)
Just in case. We then know at least what the unexpected value was.
lib/matplotlib/backends/qt_compat.py
Outdated
| def is_pyqt5(): | ||
| return QT_API == QT_API_PYQT5 | ||
| # These globals are only defined for backcompatibilty purposes. | ||
| ETS = dict(pyqt=(QT_API_PYQTv2, 4), pyside=(QT_API_PYSIDE, 4), |
There was a problem hiding this comment.
Copy the comments from the original ETS definition?
There was a problem hiding this comment.
The dict is basically irrelevant now and only left for backcompat; there's a similar comment at the top ("Otherwise check the QT_API environment variable etc.").
| @@ -46,7 +46,7 @@ | ||
| import warnings | ||
|
|
||
| from matplotlib import colors as mcolors | ||
| from matplotlib.backends.qt_compat import QtGui, QtWidgets, QtCore | ||
| from ..qt_compat import QtCore, QtGui, QtWidgets | ||
There was a problem hiding this comment.
Is there a particular reason to use relative imports. I think to remember that absolute imports are to be preferred in general. but I'm happy to learn. 😄
There was a problem hiding this comment.
because I'd rather have a block of
from .axes import Axes
from .lines import Line2D
...
than additionally repeating matplotlib in front of each line (we have pretty huge import blocks). OTOH this specific change wasn't really needed (I usually just do it in conjunction with other changes) so I reverted it.
|
Comments handled (just made the assert more explicit and reverted the change to formlayout). |
The selection logic is now described in the module's docstring. The only changes is that the QT_ENV_MAJOR_VERSION global, which would sometimes be defined (depending on the state of the import cache, the QT_API environment variable, and the requested backend) is never defined anymore.
There was a problem hiding this comment.
Modulo the sip import. I would leave this for @tacaswell to decide.
|
@tacaswell I re-approve your commit. Is it ok to merge on this basis? |
|
Yes, @timhoffm you can push the button if you want :) |
|
There seem to be a conflict, please backport manually |
Rewrite and greatly simplify qt_compat.py.
Conflicts:
INSTALL.rst
- kept changes away from App specific wording
- kept min pyqt4 version bump
doc/sphinxext/mock_gui_toolkits.py
- only removed setting up the Qt mocks
lib/matplotlib/backends/qt_compat.py
- kept backported version, conflict was in block of
constants at top (all of the contestants are still
defined in the module)
The selection logic is now described in the module's docstring. I think it is essentially unchanged...
The QT_ENV_MAJOR_VERSION global, which would sometimes be defined (depending on the state of the import cache, the QT_API environment variable, and the requested backend) is never defined anymore.
PR Summary
PR Checklist