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 Receipts, Read Markers, Notifications don't behave well with reactions and edits #9745

Closed
2 tasks done
bwindels opened this issue May 16, 2019 · 10 comments
Closed
2 tasks done

Comments

@bwindels bwindels changed the title Read receipts disappear with reactions and edits Read receipts & read marker don't behave well with reactions and edits May 16, 2019
@turt2live
Copy link
Member

Shouldn't these be using custom event types that don't trigger notifications in the first place?

@bwindels
Copy link
Contributor Author

Shouldn't these be using custom event types that don't trigger notifications in the first place?

I thought this was also a problem for m.reaction events, not 100% sure though. Also, not sure what an edit would look like if it's not a m.room.message...

@turt2live
Copy link
Member

oh right, edits are part of the problem. Nevermind that then.

@uhoreg
Copy link
Member

uhoreg commented May 22, 2019

see also #3511 (which was a similar issue, but for redactions)

@jryans
Copy link
Collaborator

jryans commented May 29, 2019

I have attempted to analyse the various issues at play here. I'll update the original comment here to cover the most critical changes we need to make and open additional issues for follow up work.

@jryans jryans changed the title Read receipts & read marker don't behave well with reactions and edits Read Receipts, Read Markers, Notifications don't behave well with reactions and edits May 29, 2019
@jryans jryans added A-Notifications A-Read-Marker Green line showing how far _you_ have read A-Read-Receipts labels May 29, 2019
@Valodim
Copy link

Valodim commented Jun 2, 2019

I was surprised about the small amount of activity on this issue: Reactions are beginning to pop up here and there from folks who enable the feature in labs, and this causes inexplicably incorrect unread markers for everyone else (although thankfully at least they don't trigger actual system notifications on riot-android).

Unreliable unread markers have a potential to be extremely disruptive to the overall user experience. Please don't underestimate this as a source of frustration for users of (even slightly) outdated riot versions.

@ara4n
Copy link
Member

ara4n commented Jun 2, 2019

if you follow the link in the previous comment you’ll see the activity switched over to a googledoc. this issue is the main pending issue right now with reactions, and as such is being actively worked on, and at top prio

@Valodim
Copy link

Valodim commented Jun 3, 2019

Great to hear, that clears up my worries! Thanks for your work, really looking forward to this feature 👍

@lampholder
Copy link
Member

Popping a phase:1 label on this to represent that its resolution blocks the delivery of the MVP

jryans added a commit to matrix-org/matrix-react-sdk that referenced this issue Jun 3, 2019
This changes how we determine read receipts for the entire message panel. We now
calculate read receipts for all events up front, which makes it easier to handle
hidden events by moving their read receipts up to the last shown event for
display purposes.

Part of element-hq/element-web#9745
jryans added a commit to matrix-org/matrix-react-sdk that referenced this issue Jun 3, 2019
This adds additional receipt storage to so that we can handle cases where the
receipts and events lists get out of sync. If we ever find a user who previously
had a receipt but momentarily no longer does, we recover their previous receipt
and go with that until we hear something new.

Part of element-hq/element-web#9745
jryans added a commit to matrix-org/matrix-react-sdk that referenced this issue Jun 3, 2019
This changes read receipt sending logic to allow it advance further into events
without tiles (such as edits or reactions) that may exist after the last
displayed event.

By allowing the read receipt to advance past such events, this also marks as
read any related notifications. For example, edits trigger notifications by
default since they are `m.room.message` events, and with this change, such edit
notifications can finally be marked read.

Part of element-hq/element-web#9745
@jryans
Copy link
Collaborator

jryans commented Jun 4, 2019

The critical bugs in this area which led to stuck notifications, etc. should now be fixed on develop (matrix-org/matrix-react-sdk#3056, matrix-org/matrix-react-sdk#3059).

If you are still seeing issues related to this, please open a new issue so we can investigate separately.

@jryans jryans closed this as completed Jun 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants