Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make notify thread-safe #3275

Merged
merged 4 commits into from
Sep 11, 2023
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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 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

### Added
Expand Down
21 changes: 13 additions & 8 deletions src/textual/app.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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 message."""
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:
Copy link
Member

Choose a reason for hiding this comment

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

I've never used this API but this method seems like it's really useful if it's public?

Is this method how we discard notifications manually? It seems useful if notifications are longer-lived - to allow us to remove ones that are no longer relevant.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The problem is not that self.notify() doesn't add the notification immediately. It posts a message, and returns None. That means that the user won't have a reference to the Notification object. And even if they did, it might not yet be active.

Programatically removing notifications is useful, but we will need another mechanism for that.

Copy link
Member

Choose a reason for hiding this comment

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

Not saying it's for this PR, but I think it'd be nice if notify still returned a Notification as a handle to a notification which may or may not be active. Then, if you try to unnotify a Notification that isn't active then it'd just prevent it from appearing.

"""Remove a notification from the notification collection.

Args:
Expand Down
9 changes: 9 additions & 0 deletions src/textual/notifications.py
Original file line number Diff line number Diff line change
Expand Up @@ -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."""
Expand Down
9 changes: 5 additions & 4 deletions src/textual/widget.py
Original file line number Diff line number Diff line change
Expand Up @@ -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.
"""
Expand Down
2 changes: 1 addition & 1 deletion src/textual/widgets/_toast.py
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
6 changes: 5 additions & 1 deletion tests/notifications/test_app_notifications.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,15 +17,17 @@ 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


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")
willmcgugan marked this conversation as resolved.
Show resolved Hide resolved
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


Expand All @@ -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
Expand All @@ -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