tkcairo backend by anntzer · Pull Request #10436 · matplotlib/matplotlib · GitHub
Skip to content

tkcairo backend - #10436

Merged
tacaswell merged 1 commit into
matplotlib:v2.2.xfrom
anntzer:tkcairo-minimal
Feb 19, 2018
Merged

tacaswell merged 1 commit into
matplotlib:v2.2.xfrom
anntzer:tkcairo-minimal

Conversation

@anntzer

@anntzer anntzer commented Feb 12, 2018

Copy link
Copy Markdown
Contributor

PR Summary

same as wxcairo and qtcairo, but for tk :-)

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

Copy link
Copy Markdown
Member

@anntzer

anntzer commented Feb 13, 2018

Copy link
Copy Markdown
Contributor Author

Thanks, got a bit delayed by real life and was also waiting for the other backends to get in first.

A few notes on the implementation:

I moved the common parts to _backend_tk.py, which plays a role similar to backend_qt5.py (but is private because I don't see a good reason to make it public... but perhaps it should? dunno). The patch looks enormous because github's UI doesn't seem to detect the move, possibly because backend_tkagg.py still exists (but is essentially completely new).

I also deprecated FigureManagerTkAgg and NavigationToolbar2TkAgg, both because FigureManagerTk and NavigationToolbar2Tk are available and because I would argue that the former should actually be accessed by third parties as from matplotlib.backends.backend_foo import FigureManager (by the design of backends this will necessarily import the manager class corresponding to the backend); while a similar "single" name is not available for NavigationToolbar2 it could be added as well. However these deprecations are not strictly necessary and can easily be reverted.

AFAICT tkagg does all its rendering to the actual widget in draw() (rather than "buffering" it similarly to how Qt separates draw() from paintEvent()). While it may be interesting to rework this I did not change the design in this PR.

@anntzer anntzer mentioned this pull request Feb 13, 2018
6 tasks

@QuLogic QuLogic left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Seemed to work for unchained.py for both TkAgg and TkCairo.

self.window.attributes('-fullscreen', not is_fullscreen)


@cbook.deprecated("2.2")

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Was this replaced by something in particular?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This class was used by the old navigationtoolbar (pre-navigationtoolbar2) so basically unused for a very long time. But I moved it to _backend_tk like everyone else for a normal deprecation period.

@QuLogic

QuLogic commented Feb 17, 2018

Copy link
Copy Markdown
Member

You might want to rebase and re-target v2.2.x directly so that tests aren't borked.

@anntzer

anntzer commented Feb 17, 2018

Copy link
Copy Markdown
Contributor Author

Rebased. Before I retarget this: do you want to later "forwardport" from 2.2 to master?

@QuLogic

QuLogic commented Feb 17, 2018

Copy link
Copy Markdown
Member

I expect v2.2.x will be merged up to master at some point (at least until 2.2.0), until it becomes too difficult with all our Python 3 changes. You'll need to rebase again though if you re-target because you'll get all the Python 3 changes in now.

@anntzer
anntzer changed the base branch from master to v2.2.x February 17, 2018 21:41
@anntzer

anntzer commented Feb 17, 2018

Copy link
Copy Markdown
Contributor Author

done

@tacaswell
tacaswell merged commit ef6e4eb into matplotlib:v2.2.x Feb 19, 2018
@anntzer
anntzer deleted the tkcairo-minimal branch February 19, 2018 22:25
@anntzer

anntzer commented Feb 20, 2018

Copy link
Copy Markdown
Contributor Author

needs to be merged into master as well.

@ImportanceOfBeingErnest

Copy link
Copy Markdown
Member

This looks like it will cause chaos when a new version is released out of the development version.

If FigureManagerTkAgg NavigationToolbar2TkAgg are meant to disappear, there should be an API change notice.
But I would argue that as long as there is no date/version set for FigureManagerTkAgg NavigationToolbar2TkAgg to disappear they should still be accessible.
And in any case the deprecation messages should include what to use instead of FigureManagerTkAgg NavigationToolbar2TkAgg

@jenshnielsen

Copy link
Copy Markdown
Member

@ImportanceOfBeingErnest As far as I can see they are both still there. They are just empty aliases for the General non Agg specific version

@ImportanceOfBeingErnest

Copy link
Copy Markdown
Member

@jenshnielsen I wouldn't preclude that you have better eyes than I have, but I don't see them anywhere.
And in any case, doing from matplotlib.backends.backend_tkagg import NavigationToolbar2TkAgg results in an ImportError on current master.

@anntzer

anntzer commented May 15, 2018

Copy link
Copy Markdown
Contributor Author

They were deprecated in 2.2 and removed in #10426 (3.0) which is admittedly a shorter cycle than normal, but an API removal notice should be enough.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@ImportanceOfBeingErnest here is the figure manager

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants