Calculate encrypted notification counts #851
Conversation
Needed for encrypted events to be able to pass some push rules.
Otherwise we'll be looking at the encrypted source, and that doesn't help anyone.
There was a problem hiding this comment.
this generally lgtm (thanks!) but i think we should be super clear that it ignores counts of events which arrived whilst the client was offline - ie the server can’t yet calculate correct counts for us.
It’s probably worth observing that either we could download all the traffic to calculate the right badges (which we will need to do for e2e indexing anyway), or matrix-org/matrix-spec-proposals#1796 needs to land.
| return true; | ||
| } | ||
|
|
||
| for (let i = this.timeline.length - 1; i >= 0; --i) { |
There was a problem hiding this comment.
doing a linear search of the timeline feels quite heavy here, especially if we calc this for every new received event?
There was a problem hiding this comment.
It's a reverse search, and very much ripped off from Unread.doesRoomHaveUnreadMessages() in the react-sdk. Worst case is the room is very popular and the loop falls behind by a couple iterations, but it should never get as bad as traversing the whole timeline.
Fair point, I didn't mention it in the PR but I did take some time to try and figure it out. The timeline stuff is impenetrable and non-trivial, and I'm not very inclined to keep bashing my head against this. A proper implementation of some MSC would almost certainly help with this. |
|
Actually, wait a minute: this implementation does fix the counts. We already get a count for the number of unread messages (grey badge) and this specifically manipulates the highlight count using the client-side calculation of the push rules. If the client disappears for a while and comes back, this should be able to do the math correctly and come out with a number. An earlier implementation didn't do this, which is where my last comment is based on. |
|
the problem is that if you’ve been binged in a room and your client wasn’t online at the time and hasn’t backfilled far enough to see it, the count will not include it. which is okay for now, but needs to be flagged in big warning letters and bugfiled |
|
lgtm - thanks! |
Fixes element-hq/element-web#7489
Required by matrix-org/matrix-react-sdk#2744Not needed.I'm not entirely convinced that the changes here are the right place to shove this code, but it does work well enough to at least open a PR.I've since shuffled things around.My GIF recorder is broken, so here's a deconstructed 2 frame GIF:
