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

[Doc]: annotated_cursor example seems broken #25107

Closed
anntzer opened this issue Jan 29, 2023 · 6 comments · Fixed by #25129
Closed

[Doc]: annotated_cursor example seems broken #25107

anntzer opened this issue Jan 29, 2023 · 6 comments · Fixed by #25129
Labels
Documentation Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. status: confirmed bug topic: widgets/UI
Milestone

Comments

@anntzer
Copy link
Contributor

anntzer commented Jan 29, 2023

Documentation Link

https://matplotlib.org/stable/gallery/widgets/annotated_cursor.html

Problem

As far as I can see, the annotated_cursor example doesn't display the cursor text position anymore (as of mpl3.7.0rc1 on qtagg).

Suggested improvement

No response

@dstansby
Copy link
Member

I can't get this to work on macosx backend either.

@dstansby
Copy link
Member

Bisects to 733fbb0

@anntzer anntzer added the Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. label Jan 30, 2023
@ksunden
Copy link
Member

ksunden commented Jan 30, 2023

Something similar (with raw multicursor) was actually noticed prior to merge and merged despite this issue: #19763 (review)

Also reference to #24845 for where the precise issue was fixed, and is likely similar to the fix needed here.

@ksunden
Copy link
Member

ksunden commented Jan 30, 2023

Simply adding the preceding _ in the example subclass does fix the issue.

Git diff
diff --git a/examples/widgets/annotated_cursor.py b/examples/widgets/annotated_cursor.py
index eabec859fe..42af364686 100644
--- a/examples/widgets/annotated_cursor.py
+++ b/examples/widgets/annotated_cursor.py
@@ -105,7 +105,7 @@ class AnnotatedCursor(Cursor):
         # The position at which the cursor was last drawn
         self.lastdrawnplotpoint = None
 
-    def onmove(self, event):
+    def _onmove(self, event):
         """
         Overridden draw callback for cursor. Called when moving the mouse.
         """
@@ -124,7 +124,7 @@ class AnnotatedCursor(Cursor):
         if event.inaxes != self.ax:
             self.lastdrawnplotpoint = None
             self.text.set_visible(False)
-            super().onmove(event)
+            super()._onmove(event)
             return
 
         # Get the coordinates, which should be displayed as text,
@@ -152,7 +152,7 @@ class AnnotatedCursor(Cursor):
         # Baseclass redraws canvas and cursor. Due to blitting,
         # the added text is removed in this call, because the
         # background is redrawn.
-        super().onmove(event)
+        super()._onmove(event)
 
         # Check if the display of text is still necessary.
         # If not, just return.
@@ -255,7 +255,7 @@ class AnnotatedCursor(Cursor):
         # Return none if there is no good related point for this x position.
         return None
 
-    def clear(self, event):
+    def _clear(self, event):
         """
         Overridden clear callback for cursor, called before drawing the figure.
         """
@@ -263,7 +263,7 @@ class AnnotatedCursor(Cursor):
         # The base class saves the clean background for blitting.
         # Text and cursor are invisible,
         # until the first mouse move event occurs.
-        super().clear(event)
+        super()._clear(event)
         if self.ignore(event):
             return
         self.text.set_visible(False)
@@ -274,7 +274,7 @@ class AnnotatedCursor(Cursor):
 
         Passes call to base class if blitting is activated, only.
         In other cases, one draw_idle call is enough, which is placed
-        explicitly in this class (see *onmove()*).
+        explicitly in this class (see *_onmove()*).
         In that case, `~matplotlib.widgets.Cursor` is not supposed to draw
         something using this method.
         """

Ultimately, the problem is that the subclass is overriding behavior using previously public methods that are called internally, but the internal calls use the _ prefixed method, so the overrides don't get called.

If not, should we undo that deprecation?

(It was also merged without updating the deprecation version, but that was remedied in #24750)

@ksunden ksunden closed this as completed Jan 30, 2023
@ksunden ksunden reopened this Jan 30, 2023
@ksunden
Copy link
Member

ksunden commented Jan 30, 2023

(accidental close while commenting)

@anntzer
Copy link
Contributor Author

anntzer commented Jan 30, 2023

Looking at it again,

  1. I think that onmove and clear actually need to be public APIs (technically, publically overriddable) on Cursor for that widget to be useful (well, other than just displaying a crosshair with absolutely no extra info, which seems a bit pointless);
  2. OTOH, even with these as public API, the overriding done in annotated_cursor.py is just extremely complicated (and tightly coupled to the class internals, as this issue shows); compare with cursor_demo.py which implements essentially the same features in ~4x fewer lines and is much easier to follow (true, the snapping is to the closest point and not only decided by x/y, but that could easily be changed).

Therefore, I would suggest 1) restoring onmove() and clear() as public APIs (grandfathering an essentially frozen version of the Cursor class in, as it goes all the way back to 2005), and 2) getting rid of annotated_cursor.py (because we really don't want to encourage users to do that, and should rather point them to cursor_demo.py). If really desired we could augment cursor_demo to implement tracking and text positioning as in annotated_cursor, but I think it's optional.

@tacaswell tacaswell added this to the v3.7.0 milestone Feb 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Documentation Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. status: confirmed bug topic: widgets/UI
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants