From 7e31e29a7bd1005b599af813e0c2b9270d68ae17 Mon Sep 17 00:00:00 2001 From: Will McGugan Date: Mon, 11 Sep 2023 11:27:28 +0100 Subject: [PATCH 1/4] Make notify thread-safe --- CHANGELOG.md | 6 ++++++ src/textual/app.py | 21 ++++++++++++------- src/textual/notifications.py | 9 ++++++++ src/textual/widget.py | 9 ++++---- src/textual/widgets/_toast.py | 2 +- tests/notifications/test_app_notifications.py | 2 +- 6 files changed, 35 insertions(+), 14 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e824df982c..eeb26a3a6b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,12 @@ and this project adheres to [Semantic Versioning](http://semver.org/). - Fixed a crash when removing an option from an `OptionList` while the mouse is hovering over the last option https://github.com/Textualize/textual/issues/3270 +### Changed + +- Widget.notify and App.notify are now thread-safe +- Breaking change: Widget.notify and App.notify now return None +- App.unnotify is now private (renamed to App._unnotify) + ## [0.36.0] - 2023-09-05 ### Added diff --git a/src/textual/app.py b/src/textual/app.py index 610bbcd604..bee2e39d2d 100644 --- a/src/textual/app.py +++ b/src/textual/app.py @@ -90,7 +90,7 @@ _get_unicode_name_from_key, ) from .messages import CallbackType -from .notifications import Notification, Notifications, SeverityLevel +from .notifications import Notification, Notifications, Notify, SeverityLevel from .reactive import Reactive from .renderables.blank import Blank from .screen import Screen, ScreenResultCallbackType, ScreenResultType @@ -2931,18 +2931,20 @@ def notify( title: str = "", severity: SeverityLevel = "information", timeout: float = Notification.timeout, - ) -> Notification: + ) -> None: """Create a notification. + !!! tip + + This method is thread-safe. + + Args: message: The message for the notification. title: The title for the notification. severity: The severity of the notification. timeout: The timeout for the notification. - Returns: - The new notification. - The `notify` method is used to create an application-wide notification, shown in a [`Toast`][textual.widgets._toast.Toast], normally originating in the bottom right corner of the display. @@ -2977,11 +2979,14 @@ def notify( ``` """ notification = Notification(message, title, severity, timeout) - self._notifications.add(notification) + self.post_message(Notify(notification)) + + def _on_notify(self, event: Notify) -> None: + """Handle notification method.""" + self._notifications.add(event.notification) self._refresh_notifications() - return notification - def unnotify(self, notification: Notification, refresh: bool = True) -> None: + def _unnotify(self, notification: Notification, refresh: bool = True) -> None: """Remove a notification from the notification collection. Args: diff --git a/src/textual/notifications.py b/src/textual/notifications.py index 242ba895e4..e1a9fbae44 100644 --- a/src/textual/notifications.py +++ b/src/textual/notifications.py @@ -10,10 +10,19 @@ from rich.repr import Result from typing_extensions import Literal, Self, TypeAlias +from .message import Message + SeverityLevel: TypeAlias = Literal["information", "warning", "error"] """The severity level for a notification.""" +@dataclass +class Notify(Message, bubble=False): + """Message to show a notification.""" + + notification: Notification + + @dataclass class Notification: """Holds the details of a notification.""" diff --git a/src/textual/widget.py b/src/textual/widget.py index f63b347749..2f79d4c20b 100644 --- a/src/textual/widget.py +++ b/src/textual/widget.py @@ -3391,18 +3391,19 @@ def notify( title: str = "", severity: SeverityLevel = "information", timeout: float = Notification.timeout, - ) -> Notification: + ) -> None: """Create a notification. + !!! tip + + This method is thread-safe. + Args: message: The message for the notification. title: The title for the notification. severity: The severity of the notification. timeout: The timeout for the notification. - Returns: - The new notification. - See [`App.notify`][textual.app.App.notify] for the full documentation for this method. """ diff --git a/src/textual/widgets/_toast.py b/src/textual/widgets/_toast.py index a09594189b..dff62b5a5d 100644 --- a/src/textual/widgets/_toast.py +++ b/src/textual/widgets/_toast.py @@ -128,7 +128,7 @@ def _expire(self) -> None: # the notification that caused us to exist. Note that we tell the # app to not bother refreshing the display on our account, we're # about to handle that anyway. - self.app.unnotify(self._notification, refresh=False) + self.app._unnotify(self._notification, refresh=False) # Note that we attempt to remove our parent, because we're wrapped # inside an alignment container. The testing that we are is as much # to keep type checkers happy as anything else. diff --git a/tests/notifications/test_app_notifications.py b/tests/notifications/test_app_notifications.py index 01f54ae605..d841de346d 100644 --- a/tests/notifications/test_app_notifications.py +++ b/tests/notifications/test_app_notifications.py @@ -25,7 +25,7 @@ async def test_app_with_removing_notifications() -> None: async with NotificationApp().run_test() as pilot: notification = pilot.app.notify("test") assert len(pilot.app._notifications) == 1 - pilot.app.unnotify(notification) + pilot.app._unnotify(notification) assert len(pilot.app._notifications) == 0 From bbb19472e1f00dd6bcbac5421e015bd96347ce95 Mon Sep 17 00:00:00 2001 From: Will McGugan Date: Mon, 11 Sep 2023 11:35:17 +0100 Subject: [PATCH 2/4] test fixes --- CHANGELOG.md | 6 +++--- tests/notifications/test_app_notifications.py | 6 +++++- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index eeb26a3a6b..f8797ddaa6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,9 +13,9 @@ and this project adheres to [Semantic Versioning](http://semver.org/). ### Changed -- Widget.notify and App.notify are now thread-safe -- Breaking change: Widget.notify and App.notify now return None -- App.unnotify is now private (renamed to App._unnotify) +- Widget.notify and App.notify are now thread-safe https://github.com/Textualize/textual/pull/3275 +- Breaking change: Widget.notify and App.notify now return None https://github.com/Textualize/textual/pull/3275 +- App.unnotify is now private (renamed to App._unnotify) https://github.com/Textualize/textual/pull/3275 ## [0.36.0] - 2023-09-05 diff --git a/tests/notifications/test_app_notifications.py b/tests/notifications/test_app_notifications.py index d841de346d..5b6eeae3c2 100644 --- a/tests/notifications/test_app_notifications.py +++ b/tests/notifications/test_app_notifications.py @@ -17,6 +17,7 @@ async def test_app_with_notifications() -> None: """An app with notifications should have notifications in the list.""" async with NotificationApp().run_test() as pilot: pilot.app.notify("test") + await pilot.pause() assert len(pilot.app._notifications) == 1 @@ -24,8 +25,9 @@ async def test_app_with_removing_notifications() -> None: """An app with notifications should have notifications in the list, which can be removed.""" async with NotificationApp().run_test() as pilot: notification = pilot.app.notify("test") + await pilot.pause() assert len(pilot.app._notifications) == 1 - pilot.app._unnotify(notification) + pilot.app._unnotify(list(pilot.app._notifications)[0]) assert len(pilot.app._notifications) == 0 @@ -34,6 +36,7 @@ async def test_app_with_notifications_that_expire() -> None: async with NotificationApp().run_test() as pilot: for n in range(100): pilot.app.notify("test", timeout=(0.5 if bool(n % 2) else 60)) + await pilot.pause() assert len(pilot.app._notifications) == 100 sleep(0.6) assert len(pilot.app._notifications) == 50 @@ -44,6 +47,7 @@ async def test_app_clearing_notifications() -> None: async with NotificationApp().run_test() as pilot: for _ in range(100): pilot.app.notify("test", timeout=120) + await pilot.pause() assert len(pilot.app._notifications) == 100 pilot.app.clear_notifications() assert len(pilot.app._notifications) == 0 From 4b7fb23e8155a788b465ad6ee03b2e7342274bfe Mon Sep 17 00:00:00 2001 From: Will McGugan Date: Mon, 11 Sep 2023 11:55:02 +0100 Subject: [PATCH 3/4] docstring --- src/textual/app.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/textual/app.py b/src/textual/app.py index bee2e39d2d..b1699f30f6 100644 --- a/src/textual/app.py +++ b/src/textual/app.py @@ -2982,7 +2982,7 @@ def notify( self.post_message(Notify(notification)) def _on_notify(self, event: Notify) -> None: - """Handle notification method.""" + """Handle notification message.""" self._notifications.add(event.notification) self._refresh_notifications() From f13f6331a724612a79a083db3a80379eb10a37e7 Mon Sep 17 00:00:00 2001 From: Will McGugan Date: Mon, 11 Sep 2023 13:24:36 +0100 Subject: [PATCH 4/4] Update tests/notifications/test_app_notifications.py Co-authored-by: Dave Pearson --- tests/notifications/test_app_notifications.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/notifications/test_app_notifications.py b/tests/notifications/test_app_notifications.py index 5b6eeae3c2..608fde812f 100644 --- a/tests/notifications/test_app_notifications.py +++ b/tests/notifications/test_app_notifications.py @@ -24,7 +24,7 @@ async def test_app_with_notifications() -> None: async def test_app_with_removing_notifications() -> None: """An app with notifications should have notifications in the list, which can be removed.""" async with NotificationApp().run_test() as pilot: - notification = pilot.app.notify("test") + pilot.app.notify("test") await pilot.pause() assert len(pilot.app._notifications) == 1 pilot.app._unnotify(list(pilot.app._notifications)[0])