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

Jump to bottom

Refactor notebook toolbar (for testing) + dynamic strategy unittesting #185805

Merged
merged 2 commits into from
Jun 21, 2023
Merged

Refactor notebook toolbar (for testing) + dynamic strategy unittesting #185805

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3502249
testing refactor + bugfix (size 0 breaking remainingSize calculations)
Yoyokrazy Jun 21, 2023
25565ea
cleanup
Yoyokrazy Jun 21, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
35 changes: 22 additions & 13 deletions src/vs/workbench/contrib/notebook/browser/viewParts/notebookEditorToolbar.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,11 @@ class WorkbenchAlwaysLabelStrategy implements IActionLayoutStrategy {
const initialPrimaryActions = this.editorToolbar.primaryActions;
const initialSecondaryActions = this.editorToolbar.secondaryActions;

return workbenchCalculateActions(initialPrimaryActions, initialSecondaryActions, leftToolbarContainerMaxWidth);
const actionOutput = workbenchCalculateActions(initialPrimaryActions, initialSecondaryActions, leftToolbarContainerMaxWidth);
return {
primaryActions: actionOutput.primaryActions.map(a => a.action),
secondaryActions: actionOutput.secondaryActions
};
}
}

Expand All @@ -111,7 +115,11 @@ class WorkbenchNeverLabelStrategy implements IActionLayoutStrategy {
const initialPrimaryActions = this.editorToolbar.primaryActions;
const initialSecondaryActions = this.editorToolbar.secondaryActions;

return workbenchCalculateActions(initialPrimaryActions, initialSecondaryActions, leftToolbarContainerMaxWidth);
const actionOutput = workbenchCalculateActions(initialPrimaryActions, initialSecondaryActions, leftToolbarContainerMaxWidth);
return {
primaryActions: actionOutput.primaryActions.map(a => a.action),
secondaryActions: actionOutput.secondaryActions
};
}
}

Expand Down Expand Up @@ -139,7 +147,11 @@ class WorkbenchDynamicLabelStrategy implements IActionLayoutStrategy {
const initialPrimaryActions = this.editorToolbar.primaryActions;
const initialSecondaryActions = this.editorToolbar.secondaryActions;

return workbenchDynamicCalculateActions(initialPrimaryActions, initialSecondaryActions, leftToolbarContainerMaxWidth);
const actionOutput = workbenchDynamicCalculateActions(initialPrimaryActions, initialSecondaryActions, leftToolbarContainerMaxWidth);
return {
primaryActions: actionOutput.primaryActions.map(a => a.action),
secondaryActions: actionOutput.secondaryActions
};
}
}

Expand Down Expand Up @@ -523,11 +535,11 @@ export class NotebookEditorWorkbenchToolbar extends Disposable {
}
}

