-
Notifications
You must be signed in to change notification settings - Fork 4
chat message reactions #319
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
Conversation
51f754b
to
077c262
Compare
textile/chat-features.textile
Outdated
* @(CHA-M11)@ A @Message@ must have a method @with@ to apply changes from events (@MessageEvent@ and @MessageReactionSummaryEvent@) to produce updated message instances. | ||
** @(CHA-M11a)@ @[Testable]@ When the method receives a @MessageEvent@ of type @created@, it must throw an @ErrorInfo@ with code @40000@ and status code @400@. | ||
** @(CHA-M11b)@ @[Testable]@ For @MessageEvent@ the method must verify that the @message.serial@ in the event matches the message's own serial. If they don't match, an error with code @40000@ and status code @400@ must be thrown. | ||
** @(CHA-M11c)@ @[Testable]@ For @MessageEvent@ of type @update@ and @delete@, if the event's @message.version@ is less than or equal to the current message's @version@, the original message must be returned unchanged. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Worth referencing this to CHA-M10
here, rather than explaining sortability again?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
textile/chat-features.textile
Outdated
** @(CHA-M11e)@ @[Testable]@ For @MessageReactionSummaryEvent@, the method must verify that the @summary.messageSerial@ in the event matches the message's own serial. If they don't match, an error with code @40000@ and status code @400@ must be thrown. | ||
** @(CHA-M11f)@ @[Testable]@ For @MessageReactionSummaryEvent@, the method must return a new @Message@ instance (deep copy) with the updated reactions, preserving all other properties of the original message. | ||
** @(CHA-M11g)@ @[Testable]@ For @MessageReactionSummaryEvent@, the method must deep-copy the reactions from the event before applying them to the returned message instance. | ||
** @(CHA-M11h)@ @[Testable]@ The method must not alter the original message instance. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This point is already covered in the points above, so is redundant
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed
textile/chat-features.textile
Outdated
|
||
Users can add reactions to messages, such as thumbs-up or heart emojis. Summaries (counts per reaction name, and who reacted) of the message reactions are stored with the message and are visible to all users in the room. Message reactions are powered by Pub/Sub message annotations. | ||
|
||
@MessagesReactions@ object is the entry point for interacting with Message Reactions. It shall be exposed to consumers via a @reactions@ property insde the @Messages@ obejct. From the @Room@ level: @room.messages.reactions@. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@MessagesReactions@ object is the entry point for interacting with Message Reactions. It shall be exposed to consumers via a @reactions@ property insde the @Messages@ obejct. From the @Room@ level: @room.messages.reactions@. | |
@MessagesReactions@ object is the entry point for interacting with Message Reactions. It shall be exposed to consumers via a @reactions@ property inside the @Messages@ obejct. From the @Room@ level: @room.messages.reactions@. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I've copy-pasted this locally, in a rebased.
textile/chat-features.textile
Outdated
* @(CHA-MR1)@ Message reactions are powered by Pub/Sub message annotations. The annotation type used is of the format @reaction:<aggregation>@. | ||
|
||
* @(CHA-MR2)@ There are three types of message reactions, each corresponding to a different annotation aggregation type. | ||
** @(CHA-MR2a)@ @[Testable]@ Reactions of type @Unique@ use the annotation type @reaction:unique.v1@. In this type, each client can add only one reaction (by @name@). Reacting again changes the reaction to the new one (similar to WhatsApp or iMessage). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this testable? What are we testing here? It feels like the testable bit would be where we say "if you receive reaction of type X, then do Y"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed testable
, this is just explaining the reaction types but not specifically tied to an action/behaviour.
textile/chat-features.textile
Outdated
** @(CHA-MR3b3)@ @[Testable]@ For @Distinct@ reaction types, the summary is at @Message.reactions.distinct@ and is of type @Dict<string, SummaryClientIdList>@ (see @TM7b1@). | ||
** @(CHA-MR3b2)@ @[Testable]@ For @Multiple@ reaction types, the summary is at @Message.reactions.multiple@ and is of type @Dict<string, SummaryClientIdCounts>@ (see @TM7b3@). | ||
|
||
* @(CHA-MR4)@ @[Testable]@ Sending a reaction is done via the @add@ method of the @MessagesReactions@ object (@room.messages.reactions.add@). The method accepts a message (or message serial), and a @params@ object where the @name@ (ie. emoji string), @type@ (optional) and @count@ (optional, only valid for type @multiple@) are specified. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is quite a long point with lots of optionals. Can we split it up at all?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
textile/chat-features.textile
Outdated
|
||
* @(CHA-MR5)@ @[Testable]@ Users may configure a default message reactions type for a room. This configuration is provided at the @RoomOptions.messages.defaultMessageReactionType@ property, or idiomatic equivalent. The default value is @distinct@. | ||
|
||
* @(CHA-MR6)@ @[Testable]@ Subscribing to message reactions is done via the @subscribe@ method of the @MessagesReactions@ object (@room.messages.reactions.subscribe@). The method accepts a callback (or similar idomatic construct) that receives message reactions summary events of type @MessageReactionSummaryEvent@. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In rest of the specification, we describe behaviour like this in terms of user outcome - i.e. "users must be able to X".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done here and some other places
textile/chat-features.textile
Outdated
* @(CHA-MR8)@ @[Testable]@ Subscribing to raw message reactions (as individual annotations) is done via the @subscribeRaw@ method of the @MessagesReactions@ object (@room.messages.reactions.subscribeRaw@). The method accepts a callback (or similar idomatic construct) that receives individual annotations of type @MessageReactionRawEvent@. | ||
* @(CHA-MR8a)@ @[Testable]@ Subscribing to raw message reactions throws an error if the room is not configured to support raw message reactions by setting room option @RoomOptions.messages.rawMessageReactions@ to @true@ (defaults to @false@). | ||
|
||
* @(CHA-MR9)@ The @MessageReactionRawEvent@ interface provides information about individual message reaction events. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have talked for a while about having an IDL for Chat. Should we move these points into an IDL section (i.e. an interface definition of the type), and perhaps just have spec points cover specific behvaiour we expect for each one (beyond plain old data)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved those next to where the example data for them is given as well.
textile/chat-features.textile
Outdated
* @(CHA-MR10)@ @[Testable]@ The room option @RoomOptions.messages.rawMessageReactions@ controls whether raw message reactions are enabled or not. The default value is @false@. | ||
** @(CHA-MR10a)@ @[Testable]@ If the room option is set to @true@, the SDK will add the @ANNOTATION_SUBSCRIBE@ channel mode to the realtime channel used for the room. | ||
|
||
* @(CHA-MR11)@ @[Testable]@ The SDK always adds the @ANNOTATION_PUBLISH@ channel mode to the realtime channel used for the room. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* @(CHA-MR11)@ @[Testable]@ The SDK always adds the @ANNOTATION_PUBLISH@ channel mode to the realtime channel used for the room. | |
* @(CHA-MR11)@ @[Testable]@ The client must always add the @ANNOTATION_PUBLISH@ channel mode to the realtime channel used for the room. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Applied locally, thanks.
2a12630
to
a458c4c
Compare
3db3be7
to
de30fe7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a couple more comments and we should be good to go :)
textile/chat-features.textile
Outdated
* @(CHA-MR6)@ @[Testable]@ Users must be able to subscribe to message reaction summaries via the @subscribe@ method of the @MessagesReactions@ object (@room.messages.reactions.subscribe@). The method accepts a callback (or similar idiomatic construct) that receives message reactions summary events of type @MessageReactionSummaryEvent@. | ||
|
||
* @(CHA-MR7)@ @[Testable]@ Users must be able to subscribe to raw message reactions (as individual annotations) via the @subscribeRaw@ method of the @MessagesReactions@ object (@room.messages.reactions.subscribeRaw@). This method accepts a callback (or similar idiomatic construct) that receives individual annotations of type @MessageReactionRawEvent@. | ||
** @(CHA-MR7a)@ @[Testable]@ The attempt to subscribe to raw message reactions must fail with an error if the room is not configured to support raw message reactions (when room option @RoomOptions.messages.rawMessageReactions@ is not set to @true@; it defaults to @false@). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What error code?
textile/chat-features.textile
Outdated
|
||
* @(CHA-MR5)@ @[Testable]@ Users may configure a default message reactions type for a room. This configuration is provided at the @RoomOptions.messages.defaultMessageReactionType@ property, or idiomatic equivalent. The default value is @distinct@. | ||
|
||
* @(CHA-MR6)@ @[Testable]@ Users must be able to subscribe to message reaction summaries via the @subscribe@ method of the @MessagesReactions@ object (@room.messages.reactions.subscribe@). The method accepts a callback (or similar idiomatic construct) that receives message reactions summary events of type @MessageReactionSummaryEvent@. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we shouldn't reference callbacks explicitly here - as in Swift/Kotlin you may not pass in a callback at all, but simply return a flow object that "acts" as the callback
textile/chat-features.textile
Outdated
* @(CHA-MR7)@ @[Testable]@ Users must be able to subscribe to raw message reactions (as individual annotations) via the @subscribeRaw@ method of the @MessagesReactions@ object (@room.messages.reactions.subscribeRaw@). This method accepts a callback (or similar idiomatic construct) that receives individual annotations of type @MessageReactionRawEvent@. | ||
** @(CHA-MR7a)@ @[Testable]@ The attempt to subscribe to raw message reactions must fail with an error if the room is not configured to support raw message reactions (when room option @RoomOptions.messages.rawMessageReactions@ is not set to @true@; it defaults to @false@). | ||
|
||
* @(CHA-MR8)@ Raw message reactions should not be used to update the message reaction summary on the client-side as this is likely to produce wrong results. Message reaction summaries are to be used for this. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this one should be a "must"
…, and avoid implying callbacks
* @(CHA-MR4)@ @[Testable]@ Users should be able to send a reaction via the `add` method of the `MessagesReactions` object (@room.messages.reactions.add@). | ||
** @(CHA-MR4a)@ @[Testable]@ The `add` method accepts a message (or message serial) as the first parameter to identify which message to react to. | ||
** @(CHA-MR4b)@ @[Testable]@ The `add` method accepts a `params` object as the second parameter with the following properties: | ||
*** @(CHA-MR4b1)@ @[Testable]@ A `name` property (required) specifying the reaction identifier (e.g., emoji string). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add non-empty client check for name
property?
Add spec for message reactions.