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

✔️✔️ Read status for chat messages #4231

Merged
merged 9 commits into from
Dec 10, 2020
Merged

Conversation

nickvergessen
Copy link
Member

@nickvergessen nickvergessen commented Sep 25, 2020

  • Add capability for the read-privacy
  • Add API to allow setting the read-privacy
  • Expose the current setting value on capability
  • Add "last common read message" to conversation API
  • Add "last common read message" header to all chat requests
  • Add integration test for the setting
  • Basic UI checks so @ma12-co can do the polishing
  • Beautiful UI by @ma12-co

Fix #1012

@nickvergessen nickvergessen changed the title Introduce a privacy settings for the read_status ✔️✔️ Read status for chat messages Dec 3, 2020
@nickvergessen nickvergessen force-pushed the feature/1012/read-status branch from aad337c to cc07bf6 Compare December 3, 2020 16:48
@nickvergessen
Copy link
Member Author

nickvergessen commented Dec 4, 2020

How do we want to handle the status?

Pre-thought

  • There is a privacy setting for the read status. If it is set to private, the reading state of this person is totally ignored and they can also not see any read-status.
  • You can only see the read-status on your own messages

Current

We store the "last read message id" per attendee in a conversation.

Current "message states"

Visual Status
⭕️ (loading circle) Message has been "submit" and is currently transferred to the server and waiting for the request to finish.
12:34 (timestamp) Message was received by the server and is published

Original proposal

The proposal was:

The screenshot shows the message delivery status indicator. A simple check mark means that the message was delivered to the server, a check mark in a circle means that the message was seen by the recipient. If the message was not yet send to the server a spinning circle will be shown

Thoughts

One-to-one

For one-to-one chats this basically means (pseudo queries):

SELECT last_read_message FROM oc_talk_attendees WHERE room_id = X AND actor_type = 'users' AND actor_id != YOU;

Groups

For a group chat the question is:

  • Does the state change when:
    1. One participant read it
    2. All participants read it

Telegram e.g. uses double checkmarks if someone read it, Whatsapp uses blue checkmarks when everyone read it, but it has a list of user->time to check individually who read it.

We pick 2. here, so the query is.

SELECT MIN(last_read_message) FROM oc_talk_attendees WHERE room_id = X AND actor_type = 'users' AND actor_id != YOU GROUP BY room_id;

If the result is null it means no one read it =>
If the result is an ID it means everything older and this message are => ✓✓ everything afterwards

Visual Status
⭕️ (Loading circle) Message has been "submit" and is currently transferred to the server and waiting for the request to finish.
❌ (Error cross) Message failed to send, network went down or server responded with an error code
12:34 ✓ Message was sent successfully and it could or could not be read by some users
12:34 ✓✓ Message was read by everyone

@nickvergessen
Copy link
Member Author

Technical question: How do we update the read status. Responding to the long polling request doesn't sound like a good idea, because that will make spam requests all the time when you write something in a busy chat, but if we just add it as a header on all those requests, it means the read status could be delayed in updating by <= 30 seconds.

Similar question applies to "other conversations". If we should show the read status in the conversation list, I would query the info there and just udpate it every 30seconds. It's also in line with current scenarios for a reply there, so that sounds good?

@marcoambrosini
Copy link
Member

Result of coworking with @nickvergessen on this :)

Frame 1asd

@nickvergessen nickvergessen force-pushed the feature/1012/read-status branch from cc07bf6 to 8983472 Compare December 8, 2020 15:03
@nickvergessen nickvergessen requested review from PVince81, danxuliu and marcoambrosini and removed request for PVince81 and danxuliu December 9, 2020 13:39
@nickvergessen
Copy link
Member Author

Stage 1 ready for review. UI will be polished by @ma12-co in a follow up.

@nickvergessen nickvergessen marked this pull request as ready for review December 9, 2020 13:40
Copy link
Member

@marcoambrosini marcoambrosini left a comment

Choose a reason for hiding this comment

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

Beautiful UI by @ma12-co
🤣

@nickvergessen nickvergessen force-pushed the feature/1012/read-status branch from 1a96849 to 314a8d2 Compare December 9, 2020 16:12
@nickvergessen
Copy link
Member Author

Had forgotten to do the fix ups. done now.

Copy link
Member

@PVince81 PVince81 left a comment

Choose a reason for hiding this comment

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

See comments on the code. Haven't tested so far.

Signed-off-by: Joas Schilling <coding@schilljs.com>
Signed-off-by: Joas Schilling <coding@schilljs.com>
…ry it easier later on

Signed-off-by: Joas Schilling <coding@schilljs.com>
… the conversation list

Signed-off-by: Joas Schilling <coding@schilljs.com>
Signed-off-by: Joas Schilling <coding@schilljs.com>
Signed-off-by: Joas Schilling <coding@schilljs.com>
Signed-off-by: Joas Schilling <coding@schilljs.com>
Signed-off-by: Joas Schilling <coding@schilljs.com>
@nickvergessen nickvergessen force-pushed the feature/1012/read-status branch from 314a8d2 to 1e31077 Compare December 10, 2020 10:47
@PVince81
Copy link
Member

UX note: while testing this I intuitively went to the personal settings, didn't think of going to the talk specific cog icon @ma12-co

Now using dispatch to properly trigger the action.
Added disabled state and notification.

Signed-off-by: Vincent Petry <vincent@nextcloud.com>
@PVince81
Copy link
Member

More comments from my tests (could be done separatly as part of improving UX):

  • hide the indicator just like we do for the time part (not sure why we even hide it)

image

@PVince81
Copy link
Member

Technical question: How do we update the read status. Responding to the long polling request doesn't sound like a good idea, because that will make spam requests all the time when you write something in a busy chat, but if we just add it as a header on all those requests, it means the read status could be delayed in updating by <= 30 seconds.

Similar question applies to "other conversations". If we should show the read status in the conversation list, I would query the info there and just udpate it every 30seconds. It's also in line with current scenarios for a reply there, so that sounds good?

What was the decision ? Leaving the current logic as is ?

During a call, I found it interesting that it makes it possible to see who doesn't have the chat panel open. Maybe in the future we could always collapse the chat panel (with localstorage preference) and only open it when there's a new message. Or pop a notification for the message to prompt people to open it. Or something.
Having it collapsed by default would then reduce the load as well.

@PVince81
Copy link
Member

Another remark: the read notification applies to everyone, not only people in the call. Not sure if someone might expect to get a double mark as soon as people within the call read it, minus external ones... Something to discuss separately I guess.

Copy link
Member

@PVince81 PVince81 left a comment

Choose a reason for hiding this comment

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

Fine to merge considering that UX will be worked on after that 👍

@nickvergessen
Copy link
Member Author

What was the decision ? Leaving the current logic as is ?

Yeah the "on every chat request" which is a max of 30s is good enough for now.

During a call, I found it interesting that it makes it possible to see who doesn't have the chat panel open.

You can't see who read it. Just if everyone read it (in stage 1 at least)

Another remark: the read notification applies to everyone, not only people in the call. Not sure if someone might expect to get a double mark as soon as people within the call read it, minus external ones... Something to discuss separately I guess.

It applies to all users (not guests) everything else would be totally unexpected.

@PVince81 PVince81 merged commit 2d68af4 into master Dec 10, 2020
@PVince81 PVince81 deleted the feature/1012/read-status branch December 10, 2020 14:00
@PVince81
Copy link
Member

raised #4738 for the UX bits

@maximelehericy
Copy link

maximelehericy commented Jul 5, 2022

Result of coworking with @nickvergessen on this :)

@marcoambrosini @nickvergessen is there plans to pursue the development of step 2 (providing details when people read messages) ?
Thanks for your help 🙏

@nickvergessen
Copy link
Member Author

Not at the moment

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Indicators for Sent/Received/Read text messages
4 participants