Skip to content

Conversation

@danxuliu
Copy link
Member

@danxuliu danxuliu commented May 3, 2023

Fixes #9272

This pull request adds support for signaling messages for typing indicators. It is assumed that the local participant will only set the typing status in a single conversation, which is the currently joined conversation. Therefore, if the local participant has not joined a conversation yet it is not possible to modify the typing status. Similarly, although other participants could send a signaling message to any other participant, even if they are not in the same conversation, only typing signaling messages from participants in the same conversation are taken into account (and the typing status of participants that leave the conversation are automatically reset, even if they did not send a stoppedTyping message).

SignalingParticipantList is a helper class that listens to events emitted by the signaling related to participants joining and leaving; it keeps track of the participants in the conversation to be able to notify them when the local participant starts or stops typing (or if another participant joins while the local participant is typing). SignalingTypingHandler is a helper class that listens to startedTyping and stoppedTyping signaling messages to update the store with the typing status of the remote participants, and to send the typing status of the local participant through signaling messages when calling setTyping(typing).

Note that SignalingTypingHandler is not used directly from the views; the views only need to get or modify properties in the store. However, the participant objects returned by store.getters.participantsList(token) are reset whenever the participants are fetched from Nextcloud. The typing property is a signaling property, and it is not returned by the Nextcloud server when the participants are fetched, so modifying the store when another participant is typing would not work, as the property would be reset on each new fetch. Therefore when another participant is typing the Nextcloud session ID of that participant is marked in a special property in the store, and when store.getters.participantsListTyping(token) is called the full list of participants is filtered to only return those that are typing.

How to use from Vue components

  • To set the current participant as typing or no longer typing, store.dispatch('setTyping', {typing: true|false}).
  • To know if other participants are typing or not, store.getters.participantsListTyping(token); this will return only the participants from store.getters.participantsList(token) that are typing (although note that there will be no typing property nor anything similar).

🚧 Tasks

  • Fix tests
  • Make it work
  • Guest can not see the typing status of other participants. Although the guests receive the typing messages guests do not have access to the endpoint that returns the participant data, so store.getters.participantsList() only contains the local participant and, therefore, store.getters.participantsListTyping() only contains the local participant as well, even if the information about what sessions are typing is known (but it would need to be joined with the user name, avatar and so on to be useful).
  • The typing messages are sent and received unconditionally; I do not know if there is a setting to enable or disable them (similar to the read message setting), but that is currently not implemented in SignalingTypingHandler. Nevertheless, it could be implemented outside it (that is, do not call setTyping if the local participant has typing notifications disabled), although in that case other participants would still send their typing status to that participant; it would prevent sending them, but not receiving them. - The signaling messages for typing indicators are now sent and handled when received only if allowed by the typing_privacy capability.

@danxuliu danxuliu added 2. developing enhancement feature: chat 💬 Chat and system messages feature: signaling 📶 Internal and external signaling backends labels May 3, 2023
@danxuliu danxuliu added this to the 💙 Next Major (27) milestone May 3, 2023
@danxuliu danxuliu force-pushed the add-signaling-messages-for-typing-indicators branch from 01e3b03 to b476620 Compare May 3, 2023 11:07
Copy link
Contributor

@Antreesy Antreesy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Noticed some errors while trying to combine it with frontend PR. Hope it will be helpful for you!

@danxuliu danxuliu force-pushed the add-signaling-messages-for-typing-indicators branch from b476620 to 9d270d8 Compare May 3, 2023 21:15
@Antreesy
Copy link
Contributor

Antreesy commented May 4, 2023

Frontend part at #9344
Works perfectly with users, just need to be adjusted for guests also:

Could we pass additional parameters with signal in order to show participant's avatar and display name (same as we have for messages)?

{
"token": "abcde",
"sessionId": "longsessionid",
"actorType":"users",
"actorId":"maksim",
"actorDisplayName":"Maksim Sukharev"
}
typing.mp4

@danxuliu
Copy link
Member Author

danxuliu commented May 4, 2023

Could we pass additional parameters with signal in order to show participant's avatar and display name (same as we have for messages)?

Yes, that should be possible, although I do not know if it would be worth the trouble 🤔 After all guests do not know who is in a conversation, so I think it would be fine to just show Someone is typing 🤷

@nickvergessen
Copy link
Member

Yes, that should be possible, although I do not know if it would be worth the trouble thinking After all guests do not know who is in a conversation, so I think it would be fine to just show Someone is typing shrug

