Skip to content

Commit

Permalink
Refactor notebook toolbar (for testing) + dynamic strategy unittesting (
Browse files Browse the repository at this point in the history 
#185805)

* testing refactor + bugfix (size 0 breaking remainingSize calculations)

* cleanup
  • Loading branch information
@Yoyokrazy
Yoyokrazy authored Jun 21, 2023
1 parent 87ee421 commit 343c389
Show file tree
Hide file tree
Showing 2 changed files with 208 additions and 22 deletions.
35 changes: 22 additions & 13 deletions src/vs/workbench/contrib/notebook/browser/viewParts/notebookEditorToolbar.ts
View file
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
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);
});
});

0 comments on commit 343c389

Please sign in to comment.