Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
Backend class for better code reuse between backend modules#8773
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
bf03f1e to544823aCompareHow does this interact with#4143 ? |
| FigureManager = FigureManagerPgf | ||
| def draw_if_interactive(): |
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 this be a class method on the preceding class?
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.
Either way istechnically fine (this def will just override the one exported by the decorator) but actually I can just leave trigger_manager_draw as None and this way we'll get a do-nothing draw_if_interactive.
Given how disruptive this change could be, I am surprisingly 👍 on this 😄 . This is a very elegant solution to a problem that has been bugging me for years. |
And to answer my own question, I think this is mostly orthogonal. |
Re#4143 (MEP27): this doesn't really change anything, MEP27 can still go in (modulo conflicts, but they're already there anyways) (and I don't really have an opinion regarding MEP27). This is really just a way to 1. use some private "templates" to define helper functions and 2. do |
811b2d0 to05aae1eCompare| # each draw command. Instead, we mark the figure as invalid, so that it | ||
| # will be redrawn as soon as the event loop resumes via PyOS_InputHook. | ||
| # This function should be called after each draw event, even if | ||
| # matplotlib is not running interactively. |
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 comment, taken from the original, has me puzzled. It seems to be contradicted by the code, which does nothing unlessis_interactive() returns True. This is not changed by your refactor, so if the comment is misleading now, then it was also misleading before; or I am just missing its point in both cases.
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'm going to leave it as it is for now...
I certainly welcome the reduced duplication (which always bothered me also) and reduced LOC. And the removal of some cruft, like the import of ctypes! |
795c171 to1c3da14Comparea422dca to2254671Comparedone |
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.
Looks like a nice clean up. I don't see anything that bothers me.
I am going to give this one more read through before pushing the merge button myself, but happy if someone else also reviews this and merges. |
Rebased. |
This is the second part of#8771, split out for simpler review (note that it also includes the first part, which is the object of#8772; thus, I would suggest specifically looking at the diff of thelast commit).
Currently, backend modules need to define a number of classes (
FigureCanvas,FigureManager) and functions (new_figure_manager,new_figure_manager_given_figure,show,draw_if_interactive) with specified names, which leads to some serious copy-pasting between the modules, as well as seemingly unused imports (see e.g. "not used" comment in backend_qt5agg.py). Instead, use a class-based approach: the_Backendclass can be subclassed to define these functions (and provides some templates for them); then a special decorator re-exports the attributes and methods of the_Backendclass to the backend-defining module.