RadioButtons: fix self._clicked method (followup to #30997) by doronbehar · Pull Request #31031 · matplotlib/matplotlib · GitHub
Skip to content

RadioButtons: fix self._clicked method (followup to #30997) - #31031

Merged
QuLogic merged 2 commits into
matplotlib:mainfrom
doronbehar:_Buttons-fixup
Jan 30, 2026
Merged

QuLogic merged 2 commits into
matplotlib:mainfrom
doronbehar:_Buttons-fixup

Conversation

@doronbehar

Copy link
Copy Markdown
Contributor

PR summary

In #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 #30997 the CheckButtons._clicked method could have used self._checks and not self._frames. Hence this change should be harmless.

PR checklist

@larsoner

Copy link
Copy Markdown
Contributor

@QuLogic QuLogic left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This worked in my testing.

@doronbehar

Copy link
Copy Markdown
Contributor Author

Adding a test is indeed a good idea. I added it before the fix, so it'd be easier to verify it works well.

Comment thread lib/matplotlib/tests/test_widgets.py Outdated
Comment thread lib/matplotlib/tests/test_widgets.py Outdated
@doronbehar

Copy link
Copy Markdown
Contributor Author

Thanks for the comments :).

@doronbehar
doronbehar requested a review from QuLogic January 29, 2026 08:20
@QuLogic

QuLogic commented Jan 29, 2026

Copy link
Copy Markdown
Member

Did you recheck the test before the change? It seems to pass if I run all of test_widgets.py, but fails if running only the one test. I'm not sure what's going on there.

PS, there's a typo in the first commit message.

@doronbehar

Copy link
Copy Markdown
Contributor Author

Did you recheck the test before the change?

Yes, I checked before the last commit of the PR and it fails there, and after the last commit of the PR it succeeds.

It seems to pass if I run all of test_widgets.py, but fails if running only the one test. I'm not sure what's going on there.

I have no idea why is that. What is the error you get? I tested it myself now and it works as explained above.

PS, there's a typo in the first commit message.

Do you mean it should be: widets: test _Buttons' callbacks and not widets: test _Buttons callbacks?

@QuLogic

QuLogic commented Jan 29, 2026

Copy link
Copy Markdown
Member

It seems to pass if I run all of test_widgets.py, but fails if running only the one test. I'm not sure what's going on there.

I have no idea why is that. What is the error you get? I tested it myself now and it works as explained above.

No error, I mean it incorrectly passes before the fix if run together.

PS, there's a typo in the first commit message.

Do you mean it should be: widets: test _Buttons' callbacks and not widets: test _Buttons callbacks?

It should be widgets.

@QuLogic

QuLogic commented Jan 29, 2026

Copy link
Copy Markdown
Member

I've narrowed it down to if you run test_span_selector_animated_artists_callback first then the test erroneously passes before the fix is made:

$ pytest lib/matplotlib/tests/test_widgets.py -k 'test_span_selector_animated_artists_callback or test__buttons' -v
========== test session starts ==========
platform linux -- Python 3.11.4, pytest-7.4.0, pluggy-1.2.0 -- python3.11
cachedir: .pytest_cache
rootdir: .../matplotlib
configfile: pyproject.toml
plugins: cov-4.1.0, rerunfailures-12.0, timeout-2.1.0, xdist-3.3.1
collected 126 items / 123 deselected / 3 selected                                                                                                                                                                  
lib/matplotlib/tests/test_widgets.py::test_span_selector_animated_artists_callback PASSED [ 33%]
lib/matplotlib/tests/test_widgets.py::test__buttons_callbacks[RadioButtons] PASSED        [ 66%]
lib/matplotlib/tests/test_widgets.py::test__buttons_callbacks[CheckButtons] PASSED        [100%]
========== 3 passed, 123 deselected in 3.25s ==========

@QuLogic

QuLogic commented Jan 29, 2026

Copy link
Copy Markdown
Member

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 on_clicked with a Mock and assert that it was called after the mouse event? You may have to tweak the coordinates a bit.

@doronbehar

Copy link
Copy Markdown
Contributor Author

PS, there's a typo in the first commit message.

Do you mean it should be: widets: test _Buttons' callbacks and not widets: test _Buttons callbacks?

It should be widgets.

OK 😄 done.

I'm going to open a separate issue for that as it's a deeper, but unrelated, testing problem.

OK 🙏 . For the record, I also tried to debug it a little bit, and this seems to unrelated to the get_ax pytest fixup, meaning the following diff:

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.

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.
@doronbehar

Copy link
Copy Markdown
Contributor Author

Edit: can you also modify the test to call on_clicked with a Mock and assert that it was called after the mouse event? You may have to tweak the coordinates a bit.

That wasn't trivial, but indeed it was a good idea.

@doronbehar

Copy link
Copy Markdown
Contributor Author

I'm going to open a separate issue for that as it's a deeper, but unrelated, testing problem.

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.

@QuLogic

QuLogic commented Jan 30, 2026

Copy link
Copy Markdown
Member

@QuLogic QuLogic added this to the v3.11.0 milestone Jan 30, 2026
@QuLogic
QuLogic merged commit e046a98 into matplotlib:main Jan 30, 2026
39 of 40 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants