matrix-spec-proposals
matrix-spec-proposals copied to clipboard
matrix-org
MSC2654: Unread counts
Synapse implementation: https://github.com/matrix-org/synapse/pull/8059 Client: https://github.com/quotient-im/libQuotient/pull/521
I think we also need a proposal for how to specify that certain rooms should be excluded from the badge count in /_matrix/push/v1/notify.
I assume you mean a separate MSC rather than an addition to this MSC?
I assume you mean a separate MSC rather than an addition to this MSC?
well no, I rather think it ought to be part of this one. See also https://docs.google.com/document/d/1pjYrGokTQGDZQLtagZvG4dY67RsjUdFDfpOzSiWtOBg/edit#
thanks for the detailed review and helping to spot the edge-cases we forgot, @turt2live !
What's the status of this? There is currently code in Synapse to make this work that is on the hot path, and we should rip it out if we're not going to finish this any time soon.
What's the status of this? There is currently code in Synapse to make this work that is on the hot path, and we should rip it out if we're not going to finish this any time soon.
I think the next steps are addressing the concerns in threads and asking for FCP to be proposed. I'm unlikely to get bandwidth to do that anytime soon though.
Is this actually being driven by something? If it's not a feature that people are asking for then I'm not sure there's much point finishing it?
Is this actually being driven by something? If it's not a feature that people are asking for then I'm not sure there's much point finishing it?
It was driven by the notification rework that was underway at the time, but isn't anymore. I have occasionally seen people use this feature, but rarely and I believe this is a pretty niche feature, so I don't mind if we decide to abandon it.
Is this actually being driven by something? If it's not a feature that people are asking for then I'm not sure there's much point finishing it?
It's being used for unread counts in SchildiChat-Android, and also fixes chats marked as unread that are actually read, but the client didn't have enough events to find out from /sync, which is a bug that users regularly point out when not using this MSC. I'd find it very unfortunate if it got removed, to the point that I might even look into forking synapse for my own homeserver in order to keep it in.
We're also using this MSC for the Beeper clients btw.
Upcoming libQuotient 0.7 also has it implemented (driving my feedback here).
I don't think it's reasonable to keep this "experimental" code in synapse's mainline indefinitely; we either need to commit to speccing it properly, or to removing it.
It's worth noting that enabling the option in Synapse has a substantial performance impact, so making it part of the spec would require us to find a solution to that.
Given that the author of this MSC has said that he is no longer interested in pursuing it, I think that it will need a volunteer to take it on; otherwise it is likely to be closed as "abandoned".
So, any volunteers to address the comments and think about how to solve the performance problems?
I'm willing to take this over - is there a write-up of the performance issues somewhere?
I'm willing to take this over - is there a write-up of the performance issues somewhere?
Not really, no. matrix-org/synapse#13694 is the PR of interest. Happy to chat over there (or we can file a separate issue on Synapse referencing that if you'd like).
@erikjohnston and I had an interesting conversation about this MSC back in December that I think is worth documenting.
We were chatting about how opinionated the Matrix spec should be about client UX. The spec doesn't really say "this is how your client should look like and work", which has allowed a large number of clients that suit different needs to exist. Push rules, for example, allow different users and different clients to configure how they would like to be notified, without defining which events deserve notifications or not in the spec. However, this MSC is a set of rules that determine whether an event should be considering unread or not, which I could see different clients or different users having different desires here.
Case in point, Beeper does use this MSC quite extensively, but we've found that we had to tweak the rules to get the UX we want: https://github.com/beeper/synapse/blob/b658fae65ae57f11f53e774d0efabdcf4a9b7c32/synapse/push/bulk_push_rule_evaluator.py#L76-L107. This has lead to a case where our clients all read this unread count from the sync payload using the prefixed msc2654 field, but the number in there that our homeserver is calculating isn't compliant with this MSC.
Anyway, just food for thought.
