From 4176af56eafc76aa4516d61f78af9fabbdbfde08 Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Sat, 21 Mar 2020 13:57:41 +0100 Subject: [PATCH 1/3] add failing test for problem --- .../session/room/timeline/TilesCollection.js | 64 +++++++++++++++++++ 1 file changed, 64 insertions(+) diff --git a/src/domain/session/room/timeline/TilesCollection.js b/src/domain/session/room/timeline/TilesCollection.js index 818bfba3..de2cb324 100644 --- a/src/domain/session/room/timeline/TilesCollection.js +++ b/src/domain/session/room/timeline/TilesCollection.js @@ -172,3 +172,67 @@ export default class TilesCollection extends BaseObservableList { return this._tiles.length; } } + +import ObservableArray from "../../../../observable/list/ObservableArray.js"; +import UpdateAction from "./UpdateAction.js"; + +export function tests() { + + class TestTile { + constructor(entry, update) { + this.entry = entry; + this.update = update; + } + tryIncludeEntry() { + return false; + } + compareEntry(b) { + return this.entry.n - b.n; + } + removeEntry() { + return true; + } + get upperEntry() { + return this.entry; + } + + get lowerEntry() { + return this.entry; + } + updateNextSibling() {} + updatePreviousSibling() {} + updateEntry() { + return UpdateAction.Nothing; + } + } + + return { + "don't emit update before add": assert => { + class UpdateOnSiblingTile extends TestTile { + updateNextSibling() { + // this happens with isContinuation + this.update(this, "next"); + } + updatePreviousSibling() { + // this happens with isContinuation + this.update(this, "previous"); + } + } + const entries = new ObservableArray([{n: 5}, {n: 10}]); + const tiles = new TilesCollection(entries, (e, u) => new UpdateOnSiblingTile(e, u)); + let receivedAdd = false; + tiles.subscribe({ + onAdd(idx, tile) { + assert(tile.entry.n, 7); + receivedAdd = true; + }, + onUpdate(idx, tile) { + assert(tile.entry.n, 7); + assert(!receivedAdd, "receiving update before add"); + } + }); + entries.insert(1, {n: 7}); + assert(receivedAdd); + }, + } +} From a3714f49cc347e7b766c03a82a7254b13f679d4e Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Sat, 21 Mar 2020 14:26:56 +0100 Subject: [PATCH 2/3] group public methods for a tile together --- .../session/room/timeline/tiles/SimpleTile.js | 34 +++++++++---------- 1 file changed, 16 insertions(+), 18 deletions(-) diff --git a/src/domain/session/room/timeline/tiles/SimpleTile.js b/src/domain/session/room/timeline/tiles/SimpleTile.js index 48935948..8733c0dc 100644 --- a/src/domain/session/room/timeline/tiles/SimpleTile.js +++ b/src/domain/session/room/timeline/tiles/SimpleTile.js @@ -21,21 +21,26 @@ export default class SimpleTile { get hasDateSeparator() { return false; } - // TilesCollection contract? unused atm - get upperEntry() { - return this._entry; - } - - // TilesCollection contract? unused atm - get lowerEntry() { - return this._entry; - } emitUpdate(paramName) { this._emitUpdate(this, paramName); } - // TilesCollection contract + get internalId() { + return this._entry.asEventKey().toString(); + } + + get isPending() { + return this._entry.isPending; + } + get upperEntry() { + return this._entry; + } + + get lowerEntry() { + return this._entry; + } + compareEntry(entry) { return this._entry.compare(entry); } @@ -65,12 +70,5 @@ export default class SimpleTile { updateNextSibling(next) { } - - get internalId() { - return this._entry.asEventKey().toString(); - } - - get isPending() { - return this._entry.isPending; - } + // TilesCollection contract above } From f72910822666471267e77d3c2192d2b5f7304c16 Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Sat, 21 Mar 2020 14:28:09 +0100 Subject: [PATCH 3/3] pass emit update fn through setter so we control when tile can update --- .../session/room/timeline/TilesCollection.js | 35 ++++++++++++++----- .../session/room/timeline/tiles/SimpleTile.js | 13 +++++-- 2 files changed, 36 insertions(+), 12 deletions(-) diff --git a/src/domain/session/room/timeline/TilesCollection.js b/src/domain/session/room/timeline/TilesCollection.js index de2cb324..cb2621d6 100644 --- a/src/domain/session/room/timeline/TilesCollection.js +++ b/src/domain/session/room/timeline/TilesCollection.js @@ -2,6 +2,10 @@ import BaseObservableList from "../../../../observable/list/BaseObservableList.j import sortedIndex from "../../../../utils/sortedIndex.js"; // maps 1..n entries to 0..1 tile. Entries are what is stored in the timeline, either an event or fragmentboundary +// for now, tileCreator should be stable in whether it returns a tile or not. +// e.g. the decision to create a tile or not should be based on properties +// not updated later on (e.g. event type) +// also see big comment in onUpdate export default class TilesCollection extends BaseObservableList { constructor(entries, tileCreator) { super(); @@ -28,7 +32,7 @@ export default class TilesCollection extends BaseObservableList { let currentTile = null; for (let entry of this._entries) { if (!currentTile || !currentTile.tryIncludeEntry(entry)) { - currentTile = this._tileCreator(entry, this._emitSpontanousUpdate); + currentTile = this._tileCreator(entry); if (currentTile) { this._tiles.push(currentTile); } @@ -45,6 +49,11 @@ export default class TilesCollection extends BaseObservableList { if (prevTile) { prevTile.updateNextSibling(null); } + // now everything is wired up, + // allow tiles to emit updates + for (const tile of this._tiles) { + tile.setUpdateEmit(this._emitSpontanousUpdate); + } } _findTileIdx(entry) { @@ -93,10 +102,11 @@ export default class TilesCollection extends BaseObservableList { return; } - const newTile = this._tileCreator(entry, this._emitSpontanousUpdate); + const newTile = this._tileCreator(entry); if (newTile) { if (prevTile) { prevTile.updateNextSibling(newTile); + // this emits an update while the add hasn't been emitted yet newTile.updatePreviousSibling(prevTile); } if (nextTile) { @@ -105,6 +115,9 @@ export default class TilesCollection extends BaseObservableList { } this._tiles.splice(tileIdx, 0, newTile); this.emitAdd(tileIdx, newTile); + // add event is emitted, now the tile + // can emit updates + newTile.setUpdateEmit(this._emitSpontanousUpdate); } // find position by sort key // ask siblings to be included? both? yes, twice: a (insert c here) b, ask a(c), if yes ask b(a), else ask b(c)? if yes then b(a)? @@ -141,6 +154,7 @@ export default class TilesCollection extends BaseObservableList { this._tiles.splice(tileIdx, 1); prevTile && prevTile.updateNextSibling(nextTile); nextTile && nextTile.updatePreviousSibling(prevTile); + tile.setUpdateEmit(null); this.emitRemove(tileIdx, tile); } @@ -177,10 +191,12 @@ import ObservableArray from "../../../../observable/list/ObservableArray.js"; import UpdateAction from "./UpdateAction.js"; export function tests() { - class TestTile { - constructor(entry, update) { + constructor(entry) { this.entry = entry; + this.update = null; + } + setUpdateEmit(update) { this.update = update; } tryIncludeEntry() { @@ -211,15 +227,15 @@ export function tests() { class UpdateOnSiblingTile extends TestTile { updateNextSibling() { // this happens with isContinuation - this.update(this, "next"); + this.update && this.update(this, "next"); } updatePreviousSibling() { // this happens with isContinuation - this.update(this, "previous"); + this.update && this.update(this, "previous"); } } const entries = new ObservableArray([{n: 5}, {n: 10}]); - const tiles = new TilesCollection(entries, (e, u) => new UpdateOnSiblingTile(e, u)); + const tiles = new TilesCollection(entries, entry => new UpdateOnSiblingTile(entry)); let receivedAdd = false; tiles.subscribe({ onAdd(idx, tile) { @@ -227,8 +243,9 @@ export function tests() { receivedAdd = true; }, onUpdate(idx, tile) { - assert(tile.entry.n, 7); - assert(!receivedAdd, "receiving update before add"); + if (tile.entry.n === 7) { + assert(!receivedAdd, "receiving update before add"); + } } }); entries.insert(1, {n: 7}); diff --git a/src/domain/session/room/timeline/tiles/SimpleTile.js b/src/domain/session/room/timeline/tiles/SimpleTile.js index 8733c0dc..7b1ed91f 100644 --- a/src/domain/session/room/timeline/tiles/SimpleTile.js +++ b/src/domain/session/room/timeline/tiles/SimpleTile.js @@ -1,9 +1,9 @@ import UpdateAction from "../UpdateAction.js"; export default class SimpleTile { - constructor({entry, emitUpdate}) { + constructor({entry}) { this._entry = entry; - this._emitUpdate = emitUpdate; + this._emitUpdate = null; } // view model props for all subclasses // hmmm, could also do instanceof ... ? @@ -23,7 +23,9 @@ export default class SimpleTile { } emitUpdate(paramName) { - this._emitUpdate(this, paramName); + if (this._emitUpdate) { + this._emitUpdate(this, paramName); + } } get internalId() { @@ -33,6 +35,11 @@ export default class SimpleTile { get isPending() { return this._entry.isPending; } + // TilesCollection contract below + setUpdateEmit(emitUpdate) { + this._emitUpdate = emitUpdate; + } + get upperEntry() { return this._entry; }