diff --git a/core/toolbox/toolbox.ts b/core/toolbox/toolbox.ts index efd5381b4..12465813c 100644 --- a/core/toolbox/toolbox.ts +++ b/core/toolbox/toolbox.ts @@ -70,9 +70,6 @@ export class Toolbox /** Whether the Toolbox is visible. */ protected isVisible_ = false; - /** The list of items in the toolbox. */ - protected contents_: IToolboxItem[] = []; - /** The width of the toolbox. */ protected width_ = 0; @@ -82,7 +79,10 @@ export class Toolbox /** The flyout for the toolbox. */ private flyout_: IFlyout | null = null; - protected contentMap_: {[key: string]: IToolboxItem}; + + /** Map from ID to the corresponding toolbox item. */ + protected contents = new Map(); + toolboxPosition: toolbox.Position; /** The currently selected item. */ @@ -118,9 +118,6 @@ export class Toolbox /** Is RTL vs LTR. */ this.RTL = workspace.options.RTL; - /** A map from toolbox item IDs to toolbox items. */ - this.contentMap_ = Object.create(null); - /** Position of the toolbox and flyout relative to the workspace. */ this.toolboxPosition = workspace.options.toolboxPosition; } @@ -367,14 +364,8 @@ export class Toolbox */ render(toolboxDef: toolbox.ToolboxInfo) { this.toolboxDef_ = toolboxDef; - for (let i = 0; i < this.contents_.length; i++) { - const toolboxItem = this.contents_[i]; - if (toolboxItem) { - toolboxItem.dispose(); - } - } - this.contents_ = []; - this.contentMap_ = Object.create(null); + this.contents.forEach((item) => item.dispose()); + this.contents.clear(); this.renderContents_(toolboxDef['contents']); this.position(); this.handleToolboxItemResize(); @@ -445,8 +436,7 @@ export class Toolbox * @param toolboxItem The item in the toolbox. */ protected addToolboxItem_(toolboxItem: IToolboxItem) { - this.contents_.push(toolboxItem); - this.contentMap_[toolboxItem.getId()] = toolboxItem; + this.contents.set(toolboxItem.getId(), toolboxItem); if (toolboxItem.isCollapsible()) { const collapsibleItem = toolboxItem as ICollapsibleToolboxItem; const childToolboxItems = collapsibleItem.getChildToolboxItems(); @@ -463,7 +453,7 @@ export class Toolbox * @returns The list of items in the toolbox. */ getToolboxItems(): IToolboxItem[] { - return this.contents_; + return [...this.contents.values()]; } /** @@ -618,7 +608,7 @@ export class Toolbox * @returns The toolbox item with the given ID, or null if no item exists. */ getToolboxItemById(id: string): IToolboxItem | null { - return this.contentMap_[id] || null; + return this.contents.get(id) || null; } /** @@ -765,14 +755,13 @@ export class Toolbox * @internal */ refreshTheme() { - for (let i = 0; i < this.contents_.length; i++) { - const child = this.contents_[i]; + this.contents.forEach((child) => { // TODO(#6097): Fix types or add refreshTheme to IToolboxItem. const childAsCategory = child as ToolboxCategory; if (childAsCategory.refreshTheme) { childAsCategory.refreshTheme(); } - } + }); } /** @@ -923,11 +912,9 @@ export class Toolbox * @param position The position of the item to select. */ selectItemByPosition(position: number) { - if (position > -1 && position < this.contents_.length) { - const item = this.contents_[position]; - if (item.isSelectable()) { - this.setSelectedItem(item); - } + const item = this.getToolboxItems()[position]; + if (item) { + this.setSelectedItem(item); } } @@ -1034,11 +1021,12 @@ export class Toolbox return false; } - let nextItemIdx = this.contents_.indexOf(this.selectedItem_) + 1; - if (nextItemIdx > -1 && nextItemIdx < this.contents_.length) { - let nextItem = this.contents_[nextItemIdx]; + const items = [...this.contents.values()]; + let nextItemIdx = items.indexOf(this.selectedItem_) + 1; + if (nextItemIdx > -1 && nextItemIdx < items.length) { + let nextItem = items[nextItemIdx]; while (nextItem && !nextItem.isSelectable()) { - nextItem = this.contents_[++nextItemIdx]; + nextItem = items[++nextItemIdx]; } if (nextItem && nextItem.isSelectable()) { this.setSelectedItem(nextItem); @@ -1058,11 +1046,12 @@ export class Toolbox return false; } - let prevItemIdx = this.contents_.indexOf(this.selectedItem_) - 1; - if (prevItemIdx > -1 && prevItemIdx < this.contents_.length) { - let prevItem = this.contents_[prevItemIdx]; + const items = [...this.contents.values()]; + let prevItemIdx = items.indexOf(this.selectedItem_) - 1; + if (prevItemIdx > -1 && prevItemIdx < items.length) { + let prevItem = items[prevItemIdx]; while (prevItem && !prevItem.isSelectable()) { - prevItem = this.contents_[--prevItemIdx]; + prevItem = items[--prevItemIdx]; } if (prevItem && prevItem.isSelectable()) { this.setSelectedItem(prevItem); @@ -1076,16 +1065,13 @@ export class Toolbox dispose() { this.workspace_.getComponentManager().removeComponent('toolbox'); this.flyout_!.dispose(); - for (let i = 0; i < this.contents_.length; i++) { - const toolboxItem = this.contents_[i]; - toolboxItem.dispose(); - } + this.contents.forEach((item) => item.dispose()); for (let j = 0; j < this.boundEvents_.length; j++) { browserEvents.unbind(this.boundEvents_[j]); } this.boundEvents_ = []; - this.contents_ = []; + this.contents.clear(); if (this.HtmlDiv) { this.workspace_.getThemeManager().unsubscribe(this.HtmlDiv); diff --git a/tests/mocha/test_helpers/toolbox_definitions.js b/tests/mocha/test_helpers/toolbox_definitions.js index 2f767ed60..f05c29620 100644 --- a/tests/mocha/test_helpers/toolbox_definitions.js +++ b/tests/mocha/test_helpers/toolbox_definitions.js @@ -243,9 +243,8 @@ export function getBasicToolbox() { } export function getCollapsibleItem(toolbox) { - const contents = toolbox.contents_; - for (let i = 0; i < contents.length; i++) { - const item = contents[i]; + const contents = toolbox.contents.values(); + for (const item of contents) { if (item.isCollapsible()) { return item; } @@ -253,9 +252,8 @@ export function getCollapsibleItem(toolbox) { } export function getNonCollapsibleItem(toolbox) { - const contents = toolbox.contents_; - for (let i = 0; i < contents.length; i++) { - const item = contents[i]; + const contents = toolbox.contents.values(); + for (const item of contents) { if (!item.isCollapsible()) { return item; } diff --git a/tests/mocha/toolbox_test.js b/tests/mocha/toolbox_test.js index 1cb8df979..c6a1c726d 100644 --- a/tests/mocha/toolbox_test.js +++ b/tests/mocha/toolbox_test.js @@ -98,7 +98,7 @@ suite('Toolbox', function () { {'kind': 'category', 'contents': []}, ], }); - assert.lengthOf(this.toolbox.contents_, 2); + assert.equal(this.toolbox.contents.size, 2); sinon.assert.called(positionStub); }); // TODO: Uncomment once implemented. @@ -153,7 +153,7 @@ suite('Toolbox', function () { ], }; this.toolbox.render(jsonDef); - assert.lengthOf(this.toolbox.contents_, 1); + assert.equal(this.toolbox.contents.size, 1); }); test('multiple icon classes can be applied', function () { const jsonDef = { @@ -176,7 +176,7 @@ suite('Toolbox', function () { assert.doesNotThrow(() => { this.toolbox.render(jsonDef); }); - assert.lengthOf(this.toolbox.contents_, 1); + assert.equal(this.toolbox.contents.size, 1); }); }); @@ -204,7 +204,7 @@ suite('Toolbox', function () { const evt = { 'target': categoryXml, }; - const item = this.toolbox.contentMap_[categoryXml.getAttribute('id')]; + const item = this.toolbox.contents.get(categoryXml.getAttribute('id')); const setSelectedSpy = sinon.spy(this.toolbox, 'setSelectedItem'); const onClickSpy = sinon.spy(item, 'onClick'); this.toolbox.onClick_(evt); @@ -356,14 +356,16 @@ suite('Toolbox', function () { assert.isFalse(handled); }); test('Next item is selectable -> Should select next item', function () { - const item = this.toolbox.contents_[0]; + const items = [...this.toolbox.contents.values()]; + const item = items[0]; this.toolbox.selectedItem_ = item; const handled = this.toolbox.selectNext_(); assert.isTrue(handled); - assert.equal(this.toolbox.selectedItem_, this.toolbox.contents_[1]); + assert.equal(this.toolbox.selectedItem_, items[1]); }); test('Selected item is last item -> Should not handle event', function () { - const item = this.toolbox.contents_[this.toolbox.contents_.length - 1]; + const items = [...this.toolbox.contents.values()]; + const item = items.at(-1); this.toolbox.selectedItem_ = item; const handled = this.toolbox.selectNext_(); assert.isFalse(handled); @@ -387,15 +389,16 @@ suite('Toolbox', function () { assert.isFalse(handled); }); test('Selected item is first item -> Should not handle event', function () { - const item = this.toolbox.contents_[0]; + const item = [...this.toolbox.contents.values()][0]; this.toolbox.selectedItem_ = item; const handled = this.toolbox.selectPrevious_(); assert.isFalse(handled); assert.equal(this.toolbox.selectedItem_, item); }); test('Previous item is selectable -> Should select previous item', function () { - const item = this.toolbox.contents_[1]; - const prevItem = this.toolbox.contents_[0]; + const items = [...this.toolbox.contents.values()]; + const item = items[1]; + const prevItem = items[0]; this.toolbox.selectedItem_ = item; const handled = this.toolbox.selectPrevious_(); assert.isTrue(handled); @@ -404,9 +407,10 @@ suite('Toolbox', function () { test('Previous item is collapsed -> Should skip over children of the previous item', function () { const childItem = getChildItem(this.toolbox); const parentItem = childItem.getParent(); - const parentIdx = this.toolbox.contents_.indexOf(parentItem); + const items = [...this.toolbox.contents.values()]; + const parentIdx = items.indexOf(parentItem); // Gets the item after the parent. - const item = this.toolbox.contents_[parentIdx + 1]; + const item = items[parentIdx + 1]; this.toolbox.selectedItem_ = item; const handled = this.toolbox.selectPrevious_(); assert.isTrue(handled); @@ -728,9 +732,10 @@ suite('Toolbox', function () { }); test('Child categories visible if all ancestors expanded', function () { this.toolbox.render(getDeeplyNestedJSON()); - const outerCategory = this.toolbox.contents_[0]; - const middleCategory = this.toolbox.contents_[1]; - const innerCategory = this.toolbox.contents_[2]; + const items = [...this.toolbox.contents.values()]; + const outerCategory = items[0]; + const middleCategory = items[1]; + const innerCategory = items[2]; outerCategory.toggleExpanded(); middleCategory.toggleExpanded(); @@ -743,8 +748,9 @@ suite('Toolbox', function () { }); test('Child categories not visible if any ancestor not expanded', function () { this.toolbox.render(getDeeplyNestedJSON()); - const middleCategory = this.toolbox.contents_[1]; - const innerCategory = this.toolbox.contents_[2]; + const items = [...this.toolbox.contents.values()]; + const middleCategory = items[1]; + const innerCategory = items[2]; // Don't expand the outermost category // Even though the direct parent of inner is expanded, it shouldn't be visible