RadioButtons: fix self._clicked method (followup to #30997) - #31031
Conversation
b793c78 to
cf302e8
Compare
|
Adding a test is indeed a good idea. I added it before the fix, so it'd be easier to verify it works well. |
cf302e8 to
73f4b14
Compare
|
Thanks for the comments :). |
|
Did you recheck the test before the change? It seems to pass if I run all of PS, there's a typo in the first commit message. |
Yes, I checked before the last commit of the PR and it fails there, and after the last commit of the PR it succeeds.
I have no idea why is that. What is the error you get? I tested it myself now and it works as explained above.
Do you mean it should be: |
No error, I mean it incorrectly passes before the fix if run together.
It should be |
|
I've narrowed it down to if you run |
|
I'm going to open a separate issue for that as it's a deeper, but unrelated, testing problem. Edit: can you also modify the test to call |
OK 😄 done.
OK 🙏 . For the record, I also tried to debug it a little bit, and this seems to unrelated to the diff --git i/lib/matplotlib/tests/test_widgets.py w/lib/matplotlib/tests/test_widgets.py
index 8b421aab33..02e653e1e8 100644
--- i/lib/matplotlib/tests/test_widgets.py
+++ w/lib/matplotlib/tests/test_widgets.py
@@ -1220,10 +1220,16 @@ def test_check_button_props(fig_test, fig_ref):
@pytest.mark.parametrize("widget", [widgets.RadioButtons, widgets.CheckButtons])
-def test__buttons_callbacks(ax, widget):
+def test__buttons_callbacks(widget):
+ fig, ax = plt.subplots()
"""Tests what https://github.com/matplotlib/matplotlib/pull/31031 fixed"""
button = widget(ax, ["Test Button"])
- MouseEvent._from_ax_coords("button_press_event", ax, (200, 200), 1)._process()
+ fig.canvas.callbacks.process(
+ "button_press_event",
+ MouseEvent(
+ name="test", canvas=ax.figure.canvas, x=200, y=200, button=1, key=None
+ ),
+ )
def test_slider_slidermin_slidermax_invalid():Doesn't fix the issue. |
73f4b14 to
92cdf9f
Compare
In matplotlib#30997 the classes `RadioButtons` & `CheckButtons` started sharing more code, as they are fundamentally similar. When copy-pasting the methods that were seemingly identical, the `_clicked` method was copied from the original `CheckButtons` class, and there the `self._frames` object was used instead of `self._buttons`. This caused an error when actually using and clicking on buttons created with `RadioButtons`, as the `RadioButtons._frames` doesn't exist - something that unfortunately the tests did not catch. Both `CheckButtons._frames` and `CheckButtons._buttons` are very similar so even before matplotlib#30997 the `CheckButtons._clicked` method could have used `self._checks` and not `self._frames`. Hence this change should be harmless.
92cdf9f to
f701624
Compare
That wasn't trivial, but indeed it was a good idea. |
I feel like the current test sort of hides this problem, so I wasn't sure whether a comment should be written to explain that. |

PR summary
In #30997 the classes
RadioButtons&CheckButtonsstarted sharing more code, as they are fundamentally similar. When copy-pasting the methods that were seemingly identical, the_clickedmethod was copied from the originalCheckButtonsclass, and there theself._framesobject was used instead ofself._buttons. This caused an error when actually using and clicking on buttons created withRadioButtons, as theRadioButtons._framesdoesn't exist - something that unfortunatelythe tests did not catch.
Both
CheckButtons._framesandCheckButtons._buttonsare very similar so even before #30997 theCheckButtons._clickedmethod could have usedself._checksand notself._frames. Hence this change should be harmless.PR checklist