Well same thing could be said for logged in users... But I think it makes sense to know if the teacher/webinar moderator, etc. is typing

@Antreesy
Copy link
Contributor

Antreesy commented May 4, 2023

After all guests do not know who is in a conversation

They could know this from received messages (name and avatar) , so I think, indicator should align with it

@danxuliu
Copy link
Member Author

danxuliu commented May 4, 2023

Yes, that should be possible, although I do not know if it would be worth the trouble 🤔 After all guests do not know who is in a conversation, so I think it would be fine to just show Someone is typing 🤷

Well same thing could be said for logged in users...

But logged in users know if certain participant is in the conversation or not, I think it is not the same case.

But I think it makes sense to know if the teacher/webinar moderator, etc. is typing

Then wouldn't it make sense to also know if the teacher/webinar moderator is actually in the conversation? That could be unified through the getParticipants endpoint, slightly opening it for guests to provide data about specific users (or maybe about specific users only if it is a webinar/breakout room).

After all guests do not know who is in a conversation

They could know this from received messages (name and avatar) , so I think, indicator should align with it

The thing is that, even if possible, I do not think that the data should be provided in the signaling message. It would add extra data that in general is already available by other means but would be needed only to overcome a limitation of guests. I do not know, it does not feel "right" 🤷

Yet providing all the participants in getParticipants may not be desirable (otherwise I guess it would not be currently prevented). Maybe adding another endpoint to fetch the avatar and display name of participants from their session ID?

Or, given that the avatar and name are known as soon as a message is posted, maybe keep the identity hidden until a participant types a message, then start to show XXX is typing instead of Someone is typing (although the message probably does not provide the session ID of the writer, so this may not be possible to do).

@danxuliu danxuliu marked this pull request as ready for review May 5, 2023 10:54
@nickvergessen
Copy link
Member

nickvergessen commented May 5, 2023

The typing messages are sent and received unconditionally; I do not know if there is a setting to enable or disable them (similar to the read message setting), but that is currently not implemented in SignalingTypingHandler.

API/capability is coming with #9455

If the capability or config is not there or the config is disabled, the messages should not be sent and should be received on ignoring. (This is also required so the UI does not show/behave weirdly when using a Talk 17 based Desktop client with a Talk 16 based Server)

@Antreesy
Copy link
Contributor

Antreesy commented May 7, 2023

API/capability handling is added at frontend code, so we're able to hide the indicator and prevent signaling. Although, I don't know, if we need an additional check for it in signaling logic.
Also we should disable receiving of the signals, because they're still coming and visible at the store.

danxuliu added 6 commits May 8, 2023 01:39
Eventually this class should be completed and replace the list kept for
WebRTC, but for now it will be just used for the typing indicators, so
it covers only that use case.

Signed-off-by: Daniel Calviño Sánchez <[email protected]>
Signed-off-by: Daniel Calviño Sánchez <[email protected]>
The handler is facaded behind the signaling/WebRTC functions and
accessed only indirectly from the store/service. The instance is created
as soon as the app starts, which will automatically update the store
with the typing status.

Signed-off-by: Daniel Calviño Sánchez <[email protected]>
In normal usage this seems to work as expected, but it failed when using
custom instances of the stores in tests (both the token of the last
joined conversation and the current token were undefined).

Signed-off-by: Daniel Calviño Sánchez <[email protected]>
@danxuliu danxuliu force-pushed the add-signaling-messages-for-typing-indicators branch from 9d270d8 to 1509a36 Compare May 8, 2023 01:31
@danxuliu
Copy link
Member Author

danxuliu commented May 8, 2023

Rebased and added commit to enable the typing indicators only when allowed by the capability.

Copy link
Member

@nickvergessen nickvergessen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works for me, but we should look into the Guests support as a follow up.

@nickvergessen nickvergessen requested a review from Antreesy May 8, 2023 12:00
Copy link
Contributor

@Antreesy Antreesy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works for me also, signals are blocked on server-side
But I'd like to add an additional checking on frontend, to ensure that client isn't doing any unnecessary requests

@Antreesy Antreesy merged commit 863b835 into master May 8, 2023
@Antreesy Antreesy deleted the add-signaling-messages-for-typing-indicators branch May 8, 2023 14:30
@nickvergessen nickvergessen mentioned this pull request May 9, 2023
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

3. to review enhancement feature: chat 💬 Chat and system messages feature: signaling 📶 Internal and external signaling backends

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Signaling "Typing indicators"

4 participants