Skip to content

Conversation

@QuLogic
Copy link
Member

@QuLogic QuLogic commented Jun 24, 2018

PR Summary

Fix import of PyQt5 5.11.

sip is not really required by this code when using PyQt5, and 5.11 now no longer requires it nor provides it globally [1].

Also, move the PyQt/PySide shim bits to the end to be near the Qt5/Qt4 shim.

[1] http://pyqt.sourceforge.net/Docs/PyQt5/incompatibilities.html#pyqt-v5-11

Remove unnecessary Qt signal connection.

This fails with PyQt5.11, but is also unnecessary. _destroying is set to True before this signal is connected, so the callback simply returns immediately. Thus, just remove the connection entirely.

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

QuLogic added 2 commits June 24, 2018 16:20
sip is not really required by this code when using PyQt5, and 5.11 now
no longer requires it nor provides it globally [1].

Also, move the PyQt/PySide shim bits to the end to be near the Qt5/Qt4
shim.

[1] http://pyqt.sourceforge.net/Docs/PyQt5/incompatibilities.html#pyqt-v5-11
This fails with PyQt5.11, but is also unnecessary. `_destroying` is set
to True before this signal is connected, so the callback simply returns
immediately. Thus, just remove the connection entirely.
Copy link
Contributor

@anntzer anntzer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM (the latest pyqt5 release was actually buggy and has been withdrawn, see https://www.riverbankcomputing.com/pipermail/pyqt/2018-June/040459.html, so the build is now unbroken and this can wait for a second reviewer; but in general the PR still looks good).

@anntzer
Copy link
Contributor

anntzer commented Jul 3, 2018

Restarted the build to check with the new release of PyQt5.11.

Copy link
Member

@jklymak jklymak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This passes the tests, so fixes the QT error all the Travis tests are having...

@timhoffm timhoffm merged commit 2fc375b into matplotlib:master Jul 3, 2018
@jklymak
Copy link
Member

jklymak commented Jul 3, 2018

Let the rebasing begin! Thanks @QuLogic and @anntzer

@tacaswell tacaswell added this to the v2.2.3 milestone Jul 3, 2018
@tacaswell
Copy link
Member

@meeseeksdev backport to v2.2.x

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.

5 participants