From c12abab52d6b4f1be95084fbb4b80578395f480a Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Tue, 4 Sep 2018 13:03:55 +0200 Subject: [PATCH 1/8] wait until rooms are available as accountData get processed before rooms, during initial sync or loading sync from cache, accountData gets emitted before any room is available, hence our patching wasn't doing anything. Just as well, because it would have failed (see next commits) --- src/utils/DMRoomMap.js | 22 +++++++++++++++++++--- 1 file changed, 19 insertions(+), 3 deletions(-) diff --git a/src/utils/DMRoomMap.js b/src/utils/DMRoomMap.js index 0c5e696c5f..789970c744 100644 --- a/src/utils/DMRoomMap.js +++ b/src/utils/DMRoomMap.js @@ -70,13 +70,13 @@ export default class DMRoomMap { this.matrixClient.removeListener("accountData", this._onAccountData); } - _onAccountData(ev) { + async _onAccountData(ev) { if (ev.getType() == 'm.direct') { const userToRooms = this.matrixClient.getAccountData('m.direct').getContent() || {}; const myUserId = this.matrixClient.getUserId(); const selfDMs = userToRooms[myUserId]; if (selfDMs && selfDMs.length) { - const neededPatching = this._patchUpSelfDMs(userToRooms); + const neededPatching = await this._patchUpSelfDMs(userToRooms); // to avoid multiple devices fighting to correct // the account data, only try to send the corrected // version once. @@ -94,10 +94,13 @@ export default class DMRoomMap { * with ourself, not the other user. Fix it by guessing the other user and * modifying userToRooms */ - _patchUpSelfDMs(userToRooms) { + async _patchUpSelfDMs(userToRooms) { const myUserId = this.matrixClient.getUserId(); const selfRoomIds = userToRooms[myUserId]; if (selfRoomIds) { + // account data gets emitted before the rooms are available + // so wait for the sync to be ready and then read the rooms. + await this._waitForSyncReady(); // any self-chats that should not be self-chats? const guessedUserIdsThatChanged = selfRoomIds.map((roomId) => { const room = this.matrixClient.getRoom(roomId); @@ -135,6 +138,19 @@ export default class DMRoomMap { } } + _waitForSyncReady() { + return new Promise((resolve) => { + const syncState = this.matrixClient.getSyncState(); + if (syncState === 'PREPARED' || syncState === 'SYNCING') { + resolve(); + } else { + // if we already got an accountData event, + // next sync should not be ERROR, so just resolve + this.matrixClient.once('sync', () => resolve()); + } + }); + } + getDMRoomsForUserId(userId) { // Here, we return the empty list if there are no rooms, // since the number of conversations you have with this user is zero. From 245dfbf957cf3705c529bb50445e665107f5659f Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Tue, 4 Sep 2018 13:06:27 +0200 Subject: [PATCH 2/8] only put NON-guessed rooms in self-chats --- src/utils/DMRoomMap.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/utils/DMRoomMap.js b/src/utils/DMRoomMap.js index 789970c744..56832a1a75 100644 --- a/src/utils/DMRoomMap.js +++ b/src/utils/DMRoomMap.js @@ -117,7 +117,7 @@ export default class DMRoomMap { return false; } userToRooms[myUserId] = selfRoomIds.filter((roomId) => { - return guessedUserIdsThatChanged + return !guessedUserIdsThatChanged .some((ids) => ids.roomId === roomId); }); From 441036ff93f546292231833e7ec068171b2d2469 Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Tue, 4 Sep 2018 13:07:24 +0200 Subject: [PATCH 3/8] remove duplicates from room lists --- src/ArrayUtils.js | 29 +++++++++++++++++++++++++++++ src/utils/DMRoomMap.js | 14 +++++--------- 2 files changed, 34 insertions(+), 9 deletions(-) create mode 100644 src/ArrayUtils.js diff --git a/src/ArrayUtils.js b/src/ArrayUtils.js new file mode 100644 index 0000000000..850fcc40f9 --- /dev/null +++ b/src/ArrayUtils.js @@ -0,0 +1,29 @@ +/* +Copyright 2018 New Vector + +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. +*/ + +/** + * creates a new array with only the unique values of the given array + * @return {array} the deduplicated array + */ +export function unique(arr) { + const cpy = []; + arr.forEach((el) => { + if (!cpy.includes(el)) { + cpy.push(el); + } + }); + return cpy; +}; diff --git a/src/utils/DMRoomMap.js b/src/utils/DMRoomMap.js index 56832a1a75..cb76610e4d 100644 --- a/src/utils/DMRoomMap.js +++ b/src/utils/DMRoomMap.js @@ -15,6 +15,7 @@ limitations under the License. */ import MatrixClientPeg from '../MatrixClientPeg'; +import {unique} from '../ArrayUtils'; /** * Class that takes a Matrix Client and flips the m.direct map @@ -120,19 +121,14 @@ export default class DMRoomMap { return !guessedUserIdsThatChanged .some((ids) => ids.roomId === roomId); }); - guessedUserIdsThatChanged.forEach(({userId, roomId}) => { - if (!userId) { - // if not able to guess the other user (unlikely) - // still put it in the map so the room stays marked - // as a DM, we just wont be able to show an avatar. - userId = ""; - } let roomIds = userToRooms[userId]; if (!roomIds) { - roomIds = userToRooms[userId] = []; + userToRooms[userId] = [roomId]; + } else { + roomIds.push(roomId); + userToRooms[userId] = unique(roomIds); } - roomIds.push(roomId); }); return true; } From 6c7cb380e06c4cfead5c916ac8f3089e7ecee4c9 Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Tue, 4 Sep 2018 13:07:45 +0200 Subject: [PATCH 4/8] add warning in console when patching rooms, so it appears in rage shakes --- src/utils/DMRoomMap.js | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/utils/DMRoomMap.js b/src/utils/DMRoomMap.js index cb76610e4d..637df4fd0f 100644 --- a/src/utils/DMRoomMap.js +++ b/src/utils/DMRoomMap.js @@ -81,6 +81,8 @@ export default class DMRoomMap { // to avoid multiple devices fighting to correct // the account data, only try to send the corrected // version once. + console.warn(`Invalid m.direct account data detected ` + + `(self-chats that shouldn't be), patching it up.`); if (neededPatching && !this._hasSentOutPatchDirectAccountDataPatch) { this._hasSentOutPatchDirectAccountDataPatch = true; this.matrixClient.setAccountData('m.direct', userToRooms); From abd9d3a11e0b2db2722d2551f2b53ebad2549f40 Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Tue, 4 Sep 2018 13:55:29 +0200 Subject: [PATCH 5/8] fix lint --- src/ArrayUtils.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/ArrayUtils.js b/src/ArrayUtils.js index 850fcc40f9..ca1aea9b5b 100644 --- a/src/ArrayUtils.js +++ b/src/ArrayUtils.js @@ -16,6 +16,7 @@ limitations under the License. /** * creates a new array with only the unique values of the given array + * @param {array} arr the array to deduplicate * @return {array} the deduplicated array */ export function unique(arr) { @@ -26,4 +27,4 @@ export function unique(arr) { } }); return cpy; -}; +} From e171296d51aeee49467622ead79f65cb9d8260e8 Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Tue, 4 Sep 2018 16:00:40 +0200 Subject: [PATCH 6/8] patch self-chats lazily in favor of awaiting sync state --- src/utils/DMRoomMap.js | 71 ++++++++++++++++++------------------------ 1 file changed, 30 insertions(+), 41 deletions(-) diff --git a/src/utils/DMRoomMap.js b/src/utils/DMRoomMap.js index 637df4fd0f..453d6c89b0 100644 --- a/src/utils/DMRoomMap.js +++ b/src/utils/DMRoomMap.js @@ -37,11 +37,8 @@ export default class DMRoomMap { this._onAccountData = this._onAccountData.bind(this); const mDirectEvent = matrixClient.getAccountData('m.direct'); - if (!mDirectEvent) { - this.userToRooms = {}; - } else { - this.userToRooms = mDirectEvent.getContent(); - } + this.mDirectEvent = mDirectEvent ? mDirectEvent.getContent() : {}; + this.userToRooms = null; } /** @@ -71,25 +68,11 @@ export default class DMRoomMap { this.matrixClient.removeListener("accountData", this._onAccountData); } - async _onAccountData(ev) { + _onAccountData(ev) { if (ev.getType() == 'm.direct') { - const userToRooms = this.matrixClient.getAccountData('m.direct').getContent() || {}; - const myUserId = this.matrixClient.getUserId(); - const selfDMs = userToRooms[myUserId]; - if (selfDMs && selfDMs.length) { - const neededPatching = await this._patchUpSelfDMs(userToRooms); - // to avoid multiple devices fighting to correct - // the account data, only try to send the corrected - // version once. - console.warn(`Invalid m.direct account data detected ` + - `(self-chats that shouldn't be), patching it up.`); - if (neededPatching && !this._hasSentOutPatchDirectAccountDataPatch) { - this._hasSentOutPatchDirectAccountDataPatch = true; - this.matrixClient.setAccountData('m.direct', userToRooms); - } - } - this.userToRooms = userToRooms; - this._populateRoomToUser(); + this.mDirectEvent = this.matrixClient.getAccountData('m.direct').getContent() || {}; + this.userToRooms = null; + this.roomToUser = null; } } /** @@ -97,13 +80,10 @@ export default class DMRoomMap { * with ourself, not the other user. Fix it by guessing the other user and * modifying userToRooms */ - async _patchUpSelfDMs(userToRooms) { + _patchUpSelfDMs(userToRooms) { const myUserId = this.matrixClient.getUserId(); const selfRoomIds = userToRooms[myUserId]; if (selfRoomIds) { - // account data gets emitted before the rooms are available - // so wait for the sync to be ready and then read the rooms. - await this._waitForSyncReady(); // any self-chats that should not be self-chats? const guessedUserIdsThatChanged = selfRoomIds.map((roomId) => { const room = this.matrixClient.getRoom(roomId); @@ -136,19 +116,6 @@ export default class DMRoomMap { } } - _waitForSyncReady() { - return new Promise((resolve) => { - const syncState = this.matrixClient.getSyncState(); - if (syncState === 'PREPARED' || syncState === 'SYNCING') { - resolve(); - } else { - // if we already got an accountData event, - // next sync should not be ERROR, so just resolve - this.matrixClient.once('sync', () => resolve()); - } - }); - } - getDMRoomsForUserId(userId) { // Here, we return the empty list if there are no rooms, // since the number of conversations you have with this user is zero. @@ -177,9 +144,31 @@ export default class DMRoomMap { return this.roomToUser[roomId]; } + _getUserToRooms() { + if (!this.userToRooms) { + const userToRooms = this.mDirectEvent; + const myUserId = this.matrixClient.getUserId(); + const selfDMs = userToRooms[myUserId]; + if (selfDMs && selfDMs.length) { + const neededPatching = this._patchUpSelfDMs(userToRooms); + // to avoid multiple devices fighting to correct + // the account data, only try to send the corrected + // version once. + console.warn(`Invalid m.direct account data detected ` + + `(self-chats that shouldn't be), patching it up.`); + if (neededPatching && !this._hasSentOutPatchDirectAccountDataPatch) { + this._hasSentOutPatchDirectAccountDataPatch = true; + this.matrixClient.setAccountData('m.direct', userToRooms); + } + } + this.userToRooms = userToRooms; + } + return this.userToRooms; + } + _populateRoomToUser() { this.roomToUser = {}; - for (const user of Object.keys(this.userToRooms)) { + for (const user of Object.keys(this._getUserToRooms())) { for (const roomId of this.userToRooms[user]) { this.roomToUser[roomId] = user; } From 44a53cfc0d2358a3702ab3b4469113290c87c0e4 Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Tue, 4 Sep 2018 16:03:15 +0200 Subject: [PATCH 7/8] use lodash for unique function instead of rolling our own --- src/ArrayUtils.js | 30 ------------------------------ src/utils/DMRoomMap.js | 4 ++-- 2 files changed, 2 insertions(+), 32 deletions(-) delete mode 100644 src/ArrayUtils.js diff --git a/src/ArrayUtils.js b/src/ArrayUtils.js deleted file mode 100644 index ca1aea9b5b..0000000000 --- a/src/ArrayUtils.js +++ /dev/null @@ -1,30 +0,0 @@ -/* -Copyright 2018 New Vector - -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. -*/ - -/** - * creates a new array with only the unique values of the given array - * @param {array} arr the array to deduplicate - * @return {array} the deduplicated array - */ -export function unique(arr) { - const cpy = []; - arr.forEach((el) => { - if (!cpy.includes(el)) { - cpy.push(el); - } - }); - return cpy; -} diff --git a/src/utils/DMRoomMap.js b/src/utils/DMRoomMap.js index 453d6c89b0..e0004ebc9d 100644 --- a/src/utils/DMRoomMap.js +++ b/src/utils/DMRoomMap.js @@ -15,7 +15,7 @@ limitations under the License. */ import MatrixClientPeg from '../MatrixClientPeg'; -import {unique} from '../ArrayUtils'; +import _uniq from 'lodash/uniq'; /** * Class that takes a Matrix Client and flips the m.direct map @@ -109,7 +109,7 @@ export default class DMRoomMap { userToRooms[userId] = [roomId]; } else { roomIds.push(roomId); - userToRooms[userId] = unique(roomIds); + userToRooms[userId] = _uniq(roomIds); } }); return true; From 3b29b7aab6488f338434f6a3d365e01bf62b058f Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Tue, 4 Sep 2018 17:36:50 +0200 Subject: [PATCH 8/8] fix getDMRoomsForUserId not calling _getUserToRooms first (thanks e2e tests) --- src/utils/DMRoomMap.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/utils/DMRoomMap.js b/src/utils/DMRoomMap.js index e0004ebc9d..bea6e702fa 100644 --- a/src/utils/DMRoomMap.js +++ b/src/utils/DMRoomMap.js @@ -119,7 +119,7 @@ export default class DMRoomMap { getDMRoomsForUserId(userId) { // Here, we return the empty list if there are no rooms, // since the number of conversations you have with this user is zero. - return this.userToRooms[userId] || []; + return this._getUserToRooms()[userId] || []; } getUserIdForRoomId(roomId) {