Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
MSC2285: Private read receipts #2285
MSC2285: Private read receipts #2285
Changes from 25 commits
97c3c03
a4e5b45
40d2aa2
4a77139
94fdb2a
2cc2ed9
c895850
84d18d0
55e2060
de850aa
f551a77
927b622
18f49eb
904582f
287c503
37f1d53
f5c2659
887cc0a
e8ba93f
9eedb28
8b1b73a
4ad1c10
714695c
aa41d84
5c891d0
252474a
33ba33c
dfd4c9f
23f4a2a
839c198
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wouldn't quite agree they do the same thing.
There's a notable difference between
m.fully_read
andm.read
in thatm.fully_read
is private to the user andm.read
is open to the world. Receipts, generally, are assumed to be federated, see the respective subsection of "Receipts" section in the client-server API spec. Even though onlym.read
is defined as of now, the text applies to all receipts. I see two ways out of it:m.read.private
andm.fully_read
to/receipt
is retracted; the MSC only sticks toread_markers
for its purpose (I personally think this is more prudent)./receipt
is federated, and homeservers have to distinguish between federated and non-federated receipts. I wouldn't want to be the one defining the rules of that distinction: note that there's an MSC for user-defined generic EDUs, and even though it doesn't deal with/receipt
directly I believe it's a matter of time for the question about adding receipt types to/receipt
if that MSC goes through.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Read markers are deliberately not able to control notifications, hence the need for a new receipt type in this case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can send
/read_markers
with a sole{"m.read": "$evt" }
and it will achieve the same effect as/receipt/m.read
(including notifications), no?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can't - the
m.fully_read
field is required (at least in the current spec - this MSC changes this). Though what @turt2live means by read markers in this case ism.fully_read
itself, not the/read_markers
endpoint, I thinkThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, right,
m.fully_read
is mandatory in/read_markers
... In that case I suggest to just addm.read.private
alongsidem.read
here, and clarify that, notwithstanding what the spec says in general about receipts,m.read.private
receipts are never federated. And I still don't think addingm.fully_read
to/receipt
would make things consistent becausem.fully_read
is not a receipt, it's a piece of room account data.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be updated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still don't think
m.fully_read
should be added to/receipt
- see above :)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To at least comment on the history of this:
/receipt
and/read_markers
have always been very similar allowing the user to update theirm.read
and in the case of/read_markers
m.fully_read
. The idea was to make/receipt
more "equal" to/read_markers
to make things less confusing (why do these two do almost the same thing but not really?). Now things would be a little more straightforwardThough a new issue arises -
/receipt
is now rarely used as most of the time it's easier to use/read_markers
; withm.read.private
even more so. The solution was supposed to be to deprecate/receipt
in favour of/read_markers
. Then another step would probably be replacing/read_markers
with a more flexible and extensible version of itself and deprecating the old version. (this would of course be a whole separate thing)