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

Add read markers #1635

Merged
merged 5 commits into from Aug 31, 2018

Conversation

2 participants
@turt2live
Member

turt2live commented Aug 30, 2018

Rendered: see 'docs' status check


This is the spec for #910

Fixes #910

Some reverse engineering was required to work out the complete details as to how this works. In particular, the 405 for setting account data and the behaviour of m.read.

References:

Add read markers
This is the spec for #910

Fixes #910

Some reverse engineering was required to work out the complete details as to how this works. In particular, the 405 for setting account data and the behaviour of m.read.

References:
* 405 for account data: https://github.com/matrix-org/synapse/blob/d69decd5c78c72abef50b597a689e2bc55a39702/synapse/rest/client/v2_alpha/account_data.py#L85-L90
* m.read behaviour: https://github.com/matrix-org/synapse/blob/d69decd5c78c72abef50b597a689e2bc55a39702/synapse/rest/client/v2_alpha/read_marker.py#L45-L52

@turt2live turt2live requested a review from matrix-org/spec-core-team Aug 30, 2018

@turt2live turt2live added this to In review (just the PRs) in August 2018 r0 via automation Aug 30, 2018

@richvdh

This comment has been minimized.

Member

richvdh commented Aug 31, 2018

Hum. That is not what 405s are for. I would be inclined to treat that as a synapse bug and spec a 400 instead.

@richvdh

This comment has been minimized.

Member

richvdh commented Aug 31, 2018

or, in fact, just leave the error case off for now. We should not be speccing M_UNKNOWN anywhere.

@turt2live

This comment has been minimized.

Member

turt2live commented Aug 31, 2018

How does speccing it as 400 with M_EVENT_NOT_ALLOWED or something sound? I feel it's important to communicate that the API can/will reject particular event types due to some restriction.

.. _module:read-markers:
A "read marker" is a bookmark in the room's history where the user has last

This comment has been minimized.

@richvdh

richvdh Aug 31, 2018

Member

This is not a description I find particularly easy to understand. I'm not sure it's helpful to distinguish between messages the user has seen but not read (we can't really tell that), and it's not true that the read receipt covers the most recently read message (if the user reads messages backwards by scrolling up, we don't update the RR - it only ever moves forward).

We need to start with the observation that since a user can scroll freely around the history of a room, there will be some parts of that history that the user has read, and some they have not.

So: we don't attempt to track the read state on a per-message basis: instead, we divide the room up into three sections: part 1, where the user has read all the messages (or indicated that they don't care); part 2, where the user may have read some messages and not others; and part 3, where the user hasn't seen any of the messages.

The read marker marks the last message in part 1 (ie, the last message where the user has seen all earlier messages); the read receipt marks the last message in part 2 (ie, the last message that the user has seen).

.. See the License for the specific language governing permissions and
.. limitations under the License.
Read Markers

This comment has been minimized.

@richvdh

richvdh Aug 31, 2018

Member

could we call this section "The fully-read marker" (and update the text below to match)?

@richvdh

This comment has been minimized.

Member

richvdh commented Aug 31, 2018

How does speccing it as 400 with M_EVENT_NOT_ALLOWED or something sound? I feel it's important to communicate that the API can/will reject particular event types due to some restriction.

I'm not keen on speccing things that synapse doesn't do, but I'd prefer that to what we have currently.

Could you just mention in the description that the api should return an error in this case?

@turt2live turt2live requested a review from richvdh Aug 31, 2018

August 2018 r0 automation moved this from In review (just the PRs) to Reviewer approved Aug 31, 2018

@richvdh

lgtm

@turt2live turt2live merged commit 108c623 into matrix-org:master Aug 31, 2018

7 checks passed

ci/circleci: build-dev-scripts Your tests passed on CircleCI!
Details
ci/circleci: build-docs Your tests passed on CircleCI!
Details
ci/circleci: build-swagger Your tests passed on CircleCI!
Details
ci/circleci: check-docs Your tests passed on CircleCI!
Details
ci/circleci: validate-docs Your tests passed on CircleCI!
Details
docs Click details to preview the HTML documentation.
Details
swagger Click to preview the swagger build.
Details

August 2018 r0 automation moved this from Reviewer approved to Done (this list will be incomplete) Aug 31, 2018

@turt2live turt2live deleted the turt2live:travis/c2s/read-markers branch Aug 31, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment