Implement API for setting RM #419

lukebarnard1 · 2017-04-12T16:27:59Z

This is now stored on the server with similar treatment to RRs. The server will only store the specified eventId as the current read marker for a room if the event is ahead in the stream when compared to the existing RM. The exception is when the RM has never been set for this room for this user, in which case the event ID will be stored as the RM without any comparison.

This API also allows for an optional RR event ID to be sent in the same request. This is because it might be the common case for some clients to update the RM at the same time as updating the RR.

See design: https://docs.google.com/document/d/1UWqdS-e1sdwkLDUY0wA4gZyIkRp-ekjsLZ8k6g_Zvso/edit

See server-side PRs: matrix-org/synapse#2120, matrix-org/synapse#2128

This is now stored on the server with similar treatment to RRs. The server will only store the specified eventId as the current read marker for a room if the event is ahead in the stream when compared to the existing RM. The exception is when the RM has never been set for this room for this user, in which case the event ID will be stored as the RM without any comparison. This API also allows for an optional RR event ID to be sent in the same request. This is because it might be the common case for some clients to update the RM at the same time as updating the RR. See design: https://docs.google.com/document/d/1UWqdS-e1sdwkLDUY0wA4gZyIkRp-ekjsLZ8k6g_Zvso/edit See server-side PR: matrix-org/synapse#2120

richvdh

can you move as much of this as possible into a low-level func in base-apis.js

might make more sense to put it next to sendReadReceipt.

richvdh · 2017-04-13T13:28:05Z

src/client.js

