Rewrite and greatly simplify qt_compat.py. #9993

anntzer · 2017-12-13T08:01:42Z

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

Has Pytest style unit tests
Code is PEP 8 compliant
New features are documented, with examples if plot related
Documentation is sphinx and numpydoc compliant
Added an entry to doc/users/next_whats_new/ if major new feature (follow instructions in README.rst there)
Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way

tacaswell · 2017-12-13T16:49:25Z

attn @fariza

tacaswell · 2017-12-13T16:52:50Z

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.

tacaswell · 2017-12-13T16:57:01Z

spyder-ide/qtpy#84 added pyside2 support xref #6118

anntzer · 2017-12-13T18:39:27Z

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.

tacaswell · 2017-12-13T18:45:49Z

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.

anntzer · 2017-12-13T18:53:52Z

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:

$ QT_API=pyside python -c 'import PyQt5.QtGui; from qtpy import QtWidgets; print(QtWidgets.QApplication)'
<class 'PySide.QtGui.QApplication'>

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).

anntzer · 2017-12-13T22:14:21Z

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.
As argued somewhere else I believe the interactive backend docs should be rewritten manually anyways.

timhoffm · 2018-07-10T23:36:48Z

lib/matplotlib/backends/qt_compat.py


-# Available APIs.
-QT_API_PYQT = 'PyQt4'       # API is not set here; Python 2.x default is V 1


The comments seem useful. I would keep them.

Added back a comment re: v1 API as v2 is the default on Py3.

NelleV · 2018-07-14T16:35:12Z

@anntzer Do you mind rebasing? There's currently a conflict.

timhoffm

Trying to get this forward to untangle #11600.

timhoffm · 2018-07-27T23:19:01Z

lib/matplotlib/backends/qt_compat.py

+        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")


Is this correct even if QT_API_ENV == 'foo'?

then we just respect whatever rcParams["backend.qt5"] originally was.

timhoffm · 2018-07-27T23:28:15Z

lib/matplotlib/backends/qt_compat.py

+        # http://pyqt.sourceforge.net/Docs/PyQt4/incompatible_apis.html
+        _sip_apis = ["QDate", "QDateTime", "QString", "QTextStream", "QTime",
+                     "QUrl", "QVariant"]
+        import sip


move import sip into the try-except as done in @tacaswell s commit to #11600.

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.

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...

I thought that was pyqt5?

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.

timhoffm · 2018-07-27T23:35:05Z

lib/matplotlib/backends/qt_compat.py

+    else:
+        raise ImportError("Failed to import any qt binding")
+else:
+    raise AssertionError  # We should not get there.


raise AssertionError("Invalid QT_API: '%s'" % QT_API)

Just in case. We then know at least what the unexpected value was.

timhoffm · 2018-07-27T23:38:37Z

lib/matplotlib/backends/qt_compat.py

-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),


Copy the comments from the original ETS definition?

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.").

timhoffm · 2018-07-27T23:44:29Z

lib/matplotlib/backends/qt_editor/formlayout.py

@@ -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


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. 😄

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.

anntzer · 2018-07-28T18:18:00Z

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.

timhoffm

Modulo the sip import. I would leave this for @tacaswell to decide.

tacaswell

I approve, but have a commit in this.

timhoffm · 2018-07-31T08:06:26Z

@tacaswell I re-approve your commit. Is it ok to merge on this basis?

tacaswell · 2018-07-31T15:13:58Z

Yes, @timhoffm you can push the button if you want :)

lumberbot-app · 2018-07-31T15:31:15Z

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)

anntzer added the GUI: Qt label Dec 13, 2017

anntzer force-pushed the qtcompat branch 3 times, most recently from df8b780 to 333a453 Compare December 13, 2017 09:33

tacaswell added this to the v2.2 milestone Dec 13, 2017

anntzer mentioned this pull request Dec 13, 2017

consider using qtpy for qt abstraction layer #6118

Closed

anntzer force-pushed the qtcompat branch 2 times, most recently from 55a9d88 to fdee996 Compare December 13, 2017 22:13

anntzer force-pushed the qtcompat branch from fdee996 to 634bc5f Compare December 13, 2017 22:20

anntzer force-pushed the qtcompat branch from 634bc5f to 15f44b8 Compare January 21, 2018 23:38

anntzer force-pushed the qtcompat branch from 15f44b8 to bb80ea5 Compare February 26, 2018 21:43

anntzer modified the milestones: needs sorting, v3.0 Feb 26, 2018

anntzer force-pushed the qtcompat branch 3 times, most recently from 679db1f to 37ac201 Compare February 26, 2018 23:09

anntzer force-pushed the qtcompat branch from 37ac201 to b4ac524 Compare May 7, 2018 07:06

