Add a model to the chat panel #1
Conversation
There was a problem hiding this comment.
Thanks for doing this @brichet !
This looks all good to me. I only have a small comment/suggestion/question concerning the method namings in the base ChatModel
src/model.ts
Outdated
| * @param message - the message to send. | ||
| * @returns whether the message has been sent or not, or nothing if not needed. | ||
| */ | ||
| sendMessage( |
There was a problem hiding this comment.
Just a naming question. I'm wondering if addMessage would be better.
In my mind, in this place we're changing the chat model to add a new message into it. Whether it is "sent" is entirely to the chatmodel implementation detail.
| sendMessage( | |
| addMessage( |
There was a problem hiding this comment.
When looking into implementing a CRDT-based chat, we'd add a message to the data structure. The sending of the changes would not be handled by the chat implementation.
There was a problem hiding this comment.
So I wouldn't make the base ChatModel depend on ChatService, it seems to me ChatService would be an implementation detail of the websocket chat implementation?
There was a problem hiding this comment.
it seems to me ChatService would be an implementation detail of the websocket chat implementation?
It is true for part of it, but some of the types/interfaces are more generics and used by the view.
I will split it, to import only the required types.
…y from an extension, and remove everything related to socket handler in the core
This PR adds a model to the chat, which must be used to implement a chat panel.
It also provides a websocket handler extending this model, as one possible implementation of the model.
The idea of this change is to be able to create a CRDT model (based on this model) in jupyter_collaboration.