Add a small indicator for when a new event is pinned #1486
Conversation
Signed-off-by: Travis Ralston <[email protected]>
| lastReadEvent = readPinsEvent.getContent().last_read_id; | ||
| } | ||
|
|
||
| if (lastReadEvent !== pinnedEvents.getId()) { |
There was a problem hiding this comment.
This doesn't make sure that the lastReadEvent is ahead in the DAG relative to pinnedEvents.getId(). This could lead to two clients (i.e. clients with potentially differing state event data) fighting over which pinned message is really the last one read.
There was a problem hiding this comment.
What's the best way to check for this? Just using .getTs()?
There was a problem hiding this comment.
We can't technically rely on the timestamp to compare events because it might not be accurate. If we want to do this with some guarantee of the event ID always "increasing" in the timeline, we'd need server-side support. This would look something like m.fully_read - the read marker.
As there's no generic API for setting a similar m.pinned_messages.fully_read or whatever, this would require server-side changes.
But if we assume that the TS on the events are accurate and comparable (which they probably are), then sure.
There was a problem hiding this comment.
m.fully_read doesn't seem to appear in the spec at all. Is it something that already exists or is that part of the server-side implementation request?
There was a problem hiding this comment.
Ah.. then you probably want this https://docs.google.com/document/d/1UWqdS-e1sdwkLDUY0wA4gZyIkRp-ekjsLZ8k6g_Zvso/edit
There was a problem hiding this comment.
(which is matrix-org/matrix-spec-proposals#910)
I'll take a look and give it a shot I suppose.
|
Just to recap (as I had to think quite hard about this): the problem is that we store the ID of the most recent pinned-message state-event in account_data. However, an older client might end up refreshing the UI whilst using a stale copy of room state, and so fight over what the 'most recent' pinned-message actually is. @lukebarnard1 - well done for spotting this edge case; I would have definitely have missed it :) I wonder whether there's a simpler solution though than mandating yet another new server API: how about just storing all of the event IDs of the pinned-message state events which the user has explicitly read in account_data. This is unlikely ever to get particularly big, but then whenever the client sees a pinned messages event whose ID isn't in the list, we declare there to be unread pinned messages? Alternatively, the 'right' server API might be one that @erikjohnston has been rumbling about forever: an entirely generic API where you can just ask the server whether $event_id1 precedes $event_id2 or not (and let the server worry about the DAG calculations). This would then let us break up the fight (and could also be useful on handling all sorts of other edge cases like the direction of RM arrows). OR, we could ignore this as an extreme edge case; comment it; merge it; and see how much it bites us (if ever) in reality. |
|
The list shouldn't get particularly nasty, although it would be ever-increasing. 65kb is a large amount of wiggle room, although I'd be concerned about rooms which enjoy pinning constantly (I've seen this happen on other platforms often enough for it to be concerning). Would checking |
|
checking Also, for pins to work, it has to have some kind of UI to tell you when a room actually has pinned messages or not (whether not you have actually read them). |
|
With this, joining a new room will display an indicator as you haven't read them yet. What would the UX be for a persistent notification? Merging with the race sounds like a bad idea to me, and it's probably best to just make the event an array for now and hope that people don't pin a few hundred thousand times in the room. I'll whip this up in a bit. |
Signed-off-by: Travis Ralston <[email protected]>
|
@ara4n I've pushed the change for storing the events as an array. Would the following make sense for UX?
|
Signed-off-by: Travis Ralston <[email protected]>
|
sgtm |
Signed-off-by: Travis Ralston <[email protected]>
|
@ara4n and the indicator is now implemented for "pins exist". Requires CSS PR element-hq/element-web#5511 Looks like this: |
Signed-off-by: Travis Ralston <[email protected]>
|
lgtm - thanks! |
Description
This PR adds a small indicator when there are "unread" pins. The implementation relies on room account data to store the last pinned events state event that was seen. When the user opens the panel, the room account data is updated. The indicator will not appear for cases when all pins are removed.
The indicator's behaviour and look is heavily influenced from Discord and Slack which have similar features.
Required PRs
What it looks like