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

Fix Annotation.contains. #12164

Merged
merged 1 commit into from Sep 20, 2018
Merged

Conversation

anntzer
Copy link
Contributor

@anntzer anntzer commented Sep 19, 2018

Annotation.contains always calls get_window_extents() without setting
the renderer arg, which is thus None (and this is implicitly allowed by
the signature of get_window_extents), but get_window_extents calls
update_positions which requires a non-None renderer (it ultimately
calls renderer.points_to_pixels).

Instead, reuse the code from Text.get_window_extents() which defaults to
the artist-or-figure renderer if the arg is None.

Example breaking code:

from matplotlib import pyplot as plt
ann = plt.annotate(
    "foo", (.5, .5), arrowprops={"arrowstyle": "->"},
    textcoords="offset points", xytext=(10, 10))
plt.gcf().canvas.mpl_connect(
    "button_press_event", lambda event: print(ann.contains(event)))
plt.show()

and click anywhere to trigger an AttributeError.

Regression due to #11801.

PR Summary

PR Checklist

  • Has Pytest style unit tests
  • Code is Flake 8 compliant
  • New features are documented, with examples if plot related
  • Documentation is sphinx and numpydoc compliant
  • Added an entry to doc/users/next_whats_new/ if major new feature (follow instructions in README.rst there)
  • Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way

Annotation.contains always calls get_window_extents() without setting
the renderer arg, which is thus None (and this is implicitly allowed by
the signature of get_window_extents), but get_window_extents calls
update_positions which *requires* a non-None renderer (it ultimately
calls renderer.points_to_pixels).

Instead, reuse the code from Text.get_window_extents() which defaults to
the artist-or-figure renderer if the arg is None.

Example breaking code:

    from matplotlib import pyplot as plt
    ann = plt.annotate(
        "foo", (.5, .5), arrowprops={"arrowstyle": "->"},
        textcoords="offset points", xytext=(10, 10))
    plt.gcf().canvas.mpl_connect(
        "button_press_event", lambda event: print(ann.contains(event)))
    plt.show()

and click anywhere to trigger an AttributeError.
@anntzer anntzer added the Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. label Sep 19, 2018
@anntzer anntzer added this to the v3.0.x milestone Sep 19, 2018
Copy link
Member

@jklymak jklymak left a comment

Choose a reason for hiding this comment

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

Modulo making these errors more verbose?

if self._renderer is None:
self._renderer = self.figure._cachedRenderer
if self._renderer is None:
raise RuntimeError('Cannot get window extent w/o renderer')
Copy link
Member

Choose a reason for hiding this comment

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

Should we be advising users to trigger a draw in these error messages?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's a few other places which already have a similar warning (Axes.draw_artist, Axes.redraw_in_frame) so I'd rather just factor it out to a helper method in a separate PR.

@timhoffm timhoffm merged commit b9bdcea into matplotlib:master Sep 20, 2018
@lumberbot-app
Copy link

lumberbot-app bot commented Sep 20, 2018

Something went wrong ... Please have a look at my logs.

@anntzer anntzer deleted the annotationcontains branch September 20, 2018 21:35
@Carreau
Copy link
Contributor

Carreau commented Sep 21, 2018

@meeseeksdev backport to v3.0.x

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants