De-dup code: use the initialiseState function #611

dbkr · 2018-02-16T17:51:29Z

This should behave identically, but the code here appeared to be
identical to the code in initialiseState, so let's use it (it also
has an extra sanity check in there that we only init empty timelines).

Edit: I take back what I said about this being functionally identical: it isn't. It appears that previously we were applying any events in the state field of the join room to both the old state and the new state, which is surely wrong. Although these are normally only ever sent in a limited or initial sync (so it wouldn't matter) they are well-defined in other syncs as events that precede the timeline events, so we should process them as we would timeline events, before we process the timeline events. The tests send sync responses with state changes in state rather than timeline.

This should behave identically, but the code here appeared to be identical to the code in initialiseState, so let's use it (it also has an extra sanity check in there that we only init empty timelines).

Don't ignore them: its valid to send them in a non-limited sync, they're state events that preceed the timeline events.

richvdh · 2018-02-19T15:23:04Z

src/sync.js

-        ), client.getEventMapper(),
-    );
-    const stateEvents = stateEventList;
+    // We'll only get state events in stateEventList if this is a limited


hrm... this is an assumption based on the way synapse behaves. Indeed it seems to be in conflict with the fact that you then do something with the state events if the timeline is not empty.

Could you rephrase this comment please.

tbh I think deleting it may make more sense: I think this was left in from when the code was relying on this behaviour in synapse and I failed to remove it.

richvdh · 2018-02-19T15:24:09Z

src/sync.js

+    if (!timelinewasEmpty) {
+        room.addLiveEvents(stateEventList || []);
+    }
+    // execute the timeline events, this will continue to diverge the current state


richvdh · 2018-02-19T15:24:19Z

src/sync.js

+
+    // If there are no events in the timeline yet, initialise it with
+    // the given state events
+    const timelinewasEmpty = room.getLiveTimeline().getEvents().length == 0;


timelineWasEmpty ?

mm, yeah - fixed

richvdh · 2018-02-19T15:25:15Z

src/sync.js

+    const timelinewasEmpty = room.getLiveTimeline().getEvents().length == 0;
+    if (timelinewasEmpty) {
+        room.getLiveTimeline().initialiseState(stateEventList);
+    }

    this._resolveInvites(room);


why does this (and recalculate) happen after adding the state events for an empty timeline, but before it for an existing timeline?

My reasoning for this was the the state events ought to be treated the same as if they'd arrived in the timeline section. That said, I opted not to chase that particular rabbit down the hole to figure out what actually happens on a room name change, etc.

well, if that's true, why isn't it true for when the timeline is empty?

"I didn't go down the rabbithole" is valid but an XXX comment saying so would be good for future reference

good point, and now looking at it, it looks a bit broken. Have added an XXX to explain how I think it might be broken.

richvdh · 2018-02-19T15:26:13Z

src/sync.js

+
+    // If there are no events in the timeline yet, initialise it with
+    // the given state events
+    const timelinewasEmpty = room.getLiveTimeline().getEvents().length == 0;


can we factor out the myriad calls to room.getLiveTimeline() into a local var?

richvdh · 2018-02-19T15:58:05Z

also: any chance of some tests for this stuff?

dbkr · 2018-02-20T11:12:41Z

Now with tests that fails on develop

richvdh

lgtm modulo grumbling about commetns

