vaadin / collaboration-engine

The simplest way to build real-time collaboration into web apps
https://vaadin.com/collaboration
Other
3 stars 1 forks source link

Enable observing topics to see how many messages there is in a chat without joining it. #35

Closed Peppe closed 2 years ago

Peppe commented 3 years ago

Is your feature request related to a use case? Please describe. Often in chats, you have a list of channels, and when a channel has new messages, it is bolded and has a badge next to it on how many unread messages there are. Right now there is no way to observe a topic without

Here's what I would like to implement, focus on the channel list on the left Image

Describe the solution you'd like You can subscribe to how many messages are in each channel, and you can update the UI to match. A listener would fire when the number of unread messages changes, so that you could update the UI to match. Having a higher level UI to manage this would be plus.

Describe alternatives you've considered

Additional context Vaadin 20 app with CE.

Legioth commented 3 years ago

https://github.com/vaadin/collaboration-engine/issues/36 describes a generic mechanism that could also be used for this case.

heruan commented 3 years ago

API discussed in grooming session:

public class MessageManager extends AbstractCollaborationManager {
    public MessageAdapter(Component component, UserInfo localUser, String topic) {}

    public MessageAdapter(Component component, UserInfo localUser, String topic, CollaborationMessagePersister persister) {}

    void setNewMessageHandler(NewMessageHandler handler) {}

    CompletableFuture<Void> submit(String text) {}
}

@FunctionalInterface
interface NewMessageHandler {
    handleNewMessage(MessageContext context);
}

interface MessageContext {
    CollaborationMessage getMessage();
}

abstract class AbstractCollaborationManager {
    AbstractCollaborationManager(ConnectionContext context,
            UserInfo localUser, String topicId,
            CollaborationEngine collaborationEngine) {}

    protected abstract Registration onConnectionActivate(TopicConnection topicConnection);

    public UserInfo getLocalUser() {}

    public String getTopicId() {}

    public void close() {}
}
heruan commented 3 years ago

Leif's concerns from Slack discussed during the daily:

Yesterday when discussing MessageAdapter, we were considering the option of not having a setTopic method so that we wouldn't have to deal with how the list of messages would have to be reset with synthetic remove events. I've realized that this won't be enough unless we also make another semantic change. The problem is that we're also doing the same thing to emulate emptying and then populating again every time the connection context is deactivated and then activated again. This means that we have two options for avoiding the need for synthetic remove events:

  • Fire some kind of clear event instead of firing individual remove events - this would either be to a separate handler (which requires bookkeeping in application code to coordinate between the two handlers) or as another event type to the existing handler (which means that the handler would have to do a switch or similar over the event type).
  • Change the semantics so that clearing would only happen by actually removing things from the topic. The easy part of this is that there wouldn't be a setTopic method but users would instead have to create a new adapter instance. The difficult part is that the adapter would have to cache contents while deactivated and then do some kind of diff operation when reactivated in order to emulate the missed events (or alternatively, the topic would preserve the list of events that it has processed so that reactivation can replay the original events that have been missed).

Luis suggested looking for common practices in similar scenarios.

Legioth commented 3 years ago

At the core of this dilemma is how getMessages() should behave when a topic connection is not available. This can happen through setTopic() or because the connection context is not activated (either because of a detached component or in the future with a standalone server also because of connectivity issues between servers).

One option is that the adapter keeps a cache of the messages so that getMessages() can return the latest known snapshot of messages in all situations. When a connection to the topic is reestablished, the adapter can compare the cached values to the values in the topic and produce synthetic events to the message handler to bring it up-to-date with the latest state from the topic. One drawback of this approach is that it leads to increased memory usage since each adapter instance will end up with its own copy of the message data, regardless of whether the application code that uses the adapter has any direct need for ever using getMessages(). This would get even worse in a possible future when we implement lazy loading for chat messages.

The opposite approach is that getMessages() is directly reading values from the topic connection and thus returns an empty stream when there's no connection. When a connection is opened again, the handler receives a new set of events for all existing messages. This means that the handler also needs to know that a reset has happened so that it can discard any state related to old copies of messages (in the case of a counter that incrementally counts unread messages, the count would have to be reset to 0 to avoid counting the same message twice). It might not be immediately obvious for a user that they need to take this case into account which might lead to bugs in the rare cases when reactivation happens. We could help the user realize they need to take the case into account by requiring that the handler for new messages returns a registration for handling removal.

There's also a possible intermediate that would require some additional low-level support from the topic itself. If the topic would record all processed events instead of only updating topic data, then it would be possible for the adapter to find all events that it has missed while disconnected and thus also be able to emit exactly the events that the message handler needs to become up-to-date again. One gotcha with this approach is that getMessages() would still be empty when the connection is missing, thus causing it to become inconsistent compared to what the message handler has seen. It might be necessary to have some way of signalling the difference between getMessages() being empty because the topic is empty and because there is no connection to the topic.

heruan commented 3 years ago

Let's get start with this as discussed in the daily: getMessages() will return an empty stream either if there are no messages or the connection is deactivated (this is also to be consistent with PresenceAdapter#getUsers()).

Then the adapter needs to keep a reference to the handled messages so as to avoid calling the handlers twice for the same message: this can be done by ~adding an identifier to CollaborationMessage and store the id of the last message the handler has been called for~ by storing the recent messages based on their timestamp and skip handler invocation for them when the connection is reactivated.

The message reference should be relative to the handler, so if the handler is replaced the new one will be called for every message in the topic.