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

Top 'unread messages' banner doesn't appear until you scroll back far enough to paginate the relevant message #1146

Closed
matrixbot opened this issue Mar 14, 2016 · 19 comments
Labels
P1 S-Major Severely degrades major functionality or product features, with no satisfactory workaround T-Defect
Milestone

Comments

@matrixbot
Copy link

Created by @ richvdh:matrix.org.

This happens when you first open a room in Vector, and your read-receipt is a few pages earlier. We initialise the RM from your RR, but don't know when that was, so don't show the banner.

@ara4n ara4n added T-Defect P2 S-Major Severely degrades major functionality or product features, with no satisfactory workaround labels Mar 14, 2016
@ara4n ara4n added this to the v2 - Public Preview milestone Mar 14, 2016
@richvdh richvdh removed their assignment Apr 5, 2016
@ara4n
Copy link
Member

ara4n commented Jul 2, 2016

how does this differ from #1192?

@richvdh
Copy link
Member

richvdh commented Jul 2, 2016

how does this differ from #1192?

The most obvious difference is that this makes the banner not appear when it should, while #1192 makes it appear when it should not. They have very different causes.

@lukebarnard1
Copy link
Contributor

lukebarnard1 commented Mar 31, 2017

Me and @dbkr had a discussion about this. The main points were:

  • The "Unread messages" banner is based on the Read Marker, which is in-turn based on the user's own Read Receipt.
  • The important thing is that it's based on the RM, which only gets put in the timeline if the event that it's marking has actually been paginated in.
  • The other problem is that event IDs are not comparable because at the time of receiving them, we have no way of knowing which order the event is supposed to be in (unless the event is actually retrieved via /sync).
  • So the solution would be Non-Trivial.

The two scenarios from the clients perspective (with the user currently not reading the RoomView) are:

  1. The view is stuck to the bottom of the timeline, meaning that we see each event coming in live, but the RR stays where it is. When the user begins reading again, they will send a new read receipt but only for the events that are at the end of the timeline. So the RM stays where it is. BUT if the pagination logic fires and a sufficient number of events have been added to the RoomView in the time the user was away, the event that the RM is associated with could get unpaginated, and the "Unread messages" banner will disappear.
  2. The view is somewhere in the middle of a timeline - i.e. not live. Events are being received down /sync, but they are not being rendered. The last RR the user sent was to some event ahead of time in relation to the current scroll position. So the unread messages are ahead of time, and yet the RM is not rendered. This is fine because we don't want to show an "up" arrow when the receipt is ahead of time.

Interestingly, on the opposite side of the RoomView is the RoomStatusBar, which bases itself on numUnreadMessages, which is literally how many messages were received since we last scrolled up! It would be nice to have a similar metric in the opposite direction. Like "number of messages received since we last stopped paying attention to the client". But this does not solve for the actual use-case of opening the client fresh and reading missed messages.

One solution could be to paginate up to the RR, but this could involve thousands of messages (and I guess that's why some apps display the number of unread messages alongside that arrow so that you can decide whether you actually want to read 999+ messages.

@ara4n
Copy link
Member

ara4n commented Mar 31, 2017

i think the most recent conclusion on this is this that we should change how RM is implemented entirely, by maintaining it in the server and so synchronising it between clients. Therefore the client would use the existing permalink logic if needed to jump back to the point that the RM actually refers to.

This is #1159.

@lukebarnard1 - implementing #1159 could be a worthwhile mission to fix this and all the other crapness to do with RMs...

@ara4n ara4n modified the milestones: RW003, v2 - Public Preview Apr 25, 2017
@ara4n
Copy link
Member

ara4n commented Apr 25, 2017

The synchronized RMs are largely useless without this. However, surely this is simple to fix - if our RM points at an event ID that we don't recognise or is off the top of the screen, we show the "Jump to unread messages" prompt. Otherwise, we don't. What am I missing?

@lukebarnard1
Copy link
Contributor

The diff for this is hilariously small:

-        var showBar = (pos < 0);
+        var showBar = pos < 0 || pos === null;

@ara4n
Copy link
Member

ara4n commented Apr 25, 2017

SHIP IT! :D

@lukebarnard1
Copy link
Contributor

@richvdh
Copy link
Member

richvdh commented Apr 25, 2017

wouldn't it be more consistent to show the bar whenever the read marker isn't on the screen?

pos != 0

?

@richvdh
Copy link
Member

richvdh commented Apr 25, 2017

!==, even

@aperezdc
Copy link

Woohoo, I just rebuilt develop locally and tested... this fix is a great improvement in usability! 🎉

@t3chguy
Copy link
Member

t3chguy commented Apr 25, 2017

the fix seems to have introduced another issue though
when joining a room, the banner comes up when it shouldn't, the link doesn't take you anywhere, just flashes the view

@ara4n
Copy link
Member

ara4n commented Apr 26, 2017

yeah, i just hit @t3chguy's issue too. also, doesn't this need a synapse which supports the new RM stuff in order to work correctly?

@ara4n ara4n added the P1 label Apr 26, 2017
@ara4n ara4n removed the P2 label Apr 26, 2017
@lukebarnard1
Copy link
Contributor

lukebarnard1 commented Apr 26, 2017

@ara4n actually it'll still work without the new RM endpoint, the position of it will still be based on the user's RR (which might refer to an event that hasn't been paginated)

@lukebarnard1
Copy link
Contributor

lukebarnard1 commented Apr 26, 2017

@richvdh yes, actually. In practice however, when scrolling down, the RM is repeatedly bumped off the bottom of the screen and scrolled up into the view-port. We could include some sort of padding but I have a feeling that wouldn't solve things for tall events.

@grahamperrin
Copy link

To help me understand current intended and future behaviours …

Can the read receipt implementation allow the system to know that (for example):

  • I have read messages that were posted before Friday
  • I have read the most recent handful of messages
  • the range between those two ranges is unread?

(When I see RR, singular, I wonder whether the implementation is based upon a single point in time, rather than ranges of read messages.)

@ara4n
Copy link
Member

ara4n commented Apr 29, 2017

The current behaviour is that RRs tell you the most recent message a given user has read; RM tracks the oldest message that you have read. There is no support for tracking ranges of read messages. It'd be better to ask this sort of thing via Matrix rather than adding sidebars to a bug like this one though :)

@grahamperrin
Copy link

(Thanks for steering/sorry for the sidebar.)

@lukebarnard1
Copy link
Contributor

lukebarnard1 commented May 2, 2017

This has been fixed by matrix-org/matrix-react-sdk@96e7479, matrix-org/matrix-react-sdk#845, matrix-org/matrix-react-sdk#846 and matrix-org/matrix-react-sdk#855

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P1 S-Major Severely degrades major functionality or product features, with no satisfactory workaround T-Defect
Projects
None yet
Development

No branches or pull requests

7 participants