Skip to content

Fixed Annotation draw method #4145

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 1 commit into from
Closed

Fixed Annotation draw method #4145

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
5 changes: 4 additions & 1 deletion lib/matplotlib/text.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
import six
from six.moves import zip

import copy
import math
import warnings

Expand Down Expand Up @@ -2092,7 +2093,9 @@ def draw(self, renderer):
self.arrow_patch.figure = self.figure
self.arrow_patch.draw(renderer)

Text.draw(self, renderer)
astext = copy.copy(self)
astext.__class__ = Text
Copy link
Member

Choose a reason for hiding this comment

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

Evidently it works, but this seems like a hack. Maybe what it is telling us is that the Annotation could be implemented more cleanly via composition than via inheritance. That's speculation; I haven't looked closely.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I agree, while going through the Text module, Annotation does seem like it might be better suited for composition rather than inheritance. There is some precedence for modifying the __class__ attribute in the codebase currently but ultimately it is a hack to get the desired MRO.

One alternative approach is to refactor the Text.draw method to not reuse bbox_artist but that just seems like a step in the wrong direction by duplicating code. Another is to design a Text initialization that takes the derived class and returns a Text instance.

Copy link
Member

Choose a reason for hiding this comment

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

I think the problem is with bbox_artist() from patch.py, which adds a patch. It calls the object's get_window_extent(). For Annotations, that includes the arrow.

Copy link
Member

Choose a reason for hiding this comment

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

Note that by "problem" I don't mean that bbox_artist() is broken, just that some assumption somewhere is broken at that point.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@WeatherGod Yes, that is correct (see my comment on issue #4139).

Copy link
Member

Choose a reason for hiding this comment

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

Well, if that is the case, then I am -1 on this kludge. We know exactly
what the problem is -- we shouldn't be using bbox_artist() for this.
Perhaps "bbox_text()"? It would work similarly to bbox_artist, but would
use the Text class's get_window_extent no matter what object is passed to
it?

As for precedence for explicitly changing an object's class, I am
assuming that you are referring to mplot3d. Yeah, let's not bring that wart
into matplotlib proper. I still haven't figured out a way to fix that.

On Mon, Feb 23, 2015 at 4:58 PM, Cimarron notifications@github.com wrote:

In lib/matplotlib/text.py
#4145 (comment):

@@ -2092,7 +2093,9 @@ def draw(self, renderer):
self.arrow_patch.figure = self.figure
self.arrow_patch.draw(renderer)

  •    Text.draw(self, renderer)

  •    astext = copy.copy(self)

  •    astext.**class** = Text

@WeatherGod https://github.com/WeatherGod Yes, that is correct (see my
comment on issue #4139
#4139).

—
Reply to this email directly or view it on GitHub
https://github.com/matplotlib/matplotlib/pull/4145/files#r25206783.

Text.draw(astext, renderer)

def get_window_extent(self, renderer=None):
'''
Expand Down