Skip to content

tkcairo backend #10436

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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Feb 19, 2018
Merged

tkcairo backend #10436

merged 1 commit into from
Feb 19, 2018

Conversation

anntzer
Copy link
Contributor

@anntzer anntzer commented Feb 12, 2018

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
Member

This did not get in under the RC, but for completeness I think we should still put it in 2.2

@anntzer
Copy link
Contributor Author

anntzer commented Feb 13, 2018

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
Copy link
Member

@QuLogic QuLogic left a comment

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.

def full_screen_toggle(self):
is_fullscreen = bool(self.window.attributes('-fullscreen'))
self.window.attributes('-fullscreen', not is_fullscreen)


@cbook.deprecated("2.2")
Copy link
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
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
Copy link
Member

QuLogic commented Feb 17, 2018

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

@anntzer
Copy link
Contributor Author

anntzer commented Feb 17, 2018

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

@QuLogic
Copy link
Member

QuLogic commented Feb 17, 2018

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
Copy link
Contributor Author

anntzer commented Feb 17, 2018

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
Copy link
Contributor Author

anntzer commented Feb 20, 2018

needs to be merged into master as well.

@ImportanceOfBeingErnest
Copy link
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
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
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
Copy link
Contributor Author

anntzer commented May 15, 2018

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.

background="#ffffe0", relief=Tk.SOLID, borderwidth=1)
label.pack(ipadx=1)
class FigureManagerTkAgg(FigureManagerTk):
pass
Copy link
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