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

Notifications #2866

Merged
merged 45 commits into from
Jul 17, 2023
Merged

Notifications #2866

merged 45 commits into from
Jul 17, 2023

Conversation

davep
Copy link
Contributor

@davep davep commented Jun 30, 2023

This PR seeks to satisfy #2846. In doing so it does the following:

  • Adds a general Notification/Notifications pair of classes for holding details on an individual notification, as well as holding a list of timed notifications.
  • Adds a small collection of widgets (which should sort of be considered to be "internal" at the moment, but which are named non-internal so as not to discourage styling) that show and handle the display of the above notifications via a Toast.
  • Adds a core App.notify method, which takes a message and a title, as well as an optional severity level (information, warning or error -- default being information) and an optional timeout (3 seconds by default).
  • Adds App.unnotify for removing a notification.
  • Adds App.clear_notifications for clearing notifications.
  • Adds a Screen.notify method, which is a thin wrapper around App.notify.
  • Adds a Widget.notify method, which is a thin wrapper around App.notify.

The Toasts are always shown above the main display, scrolling up from the bottom right, and they persist between screens and modes.

By way of illustration:

Screen.Recording.2023-07-12.at.13.14.02.mov

was produced with this code:

from pathlib import Path
from random import choice

from textual import on
from textual.app import App, ComposeResult
from textual.containers import Horizontal, Vertical, VerticalScroll
from textual.widgets import Markdown, Button, Checkbox, Input

QUOTES = [
    "Are either of you paleontologists? I'm in desperate need of a paleontologist.",
    "I should reach Defcon 1 and release my missiles in 28 hours. Would you like to see some projected kill ratios?",
    "A strange game."
    "The only winning move is not to play.",
    "How about a nice game of chess?",
    "Joshua called me.",
    "The WOPR spends all it's time thinking about World War III.",
    "Did you ever play tic-tac-toe?",
    "I loved it when you nuked Las Vegas."
]

class ToastyApp(App[None]):

    CSS = """
    Horizontal {
        align: center middle;
        height: auto;
    }
    Horizontal Button {
        margin-left: 1;
    }
    Input {
        width: 20%;
    }
    """

    def compose(self) -> ComposeResult:
        with Vertical():
            with Horizontal():
                yield Checkbox("Titled", value=True)
                yield Input(placeholder="Timeout (default 3 seconds)")
                yield Button("Information", id="information")
                yield Button("Warning", id="warning")
                yield Button("Error", id="error")
            with VerticalScroll():
                yield Markdown()

    async def on_mount(self) -> None:
        await self.query_one(Markdown).load(Path("/Users/davep/develop/python/textual/README.md"))

    @on(Button.Pressed)
    def yell(self, event: Button.Pressed) -> None:
        try:
            timeout = float(self.query_one(Input).value)
        except ValueError:
            timeout = 3
        self.notify(
            choice(QUOTES),
            severity=event.button.id,
            title=None if self.query_one(Checkbox).value else "",
            timeout=timeout
        )

if __name__ == "__main__":
    ToastyApp().run()

In addition to the above, a requirement some people may have (it's already been asked about as soon as this development was mentioned) is the ability to route the notifications elsewhere. While not directly handled right now, this can be done by overriding the App._refresh_notifications method and routing the notifications as required. As a very quick and dirty example on macOS, with the addition of this:

    def _refresh_notifications(self) -> None:
        for notification in self._notifications:
            title = notification.severity.capitalize() if notification.title is None else notification.title
            run([
                "osascript",
                "-e",
                f'display notification \"{notification.message}\" with title \"{title}\"'
            ])
        self._notifications.clear()

to ToastyApp above, the messages go via the normal macOS notification facility:

Screen.Recording.2023-07-12.at.13.50.48.mov

davep added 15 commits June 29, 2023 13:49
This provides the core classes for holding information on a single
notification, and then on top of that a class for managing a collection of
notifications.
Yes, this does go against all things Pythonic, but in this case it's likely
less costly to do the check first; moreover it works around the problem I
ran in to: Textualize#2863
This way the interface becomes "here's a bunch of notifications, you go work
this out".
When it was per-screen, it made sense that it was the timeout; now that
we're carrying them over between screens we're going to make sure they're
only around for as long as they need to be.
Except for the title, keep that.
Prefix with a - to reduce the chance of a clash with userspace.
The Toasts were removing themselves, but they're wrapped inside a helper
container that keeps them aligned right. So the problem was that the
alignment containers were leaking. This ensures that when a Toast goes away
it takes its parent with it.
This doesn't really make any difference, but it feels like it makes sense to
hide it if there's nothing to show -- it's purely for alignment.
@davep davep added enhancement New feature or request Task labels Jun 30, 2023
@davep davep self-assigned this Jun 30, 2023
@davep davep linked an issue Jun 30, 2023 that may be closed by this pull request
davep added 11 commits July 10, 2023 08:20
This is about getting the toasts to align correctly (even when you do align
things, they don't really align as expected due to the way that a container
aligns the bounding box of all if its children, not the individual
children). However, I had this named after where it aligned them to; someone
using the system may wish to change that, so let's make the name more
generic.
Add a docstring, and also change the format of the identity somewhat so that
it's even "more internal".
This might not be the final form, but it'll do for the moment. I want to get
the snapshot test in place at least.
This isn't going into the index, just yet. This is *technically* an internal
widget so I'm not sure how and where it makes sense to document it; if at
all. But let's get some documentation in here anyway.
Originally they weren't in the "internal" namespace, then I decided that
they should be so there's less chance of a clash with dev-space code; but I
forgot to reflect this in the docs.

This fixes that.
@davep davep marked this pull request as draft July 13, 2023 17:16
davep added 10 commits July 13, 2023 18:57
The addition of the ability to dismiss a toast by clicking on it had a flaw:
the notification->toast code had been written with things being one way. The
expiration of notifications happened in the notification handler, and the
expiration of Toasts was done in the Toast system, on purpose (notifications
might end up being routed via elsewhere so this needs to be done).

But... this meant that hand-removed Toasts kept coming back from the dead
when a new notification was raised iff the hand-removed ones hadn't yet
expired.

So here I add the ability the remove a notification from the notification
collection.
Sort of a hangover from what was initially looking like it was going to be a
longer body of code. It doesn't really need explaining any more.
This turns the method into one that further aids making the connection
between the notifications and the toasts two way. Now it makes sense that if
there are toasts for notifications that no longer exist, they also get
removed.

This makes it easier to add all sorts of clear options later on.
It can be seen as, and used as, a handle of sorts (see unnotify); so return
it so people can use it.
@davep davep marked this pull request as ready for review July 13, 2023 20:39
@willmcgugan
Copy link
Collaborator

@davep Forgive me for hacking your CSS. I've gone for something a little more minimal. Also made the width uniform.

Screenshot 2023-07-17 at 10 47 08

@davep
Copy link
Contributor Author

davep commented Jul 17, 2023

@willmcgugan If we're going with uniform width, I suspect we can drop ToastHolder (unless of course we want to allow for devs to have non-uniform width, I guess).

@davep
Copy link
Contributor Author

davep commented Jul 17, 2023

If we're going with uniform width, I suspect we can drop ToastHolder (unless of course we want to allow for devs to have non-uniform width, I guess).

For any other reader down the line: in local conversation we've decided to leave it as it is in case anyone does want to style notifications for their application such that they have non-uniform size.

@willmcgugan willmcgugan merged commit ca8eb3d into Textualize:main Jul 17, 2023
19 checks passed
@davep davep deleted the notifications-redux branch July 17, 2023 10:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request Task
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement Notification layer
2 participants