From 4efac368d51a00fc7fb4ce119522556951dc1bf8 Mon Sep 17 00:00:00 2001 From: vas Date: Fri, 20 May 2022 18:06:43 +0300 Subject: [PATCH] only refresh view when the server URL changes (MM-34565) (#2082) (#2094) * only refresh view when the server URL changes (MM-34565) Create views of current and incoming tabs indexed by unique [URL, TABTYPE] tuples, and diffing them. Tuples that are identical are recycled, merely porting the new server name over. * lint fixes * WIP * linting * remove dependency on by, duad * provide a more exaplanatory name for TabView.prototype.tuple (urlTypeTuple) * minor improvements in viewManager - remove stateful behaviour from makeView - more descriptive variable names when looping - create new arrays before sorting (sort is in-place by default) * resolve linting errors --- package-lock.json | 13 ++ package.json | 3 +- src/common/tabs/BaseTabView.ts | 7 +- src/common/tabs/TabView.ts | 2 + src/main/views/MattermostView.ts | 6 +- src/main/views/viewManager.test.js | 69 +++++++--- src/main/views/viewManager.ts | 125 ++++++++++++------ src/types/external/tuple-record-polyfill.d.ts | 16 +++ 8 files changed, 179 insertions(+), 62 deletions(-) create mode 100644 src/types/external/tuple-record-polyfill.d.ts diff --git a/package-lock.json b/package-lock.json index 9c34addf..9aac8027 100644 --- a/package-lock.json +++ b/package-lock.json @@ -41,6 +41,7 @@ "@babel/preset-env": "7.16.11", "@babel/preset-react": "7.16.7", "@babel/register": "7.17.7", + "@bloomberg/record-tuple-polyfill": "^0.0.4", "@electron/fuses": "1.5.0", "@electron/universal": "1.2.1", "@storybook/addon-actions": "6.4.20", @@ -2081,6 +2082,12 @@ "integrity": "sha512-0hYQ8SB4Db5zvZB4axdMHGwEaQjkZzFjQiN9LVYvIFB2nSUHW9tYpxWriPrWDASIxiaXax83REcLxuSdnGPZtw==", "dev": true }, + "node_modules/@bloomberg/record-tuple-polyfill": { + "version": "0.0.4", + "resolved": "https://registry.npmjs.org/@bloomberg/record-tuple-polyfill/-/record-tuple-polyfill-0.0.4.tgz", + "integrity": "sha512-h0OYmPR3A5Dfbetra/GzxBAzQk8sH7LhRkRUTdagX6nrtlUgJGYCTv4bBK33jsTQw9HDd8PE2x1Ma+iRKEDUsw==", + "dev": true + }, "node_modules/@develar/schema-utils": { "version": "2.6.5", "resolved": "https://registry.npmjs.org/@develar/schema-utils/-/schema-utils-2.6.5.tgz", @@ -33023,6 +33030,12 @@ "integrity": "sha512-0hYQ8SB4Db5zvZB4axdMHGwEaQjkZzFjQiN9LVYvIFB2nSUHW9tYpxWriPrWDASIxiaXax83REcLxuSdnGPZtw==", "dev": true }, + "@bloomberg/record-tuple-polyfill": { + "version": "0.0.4", + "resolved": "https://registry.npmjs.org/@bloomberg/record-tuple-polyfill/-/record-tuple-polyfill-0.0.4.tgz", + "integrity": "sha512-h0OYmPR3A5Dfbetra/GzxBAzQk8sH7LhRkRUTdagX6nrtlUgJGYCTv4bBK33jsTQw9HDd8PE2x1Ma+iRKEDUsw==", + "dev": true + }, "@develar/schema-utils": { "version": "2.6.5", "resolved": "https://registry.npmjs.org/@develar/schema-utils/-/schema-utils-2.6.5.tgz", diff --git a/package.json b/package.json index e1ac9f15..4a5e9c9b 100644 --- a/package.json +++ b/package.json @@ -9,7 +9,7 @@ "desktopName": "Mattermost.Desktop", "homepage": "https://mattermost.com", "engines": { - "node": ">=4.2.0" + "node": ">=14.6.0" }, "repository": { "type": "git", @@ -115,6 +115,7 @@ "@babel/preset-env": "7.16.11", "@babel/preset-react": "7.16.7", "@babel/register": "7.17.7", + "@bloomberg/record-tuple-polyfill": "^0.0.4", "@electron/fuses": "1.5.0", "@electron/universal": "1.2.1", "@storybook/addon-actions": "6.4.20", diff --git a/src/common/tabs/BaseTabView.ts b/src/common/tabs/BaseTabView.ts index a6338bab..fc06da41 100644 --- a/src/common/tabs/BaseTabView.ts +++ b/src/common/tabs/BaseTabView.ts @@ -1,9 +1,11 @@ // Copyright (c) 2016-present Mattermost, Inc. All Rights Reserved. // See LICENSE.txt for license information. +import {Tuple as tuple} from '@bloomberg/record-tuple-polyfill'; + import {MattermostServer} from 'common/servers/MattermostServer'; -import {getTabViewName, TabType, TabView} from './TabView'; +import {getTabViewName, TabType, TabView, TabTuple} from './TabView'; export default abstract class BaseTabView implements TabView { server: MattermostServer; @@ -14,6 +16,9 @@ export default abstract class BaseTabView implements TabView { get name(): string { return getTabViewName(this.server.name, this.type); } + get urlTypeTuple(): TabTuple { + return tuple(this.url.href, this.type) as TabTuple; + } get url(): URL { throw new Error('Not implemented'); } diff --git a/src/common/tabs/TabView.ts b/src/common/tabs/TabView.ts index 33ff0b4d..3ffead6c 100644 --- a/src/common/tabs/TabView.ts +++ b/src/common/tabs/TabView.ts @@ -13,6 +13,7 @@ export const TAB_MESSAGING = 'TAB_MESSAGING'; export const TAB_FOCALBOARD = 'TAB_FOCALBOARD'; export const TAB_PLAYBOOKS = 'TAB_PLAYBOOKS'; export type TabType = typeof TAB_MESSAGING | typeof TAB_FOCALBOARD | typeof TAB_PLAYBOOKS; +export type TabTuple = [string, TabType]; export interface TabView { server: MattermostServer; @@ -21,6 +22,7 @@ export interface TabView { get type(): TabType; get url(): URL; get shouldNotify(): boolean; + get urlTypeTuple(): TabTuple; } export function getDefaultTeamWithTabsFromTeam(team: Team) { diff --git a/src/main/views/MattermostView.ts b/src/main/views/MattermostView.ts index 1b210007..929b11d7 100644 --- a/src/main/views/MattermostView.ts +++ b/src/main/views/MattermostView.ts @@ -22,7 +22,7 @@ import { LOADSCREEN_END, } from 'common/communication'; -import {TabView} from 'common/tabs/TabView'; +import {TabView, TabTuple} from 'common/tabs/TabView'; import {ServerInfo} from 'main/server/serverInfo'; import ContextMenu from '../contextMenu'; @@ -136,6 +136,10 @@ export class MattermostView extends EventEmitter { return this.tab.name; } + get urlTypeTuple(): TabTuple { + return this.tab.urlTypeTuple; + } + resetLoadingStatus = () => { if (this.status !== Status.LOADING) { // if it's already loading, don't touch anything delete this.retryLoad; diff --git a/src/main/views/viewManager.test.js b/src/main/views/viewManager.test.js index 4a79ef8c..9bd03d54 100644 --- a/src/main/views/viewManager.test.js +++ b/src/main/views/viewManager.test.js @@ -5,6 +5,7 @@ 'use strict'; import {dialog} from 'electron'; +import {Tuple as tuple} from '@bloomberg/record-tuple-polyfill'; import {BROWSER_HISTORY_PUSH, LOAD_SUCCESS} from 'common/communication'; import {MattermostServer} from 'common/servers/MattermostServer'; @@ -29,7 +30,7 @@ jest.mock('electron', () => ({ jest.mock('common/tabs/TabView', () => ({ getServerView: jest.fn(), - getTabViewName: jest.fn(), + getTabViewName: jest.fn((a, b) => `${a}-${b}`), })); jest.mock('common/servers/MattermostServer', () => ({ @@ -65,15 +66,18 @@ describe('main/views/viewManager', () => { const viewManager = new ViewManager({}); const onceFn = jest.fn(); const loadFn = jest.fn(); + const destroyFn = jest.fn(); beforeEach(() => { viewManager.createLoadingScreen = jest.fn(); viewManager.showByName = jest.fn(); getServerView.mockImplementation((srv, tab) => ({name: `${srv.name}-${tab.name}`})); - MattermostView.mockImplementation(() => ({ + MattermostView.mockImplementation((tab) => ({ on: jest.fn(), load: loadFn, once: onceFn, + destroy: destroyFn, + name: tab.name, })); }); @@ -172,11 +176,31 @@ describe('main/views/viewManager', () => { viewManager.loadView = jest.fn(); viewManager.showByName = jest.fn(); viewManager.showInitial = jest.fn(); - getServerView.mockImplementation((srv, tab) => ({name: `${srv.name}-${tab.name}`, url: new URL(`http://${srv.name}.com`)})); + viewManager.mainWindow.webContents = { + send: jest.fn(), + }; + + getServerView.mockImplementation((srv, tab) => ({ + name: `${srv.name}-${tab.name}`, + urlTypeTuple: tuple(`http://${srv.name}.com/`, tab.name), + url: new URL(`http://${srv.name}.com`), + })); MattermostServer.mockImplementation((name, url) => ({ name, url: new URL(url), })); + const onceFn = jest.fn(); + const loadFn = jest.fn(); + const destroyFn = jest.fn(); + MattermostView.mockImplementation((tab) => ({ + on: jest.fn(), + load: loadFn, + once: onceFn, + destroy: destroyFn, + name: tab.name, + urlTypeTuple: tab.urlTypeTuple, + tab, + })); }); afterEach(() => { @@ -188,13 +212,12 @@ describe('main/views/viewManager', () => { }); it('should recycle existing views', () => { - const view = { + const makeSpy = jest.spyOn(viewManager, 'makeView'); + const view = new MattermostView({ name: 'server1-tab1', - tab: { - name: 'server1-tab1', - url: new URL('http://server1.com'), - }, - }; + urlTypeTuple: tuple(new URL('http://server1.com').href, 'tab1'), + server: 'server1', + }); viewManager.views.set('server1-tab1', view); viewManager.reloadConfiguration([ { @@ -210,7 +233,8 @@ describe('main/views/viewManager', () => { }, ]); expect(viewManager.views.get('server1-tab1')).toBe(view); - expect(viewManager.loadView).not.toHaveBeenCalled(); + expect(makeSpy).not.toHaveBeenCalled(); + makeSpy.mockRestore(); }); it('should close tabs that arent open', () => { @@ -231,6 +255,7 @@ describe('main/views/viewManager', () => { }); it('should create new views for new tabs', () => { + const makeSpy = jest.spyOn(viewManager, 'makeView'); viewManager.reloadConfiguration([ { name: 'server1', @@ -244,13 +269,19 @@ describe('main/views/viewManager', () => { ], }, ]); - expect(viewManager.loadView).toHaveBeenCalledWith({ - name: 'server1', - url: new URL('http://server1.com'), - }, expect.any(Object), { - name: 'tab1', - isOpen: true, - }); + expect(makeSpy).toHaveBeenCalledWith( + { + name: 'server1', + url: new URL('http://server1.com'), + }, + expect.any(Object), + { + name: 'tab1', + isOpen: true, + }, + 'http://server1.com/', + ); + makeSpy.mockRestore(); }); it('should set focus to current view on reload', () => { @@ -260,6 +291,8 @@ describe('main/views/viewManager', () => { name: 'server1-tab1', url: new URL('http://server1.com'), }, + urlTypeTuple: tuple('http://server1.com/', 'tab1'), + destroy: jest.fn(), }; viewManager.currentView = 'server1-tab1'; viewManager.views.set('server1-tab1', view); @@ -277,6 +310,7 @@ describe('main/views/viewManager', () => { }, ]); expect(viewManager.showByName).toHaveBeenCalledWith('server1-tab1'); + expect(viewManager.mainWindow.webContents.send).not.toHaveBeenCalled(); }); it('should show initial if currentView has been removed', () => { @@ -286,6 +320,7 @@ describe('main/views/viewManager', () => { name: 'server1-tab1', url: new URL('http://server1.com'), }, + urlTypeTuple: ['http://server.com/', 'tab1'], destroy: jest.fn(), }; viewManager.currentView = 'server1-tab1'; diff --git a/src/main/views/viewManager.ts b/src/main/views/viewManager.ts index 696f6e27..2ce193ae 100644 --- a/src/main/views/viewManager.ts +++ b/src/main/views/viewManager.ts @@ -3,6 +3,7 @@ import log from 'electron-log'; import {BrowserView, BrowserWindow, dialog, ipcMain, IpcMainEvent} from 'electron'; import {BrowserViewConstructorOptions} from 'electron/main'; +import {Tuple as tuple} from '@bloomberg/record-tuple-polyfill'; import {Tab, TeamWithTabs} from 'types/config'; @@ -24,7 +25,7 @@ import Config from 'common/config'; import urlUtils from 'common/utils/url'; import Utils from 'common/utils/util'; import {MattermostServer} from 'common/servers/MattermostServer'; -import {getServerView, getTabViewName} from 'common/tabs/TabView'; +import {getServerView, getTabViewName, TabTuple, TabType} from 'common/tabs/TabView'; import {ServerInfo} from 'main/server/serverInfo'; @@ -80,25 +81,34 @@ export class ViewManager { server.tabs.forEach((tab) => this.loadView(srv, serverInfo, tab)); } - loadView = (srv: MattermostServer, serverInfo: ServerInfo, tab: Tab, url?: string) => { + makeView = (srv: MattermostServer, serverInfo: ServerInfo, tab: Tab, url?: string): MattermostView => { const tabView = getServerView(srv, tab); - if (!tab.isOpen) { - this.closedViews.set(tabView.name, {srv, tab}); - return; - } - if (this.closedViews.has(tabView.name)) { - this.closedViews.delete(tabView.name); - } const view = new MattermostView(tabView, serverInfo, this.mainWindow, this.viewOptions); - this.views.set(tabView.name, view); - if (!this.loadingScreen) { - this.createLoadingScreen(); - } view.once(LOAD_SUCCESS, this.activateView); view.load(url); view.on(UPDATE_TARGET_URL, this.showURLView); view.on(LOADSCREEN_END, this.finishLoading); view.on(LOAD_FAILED, this.failLoading); + return view; + } + + addView = (view: MattermostView): void => { + this.views.set(view.name, view); + if (this.closedViews.has(view.name)) { + this.closedViews.delete(view.name); + } + if (!this.loadingScreen) { + this.createLoadingScreen(); + } + } + + loadView = (srv: MattermostServer, serverInfo: ServerInfo, tab: Tab, url?: string) => { + if (!tab.isOpen) { + this.closedViews.set(getTabViewName(srv.name, tab.name), {srv, tab}); + return; + } + const view = this.makeView(srv, serverInfo, tab, url); + this.addView(view); } reloadViewIfNeeded = (viewName: string) => { @@ -112,32 +122,62 @@ export class ViewManager { this.configServers.forEach((server) => this.loadServer(server)); } + /** Called when a new configuration is received + * Servers or tabs have been added or edited. We need to + * close, open, or reload tabs, taking care to reuse tabs and + * preserve focus on the currently selected tab. */ reloadConfiguration = (configServers: TeamWithTabs[]) => { - this.configServers = configServers.concat(); - const oldviews = this.views; + const focusedTuple: TabTuple | undefined = this.views.get(this.currentView as string)?.urlTypeTuple; + + const current: Map = new Map(); + for (const view of this.views.values()) { + current.set(view.urlTypeTuple, view); + } + + const views: Map = new Map(); + const closed: Map = new Map(); + + const sortedTabs = configServers.flatMap((x) => [...x.tabs]. + sort((a, b) => a.order - b.order). + map((t): [TeamWithTabs, Tab] => [x, t])); + + for (const [team, tab] of sortedTabs) { + const srv = new MattermostServer(team.name, team.url); + const info = new ServerInfo(srv); + const view = getServerView(srv, tab); + const tabTuple = tuple(new URL(team.url).href, tab.name as TabType); + const recycle = current.get(tabTuple); + if (!tab.isOpen) { + closed.set(tabTuple, {srv, tab, name: view.name}); + } else if (recycle) { + recycle.serverInfo = info; + recycle.tab.server = srv; + views.set(tabTuple, recycle); + } else { + views.set(tabTuple, this.makeView(srv, info, tab, tabTuple[0])); + } + } + + // commit the data to our local state + // destroy everything that no longer exists + for (const [k, v] of current) { + if (!views.has(k)) { + v.destroy(); + } + } + + // commit views this.views = new Map(); - const sorted = this.configServers.sort((a, b) => a.order - b.order); - let setFocus; - sorted.forEach((server) => { - const srv = new MattermostServer(server.name, server.url); - const serverInfo = new ServerInfo(srv); - server.tabs.forEach((tab) => { - const tabView = getServerView(srv, tab); - const recycle = oldviews.get(tabView.name); - if (recycle && recycle.name === this.currentView) { - setFocus = recycle.name; - } - if (!tab.isOpen) { - this.closedViews.set(tabView.name, {srv, tab}); - } else if (recycle && recycle.tab.name === tabView.name && recycle.tab.url.toString() === urlUtils.parseURL(tabView.url)!.toString()) { - oldviews.delete(recycle.name); - this.views.set(recycle.name, recycle); - } else { - this.loadView(srv, serverInfo, tab); - } - }); - }); - if (this.currentView && (oldviews.has(this.currentView) || this.closedViews.has(this.currentView))) { + for (const x of views.values()) { + this.views.set(x.name, x); + } + + // commit closed + for (const x of closed.values()) { + this.closedViews.set(x.name, {srv: x.srv, tab: x.tab}); + } + + if (focusedTuple && (current.has(focusedTuple) || closed.has(focusedTuple))) { if (configServers.length) { delete this.currentView; this.showInitial(); @@ -145,11 +185,12 @@ export class ViewManager { this.mainWindow.webContents.send(SET_ACTIVE_VIEW); } } - oldviews.forEach((unused) => { - unused.destroy(); - }); - if (setFocus) { - this.showByName(setFocus); + + // show the focused tab (or initial) + if (focusedTuple && views.has(focusedTuple)) { + const view = views.get(focusedTuple); + this.currentView = view!.name; + this.showByName(view!.name); } else { this.showInitial(); } diff --git a/src/types/external/tuple-record-polyfill.d.ts b/src/types/external/tuple-record-polyfill.d.ts new file mode 100644 index 00000000..3f0f5746 --- /dev/null +++ b/src/types/external/tuple-record-polyfill.d.ts @@ -0,0 +1,16 @@ +// Copyright (c) 2016-present Mattermost, Inc. All Rights Reserved. +// See LICENSE.txt for license information. + +declare module '@bloomberg/record-tuple-polyfill' { + export function Tuple(A): [A]; + export function Tuple(a: A, b: B): [A, B]; + export function Tuple(a: A, b: B, c: C): [A, B, C]; + export function Tuple(a: A, b: B, c: C, d: D): [A, B, C, D]; + export function Tuple(a: A, b: B, c: C, d: D, e: E): [A, B, C, D, E]; + export function Tuple(a: A, b: B, c: C, d: D, e: E, f: F): [A, B, C, D, E, F]; + export function Tuple(a: A, b: B, c: C, d: D, e: E, f: F, g: G): [A, B, C, D, E, F, G]; + export function Tuple(a: A, b: B, c: C, d: D, e: E, f: F, g: G, h: H): [A, B, C, D, E, F, G, H]; + export function Tuple(a: A, b: B, c: C, d: D, e: E, f: F, g: G, h: H, i: I): [A, B, C, D, E, F, G, H, I]; + export function Tuple(a: A, b: B, c: C, d: D, e: E, f: F, g: G, h: H, i: I, j: J): [A, B, C, D, E, F, G, H, I, J]; + export function Record(x: {[key: string]: T}): {[key: string]: T}; +}