Skip to content

MEP27 Decouple pyplot from backends (refactoring Gcf out of backend code) #4143

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.

Sign up for GitHub

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

Closed
wants to merge 55 commits into from
Closed

MEP27 Decouple pyplot from backends (refactoring Gcf out of backend code) #4143

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
55 commits
Select commit Hold shift + click to select a range
24caf2b
Refactor pass 1. Refactoring Gcf out of specific backend (backend_gt…
OceanWolf Feb 21, 2015
2b05d38
Refactor Pass 2. Refactored Gcf out of all backend code.
OceanWolf Feb 22, 2015
f4fc354
Quick fix to figure for safe unpickling.
OceanWolf Feb 25, 2015
0b31e3a
GTK3Agg
OceanWolf Feb 26, 2015
6fb452e
Refactored making `FigureManager` a *figure* manager, plus added miss…
OceanWolf Feb 27, 2015
0a868a2
keyword
OceanWolf Feb 27, 2015
61ba2b4
Make add_element more general, and make sure the code complies with it.
OceanWolf Feb 27, 2015
f0eb84c
Better destroy order.
OceanWolf Feb 27, 2015
8fe9cd7
GTK simplifications
OceanWolf Feb 28, 2015
494e5f1
Added doc and cleaned backend_managers, don't want our new file dirty.
OceanWolf Mar 3, 2015
ed16178
Improve layout!
OceanWolf Apr 6, 2015
21b8f58
Move knowledge of the backend to the manager.
OceanWolf Apr 7, 2015
cf42e3b
Incorporate MEP22 into MEP27
OceanWolf Apr 12, 2015
f027b16
Improved new toolbar and updated tool_manager example accoridingly.
OceanWolf Apr 12, 2015
24b7b73
fullscreen
OceanWolf Apr 13, 2015
bc99129
MEP update
OceanWolf Apr 14, 2015
7f7f05e
Finish MEP22 conversion
OceanWolf Apr 14, 2015
713abcb
rename window method
OceanWolf Apr 17, 2015
80adaaf
Add backend anme to widgets
OceanWolf Apr 17, 2015
4e5f69d
Handle FigureManager destroy internaly without pyplot.
OceanWolf Jun 4, 2015
160ef57
Make functionality more consistant for embedded applications
OceanWolf Jun 7, 2015
b6d6acc
Backend getter method for FigureManager
OceanWolf Jun 13, 2015
ecd5038
Improve example after new method
OceanWolf Jun 13, 2015
f8e83fe
Clean up the code a bit
OceanWolf Jun 13, 2015
c53b79a
Remove old code from backend_managers
OceanWolf Jun 15, 2015
34c6b12
Cleanup
OceanWolf Jun 22, 2015
8a4268a
Explicity get set manager as None if appropiate.
OceanWolf Jun 22, 2015
44df199
figure attribute and canvas property
fariza Jun 22, 2015
860a8ed
Fix FigureCanvasBase
OceanWolf Jun 23, 2015
c44e744
super
OceanWolf Jun 25, 2015
2fe9215
figure setter
fariza Jun 25, 2015
3b434ef
Improve MEP22 Tool Searching Structure
OceanWolf Jun 25, 2015
490629f
adding example file
fariza Jun 25, 2015
8eb987b
super dooper
OceanWolf Jun 26, 2015
224a4f3
Revert old example and fix new one.
OceanWolf Jun 26, 2015
cdbd51b
Improve MEP22 tool-searching method.
OceanWolf Jun 27, 2015
50e3719
MEP22 Save Figure Tool
OceanWolf Jun 27, 2015
85be519
pep8
OceanWolf Jun 27, 2015
7edaf5a
Make ToolConfigureSubplots a generic tool
OceanWolf Jun 28, 2015
8e6e252
Improve flow handling and make it a lot more generic
OceanWolf Jun 28, 2015
72575cb
Missing resize method
OceanWolf Jun 28, 2015
4a78246
Convert to new structure for finding tools
OceanWolf Jun 28, 2015
e300707
doc
OceanWolf Jun 29, 2015
ee76451
Add ExpandableBase
OceanWolf Jun 29, 2015
6f0c7ab
Template Backend plus fix FigManager for non-GUI backends and add gen…
OceanWolf Jun 29, 2015
ae9bf5b
rcParam and Travis
OceanWolf Jul 19, 2015
fb004e0
test
OceanWolf Jul 20, 2015
1d2095b
test always MEP27
OceanWolf Jul 20, 2015
24e43b3
Fix FigureManager to allow pyplot to work for non GUI backends
OceanWolf Jul 27, 2015
208c3be
Fix Gcf.show_all()
OceanWolf Jul 27, 2015
a44ebd9
doc
OceanWolf Jul 27, 2015
f8f9cf2
pep8
OceanWolf Jul 27, 2015
0e09a54
remove show_popup
OceanWolf Jul 27, 2015
a38b6d7
AttributeError
OceanWolf Sep 22, 2015
64f0c61
Fixes for MEP27
OceanWolf Aug 3, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
super
  • Loading branch information
@OceanWolf
OceanWolf committed Aug 3, 2016
commit c44e74443f9c4f4e3b644f470cf244871d95cb3f
10 changes: 6 additions & 4 deletions lib/matplotlib/backend_bases.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2639,8 +2639,8 @@ class WindowBase(cbook.EventEmitter):
The title of the window.
"""

def __init__(self, title):
cbook.EventEmitter.__init__(self)
def __init__(self, title, **kwargs):
super(WindowBase, self).__init__(**kwargs)
Copy link
Member

Choose a reason for hiding this comment

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

title is getting dropped

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, intentionally, when working with super, one should take of arguments as and when they get used. I find this encourages good encapsulation of classes.

Copy link
Member

Choose a reason for hiding this comment

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

devil's advocate: you are documenting that it accepts a title argument, and you even have a method for setting a title, but they do nothing. It is entirely misleading to me.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I did have the same thought last night (as I typed the message), that I probably should refactor that down now we use super...

But now checking on the other backends, and I now see I call self.set_window_title(title) in all the backends apart from Wx, and I don't think it hurts to call it again, so I can probably move that to here :). Fingers crossed the super magic holds (I think it will, but only 90% sure)...

Copy link
Member

Choose a reason for hiding this comment

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

I do not understand this. You should only explicitly list args/kwargs in the signature that you intend to use.

Copy link
Member

Choose a reason for hiding this comment

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

It should fallback to self.set_window_title(title) and let that be a no-op if needed?

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay, will do, I can't remember if I did this (combined with the get_window_title returning image on the base class) to satisfy a test, or whether just to avoid possible breakage with user code when they switch from using the old classes to using the new ones.

Anyway, if a problem occur, then I think we can find a better way to do this.


def show(self):
"""
Expand Down Expand Up @@ -3349,7 +3349,8 @@ class ToolContainerBase(object):
this `ToolContainer` wants to communicate with.
"""

def __init__(self, toolmanager):
def __init__(self, toolmanager, **kwargs):
super(ToolContainerBase, self).__init__(**kwargs)
self.toolmanager = toolmanager
self.toolmanager.toolmanager_connect('tool_removed_event',
self._remove_tool_cbk)
Expand Down Expand Up @@ -3475,7 +3476,8 @@ def remove_toolitem(self, name):

class StatusbarBase(object):
"""Base class for the statusbar"""
def __init__(self, toolmanager):
def __init__(self, toolmanager, **kwargs):
super(StatusbarBase, self).__init__(**kwargs)
self.toolmanager = toolmanager
self.toolmanager.toolmanager_connect('tool_message_event',
self._message_cbk)
Expand Down
2 changes: 1 addition & 1 deletion lib/matplotlib/backend_managers.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ class FigureManager(cbook.EventEmitter):
The figure number.
"""
def __init__(self, figure, num):
Copy link
Member

Choose a reason for hiding this comment

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

@OceanWolf why are you getting a figure instead of a canvas?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because we want to create a FigureManager not a CanvasManager right?

Also much cleaner, allowing us to get rid of the new_figure_manager and new_figure_manager_from_figure ugly methods.

Copy link
Member

Choose a reason for hiding this comment

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

Not that I don't agree with you, but for consistency please check my comment about the typical three ways of getting things running.

cbook.EventEmitter.__init__(self)
super(FigureManager, self).__init__()
self.num = num

self._backend = get_backend()
Expand Down
11 changes: 4 additions & 7 deletions lib/matplotlib/backends/backend_gtk3.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -391,9 +391,8 @@ def stop_event_loop(self):


class WindowGTK3(WindowBase, Gtk.Window):
def __init__(self, title):
WindowBase.__init__(self, title)
Gtk.Window.__init__(self)
def __init__(self, title, **kwargs):
super(WindowGTK3, self).__init__(title=title, **kwargs)
self.set_window_title(title)

try:
Expand Down Expand Up @@ -829,8 +828,7 @@ def draw_rubberband(self, x0, y0, x1, y1):

class ToolbarGTK3(ToolContainerBase, Gtk.Box):
def __init__(self, toolmanager, flow='horizontal'):
Copy link
Member

Choose a reason for hiding this comment

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

The flow keyword is only available for MEP22 classes, how do you handle that when using MEP27 without MEP22?

Copy link
Member Author

Choose a reason for hiding this comment

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

In the same way that we handle ToolbarGTK3.add_tool, i.e. by not handling it. Why would the user try to set the flow property of a toolbar (or use add_tool) if they hadn't set rcParams['toolbar'] = 'toolmanager'?

ToolContainerBase.__init__(self, toolmanager)
Gtk.Box.__init__(self)
super(ToolbarGTK3, self).__init__(toolmanager=toolmanager)
self._toolarea = Gtk.Box()
self.set_flow(flow)

Expand Down Expand Up @@ -917,8 +915,7 @@ def _add_separator(self):

class StatusbarGTK3(StatusbarBase, Gtk.Statusbar):
def __init__(self, *args, **kwargs):
StatusbarBase.__init__(self, *args, **kwargs)
Gtk.Statusbar.__init__(self)
super(StatusbarGTK3, self).__init__(*args, **kwargs)
self._context = self.get_context_id('message')

def set_message(self, s):
Expand Down
1 change: 1 addition & 0 deletions lib/matplotlib/cbook.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -545,6 +545,7 @@ def process(self, s, *args, **kwargs):

class EventEmitter(object):
def __init__(self):
super(EventEmitter, self).__init__()
Copy link
Member

Choose a reason for hiding this comment

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

Maybe add a brief comment as to why super (and any base __init__) is needed here. I had the idea that it was not needed for a class that inherits only from object.

Copy link
Member Author

Choose a reason for hiding this comment

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

Basically you need super for all classes that appear in the inheritance tree, not just because of what lies below, but what lies next to, it needs to call the next branch in the inheritance tree, what I would call a sibling class. Having learnt about super only in the past couple of days, I now find it very beautiful and elegant and don't know how I have ever lived without it.

Do you think I should also add something about this in the developers guide?

Copy link
Member

Choose a reason for hiding this comment

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

I have now read https://rhettinger.wordpress.com/2011/05/26/super-considered-super/ and scanned
https://fuhm.net/super-harmful/.
What I think the two authors agree on is that if super is to be used, it must be used very carefully and consistently, and the usage strategy and scope needs to be clearly documented. So yes, if we are going the super route, then having something about it in the dev guide would be a good idea.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, I shall get onto it in due course, and perhaps do a bit of a clearout at the same time, as I got a bit confused last time I looked as some of it looks out of date.

Doing a search for super in matplotlib https://github.com/matplotlib/matplotlib/search?utf8=%E2%9C%93&q=super shows we have gone a long way past the if stage, so I think we do need to embrace it.

The against article you linked to (I read it 2 days ago) seems to either set up straw-man arguments against super, as in they don't provide any sort of rebuttal against the ways you should use it, i.e. that explained in the pro-article; or give arguments against that also apply if you don't use super.

self._callbacks = CallbackRegistry()

def mpl_connect(self, s, func):
Expand Down