diff --git a/src/domain/session/room/timeline/ReactionsViewModel.js b/src/domain/session/room/timeline/ReactionsViewModel.js index 4ebb7fcc..886230a5 100644 --- a/src/domain/session/room/timeline/ReactionsViewModel.js +++ b/src/domain/session/room/timeline/ReactionsViewModel.js @@ -40,14 +40,13 @@ export class ReactionsViewModel { } } if (pendingAnnotations) { - for (const [key, count] of pendingAnnotations.entries()) { + for (const [key, annotation] of pendingAnnotations.entries()) { const reaction = this._map.get(key); if (reaction) { - if (reaction._tryUpdatePending(count)) { - this._map.update(key); - } + reaction._tryUpdatePending(annotation); + this._map.update(key); } else { - this._map.add(key, new ReactionViewModel(key, null, count, this._parentTile)); + this._map.add(key, new ReactionViewModel(key, null, annotation, this._parentTile)); } } } @@ -78,10 +77,10 @@ export class ReactionsViewModel { } class ReactionViewModel { - constructor(key, annotation, pendingCount, parentTile) { + constructor(key, annotation, pending, parentTile) { this._key = key; this._annotation = annotation; - this._pendingCount = pendingCount; + this._pending = pending; this._parentTile = parentTile; this._isToggling = false; } @@ -101,12 +100,12 @@ class ReactionViewModel { return false; } - _tryUpdatePending(pendingCount) { - if (pendingCount !== this._pendingCount) { - this._pendingCount = pendingCount; - return true; + _tryUpdatePending(pending) { + if (!pending && !this._pending) { + return false; } - return false; + this._pending = pending; + return true; } get key() { @@ -114,17 +113,11 @@ class ReactionViewModel { } get count() { - let count = this._pendingCount || 0; - if (this._annotation) { - count += this._annotation.count; - } - return count; + return (this._pending?.count || 0) + (this._annotation?.count || 0); } get isPending() { - // even if pendingCount is 0, - // it means we have both a pending reaction and redaction - return this._pendingCount !== null; + return this._pending !== null; } /** @returns {boolean} true if the user has a (pending) reaction @@ -138,19 +131,19 @@ class ReactionViewModel { /** @returns {boolean} Whether the user has reacted with this key, * taking the local reaction and reaction redaction into account. */ get haveReacted() { - // determine whether if everything pending is sent, if we have a - // reaction or not. This depends on the order of the pending events ofcourse, - // which we don't have access to here, but we assume that a redaction comes first - // if we have a remote reaction - const {isPending} = this; - const haveRemoteReaction = this._annotation?.me; - const haveLocalRedaction = isPending && this._pendingCount <= 0; - const haveLocalReaction = isPending && this._pendingCount >= 0; - const haveReaction = (haveRemoteReaction && !haveLocalRedaction) || - // if remote, then assume redaction comes first and reaction last, so final state is reacted - (haveRemoteReaction && haveLocalRedaction && haveLocalReaction) || - (!haveRemoteReaction && !haveLocalRedaction && haveLocalReaction); - return haveReaction; + // TODO: cleanup + return this._parentTile._entry.haveAnnotation(this.key); + } + + get firstTimestamp() { + let ts = Number.MAX_SAFE_INTEGER; + if (this._annotation) { + ts = Math.min(ts, this._annotation.firstTimestamp); + } + if (this._pending) { + ts = Math.min(ts, this._pending.firstTimestamp); + } + return ts; } _compare(other) { @@ -163,40 +156,16 @@ class ReactionViewModel { if (this.count !== other.count) { return other.count - this.count; } else { - const a = this._annotation; - const b = other._annotation; - if (a && b) { - const cmp = a.firstTimestamp - b.firstTimestamp; - if (cmp === 0) { - return this.key < other.key ? -1 : 1; - } else { - return cmp; - } - } else if (a) { - return -1; - } else { - return 1; + const cmp = this.firstTimestamp - other.firstTimestamp; + if (cmp === 0) { + return this.key < other.key ? -1 : 1; } + return cmp; } } toggleReaction(log = null) { - return this._parentTile.logger.wrapOrRun(log, "toggleReaction", async log => { - if (this._isToggling) { - log.set("busy", true); - return; - } - this._isToggling = true; - try { - if (this.haveReacted) { - await log.wrap("redactReaction", log => this._parentTile._redactReaction(this.key, log)); - } else { - await log.wrap("react", log => this._parentTile._react(this.key, log)); - } - } finally { - this._isToggling = false; - } - }); + return this._parentTile.toggleReaction(this.key, log); } } @@ -257,18 +226,6 @@ export function tests() { } return { - "haveReacted": assert => { - assert.equal(false, new ReactionViewModel("🚀", null, null).haveReacted); - assert.equal(false, new ReactionViewModel("🚀", {me: false, count: 1}, null).haveReacted); - assert.equal(true, new ReactionViewModel("🚀", {me: true, count: 1}, null).haveReacted); - assert.equal(true, new ReactionViewModel("🚀", {me: true, count: 2}, null).haveReacted); - assert.equal(true, new ReactionViewModel("🚀", null, 1).haveReacted); - assert.equal(false, new ReactionViewModel("🚀", {me: true, count: 1}, -1).haveReacted); - // pending count 0 means the remote reaction has been redacted and is sending, then a new reaction was queued - assert.equal(true, new ReactionViewModel("🚀", {me: true, count: 1}, 0).haveReacted); - // should typically not happen without a remote reaction already present, but should still be false - assert.equal(false, new ReactionViewModel("🚀", null, 0).haveReacted); - }, // these are more an integration test than unit tests, // but fully test the local echo when toggling and // the correct send queue modifications happen diff --git a/src/domain/session/room/timeline/tiles/BaseMessageTile.js b/src/domain/session/room/timeline/tiles/BaseMessageTile.js index e56a160a..6e43b138 100644 --- a/src/domain/session/room/timeline/tiles/BaseMessageTile.js +++ b/src/domain/session/room/timeline/tiles/BaseMessageTile.js @@ -125,8 +125,7 @@ export class BaseMessageTile extends SimpleTile { react(key, log = null) { return this.logger.wrapOrRun(log, "react", log => { - const keyVM = this.reactions?.getReaction(key); - if (keyVM?.haveReacted) { + if (this._entry.haveAnnotation(key)) { log.set("already_reacted", true); return; } @@ -141,7 +140,7 @@ export class BaseMessageTile extends SimpleTile { log.set("ongoing", true); return; } - const redaction = this._entry.getAnnotationPendingRedaction(key); + const redaction = this._entry.pendingAnnotations?.get(key)?.redactionEntry; try { const updatePromise = new Promise(resolve => this._pendingReactionChangeCallback = resolve); if (redaction && !redaction.pendingEvent.hasStartedSending) { @@ -159,7 +158,7 @@ export class BaseMessageTile extends SimpleTile { redactReaction(key, log = null) { return this.logger.wrapOrRun(log, "redactReaction", log => { const keyVM = this.reactions?.getReaction(key); - if (!keyVM?.haveReacted) { + if (!this._entry.haveAnnotation(key)) { log.set("not_yet_reacted", true); return; } @@ -170,11 +169,16 @@ export class BaseMessageTile extends SimpleTile { async _redactReaction(key, log) { // This will also block concurently removing multiple reactions, // but in practice it happens fast enough. + + // TODO: remove this as we'll protect against reentry in the SendQueue if (this._pendingReactionChangeCallback) { log.set("ongoing", true); return; } - const entry = await this._entry.getOwnAnnotationEntry(this._timeline, key); + let entry = this._entry.pendingAnnotations?.get(key)?.annotationEntry; + if (!entry) { + entry = await this._timeline.getOwnAnnotationEntry(this._entry.id, key); + } if (entry) { try { const updatePromise = new Promise(resolve => this._pendingReactionChangeCallback = resolve); @@ -188,6 +192,16 @@ export class BaseMessageTile extends SimpleTile { } } + toggleReaction(key, log = null) { + return this.logger.wrapOrRun(log, "toggleReaction", async log => { + if (this._entry.haveAnnotation(key)) { + await log.wrap("redactReaction", log => this._redactReaction(key, log)); + } else { + await log.wrap("react", log => this._react(key, log)); + } + }); + } + _updateReactions() { const {annotations, pendingAnnotations} = this._entry; if (!annotations && !pendingAnnotations) { diff --git a/src/matrix/room/timeline/PendingAnnotation.js b/src/matrix/room/timeline/PendingAnnotation.js new file mode 100644 index 00000000..0b14159c --- /dev/null +++ b/src/matrix/room/timeline/PendingAnnotation.js @@ -0,0 +1,76 @@ +/* +Copyright 2021 The Matrix.org Foundation C.I.C. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +export class PendingAnnotation { + constructor() { + // TODO: use simple member for reaction and redaction as we can't/shouldn't really have more than 2 entries + // this contains both pending annotation entries, and pending redactions of remote annotation entries + this._entries = []; + } + + get firstTimestamp() { + return this._entries.reduce((ts, e) => { + if (e.isRedaction) { + return ts; + } + return Math.min(e.timestamp, ts); + }, Number.MAX_SAFE_INTEGER); + } + + get annotationEntry() { + return this._entries.find(e => !e.isRedaction); + } + + get redactionEntry() { + return this._entries.find(e => e.isRedaction); + } + + get count() { + return this._entries.reduce((count, e) => { + return count + (e.isRedaction ? -1 : 1); + }, 0); + } + + add(entry) { + this._entries.push(entry); + } + + remove(entry) { + const idx = this._entries.indexOf(entry); + if (idx === -1) { + return false; + } + this._entries.splice(idx, 1); + return true; + } + + get willAnnotate() { + const lastEntry = this._entries.reduce((lastEntry, e) => { + if (!lastEntry || e.pendingEvent.queueIndex > lastEntry.pendingEvent.queueIndex) { + return e; + } + return lastEntry; + }, null); + if (lastEntry) { + return !lastEntry.isRedaction; + } + return false; + } + + get isEmpty() { + return this._entries.length === 0; + } +} diff --git a/src/matrix/room/timeline/PendingAnnotations.js b/src/matrix/room/timeline/PendingAnnotations.js deleted file mode 100644 index 1dd32abd..00000000 --- a/src/matrix/room/timeline/PendingAnnotations.js +++ /dev/null @@ -1,74 +0,0 @@ -/* -Copyright 2021 The Matrix.org Foundation C.I.C. - -Licensed under the Apache License, Version 2.0 (the "License"); -you may not use this file except in compliance with the License. -You may obtain a copy of the License at - - http://www.apache.org/licenses/LICENSE-2.0 - -Unless required by applicable law or agreed to in writing, software -distributed under the License is distributed on an "AS IS" BASIS, -WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -See the License for the specific language governing permissions and -limitations under the License. -*/ - -export class PendingAnnotations { - constructor() { - this.aggregatedAnnotations = new Map(); - // this contains both pending annotation entries, and pending redactions of remote annotation entries - this._entries = []; - } - - /** adds either a pending annotation entry, or a remote annotation entry with a pending redaction */ - add(entry) { - const {key} = (entry.redactingEntry || entry).relation; - if (!key) { - return; - } - const count = this.aggregatedAnnotations.get(key) || 0; - const addend = entry.isRedaction ? -1 : 1; - this.aggregatedAnnotations.set(key, count + addend); - this._entries.push(entry); - } - - /** removes either a pending annotation entry, or a remote annotation entry with a pending redaction */ - remove(entry) { - const idx = this._entries.indexOf(entry); - if (idx === -1) { - return; - } - this._entries.splice(idx, 1); - const {key} = (entry.redactingEntry || entry).relation; - let count = this.aggregatedAnnotations.get(key); - if (count !== undefined) { - const addend = entry.isRedaction ? 1 : -1; - count += addend; - this.aggregatedAnnotations.set(key, count); - } - if (!this._entries.length) { - this.aggregatedAnnotations.clear(); - } - } - - findForKey(key) { - return this._entries.find(e => { - if (e.relation?.key === key) { - return e; - } - }); - } - - findRedactionForKey(key) { - return this._entries.find(e => { - if (e.redactingEntry?.relation?.key === key) { - return e; - } - }); - } - - get isEmpty() { - return this._entries.length === 0; - } -} diff --git a/src/matrix/room/timeline/Timeline.js b/src/matrix/room/timeline/Timeline.js index 423643cf..8fd84075 100644 --- a/src/matrix/room/timeline/Timeline.js +++ b/src/matrix/room/timeline/Timeline.js @@ -416,7 +416,7 @@ export function tests() { relatedEventId: entry.id }})); await poll(() => timeline.entries.length === 2); - assert.equal(entry.pendingAnnotations.get("👋"), 1); + assert.equal(entry.pendingAnnotations.get("👋").count, 1); const reactionEntry = getIndexFromIterable(timeline.entries, 1); // 3. add redaction to timeline pendingEvents.append(new PendingEvent({data: { @@ -429,11 +429,11 @@ export function tests() { }})); // TODO: await nextUpdate here with ListObserver, to ensure entry emits an update when pendingAnnotations changes await poll(() => timeline.entries.length === 3); - assert.equal(entry.pendingAnnotations.get("👋"), 0); + assert.equal(entry.pendingAnnotations.get("👋").count, 0); // 4. cancel redaction pendingEvents.remove(1); await poll(() => timeline.entries.length === 2); - assert.equal(entry.pendingAnnotations.get("👋"), 1); + assert.equal(entry.pendingAnnotations.get("👋").count, 1); // 5. cancel reaction pendingEvents.remove(0); await poll(() => timeline.entries.length === 1); @@ -507,7 +507,7 @@ export function tests() { relatedEventId: reactionEntry.id }})); await poll(() => timeline.entries.length >= 3); - assert.equal(messageEntry.pendingAnnotations.get("👋"), -1); + assert.equal(messageEntry.pendingAnnotations.get("👋").count, -1); }, "local reaction gets applied after remote echo is added to timeline": async assert => { const messageEntry = new EventEntry({event: withTextBody("hi bob!", withSender(alice, createEvent("m.room.message", "!abc"))), @@ -533,7 +533,7 @@ export function tests() { await poll(() => timeline.entries.length === 2); const entry = getIndexFromIterable(timeline.entries, 0); assert.equal(entry, messageEntry); - assert.equal(entry.pendingAnnotations.get("👋"), 1); + assert.equal(entry.pendingAnnotations.get("👋").count, 1); }, "local reaction removal gets applied after remote echo is added to timeline with reaction not loaded": async assert => { const messageId = "!abc"; @@ -570,7 +570,7 @@ export function tests() { await poll(() => timeline.entries.length === 2); // 5. check that redaction was linked to reaction target const entry = getIndexFromIterable(timeline.entries, 0); - assert.equal(entry.pendingAnnotations.get("👋"), -1); + assert.equal(entry.pendingAnnotations.get("👋").count, -1); }, "decrypted entry preserves content when receiving other update without decryption": async assert => { // 1. create encrypted and decrypted entry diff --git a/src/matrix/room/timeline/entries/BaseEventEntry.js b/src/matrix/room/timeline/entries/BaseEventEntry.js index 45874dcb..9302e009 100644 --- a/src/matrix/room/timeline/entries/BaseEventEntry.js +++ b/src/matrix/room/timeline/entries/BaseEventEntry.js @@ -17,7 +17,7 @@ limitations under the License. import {BaseEntry} from "./BaseEntry.js"; import {REDACTION_TYPE} from "../../common.js"; import {createAnnotation, ANNOTATION_RELATION_TYPE, getRelationFromContent} from "../relations.js"; -import {PendingAnnotations} from "../PendingAnnotations.js"; +import {PendingAnnotation} from "../PendingAnnotation.js"; /** Deals mainly with local echo for relations and redactions, * so it is shared between PendingEventEntry and EventEntry */ @@ -65,10 +65,18 @@ export class BaseEventEntry extends BaseEntry { if (relationEntry.isRelatedToId(this.id)) { if (relationEntry.relation.rel_type === ANNOTATION_RELATION_TYPE) { if (!this._pendingAnnotations) { - this._pendingAnnotations = new PendingAnnotations(); + this._pendingAnnotations = new Map(); + } + const {key} = (entry.redactingEntry || entry).relation; + if (key) { + let annotation = this._pendingAnnotations.get(key); + if (!annotation) { + annotation = new PendingAnnotation(); + this._pendingAnnotations.set(key, annotation); + } + annotation.add(entry); + return "pendingAnnotations"; } - this._pendingAnnotations.add(entry); - return "pendingAnnotations"; } } } @@ -92,11 +100,17 @@ export class BaseEventEntry extends BaseEntry { const relationEntry = entry.redactingEntry || entry; if (relationEntry.isRelatedToId(this.id)) { if (relationEntry.relation?.rel_type === ANNOTATION_RELATION_TYPE && this._pendingAnnotations) { - this._pendingAnnotations.remove(entry); - if (this._pendingAnnotations.isEmpty) { - this._pendingAnnotations = null; + const {key} = (entry.redactingEntry || entry).relation; + if (key) { + let annotation = this._pendingAnnotations.get(key); + if (annotation.remove(entry) && annotation.isEmpty) { + this._pendingAnnotations.delete(key); + } + if (this._pendingAnnotations.size === 0) { + this._pendingAnnotations = null; + } + return "pendingAnnotations"; } - return "pendingAnnotations"; } } } @@ -128,19 +142,29 @@ export class BaseEventEntry extends BaseEntry { return id && this.relatedEventId === id; } + haveAnnotation(key) { + const haveRemoteReaction = this.annotations?.[key]?.me || false; + const pendingAnnotation = this.pendingAnnotations?.get(key); + const willAnnotate = pendingAnnotation?.willAnnotate || false; + /* + We have an annotation in these case: + - remote annotation with me, no pending + - remote annotation with me, pending redaction and then annotation + - pending annotation without redaction after it + */ + return (haveRemoteReaction && (!pendingAnnotation || willAnnotate)) || + (!haveRemoteReaction && willAnnotate); + } + get relation() { return getRelationFromContent(this.content); } get pendingAnnotations() { - return this._pendingAnnotations?.aggregatedAnnotations; + return this._pendingAnnotations; } - async getOwnAnnotationEntry(timeline, key) { - return this._pendingAnnotations?.findForKey(key); - } - - getAnnotationPendingRedaction(key) { - return this._pendingAnnotations?.findRedactionForKey(key); + get annotations() { + return null; //overwritten in EventEntry } } diff --git a/src/matrix/room/timeline/entries/EventEntry.js b/src/matrix/room/timeline/entries/EventEntry.js index a0c3799d..f98801f9 100644 --- a/src/matrix/room/timeline/entries/EventEntry.js +++ b/src/matrix/room/timeline/entries/EventEntry.js @@ -139,13 +139,89 @@ export class EventEntry extends BaseEventEntry { get annotations() { return this._eventEntry.annotations; } +} - async getOwnAnnotationEntry(timeline, key) { - const localId = await super.getOwnAnnotationEntry(timeline, key); - if (localId) { - return localId; - } else { - return timeline.getOwnAnnotationEntry(this.id, key); +import {withTextBody, withContent, createEvent} from "../../../../mocks/event.js"; +import {Clock as MockClock} from "../../../../mocks/Clock.js"; +import {PendingEventEntry} from "./PendingEventEntry.js"; +import {PendingEvent} from "../../sending/PendingEvent.js"; +import {createAnnotation} from "../relations.js"; + +export function tests() { + let queueIndex = 0; + const clock = new MockClock(); + + function addPendingReaction(target, key) { + queueIndex += 1; + target.addLocalRelation(new PendingEventEntry({ + pendingEvent: new PendingEvent({data: { + eventType: "m.reaction", + content: createAnnotation(target.id, key), + queueIndex, + txnId: `t${queueIndex}` + }}), + clock + })); + return target; + } + + function addPendingRedaction(target, key) { + const pendingReaction = target.pendingAnnotations?.get(key)?.annotationEntry; + let redactingEntry = pendingReaction; + // make up a remote entry if we don't have a pending reaction and have an aggregated remote entry + if (!pendingReaction && target.annotations[key].me) { + redactingEntry = new EventEntry({ + event: withContent(createAnnotation(target.id, key), createEvent("m.reaction", "!def")) + }); + } + queueIndex += 1; + target.addLocalRelation(new PendingEventEntry({ + pendingEvent: new PendingEvent({data: { + eventType: "m.room.redaction", + relatedTxnId: pendingReaction ? pendingReaction.id : null, + relatedEventId: pendingReaction ? null : redactingEntry.id, + queueIndex, + txnId: `t${queueIndex}` + }}), + redactingEntry, + clock + })); + return target; + } + + function remoteAnnotation(key, me, count, obj = {}) { + obj[key] = {me, count}; + return obj; + } + + return { + // testing it here because parent class always assumes annotations is null + "haveAnnotation": assert => { + const msgEvent = withTextBody("hi!", createEvent("m.room.message", "!abc")); + const e1 = new EventEntry({event: msgEvent}); + assert.equal(false, e1.haveAnnotation("🚀")); + const e2 = new EventEntry({event: msgEvent, annotations: remoteAnnotation("🚀", false, 1)}); + assert.equal(false, e2.haveAnnotation("🚀")); + const e3 = new EventEntry({event: msgEvent, annotations: remoteAnnotation("🚀", true, 1)}); + assert.equal(true, e3.haveAnnotation("🚀")); + const e4 = new EventEntry({event: msgEvent, annotations: remoteAnnotation("🚀", true, 2)}); + assert.equal(true, e4.haveAnnotation("🚀")); + const e5 = addPendingReaction(new EventEntry({event: msgEvent}), "🚀"); + assert.equal(true, e5.haveAnnotation("🚀")); + const e6 = addPendingRedaction(new EventEntry({event: msgEvent, annotations: remoteAnnotation("🚀", true, 1)}), "🚀"); + assert.equal(false, e6.haveAnnotation("🚀")); + const e7 = addPendingReaction( + addPendingRedaction( + new EventEntry({event: msgEvent, annotations: remoteAnnotation("🚀", true, 1)}), + "🚀"), + "🚀"); + assert.equal(true, e7.haveAnnotation("🚀")); + const e8 = addPendingRedaction( + addPendingReaction( + new EventEntry({event: msgEvent}), + "🚀"), + "🚀"); + assert.equal(false, e8.haveAnnotation("🚀")); } } } diff --git a/src/matrix/room/timeline/entries/PendingEventEntry.js b/src/matrix/room/timeline/entries/PendingEventEntry.js index d42211ef..742bff49 100644 --- a/src/matrix/room/timeline/entries/PendingEventEntry.js +++ b/src/matrix/room/timeline/entries/PendingEventEntry.js @@ -23,7 +23,10 @@ export class PendingEventEntry extends BaseEventEntry { this._pendingEvent = pendingEvent; /** @type {RoomMember} */ this._member = member; - this._clock = clock; + // try to come up with a timestamp that is around construction time and + // will be roughly sorted by queueIndex, so it can be used to as a secondary + // sorting dimension for reactions + this._timestamp = clock.now() - (100 - pendingEvent.queueIndex); this._redactingEntry = redactingEntry; } @@ -64,7 +67,7 @@ export class PendingEventEntry extends BaseEventEntry { } get timestamp() { - return this._clock.now(); + return this._timestamp; } get isPending() {