Issue835 2: replacement for #835 #1192

Merged
merged 2 commits into from Sep 5, 2012

Conversation

Projects
None yet
5 participants
@efiring
Member

efiring commented Sep 2, 2012

As far as I can see, the monkey patching was not necessary at all, so I removed it. I think the result is much cleaner, and satisfies all requirements, to the extent that I have understood them. Testing on all backends is needed; I can do more of that tomorrow. macosx, tkagg, and qt4agg are tested. Testing on ipython notebook would be good, also.

@travisbot

This comment has been minimized.

Show comment Hide comment
@travisbot

travisbot Sep 2, 2012

This pull request fails (merged b76166f1 into 92721ed).

This pull request fails (merged b76166f1 into 92721ed).

@pelson

This comment has been minimized.

Show comment Hide comment
@pelson

pelson Sep 2, 2012

Member

Excuse the stupid question, but I cannot see where the interactive backends are overriding the backend_base manager's show method. I must be missing something very obvious...

Other than that, looks good to me.

Member

pelson commented Sep 2, 2012

Excuse the stupid question, but I cannot see where the interactive backends are overriding the backend_base manager's show method. I must be missing something very obvious...

Other than that, looks good to me.

@efiring

This comment has been minimized.

Show comment Hide comment
@efiring

efiring Sep 2, 2012

Member

@pelson, it turned out that the GUI backend FigureManager classes already had show methods, so I didn't need to add anything.

Member

efiring commented Sep 2, 2012

@pelson, it turned out that the GUI backend FigureManager classes already had show methods, so I didn't need to add anything.

@pelson

This comment has been minimized.

Show comment Hide comment
@pelson

pelson Sep 2, 2012

Member

Looks good. I would like to get sign-off from @mdboom and @WeatherGod, but I think this is a really favourable approach.

Member

pelson commented Sep 2, 2012

Looks good. I would like to get sign-off from @mdboom and @WeatherGod, but I think this is a really favourable approach.

@@ -375,9 +375,6 @@ def notify_axes_change(fig):
if self.toolbar != None: self.toolbar.update()
self.canvas.figure.add_axobserver(notify_axes_change)
- # This is ugly, but this is what tkagg and gtk are doing.
- # It is needed to get ginput() working.
- self.canvas.figure.show = lambda *args: self.show()

This comment has been minimized.

Show comment Hide comment
@WeatherGod

WeatherGod Sep 4, 2012

Member

Note a subtle difference between the macosx backend and most others for the show. macosx backend uses self.show() instead of self.window.show(). I am also concerned about the note saying that this is needed to make ginput() work correctly.

@WeatherGod

WeatherGod Sep 4, 2012

Member

Note a subtle difference between the macosx backend and most others for the show. macosx backend uses self.show() instead of self.window.show(). I am also concerned about the note saying that this is needed to make ginput() work correctly.

This comment has been minimized.

Show comment Hide comment
@efiring

efiring Sep 4, 2012

Member

@WeatherGod: No problem, as far as I can see. The backends that were using self.window.show also had methods self.show that were defined as self.window.show, so the difference between patching in self.window.show and self.show was zilch.

Ginput works fine on macosx after this PR.

@efiring

efiring Sep 4, 2012

Member

@WeatherGod: No problem, as far as I can see. The backends that were using self.window.show also had methods self.show that were defined as self.window.show, so the difference between patching in self.window.show and self.show was zilch.

Ginput works fine on macosx after this PR.

mdboom and others added some commits Aug 20, 2012

@efiring

This comment has been minimized.

Show comment Hide comment
@efiring

efiring Sep 4, 2012

Member

Rebased.

Member

efiring commented Sep 4, 2012

Rebased.

@travisbot

This comment has been minimized.

Show comment Hide comment
@travisbot

travisbot Sep 4, 2012

This pull request fails (merged 51ffd4b into 0f9f85f).

This pull request fails (merged 51ffd4b into 0f9f85f).

@pelson

This comment has been minimized.

Show comment Hide comment
@pelson

pelson Sep 5, 2012

Member

@mdboom : Happy to merge?

Member

pelson commented Sep 5, 2012

@mdboom : Happy to merge?

@mdboom

This comment has been minimized.

Show comment Hide comment
@mdboom

mdboom Sep 5, 2012

Member

Yep. Merging now.

Member

mdboom commented Sep 5, 2012

Yep. Merging now.

mdboom added a commit that referenced this pull request Sep 5, 2012

@mdboom mdboom merged commit 15fd0ae into matplotlib:master Sep 5, 2012

1 check failed

default The Travis build failed
Details

@efiring efiring deleted the efiring:issue835_2 branch May 29, 2013

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