Skip to content
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

Rebase of #1418 on v1.2.x #1425

Merged
merged 1 commit into from Oct 23, 2012
Merged

Conversation

jenshnielsen
Copy link
Member

#1420 is not needed on v1.2.x. This is a bug only present on master introduced in 2f11dee

@dmcdougall
Copy link
Member

This gets my +1. Perhaps this should go in before the rc3 cut. What do the other developers think?

@mdboom
Copy link
Member

mdboom commented Oct 22, 2012

It seems a little convoluted -- maybe we should add a short comment about why it iterates through the keys and looks them up, rather than just iterating through the values.

@dmcdougall
Copy link
Member

@mdboom Perhaps we don't even need to iterate through the keys at all!

@dmcdougall
Copy link
Member

boxes = [cell.get_window_extent(renderer) for cell in self._cells.values()]

… bbox_extra_artists.

The get_window_extent is a method on the cells not the key of the cells.
@jenshnielsen
Copy link
Member Author

Yes that would indeed be simpler. I updated the pull request to iterate over the keys instead.

@dmcdougall
Copy link
Member

I updated the pull request to iterate over the keys instead.

Do you mean the values?

This solution looks much neater. +1.

@jenshnielsen
Copy link
Member Author

Sorry, yes of course

@dmcdougall
Copy link
Member

Thanks to @mdboom's feedback, I'm confident this is the correct solution now. There's an rc3 cut scheduled for tomorrow. I will leave this open so it can get some visibility before the cut. If there are no gripes by tomorrow morning I think this is good to go.

@dmcdougall
Copy link
Member

Mergifying!

dmcdougall added a commit that referenced this pull request Oct 23, 2012
@dmcdougall dmcdougall merged commit 070425c into matplotlib:v1.2.x Oct 23, 2012
@jenshnielsen jenshnielsen deleted the bbox_rebase branch July 20, 2014 12:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants