Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
Rewrite and greatly simplify qt_compat.py.#9993
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
df8b780 to333a453Compareattn@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 usinghttps://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 tofdee996CompareAlso 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 to37ac201Compare05c4747 to7b75f59Compare| # 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The comments seem useful. I would keep them.
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.
Added back a comment re: v1 API as v2 is the default on Py3.
@anntzer Do you mind rebasing? There's currently a conflict. |
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.
Trying to get this forward to untangle#11600.
| 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Is this correct even ifQT_API_ENV == 'foo'?
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
moveimport sip into the try-except as done in@tacaswell s commit to#11600.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Because riverbank (in thelast planned pyqt4 release) made sip a 'private' library such that before you import pyqtimport sip fails.
I am a bit unclear how to set the sip api in that case, but...
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.
I thought that was pyqt5?
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Copy the comments from the original ETS definition?
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.
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 | |||
| frommatplotlib.backends.qt_compat importQtGui, QtWidgets, QtCore | |||
| from..qt_compat importQtCore, QtGui, QtWidgets | |||
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
because I'd rather have a block of
from .axes import Axesfrom .lines import Line2D...than additionally repeatingmatplotlib 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. Theonly changes is that the QT_ENV_MAJOR_VERSION global, which wouldsometimes be defined (depending on the state of the import cache, theQT_API environment variable, and the requested backend) is never definedanymore.
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.
Modulo the sip import. I would leave this for@tacaswell to decide.
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.
I approve, but have a commit in this.
@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 bumpdoc/sphinxext/mock_gui_toolkits.py - only removed setting up the Qt mockslib/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)
Uh oh!
There was an error while loading.Please reload this page.
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