export function workbenchCalculateActions(initialPrimaryActions: IActionModel[], initialSecondaryActions: IAction[], leftToolbarContainerMaxWidth: number): { primaryActions: IAction[]; secondaryActions: IAction[] } {
export function workbenchCalculateActions(initialPrimaryActions: IActionModel[], initialSecondaryActions: IAction[], leftToolbarContainerMaxWidth: number): { primaryActions: IActionModel[]; secondaryActions: IAction[] } {
return actionOverflowHelper(initialPrimaryActions, initialSecondaryActions, leftToolbarContainerMaxWidth, false);
}

export function workbenchDynamicCalculateActions(initialPrimaryActions: IActionModel[], initialSecondaryActions: IAction[], leftToolbarContainerMaxWidth: number): { primaryActions: IAction[]; secondaryActions: IAction[] } {
export function workbenchDynamicCalculateActions(initialPrimaryActions: IActionModel[], initialSecondaryActions: IAction[], leftToolbarContainerMaxWidth: number): { primaryActions: IActionModel[]; secondaryActions: IAction[] } {

if (initialPrimaryActions.length === 0) {
return { primaryActions: [], secondaryActions: initialSecondaryActions };
Expand All @@ -542,10 +554,7 @@ export function workbenchDynamicCalculateActions(initialPrimaryActions: IActionM
initialPrimaryActions.forEach(action => {
action.renderLabel = true;
});
return {
primaryActions: initialPrimaryActions.map(action => action.action),
secondaryActions: initialSecondaryActions
};
return actionOverflowHelper(initialPrimaryActions, initialSecondaryActions, leftToolbarContainerMaxWidth, false);
}

// step 2: check if they fit without labels
Expand All @@ -562,7 +571,7 @@ export function workbenchDynamicCalculateActions(initialPrimaryActions: IActionM

if (initialPrimaryActions[i].action instanceof Separator) {
// find group separator
const remainingItems = initialPrimaryActions.slice(i + 1);
const remainingItems = initialPrimaryActions.slice(i + 1).filter(action => action.size !== 0); // todo: need to exclude size 0 items from this
const newTotalSum = sum + (remainingItems.length === 0 ? 0 : (remainingItems.length * ICON_ONLY_ACTION_WIDTH + (remainingItems.length - 1) * ACTION_PADDING));
if (newTotalSum <= leftToolbarContainerMaxWidth) {
lastActionWithLabel = i;
Expand All @@ -582,12 +591,12 @@ export function workbenchDynamicCalculateActions(initialPrimaryActions: IActionM
initialPrimaryActions.slice(0, lastActionWithLabel + 1).forEach(action => { action.renderLabel = true; });
initialPrimaryActions.slice(lastActionWithLabel + 1).forEach(action => { action.renderLabel = false; });
return {
primaryActions: initialPrimaryActions.map(action => action.action),
primaryActions: initialPrimaryActions,
secondaryActions: initialSecondaryActions
};
}

function actionOverflowHelper(initialPrimaryActions: IActionModel[], initialSecondaryActions: IAction[], leftToolbarContainerMaxWidth: number, iconOnly: boolean): { primaryActions: IAction[]; secondaryActions: IAction[] } {
function actionOverflowHelper(initialPrimaryActions: IActionModel[], initialSecondaryActions: IAction[], leftToolbarContainerMaxWidth: number, iconOnly: boolean): { primaryActions: IActionModel[]; secondaryActions: IAction[] } {
const renderActions: IActionModel[] = [];
const overflow: IAction[] = [];

Expand Down Expand Up @@ -665,7 +674,7 @@ function actionOverflowHelper(initialPrimaryActions: IActionModel[], initialSeco
}

return {
primaryActions: renderActions.map(action => action.action),
primaryActions: renderActions,
secondaryActions: [...overflow, ...initialSecondaryActions]
};
}
195 changes: 186 additions & 9 deletions src/vs/workbench/contrib/notebook/test/browser/notebookWorkbenchToolbar.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
* Licensed under the MIT License. See License.txt in the project root for license information.
*--------------------------------------------------------------------------------------------*/

import { workbenchCalculateActions } from 'vs/workbench/contrib/notebook/browser/viewParts/notebookEditorToolbar';
import { workbenchCalculateActions, workbenchDynamicCalculateActions } from 'vs/workbench/contrib/notebook/browser/viewParts/notebookEditorToolbar';
import { Action, IAction, Separator } from 'vs/base/common/actions';
import * as assert from 'assert';

Expand All @@ -24,7 +24,7 @@ interface IActionModel {
*
* ex: action with size 50 requires 58px of space
*/
suite('workbenchCalculateActions', () => {
suite('Workbench Toolbar calculateActions (strategy always + never)', () => {

const defaultSecondaryActionModels: IActionModel[] = [
{ action: new Action('secondaryAction0', 'Secondary Action 0'), size: 50, visible: true, renderLabel: true },
Expand All @@ -48,7 +48,7 @@ suite('workbenchCalculateActions', () => {
{ action: new Action('action2', 'Action 2'), size: 50, visible: true, renderLabel: true },
];
const result = workbenchCalculateActions(actions, defaultSecondaryActions, 200);
assert.deepEqual(result.primaryActions, actions.map(action => action.action));
assert.deepEqual(result.primaryActions, actions);
assert.deepEqual(result.secondaryActions, defaultSecondaryActions);
});

Expand All @@ -59,7 +59,7 @@ suite('workbenchCalculateActions', () => {
{ action: new Action('action2', 'Action 2'), size: 50, visible: true, renderLabel: true },
];
const result = workbenchCalculateActions(actions, defaultSecondaryActions, 100);
assert.deepEqual(result.primaryActions, [actions[0]].map(action => action.action));
assert.deepEqual(result.primaryActions, [actions[0]]);
assert.deepEqual(result.secondaryActions, [actions[1], actions[2], separator, ...defaultSecondaryActionModels].map(action => action.action));
});

Expand All @@ -71,7 +71,7 @@ suite('workbenchCalculateActions', () => {
{ action: new Action('action1', 'Action 1'), size: 50, visible: true, renderLabel: true },
];
const result = workbenchCalculateActions(actions, defaultSecondaryActions, 125);
assert.deepEqual(result.primaryActions, [actions[0], actions[1], actions[3]].map(action => action.action));
assert.deepEqual(result.primaryActions, [actions[0], actions[1], actions[3]]);
assert.deepEqual(result.secondaryActions, defaultSecondaryActions);
});

Expand All @@ -83,7 +83,7 @@ suite('workbenchCalculateActions', () => {
{ action: new Separator(), size: 1, visible: true, renderLabel: true },
];
const result = workbenchCalculateActions(actions, defaultSecondaryActions, 200);
assert.deepEqual(result.primaryActions, [actions[0], actions[1], actions[2]].map(action => action.action));
assert.deepEqual(result.primaryActions, [actions[0], actions[1], actions[2]]);
assert.deepEqual(result.secondaryActions, defaultSecondaryActions);
});

Expand All @@ -95,7 +95,7 @@ suite('workbenchCalculateActions', () => {
{ action: new Action('action3', 'Action 3'), size: 0, visible: true, renderLabel: true },
];
const result = workbenchCalculateActions(actions, defaultSecondaryActions, 116);
assert.deepEqual(result.primaryActions, [actions[0], actions[1], actions[3]].map(action => action.action));
assert.deepEqual(result.primaryActions, [actions[0], actions[1], actions[3]]);
assert.deepEqual(result.secondaryActions, [actions[2], separator, ...defaultSecondaryActionModels].map(action => action.action));
});

Expand All @@ -106,7 +106,7 @@ suite('workbenchCalculateActions', () => {
{ action: new Action('action1', 'Action 1'), size: 50, visible: true, renderLabel: true },
];
const result = workbenchCalculateActions(actions, defaultSecondaryActions, 116);
assert.deepEqual(result.primaryActions, [actions[0], actions[2]].map(action => action.action));
assert.deepEqual(result.primaryActions, [actions[0], actions[2]]);
assert.deepEqual(result.secondaryActions, defaultSecondaryActions);
});

Expand All @@ -120,7 +120,184 @@ suite('workbenchCalculateActions', () => {
{ action: new Action('action3', 'Action 3'), size: 50, visible: true, renderLabel: true },
];
const result = workbenchCalculateActions(actions, defaultSecondaryActions, 300);
assert.deepEqual(result.primaryActions, [actions[0], actions[1], actions[2], actions[3], actions[5]].map(action => action.action));
assert.deepEqual(result.primaryActions, [actions[0], actions[1], actions[2], actions[3], actions[5]]);
assert.deepEqual(result.secondaryActions, defaultSecondaryActions);
});
});

suite('Workbench Toolbar Dynamic calculateActions (strategy dynamic)', () => {

const actionTemplate = [
new Action('action0', 'Action 0'),
new Action('action1', 'Action 1'),
new Action('action2', 'Action 2'),
new Action('action3', 'Action 3')
];

const defaultSecondaryActionModels: IActionModel[] = [
{ action: new Action('secondaryAction0', 'Secondary Action 0'), size: 50, visible: true, renderLabel: true },
{ action: new Action('secondaryAction1', 'Secondary Action 1'), size: 50, visible: true, renderLabel: true },
{ action: new Action('secondaryAction2', 'Secondary Action 2'), size: 50, visible: true, renderLabel: true },
];
const defaultSecondaryActions: IAction[] = defaultSecondaryActionModels.map(action => action.action);


test('should return empty primary and secondary actions when given empty initial actions', () => {
const result = workbenchDynamicCalculateActions([], [], 100);
assert.deepEqual(result.primaryActions, []);
assert.deepEqual(result.secondaryActions, []);
});

test('should return all primary actions as visiblewhen they fit within the container width', () => {
const constainerSize = 200;
const input: IActionModel[] = [
{ action: actionTemplate[0], size: 50, visible: true, renderLabel: true },
{ action: actionTemplate[1], size: 50, visible: true, renderLabel: true },
{ action: actionTemplate[2], size: 50, visible: true, renderLabel: true },
];
const expected: IActionModel[] = [
{ action: actionTemplate[0], size: 50, visible: true, renderLabel: true },
{ action: actionTemplate[1], size: 50, visible: true, renderLabel: true },
{ action: actionTemplate[2], size: 50, visible: true, renderLabel: true },
];
const result = workbenchDynamicCalculateActions(input, defaultSecondaryActions, constainerSize);
assert.deepEqual(result.primaryActions, expected);
assert.deepEqual(result.secondaryActions, defaultSecondaryActions);
});

test('actions all within a group that cannot all fit, will all be icon only', () => {
const containerSize = 150;
const input: IActionModel[] = [
{ action: actionTemplate[0], size: 50, visible: true, renderLabel: true },
{ action: actionTemplate[1], size: 50, visible: true, renderLabel: true },
{ action: actionTemplate[2], size: 50, visible: true, renderLabel: true },
];
const expected: IActionModel[] = [
{ action: actionTemplate[0], size: 50, visible: true, renderLabel: false },
{ action: actionTemplate[1], size: 50, visible: true, renderLabel: false },
{ action: actionTemplate[2], size: 50, visible: true, renderLabel: false },
];


const result = workbenchDynamicCalculateActions(input, defaultSecondaryActions, containerSize);
assert.deepEqual(result.primaryActions, expected);
assert.deepEqual(result.secondaryActions, [...defaultSecondaryActionModels].map(action => action.action));
});

test('should ignore second separator when two separators are in a row', () => {
const containerSize = 200;
const input: IActionModel[] = [
{ action: actionTemplate[0], size: 50, visible: true, renderLabel: true },
{ action: new Separator(), size: 1, visible: true, renderLabel: true },
{ action: new Separator(), size: 1, visible: true, renderLabel: true },
{ action: actionTemplate[1], size: 50, visible: true, renderLabel: true },
];
const expected: IActionModel[] = [
{ action: actionTemplate[0], size: 50, visible: true, renderLabel: true },
{ action: new Separator(), size: 1, visible: true, renderLabel: true },
{ action: actionTemplate[1], size: 50, visible: true, renderLabel: true },
];
const result = workbenchDynamicCalculateActions(input, defaultSecondaryActions, containerSize);
assert.deepEqual(result.primaryActions, expected);
assert.deepEqual(result.secondaryActions, defaultSecondaryActions);
});

test('check label visibility in different groupings', () => {
const containerSize = 150;
const actions: IActionModel[] = [
{ action: actionTemplate[0], size: 50, visible: true, renderLabel: true },
{ action: new Separator(), size: 1, visible: true, renderLabel: true },
{ action: actionTemplate[1], size: 50, visible: true, renderLabel: true },
{ action: actionTemplate[2], size: 50, visible: true, renderLabel: true },
];
const expectedOutputActions: IActionModel[] = [
{ action: actionTemplate[0], size: 50, visible: true, renderLabel: true },
{ action: new Separator(), size: 1, visible: true, renderLabel: true },
{ action: actionTemplate[1], size: 50, visible: true, renderLabel: false },
{ action: actionTemplate[2], size: 50, visible: true, renderLabel: false },
];


const result = workbenchDynamicCalculateActions(actions, defaultSecondaryActions, containerSize);
assert.deepEqual(result.primaryActions, expectedOutputActions);
assert.deepEqual(result.secondaryActions, defaultSecondaryActions);
});

test('should ignore separators when they are at the end of the resulting primary actions', () => {
const containerSize = 200;
const input: IActionModel[] = [
{ action: actionTemplate[0], size: 50, visible: true, renderLabel: true },
{ action: new Separator(), size: 1, visible: true, renderLabel: true },
{ action: actionTemplate[1], size: 50, visible: true, renderLabel: true },
{ action: new Separator(), size: 1, visible: true, renderLabel: true },
];
const expected: IActionModel[] = [
{ action: actionTemplate[0], size: 50, visible: true, renderLabel: true },
{ action: new Separator(), size: 1, visible: true, renderLabel: true },
{ action: actionTemplate[1], size: 50, visible: true, renderLabel: true },
];
const result = workbenchDynamicCalculateActions(input, defaultSecondaryActions, containerSize);
assert.deepEqual(result.primaryActions, expected);
assert.deepEqual(result.secondaryActions, defaultSecondaryActions);
});

test('should keep actions with size 0 in primary actions', () => {
const containerSize = 170;
const input: IActionModel[] = [
{ action: actionTemplate[0], size: 50, visible: true, renderLabel: true },
{ action: actionTemplate[1], size: 50, visible: true, renderLabel: true },
{ action: new Separator(), size: 1, visible: true, renderLabel: true },
{ action: actionTemplate[2], size: 50, visible: true, renderLabel: true },
{ action: actionTemplate[3], size: 0, visible: true, renderLabel: true },
];
const expected: IActionModel[] = [
{ action: actionTemplate[0], size: 50, visible: true, renderLabel: true },
{ action: actionTemplate[1], size: 50, visible: true, renderLabel: true },
{ action: new Separator(), size: 1, visible: true, renderLabel: true },
{ action: actionTemplate[2], size: 50, visible: true, renderLabel: false },
{ action: actionTemplate[3], size: 0, visible: true, renderLabel: false },
];
const result = workbenchDynamicCalculateActions(input, defaultSecondaryActions, containerSize);
assert.deepEqual(result.primaryActions, expected);
assert.deepEqual(result.secondaryActions, defaultSecondaryActions);
});

test('should not render separator if preceeded by size 0 action(s), but keep size 0 action in primary.', () => {
const containerSize = 116;
const input: IActionModel[] = [
{ action: actionTemplate[0], size: 0, visible: true, renderLabel: true }, // hidden
{ action: new Separator(), size: 1, visible: true, renderLabel: true }, // sep
{ action: actionTemplate[1], size: 50, visible: true, renderLabel: true }, // visible
];
const expected: IActionModel[] = [
{ action: actionTemplate[0], size: 0, visible: true, renderLabel: true }, // hidden
{ action: actionTemplate[1], size: 50, visible: true, renderLabel: true } // visible
];
const result = workbenchDynamicCalculateActions(input, defaultSecondaryActions, containerSize);
assert.deepEqual(result.primaryActions, expected);
assert.deepEqual(result.secondaryActions, defaultSecondaryActions);
});

test('should not render second separator if space between is hidden (size 0) actions.', () => {
const containerSize = 300;
const input: IActionModel[] = [
{ action: actionTemplate[0], size: 50, visible: true, renderLabel: true },
{ action: new Separator(), size: 1, visible: true, renderLabel: true },
{ action: actionTemplate[1], size: 0, visible: true, renderLabel: true },
{ action: actionTemplate[2], size: 0, visible: true, renderLabel: true },
{ action: new Separator(), size: 1, visible: true, renderLabel: true },
{ action: actionTemplate[3], size: 50, visible: true, renderLabel: true },
];
const expected: IActionModel[] = [
{ action: actionTemplate[0], size: 50, visible: true, renderLabel: true },
{ action: new Separator(), size: 1, visible: true, renderLabel: true },
{ action: actionTemplate[1], size: 0, visible: true, renderLabel: true },
{ action: actionTemplate[2], size: 0, visible: true, renderLabel: true },
// remove separator here
{ action: actionTemplate[3], size: 50, visible: true, renderLabel: true },
];
const result = workbenchDynamicCalculateActions(input, defaultSecondaryActions, containerSize);
assert.deepEqual(result.primaryActions, expected);
assert.deepEqual(result.secondaryActions, defaultSecondaryActions);
});
});