FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

FIX: shim Qt4 and Qt5 together better by tacaswell · Pull Request #9048 · matplotlib/matplotlib · GitHub

  1. matplotlib
  2. matplotlib

Repository navigation

FIX: shim Qt4 and Qt5 together better - #9048

Closed
tacaswell wants to merge 1 commit into
matplotlib:masterfrom
tacaswell:fix_qt4_MI
ConversationCommits (1)ChecksFiles changed
Closed

FIX: shim Qt4 and Qt5 together better#9048
tacaswell wants to merge 1 commit into
matplotlib:masterfrom
tacaswell:fix_qt4_MI

Conversation

tacaswell commented Aug 17, 2017

Copy link
Copy Markdown
Member

in two places super needs to be used in the Qt5 case and
frustrated in the Qt4 case. These are handled by the two
_fake_super_* methods. The must be just private as we need to
override in sub-classes.

Closes #9040

FIX: shim Qt4 and Qt5 together better
b181d50 
in two places `super` needs to be used in the Qt5 case and
frustrated in the Qt4 case.  These are handled by the two
`_fake_super_*` methods.  The must be just private as we need to
override in sub-classes.

Closes matplotlib#9040
tacaswell added this to the 2.1 (next point release) milestone Aug 17, 2017
tacaswell added Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. GUI: Qt labels Aug 17, 2017
tacaswell requested review from anntzer and astrofrog August 17, 2017 03:35

anntzer commented Aug 17, 2017

Copy link
Copy Markdown
Contributor

Would the patch at https://github.com/matplotlib/matplotlib/pull/8771/files#diff-76b5bbaa0c6a702660db7feae68c1d20 (starting at "Work around lack of cooperative inheritance") solve the issue too?

tacaswell commented Aug 17, 2017

Copy link
Copy Markdown
Member Author

I am not sure if I am more comfortable with this approach or via re-implementing the MRO innards.

dopplershift commented Aug 17, 2017

Copy link
Copy Markdown
Contributor

If we only need to fix this for these classes, I say go with @tacaswell because there's less introspection going on (we've codified the special case in comments and the inheritance structure).

If we get to the point where we have lots of classes dealing with this nonsense, then I'd say the complexity of the solution in #8771 is warranted.

dopplershift reviewed Aug 17, 2017
View reviewed changes
Comment thread lib/matplotlib/backends/backend_qt4.py
# Note different super-calling style to backend_qt5
QtWidgets.QWidget.__init__(self)
FigureCanvasBase.__init__(self, figure)
self.figure = figure

dopplershift Aug 17, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Why'd all this other stuff, like setMouseTracking go away?

tacaswell Aug 17, 2017

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Because that stuff is handled in the FigureCanvasQt5 init and this function injected in.

anntzer commented Aug 17, 2017

Copy link
Copy Markdown
Contributor

I think @tacaswell should just decide whether we want to try to get #8771 in any time soon or not, and decide accordingly for this PR.

dopplershift commented Aug 17, 2017

Copy link
Copy Markdown
Contributor

Not knowing the state of #8771, I'm not sure if it's good to merge something so large towards the end of the 2.1 release cycle, whereas this particular issue is probably a showstopper for 2.1.

anntzer mentioned this pull request Aug 17, 2017
Merged
6 tasks

anntzer commented Aug 17, 2017

Copy link
Copy Markdown
Contributor

See #9052 for the alternate approach, without the cairo parts which are irrelevant for this.

tacaswell commented Aug 17, 2017

Copy link
Copy Markdown
Member Author

I prefer @anntzer 's solution (see #9052 for reasons).

fariza commented Aug 21, 2017

Copy link
Copy Markdown
Member

I confirm that this solves the problem with #9040

tacaswell commented Aug 21, 2017

Copy link
Copy Markdown
Member Author

@fariza Can you also test out #9052 ? I think we are going to go with that solution.

tacaswell closed this Aug 21, 2017
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

dopplershift dopplershift approved these changes
astrofrog Awaiting requested review from astrofrog
anntzer Awaiting requested review from anntzer

Assignees

No one assigned

Labels

GUI: Qt Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions.

Projects

None yet

Milestone

v2.1

Development

Successfully merging this pull request may close these issues.

4 participants

Back | FazBrowse Home | New Git URL