Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork8.1k
Fix Legend.set_draggable() with update="bbox"#25428
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| self._update_loc(self.get_loc_in_canvas()) | ||
| elifself._update=="bbox": | ||
| self._bbox_to_anchor(self.get_loc_in_canvas()) | ||
| self._update_bbox_to_anchor(self.get_loc_in_canvas()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Does that mean none of this is tested?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
It only gets triggered when dragging a legend, afaik mpl isn't testing any interactivity, is it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I think it's possible to test interactivity. However, whoever put the typo in didn't test, or tested the wrong thing, so perhaps add to the PR description whatever test you are using to ensure this is the correct fix?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
It is possible to test the interactive code, but I think that is way too big of an ask to get a 1 line patch in!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Yes, sorry, I wasn't meaning to ask for that if its hard, but at least some idea how this should be tested manually.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@daniilS, since you seem to be doing some interactive PRs, you might be interested in taking a look here for how zoom and other interactive things like widgets are tested:https://github.com/matplotlib/matplotlib/blob/b636e9ed82b930ad4bde0b51360855fa79a7b438/lib/matplotlib/tests/test_backend_bases.py#LL142C3-L142C3
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@greglucas good point - I think all my other PRs were visual changes or toolbar behaviour which is hard to test automatically, but I might be able to write a test for dragging legends as the interaction is happening entirely within the axes.
tacaswell commentedMar 10, 2023
Looks like this was broken in0f7770a (?!) so this is apparently not a well used bit of functionality. |
tacaswell commentedMar 10, 2023
importmatplotlib.pyplotaspltplt.plot(range(5),label='bob')leg=plt.legend()leg.set_draggable(True,update='bbox')plt.show() is enough (with some dragging) to reproduce. |
tacaswell left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
manually tested with code in comments.
tacaswell commentedMar 10, 2023
the appeveyor failure is#25416 |
…428-on-v3.7.xBackport PR#25428 on branch v3.7.x (Fix Legend.set_draggable() with update="bbox")
Typo causes
Legend.set_draggable(True, update="bbox")to be broken.