Skip to content

Commit

Permalink
refactor!: Use one map for toolbox contents. (#8654)
Browse files Browse the repository at this point in the history
  • Loading branch information
@gonfunko
gonfunko authored Nov 12, 2024
1 parent 7bbbb95 commit ae2a140
Show file tree
Hide file tree
Showing 3 changed files with 53 additions and 63 deletions.
66 changes: 26 additions & 40 deletions core/toolbox/toolbox.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand All @@ -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<string, IToolboxItem>();

toolboxPosition: toolbox.Position;

/** The currently selected item. */
Expand Down Expand Up @@ -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;
}
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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();
Expand All @@ -463,7 +453,7 @@ export class Toolbox
* @returns The list of items in the toolbox.
*/
getToolboxItems(): IToolboxItem[] {
return this.contents_;
return [...this.contents.values()];
}

/**
Expand Down Expand Up @@ -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;
}

/**
Expand Down Expand Up @@ -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();
}
}
});
}

/**
Expand Down Expand Up @@ -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);
}
}

Expand Down Expand Up @@ -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);
Expand All @@ -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);
Expand All @@ -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);
Expand Down
10 changes: 4 additions & 6 deletions tests/mocha/test_helpers/toolbox_definitions.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -243,19 +243,17 @@ 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;
}
}
}

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;
}
Expand Down
40 changes: 23 additions & 17 deletions tests/mocha/toolbox_test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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 = {
Expand All @@ -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);
});
});

Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand All @@ -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);
Expand All @@ -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);
Expand Down Expand Up @@ -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();
Expand All @@ -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
Expand Down

0 comments on commit ae2a140

Please sign in to comment.