Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor!: Use one map for toolbox contents. #8654

Merged
merged 1 commit into from
Nov 12, 2024
Merged

refactor!: Use one map for toolbox contents. #8654

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
66 changes: 26 additions & 40 deletions core/toolbox/toolbox.ts
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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
Loading