Skip to content

Fixes a bug that causes Click events to bubble up from Switch widgets.#2981

Merged
rodrigogiraoserrao merged 1 commit intoTextualize:mainfrom
seifertm:2366-switch-click-bubbling
Jul 22, 2023
Merged

Fixes a bug that causes Click events to bubble up from Switch widgets.#2981
rodrigogiraoserrao merged 1 commit intoTextualize:mainfrom
seifertm:2366-switch-click-bubbling

Conversation

@seifertm
Copy link
Copy Markdown
Contributor

Please review the following checklist.

  • Docstrings on all new or modified functions / classes
  • Updated documentation
  • Updated CHANGELOG.md (where appropriate)

This PR stops Click events from bubbling up from Switch widgets.

The PR adds a corresponding test that fails, if the App containing a Switch receives a Click event when the Switch is clicked and adjusts the changelog.

Closes #2366

from textual.widgets import Switch


async def test_switch_click_doesnt_bubble_up():
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can you please add a comment with a link to the issue?
We usually say something like "Regression test for ".

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks. You mentioned this in the meta issue, but for some reason I thought it referred to the changelog.

I added a docstring that references the original ticket.

Signed-off-by: Michael Seifert <m.seifert@digitalernachschub.de>
@seifertm seifertm force-pushed the 2366-switch-click-bubbling branch from 82af49e to febe363 Compare July 22, 2023 09:42
Copy link
Copy Markdown
Contributor

@rodrigogiraoserrao rodrigogiraoserrao left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

@willmcgugan I've rodrigoed this PR.
Let me know what you think.

yield Switch()

async def on_click(self) -> None:
raise AssertionError(
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.

Can you check this fails without the fix? I seem to recall an issue where assertion errors where swallowed when raised via pilot. (just being paranoid)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ahhh I see. I seem to recall Darren taking care of this, but you can never be too sure.
@seifertm, let us know what you find.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'm equally paranoid about these things, so I wanted to see my test fail without the fix as well. I commented out event.stop() and got this stack trace for the test when running pytest -k test_switch:

self = SwitchApp(title='SwitchApp', classes={'-dark-mode'})

    async def on_click(self) -> None:
>       raise AssertionError(
            "The app should never receive a click event when Switch is clicked."
        )
E       AssertionError: The app should never receive a click event when Switch is clicked.

tests/test_switch.py:13: AssertionError
-----Captured stderr call -----
╭───────────────────── Traceback (most recent call last) ──────────────────────╮
│ /home/michael/oss/textual/tests/test_switch.py:13 in on_click                │
│                                                                              │
│   10 │   │   │   yield Switch()                                              │
│   11 │   │                                                                   │
│   12 │   │   async def on_click(self) -> None:                               │
│ ❱ 13 │   │   │   raise AssertionError(                                       │
│   14 │   │   │   │   "The app should never receive a click event when Switch │
│   15 │   │   │   )                                                           │
│   16                                                                         │
│                                                                              │
│ ╭────────────────────────── locals ───────────────────────────╮              │
│ │ self = SwitchApp(title='SwitchApp', classes={'-dark-mode'}) │              │
│ ╰─────────────────────────────────────────────────────────────╯              │
╰──────────────────────────────────────────────────────────────────────────────╯
AssertionError: The app should never receive a click event when Switch is 
clicked.
----- snapshot report summary -----
1 snapshot passed.
===== short test summary info =====
FAILED tests/test_switch.py::test_switch_click_doesnt_bubble_up - AssertionError: The app should never receive a click event when Switch is clicked.
====== 1 failed, 6 passed, 1518 deselected in 2.42s =====
(

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.

Works for me!

@rodrigogiraoserrao rodrigogiraoserrao merged commit 42dc3af into Textualize:main Jul 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Switch should stop the Click event from bubbling

3 participants