From 6c58c61da9a0a8ae0e7a7ceac9e85d016c95bc13 Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Fri, 7 May 2021 13:10:35 +0200 Subject: [PATCH] move switching room view models to a dedicated observable based on the observing the room status --- src/domain/session/RoomGridViewModel.js | 116 ++++++++---------- src/domain/session/RoomViewModelObservable.js | 81 ++++++++++++ src/domain/session/SessionViewModel.js | 114 ++++++++--------- src/observable/BaseObservable.js | 7 ++ 4 files changed, 196 insertions(+), 122 deletions(-) create mode 100644 src/domain/session/RoomViewModelObservable.js diff --git a/src/domain/session/RoomGridViewModel.js b/src/domain/session/RoomGridViewModel.js index ce31e22c..aee80b6a 100644 --- a/src/domain/session/RoomGridViewModel.js +++ b/src/domain/session/RoomGridViewModel.js @@ -15,7 +15,6 @@ limitations under the License. */ import {ViewModel} from "../ViewModel.js"; -import {removeRoomFromPath} from "../navigation/index.js"; function dedupeSparse(roomIds) { return roomIds.map((id, idx) => { @@ -33,10 +32,9 @@ export class RoomGridViewModel extends ViewModel { this._width = options.width; this._height = options.height; - this._createRoomViewModel = options.createRoomViewModel; + this._createRoomViewModelObservable = options.createRoomViewModelObservable; this._selectedIndex = 0; - this._viewModels = []; - this._refreshRoomViewModel = this._refreshRoomViewModel.bind(this); + this._viewModelsObservables = []; this._setupNavigation(); } @@ -55,38 +53,17 @@ export class RoomGridViewModel extends ViewModel { this.track(focusedRoom.subscribe(roomId => { if (roomId) { // as the room will be in the "rooms" observable - // (monitored by the parent vm) as well, + // (monitored by the parent vmo) as well, // we only change the focus here and trust - // setRoomIds to have created the vm already + // setRoomIds to have created the vmo already this._setFocusRoom(roomId); } })); // initial focus for a room is set by initializeRoomIdsAndTransferVM } - _refreshRoomViewModel(roomId) { - const index = this._viewModels.findIndex(vm => vm?.id === roomId); - if (index === -1) { - return; - } - this._viewModels[index] = this.disposeTracked(this._viewModels[index]); - // this will create a RoomViewModel because the invite is already - // removed from the collection (see Invite.afterSync) - const roomVM = this._createRoomViewModel(roomId, this._refreshRoomViewModel); - if (roomVM) { - this._viewModels[index] = this.track(roomVM); - if (this.focusIndex === index) { - roomVM.focus(); - } - } else { - // close room id - this.navigation.applyPath(removeRoomFromPath(this.navigation.path, roomId)); - } - this.emitChange(); - } - roomViewModelAt(i) { - return this._viewModels[i]; + return this._viewModelsObservables[i]?.get(); } get focusIndex() { @@ -105,9 +82,9 @@ export class RoomGridViewModel extends ViewModel { if (index === this._selectedIndex) { return; } - const vm = this._viewModels[index]; - if (vm) { - this.navigation.push("room", vm.id); + const vmo = this._viewModelsObservables[index]; + if (vmo) { + this.navigation.push("room", vmo.id); } else { this.navigation.push("empty-grid-tile", index); } @@ -120,7 +97,8 @@ export class RoomGridViewModel extends ViewModel { if (existingRoomVM) { const index = roomIds.indexOf(existingRoomVM.id); if (index !== -1) { - this._viewModels[index] = this.track(existingRoomVM); + this._viewModelsObservables[index] = this.track(existingRoomVM); + existingRoomVM.subscribe(viewModel => this._refreshRoomViewModel(viewModel)); transfered = true; } } @@ -128,7 +106,7 @@ export class RoomGridViewModel extends ViewModel { // now all view models exist, set the focus to the selected room const focusedRoom = this.navigation.path.get("room"); if (focusedRoom) { - const index = this._viewModels.findIndex(vm => vm && vm.id === focusedRoom.value); + const index = this._viewModelsObservables.findIndex(vmo => vmo && vmo.id === focusedRoom.value); if (index !== -1) { this._selectedIndex = index; } @@ -143,17 +121,17 @@ export class RoomGridViewModel extends ViewModel { const len = this._height * this._width; for (let i = 0; i < len; i += 1) { const newId = roomIds[i]; - const vm = this._viewModels[i]; + const vmo = this._viewModelsObservables[i]; // did anything change? - if ((!vm && newId) || (vm && vm.id !== newId)) { - if (vm) { - this._viewModels[i] = this.disposeTracked(vm); + if ((!vmo && newId) || (vmo && vmo.id !== newId)) { + if (vmo) { + this._viewModelsObservables[i] = this.disposeTracked(vmo); } if (newId) { - const newVM = this._createRoomViewModel(newId, this._refreshRoomViewModel); - if (newVM) { - this._viewModels[i] = this.track(newVM); - } + const vmo = this._createRoomViewModelObservable(newId); + this._viewModelsObservables[i] = this.track(vmo); + vmo.subscribe(viewModel => this._refreshRoomViewModel(viewModel)); + vmo.initialize(); } changed = true; } @@ -163,15 +141,21 @@ export class RoomGridViewModel extends ViewModel { } return changed; } + + _refreshRoomViewModel(viewModel) { + this.emitChange(); + viewModel?.focus(); + } /** called from SessionViewModel */ releaseRoomViewModel(roomId) { - const index = this._viewModels.findIndex(vm => vm && vm.id === roomId); + const index = this._viewModelsObservables.findIndex(vmo => vmo && vmo.id === roomId); if (index !== -1) { - const vm = this._viewModels[index]; - this.untrack(vm); - this._viewModels[index] = null; - return vm; + const vmo = this._viewModelsObservables[index]; + this.untrack(vmo); + vmo.unsubscribeAll(); + this._viewModelsObservables[index] = null; + return vmo; } } @@ -180,13 +164,13 @@ export class RoomGridViewModel extends ViewModel { return; } this._selectedIndex = idx; - const vm = this._viewModels[this._selectedIndex]; - vm?.focus(); + const vmo = this._viewModelsObservables[this._selectedIndex]; + vmo?.get()?.focus(); this.emitChange("focusIndex"); } _setFocusRoom(roomId) { - const index = this._viewModels.findIndex(vm => vm?.id === roomId); + const index = this._viewModelsObservables.findIndex(vmo => vmo?.id === roomId); if (index >= 0) { this._setFocusIndex(index); } @@ -194,6 +178,8 @@ export class RoomGridViewModel extends ViewModel { } import {createNavigation} from "../navigation/index.js"; +import {ObservableValue} from "../../observable/ObservableValue.js"; + export function tests() { class RoomVMMock { constructor(id) { @@ -209,6 +195,12 @@ export function tests() { } } + class RoomViewModelObservableMock extends ObservableValue { + async initialize() {} + dispose() { this.get()?.dispose(); } + get id() { return this.get()?.id; } + } + function createNavigationForRoom(rooms, room) { const navigation = createNavigation(); navigation.applyPath(navigation.pathFrom([ @@ -233,7 +225,7 @@ export function tests() { "initialize with duplicate set of rooms": assert => { const navigation = createNavigationForRoom(["c", "a", "b", undefined, "a"], "a"); const gridVM = new RoomGridViewModel({ - createRoomViewModel: id => new RoomVMMock(id), + createRoomViewModelObservable: id => new RoomViewModelObservableMock(new RoomVMMock(id)), navigation, width: 3, height: 2, @@ -250,12 +242,12 @@ export function tests() { "transfer room view model": assert => { const navigation = createNavigationForRoom(["a"], "a"); const gridVM = new RoomGridViewModel({ - createRoomViewModel: () => assert.fail("no vms should be created"), + createRoomViewModelObservable: () => assert.fail("no vms should be created"), navigation, width: 3, height: 2, }); - const existingRoomVM = new RoomVMMock("a"); + const existingRoomVM = new RoomViewModelObservableMock(new RoomVMMock("a")); const transfered = gridVM.initializeRoomIdsAndTransferVM(navigation.path.get("rooms").value, existingRoomVM); assert.equal(transfered, true); assert.equal(gridVM.focusIndex, 0); @@ -264,12 +256,12 @@ export function tests() { "reject transfer for non-matching room view model": assert => { const navigation = createNavigationForRoom(["a"], "a"); const gridVM = new RoomGridViewModel({ - createRoomViewModel: id => new RoomVMMock(id), + createRoomViewModelObservable: id => new RoomViewModelObservableMock(new RoomVMMock(id)), navigation, width: 3, height: 2, }); - const existingRoomVM = new RoomVMMock("f"); + const existingRoomVM = new RoomViewModelObservableMock(new RoomVMMock("f")); const transfered = gridVM.initializeRoomIdsAndTransferVM(navigation.path.get("rooms").value, existingRoomVM); assert.equal(transfered, false); assert.equal(gridVM.focusIndex, 0); @@ -278,7 +270,7 @@ export function tests() { "created & released room view model is not disposed": assert => { const navigation = createNavigationForRoom(["a"], "a"); const gridVM = new RoomGridViewModel({ - createRoomViewModel: id => new RoomVMMock(id), + createRoomViewModelObservable: id => new RoomViewModelObservableMock(new RoomVMMock(id)), navigation, width: 3, height: 2, @@ -287,27 +279,27 @@ export function tests() { assert.equal(transfered, false); const releasedVM = gridVM.releaseRoomViewModel("a"); gridVM.dispose(); - assert.equal(releasedVM.disposed, false); + assert.equal(releasedVM.get().disposed, false); }, "transfered & released room view model is not disposed": assert => { const navigation = createNavigationForRoom([undefined, "a"], "a"); const gridVM = new RoomGridViewModel({ - createRoomViewModel: () => assert.fail("no vms should be created"), + createRoomViewModelObservable: () => assert.fail("no vms should be created"), navigation, width: 3, height: 2, }); - const existingRoomVM = new RoomVMMock("a"); + const existingRoomVM = new RoomViewModelObservableMock(new RoomVMMock("a")); const transfered = gridVM.initializeRoomIdsAndTransferVM(navigation.path.get("rooms").value, existingRoomVM); assert.equal(transfered, true); const releasedVM = gridVM.releaseRoomViewModel("a"); gridVM.dispose(); - assert.equal(releasedVM.disposed, false); + assert.equal(releasedVM.get().disposed, false); }, "try release non-existing room view model is": assert => { const navigation = createNavigationForEmptyTile([undefined, "b"], 3); const gridVM = new RoomGridViewModel({ - createRoomViewModel: id => new RoomVMMock(id), + createRoomViewModelObservable: id => new RoomViewModelObservableMock(new RoomVMMock(id)), navigation, width: 3, height: 2, @@ -319,7 +311,7 @@ export function tests() { "initial focus is set to empty tile": assert => { const navigation = createNavigationForEmptyTile(["a"], 1); const gridVM = new RoomGridViewModel({ - createRoomViewModel: id => new RoomVMMock(id), + createRoomViewModelObservable: id => new RoomViewModelObservableMock(new RoomVMMock(id)), navigation, width: 3, height: 2, @@ -331,7 +323,7 @@ export function tests() { "change room ids after creation": assert => { const navigation = createNavigationForRoom(["a", "b"], "a"); const gridVM = new RoomGridViewModel({ - createRoomViewModel: id => new RoomVMMock(id), + createRoomViewModelObservable: id => new RoomViewModelObservableMock(new RoomVMMock(id)), navigation, width: 3, height: 2, diff --git a/src/domain/session/RoomViewModelObservable.js b/src/domain/session/RoomViewModelObservable.js new file mode 100644 index 00000000..030ba5e4 --- /dev/null +++ b/src/domain/session/RoomViewModelObservable.js @@ -0,0 +1,81 @@ +/* +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. +*/ + +import {ObservableValue} from "../../observable/ObservableValue.js"; + +/** +Depending on the status of a room (invited, joined, archived, or none), +we want to show a different view with a different view model +when showing a room. Furthermore, this logic is needed both in the +single room view and in the grid view. So this logic is extracted here, +and this observable updates with the right view model as the status for +a room changes. + +To not have to track the subscription manually in the SessionViewModel and +the RoomGridViewModel, all subscriptions are removed in the dispose method. +Only when transferring a RoomViewModelObservable between the SessionViewModel +and RoomGridViewModel, unsubscribeAll should be called prior to doing +the transfer, so either parent view model don't keep getting updates for +the now transferred child view model. + +This is also why there is an explicit initialize method, see comment there. +*/ +export class RoomViewModelObservable extends ObservableValue { + constructor(sessionViewModel, roomId) { + super(null); + this._sessionViewModel = sessionViewModel; + this.id = roomId; + } + + /** + Separate initialize method rather than doing this onSubscribeFirst because + we don't want to run this again when transferring this value between + SessionViewModel and RoomGridViewModel, as onUnsubscribeLast and onSubscribeFirst + are called in that case. + */ + async initialize() { + const {session} = this._sessionViewModel._sessionContainer; + this._statusObservable = await session.observeRoomStatus(this.id); + this.set(await this._statusToViewModel(this._statusObservable.get())); + this._statusObservable.subscribe(async status => { + this.set(await this._statusToViewModel(status)); + }); + } + + async _statusToViewModel(status) { + if (status.invited) { + return this._sessionViewModel._createInviteViewModel(this.id); + } else if (status.joined) { + return this._sessionViewModel._createRoomViewModel(this.id); + } else if (status.archived) { + if (!this.get() || this.get().kind !== "room") { + return await this._sessionViewModel._createArchivedRoomViewModel(this.id); + } else { + // reuse existing Room + return this.get(); + } + } + return null; + } + + dispose() { + if (this._statusSubscription) { + this._statusSubscription = this._statusSubscription(); + } + this.unsubscribeAll(); + this.get()?.dispose(); + } +} \ No newline at end of file diff --git a/src/domain/session/SessionViewModel.js b/src/domain/session/SessionViewModel.js index 1e59a9d5..c847ce79 100644 --- a/src/domain/session/SessionViewModel.js +++ b/src/domain/session/SessionViewModel.js @@ -15,7 +15,6 @@ See the License for the specific language governing permissions and limitations under the License. */ -import {removeRoomFromPath} from "../navigation/index.js"; import {LeftPanelViewModel} from "./leftpanel/LeftPanelViewModel.js"; import {RoomViewModel} from "./room/RoomViewModel.js"; import {InviteViewModel} from "./room/InviteViewModel.js"; @@ -24,6 +23,7 @@ import {SessionStatusViewModel} from "./SessionStatusViewModel.js"; import {RoomGridViewModel} from "./RoomGridViewModel.js"; import {SettingsViewModel} from "./settings/SettingsViewModel.js"; import {ViewModel} from "../ViewModel.js"; +import {RoomViewModelObservable} from "./RoomViewModelObservable.js"; export class SessionViewModel extends ViewModel { constructor(options) { @@ -40,10 +40,8 @@ export class SessionViewModel extends ViewModel { rooms: this._sessionContainer.session.rooms }))); this._settingsViewModel = null; - this._currentRoomViewModel = null; + this._roomViewModelObservable = null; this._gridViewModel = null; - this._refreshRoomViewModel = this._refreshRoomViewModel.bind(this); - this._createRoomViewModel = this._createRoomViewModel.bind(this); this._setupNavigation(); } @@ -90,7 +88,7 @@ export class SessionViewModel extends ViewModel { } get activeMiddleViewModel() { - return this._currentRoomViewModel || this._gridViewModel || this._settingsViewModel; + return this._roomViewModelObservable?.get() || this._gridViewModel || this._settingsViewModel; } get roomGridViewModel() { @@ -110,7 +108,7 @@ export class SessionViewModel extends ViewModel { } get currentRoomViewModel() { - return this._currentRoomViewModel; + return this._roomViewModelObservable?.get(); } _updateGrid(roomIds) { @@ -121,12 +119,14 @@ export class SessionViewModel extends ViewModel { this._gridViewModel = this.track(new RoomGridViewModel(this.childOptions({ width: 3, height: 2, - createRoomViewModel: this._createRoomViewModel, + createRoomViewModelObservable: roomId => new RoomViewModelObservable(this, roomId), }))); - if (this._gridViewModel.initializeRoomIdsAndTransferVM(roomIds, this._currentRoomViewModel)) { - this._currentRoomViewModel = this.untrack(this._currentRoomViewModel); - } else if (this._currentRoomViewModel) { - this._currentRoomViewModel = this.disposeTracked(this._currentRoomViewModel); + // try to transfer the current room view model, so we don't have to reload the timeline + this._roomViewModelObservable?.unsubscribeAll(); + if (this._gridViewModel.initializeRoomIdsAndTransferVM(roomIds, this._roomViewModelObservable)) { + this._roomViewModelObservable = this.untrack(this._roomViewModelObservable); + } else if (this._roomViewModelObservable) { + this._roomViewModelObservable = this.disposeTracked(this._roomViewModelObservable); } } else { this._gridViewModel.setRoomIds(roomIds); @@ -134,14 +134,12 @@ export class SessionViewModel extends ViewModel { } else if (this._gridViewModel && !roomIds) { // closing grid, try to show focused room in grid if (currentRoomId) { - const vm = this._gridViewModel.releaseRoomViewModel(currentRoomId.value); - if (vm) { - this._currentRoomViewModel = this.track(vm); - } else { - const newVM = this._createRoomViewModel(currentRoomId.value, this._refreshRoomViewModel); - if (newVM) { - this._currentRoomViewModel = this.track(newVM); - } + const vmo = this._gridViewModel.releaseRoomViewModel(currentRoomId.value); + if (vmo) { + this._roomViewModelObservable = this.track(vmo); + this._roomViewModelObservable.subscribe(() => { + this.emitChange("activeMiddleViewModel"); + }); } } this._gridViewModel = this.disposeTracked(this._gridViewModel); @@ -151,63 +149,59 @@ export class SessionViewModel extends ViewModel { } } - /** - * @param {string} roomId - * @param {function} refreshRoomViewModel passed in as an argument, because the grid needs a different impl of this - * @return {RoomViewModel | InviteViewModel} - */ - _createRoomViewModel(roomId, refreshRoomViewModel) { + _createRoomViewModel(roomId) { + const room = this._sessionContainer.session.rooms.get(roomId); + if (room) { + const roomVM = new RoomViewModel(this.childOptions({ + room, + ownUserId: this._sessionContainer.session.user.id, + })); + roomVM.load(); + return roomVM; + } + return null; + } + + async _createArchivedRoomViewModel(roomId) { + const room = await this._sessionContainer.session.loadArchivedRoom(roomId); + if (room) { + const roomVM = new RoomViewModel(this.childOptions({ + room, + ownUserId: this._sessionContainer.session.user.id, + })); + roomVM.load(); + return roomVM; + } + return null; + } + + _createInviteViewModel(roomId) { const invite = this._sessionContainer.session.invites.get(roomId); if (invite) { return new InviteViewModel(this.childOptions({ invite, mediaRepository: this._sessionContainer.session.mediaRepository, - refreshRoomViewModel, })); - } else { - const room = this._sessionContainer.session.rooms.get(roomId); - if (room) { - const roomVM = new RoomViewModel(this.childOptions({ - room, - ownUserId: this._sessionContainer.session.user.id, - refreshRoomViewModel - })); - roomVM.load(); - return roomVM; - } } return null; } - /** refresh the room view model after an internal change that needs - to change between invite, room or none state */ - _refreshRoomViewModel(roomId) { - this._currentRoomViewModel = this.disposeTracked(this._currentRoomViewModel); - const roomVM = this._createRoomViewModel(roomId, this._refreshRoomViewModel); - if (roomVM) { - this._currentRoomViewModel = this.track(roomVM); - } else { - // close room id - this.navigation.applyPath(removeRoomFromPath(this.navigation.path, roomId)); - } - this.emitChange("activeMiddleViewModel"); - } - _updateRoom(roomId) { // opening a room and already open? - if (this._currentRoomViewModel?.id === roomId) { + if (this._roomViewModelObservable?.id === roomId) { return; } // close if needed - if (this._currentRoomViewModel) { - this._currentRoomViewModel = this.disposeTracked(this._currentRoomViewModel); + if (this._roomViewModelObservable) { + this._roomViewModelObservable = this.disposeTracked(this._roomViewModelObservable); } - // and try opening again - const roomVM = this._createRoomViewModel(roomId, this._refreshRoomViewModel); - if (roomVM) { - this._currentRoomViewModel = this.track(roomVM); - } - this.emitChange("activeMiddleViewModel"); + const vmo = new RoomViewModelObservable(this, roomId); + this._roomViewModelObservable = this.track(vmo); + // subscription is unsubscribed in RoomViewModelObservable.dispose, and thus handled by track + this._roomViewModelObservable.subscribe(() => { + this.emitChange("activeMiddleViewModel"); + }); + vmo.initialize(); } _updateSettings(settingsOpen) { diff --git a/src/observable/BaseObservable.js b/src/observable/BaseObservable.js index 29387020..0f9934c3 100644 --- a/src/observable/BaseObservable.js +++ b/src/observable/BaseObservable.js @@ -48,6 +48,13 @@ export class BaseObservable { return null; } + unsubscribeAll() { + if (this._handlers.size !== 0) { + this._handlers.clear(); + this.onUnsubscribeLast(); + } + } + get hasSubscriptions() { return this._handlers.size !== 0; }