-
-
Notifications
You must be signed in to change notification settings - Fork 7.8k
Backend plt/gcf refactor #2624
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
Backend plt/gcf refactor #2624
Conversation
@tacaswell why remove key_press_handler for just one method? |
@tacaswell Why to leave |
I like the concept here, as illustrated by the While I see why it's an improvement, I'm concerned about the API breakage of moving |
I'll add a commit re-arranging it so that everything but the close key get handled in |
That's what I meant, removed from Gcf independent part. |
I left I am not sure on replacing the current example, I think it is good to keep an example of how to do the embedding at a low-level around (instead of telling people to go look at the source of
|
|
||
from matplotlib.backends.qt4_compat import QtCore, QtGui | ||
import numpy as np | ||
from matplotlib.backends._backend_qt4agg import (FigureCanvasQTAgg, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think that we want to have examples importing from private modules, or using private variables. My interpretation of the leading underscore is, "for internal use only, not part of the public API, please don't use this, but if you do, it is at your own risk."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would like suggestions of how to rename things, I went with _
because it was easy. What I really want to do is make a module backend_*_pyplot
which contains the Gcf
related code, but that would break badly break the API. backend_*_clean
seems misleading, backend_*_stateless
seems too long, any abbreviation seems too cryptic. Should have made clear that I don't think this is near being ready to merge.
Ah, I see what you're going with the So, you're plan is to separate out the pyplot-related things into separate modules? Maybe it would be helpful to be explicit about those design principles -- I'd probably better understand what's happening here (or just point me to the discussion, if it already happened elsewhere and I missed it). |
The discussion of this is rather scattered across a number of issues/PRs related to the tabbed gui between @fariza and my self. The basic idea here is that we already have in the codebase everything we need to smooth over the differences between different tool kits, but the code is mixed up with plt/gcf which makes re-using it hard. The first step is to get the gui stuff into modules that are clean from any of the global state in The next step is to rip the
There is very little new code that needs to be written for this, we just need to re-arrange what is already there. |
even if this PR goes no where, the pep8 commits should be cherry-picked out. |
re-based to so it merges nicely again. |
As a side question, gkt3 + ipython does not seem to work on linux and closing/deleting figures seems to raise some extraneous exceptions. Are these known issues? |
self.canvas.destroy() | ||
if self.toolbar: | ||
self.toolbar.destroy() | ||
self.__dict__.clear() # Is this needed? Other backends don't have it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the line that causes the strang exceptions when closing and/or deleting figures.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems to me that is safe to remove that line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should that line be removed?
I am not a big user of ipython but last time I checked there was no mode for Gtk3. But to be fair, I didn't even try more than that. |
@fariza Can you check that I have not made a hash of the gtk3 code? Does this structure make sense? Can you verify that what I am suggesting with the |
@mdboom What can I do to make reviewing this easier? |
I see the issue with Gtk3 even on master -- I'm looking at making a separate bugfix for that now, then I'll come back and look at this. Thanks for all this hard work -- this is a tricky bit of refactoring. |
See #2646 for a fix for the Gtk3 issue. |
A thought occurred to me: Am I correct that the reason for splitting the backend* files is so that users who don't want pyplot functionality don't inadvertently import pyplot and enable it? Perhaps that could be instead addressed through importing within functions rather than at the top level... It seems that should work for everything with the exception of Does that make any sense, or am I missing something? |
Importing within functions? @mdboom, are you sipping the eggnog a bit early |
@WeatherGod: Any better ideas? I understand there's an overhead, but |
The other gotcha is all of the There are ways of pushing the call-back registration up the stack to
|
It isn't the overhead that I am worried about, it is just really bad form |
Keep in mind, the main objective of this splitting is to be able to use the "manager classes" in other gui applications. Personally the only thing that bothers me from pyplot is that the way it handles the gtk/gtk3 destroy. |
I mentioned this before, Please illuminate me, but I don't see any bad side effect of having Gcf outside pyplot. The right imports will make it transparent. |
In some sense,
|
@tacaswell I don't know how to do a PR against your PR (git is messing with my brain), but your example is not working |
@fariza If you have a local copy of my branch, make your changes on the tip and then push your branch to your github repo (just like making changes against master). The PR interface in github has issues with large networks. The advice they gave us was to edit the URL by hand to point the PR against the correct repo/branch. I fixed this by removing the |
Thanks for the git advice, I will try later. I know that
But not a mix. |
I would push a head with what you want to do and I will deal with the complications for my stuff later. Sorry for dropping the ball so badly on this. |
@tacaswell In #2759, @fariza suggested in response to one of my comments that a global event handling framework could become the subject of a future MEP. As at the moment it seems very self-contained within What do you think, should I begin writing one? As events form a major part the side covered by this refactor, I thought it the best place to ask. |
If you are interested, then yes. Also look at the discussion in #2986 about how to handle pick events. |
@tacaswell Do you have a plan for the breaking of this up into smaller commits? Or even an MEP document to look which outlines this? I would like to have a look at this, but over 13k changes in the codebase looks a bit daunting to start reading through... |
most of those lines are just copies as I split files up. The outline of this is that nothing in backend* should import Getting back to this has been on my to-do list for over a year now.... This should be split up, one PR to do backend_bases.py and one PR per backend after that. |
Perhaps something like the Navigation refactor, i.e. one PR for I have very little time to look at this right now, but I would imagine the way to do this refactor would just mean splitting out the pure backend logic from the general mpl logic, in master we have the self.set_window_title("Figure %d" % num) Here it combines the mpl logic As we have |
I am starting to implement the MEP23 https://github.com/matplotlib/matplotlib/wiki/Mep23 and I have conflicts with @tacaswell just a crazy idea, what do you think if I implement the MEP23 in a completely new set of MultiFigureManagers, that don't include any |
@tacaswell Anyway, sounds simple enough, I shall work on getting a PR on this done in the next few hours :). |
@OceanWolf Great! Just to be clear, I am ecstatic that you are taking over this project. |
@tacaswell Just to make it clear, I don't know much about the various backends, so I will put all my focus on refactoring just |
@tacaswell Getting there, just finished the 2nd class :). One question though, I see the method |
We have to be very careful about changing public API. It either needs to stay or to go through a proper deprecation cycle. |
@tacaswell I make completely new classes, so by remove, I mean not putting them in the new one... I think I shall leave it out for now, and make a comment about it in the PR. |
👎 on completely new classes for this core stuff unless they match the old API exactly. There is too much existing code (particularly for embedding). I don't want to maintain two version of code that do the same thing, that way lies madness confusion and angry users. I am now very confused about what you are working on. |
DW, I have |
Replaced by MEP27 and #4143 |
This is an alternative to #2617. The idea is to factor Gcf out of the toolkit backends so that the manager classes can be reused in external gui applications.
These changes should be backwards compatible.
Added an example for Qt4 to show how to use the managers in an external program.
Currently only backend_bases, qt4 and qt4agg are done, but the other backends should be doable incrementally.
The relavent backends are (I think)
PdfPages
)This possibly should get a MEP.