One of the issues here is that templates.js uses things such as {strong_}{_strong}, which aren't supported in native notifications. We could use Unicode characters for this, to an extent, but converting may need large lookup tables. We could also ignore the tags.

Also, how should the native notifications behave? I mean, when should they be shown? Does the app being open make a difference? Does the app being open and active make a difference?

We could also ignore the tags.

👍 Yeah let's just do that.

Also, how should the native notifications behave? I mean, when should they be shown? Does the app being open make a difference? Does the app being open and active make a difference?

Great questions!

I think we should behave like all other apps. IIRC Slack doesn't show these notifications when the user has it open and active.

Since you asked that last question twice, it sounds to me like you're saying there's a difference between "open" and "open and active". I know this difference exists on Desktop but I'm not sure it exist on mobile.

On Desktop, push notifications are still shown when "open", but not "open and active".

IIRC Slack doesn't show these notifications when the user has it open and active.
it sounds to me like you're saying there's a difference between "open" and "open and active"

Correct, the difference exists, although on mobile, as you say, I think it's generally not possible to be 'open' but not 'active' (maybe some tablets / phablets have this though).

I've looked at possible ways to implement this, and it seems like we could either do it in gi.notifications/emit or on the NOTIFICATION_EMITTED event. This would call makeNotification from nativeNotification.js and emit a notification.

Currently, makeNotification shows notifications when 'open and active' when it's called from the SW, but it's always shown when called from outside of the SW.

At first, assuming that we want to support non-SW environments, I thought that we could add a flag to makeNotification for non-SW environments (to skip the notification if the window is focused). However, this requires some careful thought because showing native notifications without a SW in this case would still result in a notification being shown if multiple tabs are open. In fact, it could result in several notifications being shown for the same event. So, maybe the simplest thing so far is to just implement this in the SW.

The second issue is that in-app notifications and native notifications work a bit differently. One of those differences is what I've already pointed out about the use of markup. Native notifications have these fields: { title, body, icon, path }, which may or may not be the same as for in-app notifications (for one, in app notifications don't have a title AFAICT).

À propos the path, one potential issue that I believe we missed, and we need to fix, is that it's insufficient for group-scope notifications. We also need to set the currentGroupId, which isn't part of the URL. This would need to be done in the handler for the notificationclick event, and we need to see how to do that when opening a new window.

Regarding templates.js, I think that the cleanest way to go about this is to define fields that are specific to native notifications. path seems to be directly or closely related to linkTo, but we'd still need to define title, body and icon (icon could maybe be the avatarUserID, when defined?; in-app notifications have icon, but we can't directly use those as they need to be image URLs).

Regarding 'periodic' notifications, you write:

For the ones that are on a timer, you'll probably need to have the server send a periodic push notification to wake up the service worker periodically (e.g. twice a day or something like that)

This is an option with its advantages, but it also has some disadvantages, in particular that we aren't supposed to send push events that won't result in notifications. We already send plenty of these, so it's not necessarily worse. However, an alternative approach would be for clients to request a 'wakeup' from the server at a specified time. This would reduce the likelihood of waking up the SW unnecessarily, but at the expense of some privacy from the server and more complexity.

In summary, this is how we could go about this:

1. Augment templates.js with fields specific to native notifications (I think at least title and body, possibly also icon).
2. Modify gi.notifications/emit or define an event handler for NOTIFICATION_EMITTED calling makeNotification.
3. Solve the currentGroupId issue for group-scoped native notifications (relevant for when the native notification is clicked)
4. Implement periodic notifications by either waking up the service worker at regular intervals or through on-demand wake up requests.

So, maybe the simplest thing so far is to just implement this in the SW.

👍

However, an alternative approach would be for clients to request a 'wakeup' from the server at a specified time. This would reduce the likelihood of waking up the SW unnecessarily, but at the expense of some privacy from the server and more complexity.

Yeah... let's avoid that complexity for now.

Augment templates.js with fields specific to native notifications (I think at least title and body, possibly also icon).

Templates does have body. For title, it's possible to maybe use the app name? See what looks good before deciding, and if you don't have to modify templates.js to add yet another field, don't. But it's also not a big deal if you decide to add title too. Again, go with what looks good.

Re icon, the SW should at the very least have the app's icon URL passed in to it.

Individual notifications could also specify icon to override the app's icon (if needed).

Templates does have body.

They do, but they use tags like {b_}, etc. Stripping them is error prone, while creating a new definition won't have this issue. Also, for some of them, the body should be made shorter to accommodate phone displays for notifications, and to account for the title attribute.

For title, it's possible to maybe use the app name

It's possible, but the app name already is part of the notification. We can leave it empty, but if it's a notification about a group, using, e.g., the group name could make more sense.

icon ...

If you look at templates.js, notifications have an icon, the issue is that these are things like 'exclamation-triangle'. I think we need image URLs for these.

They do, but they use tags like {b_}, etc. Stripping them is error prone, while creating a new definition won't have this issue.

It doesn't need to be error-prone. Introducing a duplicate would violate DRY.

We already have code that replaces the {b_} strings with other strings. All you would need to do would be to use that code to replace it with nothing instead.

Also, for some of them, the body should be made shorter to accommodate phone displays for notifications, and to account for the title attribute.

... The phone code could just trim the excess words and add an ellipsis if this is an issue, or the original string can be shortened. I'd really rather avoid violating DRY and writing the same thing twice.

We can leave it empty, but if it's a notification about a group, using, e.g., the group name could make more sense.

👍 (yeah, or chat channel name)

Introducing a duplicate would violate DRY.

Not so sure about this, as they're two similar things but used in different contexts. However,

We already have code that replaces the {b_} strings with other strings

Yeah, but that code is in the L function. See, e.g.,

      body: L('{strong_}{name}{_strong} was kicked out of the group. Contributions were updated accordingly.', {
        name: `${CHATROOM_MEMBER_MENTION_SPECIAL_CHAR}${data.memberID}`,
        ...LTags('strong')
      })

So, we'd be removing not the {} strings but rather the resulting HTML. (Correct) HTML processing is extremely difficult.

Now, if we followed my (more general) suggestion not to have L inject HTML directly (such as by using Markdown or otherwise deferring the conversion to HTML), we wouldn't have to be stripping HTML.