From 8e96c19126aa4cba6418de5ccaa9e2dcf8faab78 Mon Sep 17 00:00:00 2001 From: undergroundwires Date: Fri, 7 Jan 2022 22:06:05 +0100 Subject: [PATCH] Improve performance of selecting scripts Increase performance by only notifying GUI about changes in selection when there really is a change. It removes extra processing from all event listeners that act on selection state change. --- .../Context/State/Selection/UserSelection.ts | 41 +++- .../State/Selection/UserSelection.spec.ts | 190 ++++++++++++------ 2 files changed, 157 insertions(+), 74 deletions(-) diff --git a/src/application/Context/State/Selection/UserSelection.ts b/src/application/Context/State/Selection/UserSelection.ts index 0ff7b1a3..1aee15f5 100644 --- a/src/application/Context/State/Selection/UserSelection.ts +++ b/src/application/Context/State/Selection/UserSelection.ts @@ -109,6 +109,9 @@ export class UserSelection implements IUserSelection { .getAllScripts() .filter((script) => !this.scripts.exists(script.id)) .map((script) => new SelectedScript(script, false)); + if (scriptsToSelect.length === 0) { + return; + } for (const script of scriptsToSelect) { this.scripts.addItem(script); } @@ -116,6 +119,9 @@ export class UserSelection implements IUserSelection { } public deselectAll(): void { + if (this.scripts.length === 0) { + return; + } const selectedScriptIds = this.scripts.getItems().map((script) => script.id); for (const scriptId of selectedScriptIds) { this.scripts.removeItem(scriptId); @@ -127,20 +133,35 @@ export class UserSelection implements IUserSelection { if (!scripts || scripts.length === 0) { throw new Error('Scripts are empty. Use deselectAll() if you want to deselect everything'); } - // Unselect from selected scripts - if (this.scripts.length !== 0) { - this.scripts.getItems() - .filter((existing) => !scripts.some((script) => existing.id === script.id)) - .map((script) => script.id) - .forEach((scriptId) => this.scripts.removeItem(scriptId)); + let totalChanged = 0; + totalChanged += this.unselectMissingWithoutNotifying(scripts); + totalChanged += this.selectNewWithoutNotifying(scripts); + if (totalChanged > 0) { + this.changed.notify(this.scripts.getItems()); } - // Select from unselected scripts + } + + private unselectMissingWithoutNotifying(scripts: readonly IScript[]): number { + if (this.scripts.length === 0 || scripts.length === 0) { + return 0; + } + const existingItems = this.scripts.getItems(); + const missingIds = existingItems + .filter((existing) => !scripts.some((script) => existing.id === script.id)) + .map((script) => script.id); + for (const id of missingIds) { + this.scripts.removeItem(id); + } + return missingIds.length; + } + + private selectNewWithoutNotifying(scripts: readonly IScript[]): number { const unselectedScripts = scripts .filter((script) => !this.scripts.exists(script.id)) .map((script) => new SelectedScript(script, false)); - for (const toSelect of unselectedScripts) { - this.scripts.addItem(toSelect); + for (const newScript of unselectedScripts) { + this.scripts.addItem(newScript); } - this.changed.notify(this.scripts.getItems()); + return unselectedScripts.length; } } diff --git a/tests/unit/application/Context/State/Selection/UserSelection.spec.ts b/tests/unit/application/Context/State/Selection/UserSelection.spec.ts index ee7eadac..d9eb9be7 100644 --- a/tests/unit/application/Context/State/Selection/UserSelection.spec.ts +++ b/tests/unit/application/Context/State/Selection/UserSelection.spec.ts @@ -38,72 +38,134 @@ describe('UserSelection', () => { .expectFinalScripts(scripts); }); }); - describe('deselectAll removes all items', () => { - // arrange - const allScripts = [ - new SelectedScriptStub('s1', false), - new SelectedScriptStub('s2', false), - new SelectedScriptStub('s3', false), - new SelectedScriptStub('s4', false), - ]; - const selectedScripts = allScripts.filter( - (s) => ['s1', 's2', 's3'].includes(s.id), - ); - new UserSelectionTestRunner() - .withSelectedScripts(selectedScripts) - .withCategory(1, allScripts.map((s) => s.script)) - // act - .run((sut) => { - sut.deselectAll(); - }) - // assert - .expectTotalFiredEvents(1) - .expectFinalScripts([]) - .expectFinalScriptsInEvent(0, []); + describe('deselectAll', () => { + describe('removes existing items', () => { + // arrange + const allScripts = [ + new SelectedScriptStub('s1', false), + new SelectedScriptStub('s2', false), + new SelectedScriptStub('s3', false), + new SelectedScriptStub('s4', false), + ]; + const selectedScripts = allScripts.filter( + (s) => ['s1', 's2', 's3'].includes(s.id), + ); + new UserSelectionTestRunner() + .withSelectedScripts(selectedScripts) + .withCategory(1, allScripts.map((s) => s.script)) + // act + .run((sut) => { + sut.deselectAll(); + }) + // assert + .expectTotalFiredEvents(1) + .expectFinalScripts([]) + .expectFinalScriptsInEvent(0, []); + }); + describe('does not notify if nothing is selected', () => { + new UserSelectionTestRunner() + // arrange + .withSelectedScripts([]) + // act + .run((sut) => { + sut.deselectAll(); + }) + // assert + .expectTotalFiredEvents(0); + }); }); - describe('selectOnly selects expected', () => { - // arrange - const allScripts = [ - new SelectedScriptStub('s1', false), - new SelectedScriptStub('s2', false), - new SelectedScriptStub('s3', false), - new SelectedScriptStub('s4', false), - ]; - const selectedScripts = allScripts.filter( - (s) => ['s1', 's2', 's3'].includes(s.id), - ); - const scriptsToSelect = allScripts.filter( - (s) => ['s2', 's3', 's4'].includes(s.id), - ); - new UserSelectionTestRunner() - .withSelectedScripts(selectedScripts) - .withCategory(1, allScripts.map((s) => s.script)) - // act - .run((sut) => { - sut.selectOnly(scriptsToSelect.map((s) => s.script)); - }) - // assert - .expectTotalFiredEvents(1) - .expectFinalScripts(scriptsToSelect) - .expectFinalScriptsInEvent(0, scriptsToSelect); + describe('selectAll', () => { + describe('selects as expected', () => { + // arrange + const expected = [ + new SelectedScriptStub('s1', false), + new SelectedScriptStub('s2', false), + ]; + new UserSelectionTestRunner() + .withSelectedScripts([]) + .withCategory(1, expected.map((s) => s.script)) + // act + .run((sut) => { + sut.selectAll(); + }) + // assert + .expectTotalFiredEvents(1) + .expectFinalScripts(expected) + .expectFinalScriptsInEvent(0, expected); + }); + describe('does not notify if nothing new is selected', () => { + const allScripts = [new ScriptStub('s1'), new ScriptStub('s2')]; + const selectedScripts = allScripts.map((s) => s.toSelectedScript(false)); + new UserSelectionTestRunner() + // arrange + .withSelectedScripts(selectedScripts) + .withCategory(0, allScripts) + // act + .run((sut) => { + sut.selectAll(); + }) + // assert + .expectTotalFiredEvents(0); + }); }); - describe('selectAll selects as expected', () => { - // arrange - const expected = [ - new SelectedScriptStub('s1', false), - new SelectedScriptStub('s2', false), - ]; - new UserSelectionTestRunner() - .withSelectedScripts([]) - .withCategory(1, expected.map((s) => s.script)) - // act - .run((sut) => { - sut.selectAll(); - }) - // assert - .expectTotalFiredEvents(1) - .expectFinalScripts(expected) - .expectFinalScriptsInEvent(0, expected); + describe('selectOnly', () => { + describe('selects as expected', () => { + // arrange + const allScripts = [ + new SelectedScriptStub('s1', false), + new SelectedScriptStub('s2', false), + new SelectedScriptStub('s3', false), + new SelectedScriptStub('s4', false), + ]; + const getScripts = (...ids: string[]) => allScripts.filter((s) => ids.includes(s.id)); + const testCases = [ + { + name: 'adds as expected', + preSelected: getScripts('s1'), + toSelect: getScripts('s1', 's2'), + }, + { + name: 'removes as expected', + preSelected: getScripts('s1', 's2', 's3'), + toSelect: getScripts('s1'), + }, + { + name: 'adds and removes as expected', + preSelected: getScripts('s1', 's2', 's3'), + toSelect: getScripts('s2', 's3', 's4'), + }, + ]; + for (const testCase of testCases) { + describe(testCase.name, () => { + new UserSelectionTestRunner() + .withSelectedScripts(testCase.preSelected) + .withCategory(1, testCase.toSelect.map((s) => s.script)) + // act + .run((sut) => { + sut.selectOnly(testCase.toSelect.map((s) => s.script)); + }) + // assert + .expectTotalFiredEvents(1) + .expectFinalScripts(testCase.toSelect) + .expectFinalScriptsInEvent(0, testCase.toSelect); + }); + } + }); + describe('does not notify if selection does not change', () => { + const allScripts = [new ScriptStub('s1'), new ScriptStub('s2'), new ScriptStub('s3')]; + const toSelect = [allScripts[0], allScripts[1]]; + const preSelected = toSelect.map((s) => s.toSelectedScript(false)); + new UserSelectionTestRunner() + // arrange + .withSelectedScripts(preSelected) + .withCategory(0, allScripts) + // act + .run((sut) => { + sut.selectOnly(toSelect); + }) + // assert + .expectTotalFiredEvents(0); + }); }); describe('addOrUpdateSelectedScript', () => { describe('adds when item does not exist', () => {