Add support for desktop notifications

[benjajaja]

Basic notifications feature.

Can be enabled in config.json, disabled by default.

[paulcarroty]

Wow, nice work! Maybe worth telling about notifications: true in iamb config.

[benjajaja]

@paulcarroty I put it in docs/iamb.5.md, is there somewhere else where I should add it?

[benjajaja]

@ulyssa do you have some idea how I could get on about detecting if a room/dm is currently "open", to skip showing a notification in that case?

I suppose that the notification handler should only have access to ProgramStore and there isn't anything there that could tell if a chat is open.

BTW, it seems like modalkit::widgets::WindowOps<_>::close never gets called. If it would, perhaps that would be a good place to (un)track current windows, together with something else for opening.

[ulyssa]

BTW, it seems like modalkit::widgets::WindowOps<_>::close never gets called. If it would, perhaps that would be a good place to (un)track current windows, together with something else for opening.

It looks like it's missing a WindowOps::close call on the return value here. That should get fixed, but I don't think it will be enough to detect what's open here since it only gets called when the window is actually closed and not when navigated away from with a WindowAction::Switch.

do you have some idea how I could get on about detecting if a room/dm is currently "open", to skip showing a notification in that case?

There's not a great way to get this from outside the Application struct at the moment, since the ScreenState has all of the information need to make this work right. I think there are two choices:

1. Each time Application::redraw is called, update a draw_curr: Instant field in ChatStore; then, when rooms are drawn, copy draw_curr to a draw_last field in the RoomInfo. To determine if a room is open, you then only need to check whether draw_last == draw_curr.
2. Delay processing messages into Application. There are several ways this could be done, but I'm thinking that messages could be placed in a queue in ChatStore and then dequeued in Application::step just before the self.redraw call, so that we always show new messages when we draw. Another way to do this would be to send fetched messages over a tokio mpsc channel and try_recv them in Application, which could have the benefit of not needing to lock the store.

I think I like the second option the most. I've been taking a look at what handling intentional mentions could look like, and I think a similar processing mechanism could be used there. I'll try to implement this along with the current window checking, and then you can use that for notifications.

[ulyssa]

Can you make this a String instead that can be set to "all" or "none"? That way it can be configurable to other values in the future, like "mentions".

[ulyssa]

Or rather, a string in the config.json, but an enum that derives Deserialize here.

[benjajaja]

In Element.io, notifications are very fine-grained:

This then gets "multiplied" with per-chat settings, which only offer a few settings:

I have to investigate a bit more. I don't know if these settings are private to the client or shared with the server. It would make sense to have the server only send out necessary notifications per each client, that will actually be used. Looking at the matrix sdk, I don't fully understand what state goes where. At a glance, it looks like it does save some state to the homeserver.

In our config.json I think we should go further and start with something like this, if we want to deal with mentions already:

pub struct Notifications {
    pub one_to_one: NotificationProcessing,
    pub group_chats: NotificationProcessing,
    // TODO: separate displayname, username, @room, and keywords.
    pub mentions: NotificationProcessing,
    // TODO: invitations, call-invitation, bots, and room-upgrades.
}

pub enum NotificationProcessing {
    #[default]
    Off,
    On,
    Noisy, // Not sure, it's in element.io, probably means to stop after N amount of unacknowledged notifications.
}

[benjajaja]

Regarding option 1 or 2, my first instinct is against processing / showing notifications in the draw-loop. The app draws in an interval now, if I'm not mistaken, and it would maybe be nice to change it so it only draws on state changes or events in the future. In that scenario, notification could just be another event to trigger a draw, but overall it sounds like these two shouldn't be coupled together.

I now realize that the step and render can be independent, never mind the above. In the PR I tried for option 1 because that was easier.

[benjajaja]

I found out that the notification settings matrix of the screenshots can be retrieved and set with the SDK, which makes a lot of sense so that the user doesn't have to manually copy it over to each client. The client basically only needs one "enable in this session" toggle. We could later add commands to set or display those settings, but for now I think it's quite okay to leave it out.

I had to add a check that the notification-timestamp is greater than the app-startup-timestamp, because I am getting a barrage of past notifications. I'm not sure how a push notification is supposed to be "marked as read", perhaps we need to send a read-receipt.

The >100 characters check is also somewhat primitive. If the body contains a quote / is a reply, that should also be stripped out.

[ulyssa]

I found out that the notification settings matrix of the screenshots can be retrieved and set with the SDK, which makes a lot of sense so that the user doesn't have to manually copy it over to each client. The client basically only needs one "enable in this session" toggle. We could later add commands to set or display those settings, but for now I think it's quite okay to leave it out.

Yep, I agree with this. Getting notifications enabled is the first step, and eventually there can be :notifications ... commands for updating the shared notification settings for the room.

I noticed that the CI build failed on Windows. I've put up a fix for that in #221, and verified that it builds and notifications show up on Windows. (Although I'm not sure whether anyone is using iamb on Windows, since it looks like keyboard input got screwy after either a crossterm change or a Windows Terminal change, #220). If you pull in main, it should be able to pass CI.

Having tried the PR out locally, I think it's ready to merge. It looks like the manual page will need to be updated to account for notifications now being an object. Was there anything else you wanted to do with it before merging?

[benjajaja]

@ulyssa thank you for fixing that! I think it's good to merge now.

I updated the manual page.

The barrage of past notifications is gone, it seems like the SDK itself takes care of marking as delivered. I only see notifications that have never been delivered to this client / push-session. I am still leaving in the line that discard notifications that are from before startup_time, but we could make that configurable later on.