anntzer mentioned this pull request Jun 18, 2018

PyQt GPL "infecting" even when using PySide backend #11453

Closed

anntzer force-pushed the qtcompat branch from b4ac524 to 01170f0 Compare June 18, 2018 20:15

tacaswell added the Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. label Jun 18, 2018

tacaswell closed this Jun 18, 2018

tacaswell reopened this Jun 18, 2018

anntzer force-pushed the qtcompat branch from 01170f0 to 1d7bb35 Compare June 19, 2018 12:01

anntzer force-pushed the qtcompat branch 2 times, most recently from 05c4747 to 7b75f59 Compare July 9, 2018 14:58

anntzer mentioned this pull request Jul 11, 2018

Importing qt_compat when no Qt binding is installed fails with NameError instead of ImportError #11629

Closed

timhoffm reviewed Jul 11, 2018

View reviewed changes

anntzer force-pushed the qtcompat branch from 7b75f59 to 0100ee7 Compare July 12, 2018 09:01

anntzer force-pushed the qtcompat branch from 0100ee7 to 42bbc68 Compare July 15, 2018 09:24

timhoffm reviewed Jul 27, 2018

View reviewed changes

anntzer force-pushed the qtcompat branch from 42bbc68 to 5d17e6c Compare July 28, 2018 18:17

anntzer force-pushed the qtcompat branch from 5d17e6c to b8071e0 Compare July 28, 2018 18:41

tacaswell modified the milestones: v3.1, v3.0, v2.2.3 Jul 28, 2018

timhoffm approved these changes Jul 28, 2018

View reviewed changes

MNT: be more paranoid about importing sip

1a7d56d

tacaswell approved these changes Jul 30, 2018

View reviewed changes

timhoffm merged commit f677f36 into matplotlib:master Jul 31, 2018

lumberbot-app bot added the status: needs manual backport label Jul 31, 2018

anntzer deleted the qtcompat branch July 31, 2018 15:51

tacaswell removed the status: needs manual backport label Aug 5, 2018

anntzer mentioned this pull request Sep 11, 2018

Respect QT_API even when the backend is not Qt{4,5}{Agg,Cairo}. #12091

Merged

6 tasks

anntzer removed the status: needs review label Sep 14, 2018

anntzer mentioned this pull request Nov 12, 2019

Avoid a RuntimeError at animation shutdown with PySide2. #15685

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rewrite and greatly simplify qt_compat.py. #9993

Rewrite and greatly simplify qt_compat.py. #9993

anntzer commented Dec 13, 2017 •

edited

Loading

tacaswell commented Dec 13, 2017

tacaswell commented Dec 13, 2017

tacaswell commented Dec 13, 2017

anntzer commented Dec 13, 2017

tacaswell commented Dec 13, 2017

anntzer commented Dec 13, 2017

anntzer commented Dec 13, 2017

timhoffm Jul 10, 2018

anntzer Jul 12, 2018

NelleV commented Jul 14, 2018

timhoffm left a comment

timhoffm Jul 27, 2018

anntzer Jul 28, 2018

timhoffm Jul 27, 2018

anntzer Jul 28, 2018

tacaswell Jul 28, 2018

QuLogic Jul 29, 2018

anntzer Jul 30, 2018

timhoffm Jul 27, 2018

timhoffm Jul 27, 2018

anntzer Jul 28, 2018

timhoffm Jul 27, 2018

anntzer Jul 28, 2018

anntzer commented Jul 28, 2018

timhoffm left a comment

tacaswell left a comment

timhoffm commented Jul 31, 2018

tacaswell commented Jul 31, 2018

lumberbot-app bot commented Jul 31, 2018


		# Available APIs.
		QT_API_PYQT = 'PyQt4' # API is not set here; Python 2.x default is V 1

Rewrite and greatly simplify qt_compat.py. #9993

Rewrite and greatly simplify qt_compat.py. #9993

Conversation

anntzer commented Dec 13, 2017 • edited Loading

PR Summary

PR Checklist

tacaswell commented Dec 13, 2017

tacaswell commented Dec 13, 2017

tacaswell commented Dec 13, 2017

anntzer commented Dec 13, 2017

tacaswell commented Dec 13, 2017

anntzer commented Dec 13, 2017

anntzer commented Dec 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NelleV commented Jul 14, 2018

timhoffm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anntzer commented Jul 28, 2018

timhoffm left a comment

Choose a reason for hiding this comment

tacaswell left a comment

Choose a reason for hiding this comment

timhoffm commented Jul 31, 2018

tacaswell commented Jul 31, 2018

lumberbot-app bot commented Jul 31, 2018

anntzer commented Dec 13, 2017 •

edited

Loading