[Full Changelog](matrix-org/matrix-js-sdk@v0.9.2...v0.10.0-rc.1) * Fix duplicated state events in timeline from peek [\matrix-org#630](matrix-org#630) * Create indexeddb worker when starting the store [\matrix-org#627](matrix-org#627) * Fix indexeddb logging [\matrix-org#626](matrix-org#626) * Don't do /keys/changes on incremental sync [\matrix-org#625](matrix-org#625) * Don't mark devicelist dirty unnecessarily [\matrix-org#623](matrix-org#623) * Cache the joined member count for a room state [\matrix-org#619](matrix-org#619) * Fix JS doc [\matrix-org#618](matrix-org#618) * Precompute push actions for state events [\matrix-org#617](matrix-org#617) * Fix bug where global "Never send to unverified..." is ignored [\matrix-org#616](matrix-org#616) * Intern legacy top-level 'membership' field [\matrix-org#615](matrix-org#615) * Don't synthesize RR for m.room.redaction as causes the RR to go missing. [\matrix-org#598](matrix-org#598) * Make Events create Dates on demand [\matrix-org#613](matrix-org#613) * Stop cloning events when adding to state [\matrix-org#612](matrix-org#612) * De-dup code: use the initialiseState function [\matrix-org#611](matrix-org#611) * Create sentinel members on-demand [\matrix-org#610](matrix-org#610) * Some more doc on how sentinels work [\matrix-org#609](matrix-org#609) * Migrate room encryption store to crypto store [\matrix-org#597](matrix-org#597) * add parameter to getIdentityServerUrl to strip the protocol for invites [\matrix-org#600](matrix-org#600) * Move Device Tracking Data to Crypto Store [\matrix-org#594](matrix-org#594) * Optimise pushprocessor [\matrix-org#591](matrix-org#591) * Set event error before emitting [\matrix-org#592](matrix-org#592) * Add event type for stickers [WIP] [\matrix-org#590](matrix-org#590) * Migrate inbound sessions to cryptostore [\matrix-org#587](matrix-org#587) * Disambiguate names if they contain an mxid [\matrix-org#588](matrix-org#588) * Check for sessions in indexeddb before migrating [\matrix-org#585](matrix-org#585) * Emit an event for crypto store migration [\matrix-org#586](matrix-org#586) * Supporting fixes For making UnknownDeviceDialog not pop up automatically [\matrix-org#575](matrix-org#575) * Move sessions to the crypto store [\matrix-org#584](matrix-org#584) * Change crypto store transaction API [\matrix-org#582](matrix-org#582) * Add some missed copyright notices [\matrix-org#581](matrix-org#581) * Move Olm account to IndexedDB [\matrix-org#579](matrix-org#579) * Fix logging of DecryptionErrors to be more useful [\matrix-org#580](matrix-org#580) * [BREAKING] Change the behaviour of the unverfied devices blacklist flag [\matrix-org#568](matrix-org#568) * Support set_presence=offline for syncing [\matrix-org#557](matrix-org#557) * Consider cases where the sender may not redact their own event [\matrix-org#556](matrix-org#556)

dbkr added 2 commits February 16, 2018 17:49

De-dup code: use the initialiseState function

275eb8d

This should behave identically, but the code here appeared to be identical to the code in initialiseState, so let's use it (it also has an extra sanity check in there that we only init empty timelines).

comment

8c02e7b

dbkr assigned richvdh Feb 16, 2018

Process state events before timeline events

b11bacc

Don't ignore them: its valid to send them in a non-limited sync, they're state events that preceed the timeline events.

richvdh requested changes Feb 19, 2018

View reviewed changes

richvdh assigned dbkr and unassigned richvdh Feb 19, 2018

dbkr added 5 commits February 19, 2018 16:05

Comments

21a3245

rename variable

c77277b

Factor out calls to getLiveTimeline

9a08194

Add test for fixed state misbehaviour

e810ee7

lint

8aa68b3

dbkr assigned richvdh and unassigned dbkr Feb 20, 2018

richvdh approved these changes Feb 20, 2018

View reviewed changes

richvdh assigned dbkr and unassigned richvdh Feb 20, 2018

Add XXX comment

1ddf7fb

dbkr merged commit 4351c4d into develop Feb 20, 2018

t3chguy deleted the dbkr/use_initialisestate branch May 10, 2022 14:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

De-dup code: use the initialiseState function #611

De-dup code: use the initialiseState function #611

dbkr commented Feb 16, 2018 •

edited

Loading

richvdh Feb 19, 2018

dbkr Feb 19, 2018

richvdh Feb 19, 2018

dbkr Feb 19, 2018

richvdh Feb 19, 2018

dbkr Feb 19, 2018

richvdh Feb 19, 2018

dbkr Feb 19, 2018

richvdh Feb 20, 2018

dbkr Feb 20, 2018

richvdh Feb 19, 2018

richvdh commented Feb 19, 2018

dbkr commented Feb 20, 2018

richvdh left a comment

De-dup code: use the initialiseState function #611

De-dup code: use the initialiseState function #611

Conversation

dbkr commented Feb 16, 2018 • 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

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

richvdh commented Feb 19, 2018

dbkr commented Feb 20, 2018

richvdh left a comment

Choose a reason for hiding this comment

dbkr commented Feb 16, 2018 •

edited

Loading