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

Synchronize notification state among clients #266

Open
eikendev opened this issue Feb 8, 2020 · 7 comments
Open

Synchronize notification state among clients #266

eikendev opened this issue Feb 8, 2020 · 7 comments
Labels
a:feature New feature or request

Comments

@eikendev
Copy link
Contributor

eikendev commented Feb 8, 2020

Currently, notifications that are read on one client are not automatically read on other clients. This means, when working on a PC and reading the notifications, the mobile phone would end up showing a collection of messages that have already been read.

A change was proposed in #171 to introduce a new type property to messages and let clients subscribe to new messages only. Although this would probably work, it's a bit hacky in my view.

I propose that we introduce a new API, that supports different events. To make Gotify backwards compatible with "old" clients, the current /stream endpoint will be enhanced by a GET parameter that specifies an API version, like /stream?version=1. The default API (if no version is specified), for now version 1, will support the current messages only. The clients will receive the same message format as right now, so that "old" clients do not break.

The new notification system is then reachable under /stream?version=2, and will push the types new_message and read_message.

This gives us more flexibility for future changes. If breaking changes occur, we can decide if older versions are still worth supporting, and drop support for versions we no longer see value in supporting.

I'd like to implement this in the following days, so please tell me if these changes are acceptable.

@eikendev eikendev added the a:feature New feature or request label Feb 8, 2020
@jmattheis
Copy link
Member

Currently, notifications that are read on one client are not automatically read on other clients. This means, when working on a PC and reading the notifications, the mobile phone would end up showing a collection of messages that have already been read.

Do you mean with read deleted or do you plan on adding a read flag to the message? In case it is really read then have a look at fc491f4 I've started implementing it months ago :D.

A change was proposed in #171 to introduce a new type property to messages and let clients subscribe to new messages only. Although this would probably work, it's a bit hacky in my view.

We need a type property. Otherwise, it wouldn't be possible to differentiate events or what exactly do you find hacky in this? Maybe you could add examples of the events that will be transmitted via websocket.

API versionizing seems ok.

@eikendev
Copy link
Contributor Author

eikendev commented Feb 9, 2020

Do you mean with read deleted or do you plan on adding a read flag to the message? In case it is really read then have a look at fc491f4 I've started implementing it months ago :D.

The point here is that the read/delete status is not communicated over the WebSocket connection. Clients have to poll for the read/delete status. But since there already is a WebSocket, we could announce changes of the read/delete status through that socket. That way, clients do not have to poll for it.

I think having a read flag does not hurt. Having a history of the recent notifications (including read ones) is not a bad idea. As I see you closed #21 because the flag was not needed. For having a history, I assume this does no longer hold?

For the synchronization this does not really matter. If you think it's worth having a read status instead of deletions we can try to build on top of your work in fc491f4.

We need a type property. Otherwise, it wouldn't be possible to differentiate events or what exactly do you find hacky in this? Maybe you could add examples of the events that will be transmitted via websocket.

Sorry for the poor phrasing. What I meant is that the proposal does not explain how new subscriptions are added or removed. The idea was to send all message types by default for the new API version.

As for the message examples, I'd add the field "type": "notification_new" to the existing messages for new messages. The new messages, which announce the read state of a notification, could look like the following.

{
  "type": "notification_read",
  "id": 25,
  "status": true
}

Of course, when sticking to notification deletions instead, this message then communicates the deletion.

@jmattheis
Copy link
Member

I think having a read flag does not hurt. Having a history of the recent notifications (including read ones) is not a bad idea. As I see you closed #21 because the flag was not needed. For having a history, I assume this does no longer hold?

For the synchronization this does not really matter. If you think it's worth having a read status instead of deletions we can try to build on top of your work in fc491f4.

I'm just asking what the scope of this issue is. I'd like to have it as small as possible. From what I've read, the first thing would be supporting the new stream version and adding the message deleted event. Events like application/client deletion or having a read flag on messages can be done in the future, as they won't break the API.

Having #21 implemented would have benefits because the android client then could hide notifications when they were read inside the browser.

I think we should have two events for message deletion

  • message_deleted
  • application_messages_deleted

Otherwise, we have a lot message_deleted events when deleting all messages of an application. The message_deleted event also could transmit a list of ids but that would (I think) be more complex to handle on clientside.

@eikendev
Copy link
Contributor Author

eikendev commented Feb 9, 2020

If we do not pass the IDs, we have a race condition. The impact is minimal, but I think implementing this cleanly would be preferable. Imagine a message is sent from an application at the same time a user deletes all their messages for that application. Clients could receive those two events in different orders.

I don't necessarily think having to parse a list of IDs is too complex with modern libraries like GSON. Do you mind if we try it?

@wsw70
Copy link

wsw70 commented Jul 11, 2020

+1 for that feature.

I just realized that my automated creation and deletion of stale messages via the API is not propagated to clients (in my case - the Android app).

Would it be possible, alternatively, to send a "refresh" message via the API, picked by the clients?

@jmattheis
Copy link
Member

Would it be possible, alternatively, to send a "refresh" message via the API, picked by the clients?

Yes, but we can just implement it correctly without a hacky workaround (:.

@wsw70
Copy link

wsw70 commented Jul 11, 2020

Yes, but we can just implement it correctly without a hacky workaround (:.

What I meant, reading the thread, that it might be easier / a first step to implement the refresh each client can make on its own anyway already today (when it starts, or when it is reloaded) without implications on race conditions and similar issues. Just a thought.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a:feature New feature or request
Development

Successfully merging a pull request may close this issue.

3 participants