+ * @return {module:http-api.MatrixError} Rejects: with an error response.
+ */
+MatrixClient.prototype.setRoomReadMarker = function(roomId, eventId, rrEvent, callback) {
+    if (typeof rrEventId === 'function') {


why is this a thing? some functions have this sort of hackery, but it's just for backwards compat. You shouldn't need it for a new func

given that callback will be removed, this is no longer required!

richvdh · 2017-04-13T13:28:20Z

src/client.js

+ * @param {string} rrEvent the event tracked by the read receipt. This is here for
+ * convenience because the RR and the RM are commonly updated at the same time as each
+ * other. Optional.
+ * @param {module:client.callback} callback Optional.


we're deprecating callbacks and not adding them for new funcs

richvdh · 2017-04-13T13:28:40Z

src/client.js

+ * convenience because the RR and the RM are commonly updated at the same time as each
+ * other. Optional.
+ * @param {module:client.callback} callback Optional.
+ * @return {module:client.Promise} Resolves: TODO


can you find something better than this please

Would it to be terrible to do (See @http.authedRequest) or whatever?

richvdh · 2017-04-13T13:28:56Z

src/client.js

@@ -843,6 +843,43 @@ MatrixClient.prototype.setRoomAccountData = function(roomId, eventType,
 };

 /**


what does it DOOOOOOOOOO?

- Describe setRoomReadMarker - remove callback; these are deprecated - document "Resolves:"

lukebarnard1 · 2017-04-18T08:46:52Z

can you move as much of this as possible into a low-level func in base-apis.js

I'm guessing you mean

const room = this.getRoom(roomId);
if (room) {
    room._addLocalEchoReceipt(this.credentials.userId, rrEvent, "m.read");
}

but getRoom is in client.js so it'd feel wrong to move it.

richvdh · 2017-04-18T09:01:58Z

No, I mean the all the rest of it.

…

On 18/04/17 09:46, Luke Barnard wrote: can you move as much of this as possible into a low-level func in base-apis.js I'm guessing you mean const room = this.getRoom(roomId); if (room) { room._addLocalEchoReceipt(this.credentials.userId, rrEvent,"m.read"); } but |getRoom| is in |client.js| so it'd feel wrong to move it. — You are receiving this because you were assigned. Reply to this email directly, view it on GitHub <#419 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABU1VImGWRCRAlBQz7baG6dSqTp6wos-ks5rxHh8gaJpZM4M7qgI>.

Because typeof null is object

richvdh

can you base this on develop, not master, please

richvdh · 2017-04-18T15:56:12Z

release.sh

@@ -237,6 +237,7 @@ fi

 # if it is a pre-release, leave it on the release branch for now.
 if [ $prerelease -eq 1 ]; then
+    git checkout "$rel_branch"


richvdh · 2017-04-18T16:11:16Z

src/base-apis.js

+ * for convenience because the RR and the RM are commonly updated at the same time as
+ * each other. Optional.
+ * @return {module:client.Promise} Resolves: (@see module:http-api.authedRequest)
+ * @return {module:http-api.MatrixError} Rejects: with an error response.


it never returns a MatrixError; it always returns a Promise.

Sorry, I should have been clearer about what to put here. There are a lot of historical functions (including authedRequest) which are basically full of shit. Note also that httpApi.opts.onlyData is true as far as base-apis is concerned, so authedRequest always just returns the result of the api call, so linking to that isn't particularly helpful.

Basically the best thing to do here is just have one @return line, along the lines of:

@return {module:client.Promise} resolves to the result object for the API call.

If you have the time and energy to document the shame of that object, so much the better, but generally we expect people to go and read the spec.

richvdh · 2017-04-18T16:12:39Z

src/base-apis.js

@@ -470,6 +470,33 @@ MatrixBaseApis.prototype.roomInitialSync = function(roomId, limit, callback) {
    );
 };

+/**
+ * Set a marker to represent which event the user was last reading in a room. This can


it's not the event the user was last reading though, is it?

richvdh · 2017-04-18T16:19:18Z

src/client.js

+ * @return {module:client.Promise} Resolves: (@see module:http-api.authedRequest)
+ * @return {module:http-api.MatrixError} Rejects: with an error response.
+ */
+MatrixClient.prototype.setRoomReadMarker = function(roomId, eventId, rrEvent) {


let's not distinguish these two by the presence or absence of 's'.

How about calling the other setRoomReadMarkersHttpRequest or something (we have a _sendEventHttpRequest for distinction from _sendEvent)

richvdh · 2017-04-18T16:19:48Z

src/client.js

@@ -1201,6 +1201,33 @@ MatrixClient.prototype.sendReadReceipt = function(event, callback) {
    return this.sendReceipt(event, "m.read", callback);
 };

+/**


jsdocs need tweaks as above

richvdh · 2017-04-18T16:19:55Z

src/interactive-auth.js

@@ -318,6 +318,13 @@ InteractiveAuth.prototype = {
        }
        this._currentStage = nextStage;

+        if (nextStage == 'm.login.dummy') {


lukebarnard1 · 2017-04-19T09:00:57Z

Now that I've changed the base of the merge, those changes to release.sh and interactive-auth.js should've disappeared

richvdh

lgtm

lukebarnard1 assigned richvdh Apr 12, 2017

richvdh requested changes Apr 13, 2017

View reviewed changes

richvdh assigned lukebarnard1 and unassigned richvdh Apr 13, 2017

Luke Barnard added 2 commits April 18, 2017 09:40

Code review response

202d8e6

- Describe setRoomReadMarker - remove callback; these are deprecated - document "Resolves:"

Move setRoomReadMarker next to sendReadReceipt

caf214b

lukebarnard1 assigned richvdh and unassigned lukebarnard1 Apr 18, 2017

Luke Barnard added 3 commits April 18, 2017 10:29

Move http req to base-apis

9ce8047

Update to reflect API changes

ce49124

Merge branch 'develop' into luke/feature-read-marker-api

a6b75fe

This was referenced Apr 18, 2017

Self-read markers should be synchronised across clients element-hq/element-web#1159

Closed

Sync RM across instances of Riot matrix-org/matrix-react-sdk#805

Merged

pass an undefined callback, not null

bf24f7a

Because typeof null is object

richvdh requested changes Apr 18, 2017

View reviewed changes

lukebarnard1 changed the base branch from master to develop April 18, 2017 16:33

richvdh assigned lukebarnard1 and unassigned richvdh Apr 18, 2017

Luke Barnard added 2 commits April 19, 2017 09:51

Clarify jsdocs

562a3c2

Rename ReadMarker functions, clarify doc

edaa646

lukebarnard1 assigned richvdh and unassigned lukebarnard1 Apr 19, 2017

richvdh approved these changes Apr 19, 2017

View reviewed changes

richvdh assigned lukebarnard1 and unassigned richvdh Apr 19, 2017

lukebarnard1 merged commit 4c63906 into develop Apr 20, 2017

t3chguy deleted the luke/feature-read-marker-api branch May 10, 2022 14:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement API for setting RM #419

Implement API for setting RM #419

lukebarnard1 commented Apr 12, 2017 •

edited

Loading

richvdh left a comment

richvdh Apr 13, 2017

lukebarnard1 Apr 13, 2017 •

edited

Loading

richvdh Apr 13, 2017

lukebarnard1 Apr 13, 2017

richvdh Apr 13, 2017

lukebarnard1 Apr 13, 2017

richvdh Apr 13, 2017

lukebarnard1 commented Apr 18, 2017

richvdh commented Apr 18, 2017 via email

richvdh left a comment

richvdh Apr 18, 2017

richvdh Apr 18, 2017

richvdh Apr 18, 2017

richvdh Apr 18, 2017

richvdh Apr 18, 2017

richvdh Apr 18, 2017

lukebarnard1 commented Apr 19, 2017

richvdh left a comment

		@@ -843,6 +843,43 @@ MatrixClient.prototype.setRoomAccountData = function(roomId, eventType,
		};

		/**

Implement API for setting RM #419

Implement API for setting RM #419

Conversation

lukebarnard1 commented Apr 12, 2017 • edited Loading

richvdh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lukebarnard1 Apr 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lukebarnard1 commented Apr 18, 2017

richvdh commented Apr 18, 2017 via email

richvdh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lukebarnard1 commented Apr 19, 2017

richvdh left a comment

Choose a reason for hiding this comment

lukebarnard1 commented Apr 12, 2017 •

edited

Loading

lukebarnard1 Apr 13, 2017 •

edited

Loading