Skip to content

Commit 343c389

Browse files
YoyokrazyYoyokrazy
authored
Refactor notebook toolbar (for testing) + dynamic strategy unittesting (microsoft#185805)
* testing refactor + bugfix (size 0 breaking remainingSize calculations) * cleanup
1 parent 87ee421 commit 343c389

File tree

2 files changed

+208
-22
lines changed

2 files changed

+208
-22
lines changed

‎src/vs/workbench/contrib/notebook/browser/viewParts/notebookEditorToolbar.ts

Lines changed: 22 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -88,7 +88,11 @@ class WorkbenchAlwaysLabelStrategy implements IActionLayoutStrategy {
8888
const initialPrimaryActions = this.editorToolbar.primaryActions;
8989
const initialSecondaryActions = this.editorToolbar.secondaryActions;
9090

91-
return workbenchCalculateActions(initialPrimaryActions, initialSecondaryActions, leftToolbarContainerMaxWidth);
91+
const actionOutput = workbenchCalculateActions(initialPrimaryActions, initialSecondaryActions, leftToolbarContainerMaxWidth);
92+
return {
93+
primaryActions: actionOutput.primaryActions.map(a => a.action),
94+
secondaryActions: actionOutput.secondaryActions
95+
};
9296
}
9397
}
9498

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

114-
return workbenchCalculateActions(initialPrimaryActions, initialSecondaryActions, leftToolbarContainerMaxWidth);
118+
const actionOutput = workbenchCalculateActions(initialPrimaryActions, initialSecondaryActions, leftToolbarContainerMaxWidth);
119+
return {
120+
primaryActions: actionOutput.primaryActions.map(a => a.action),
121+
secondaryActions: actionOutput.secondaryActions
122+
};
115123
}
116124
}
117125

@@ -139,7 +147,11 @@ class WorkbenchDynamicLabelStrategy implements IActionLayoutStrategy {
139147
const initialPrimaryActions = this.editorToolbar.primaryActions;
140148
const initialSecondaryActions = this.editorToolbar.secondaryActions;
141149

142-
return workbenchDynamicCalculateActions(initialPrimaryActions, initialSecondaryActions, leftToolbarContainerMaxWidth);
150+
const actionOutput = workbenchDynamicCalculateActions(initialPrimaryActions, initialSecondaryActions, leftToolbarContainerMaxWidth);
151+
return {
152+
primaryActions: actionOutput.primaryActions.map(a => a.action),
153+
secondaryActions: actionOutput.secondaryActions
154+
};
143155
}
144156
}
145157

@@ -523,11 +535,11 @@ export class NotebookEditorWorkbenchToolbar extends Disposable {
523535
}
524536
}
525537

526-
export function workbenchCalculateActions(initialPrimaryActions: IActionModel[], initialSecondaryActions: IAction[], leftToolbarContainerMaxWidth: number): { primaryActions: IAction[]; secondaryActions: IAction[] } {
538+
export function workbenchCalculateActions(initialPrimaryActions: IActionModel[], initialSecondaryActions: IAction[], leftToolbarContainerMaxWidth: number): { primaryActions: IActionModel[]; secondaryActions: IAction[] } {
527539
return actionOverflowHelper(initialPrimaryActions, initialSecondaryActions, leftToolbarContainerMaxWidth, false);
528540
}
529541

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

532544
if (initialPrimaryActions.length === 0) {
533545
return { primaryActions: [], secondaryActions: initialSecondaryActions };
@@ -542,10 +554,7 @@ export function workbenchDynamicCalculateActions(initialPrimaryActions: IActionM
542554
initialPrimaryActions.forEach(action => {
543555
action.renderLabel = true;
544556
});
545-
return {
546-
primaryActions: initialPrimaryActions.map(action => action.action),
547-
secondaryActions: initialSecondaryActions
548-
};
557+
return actionOverflowHelper(initialPrimaryActions, initialSecondaryActions, leftToolbarContainerMaxWidth, false);
549558
}
550559

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

563572
if (initialPrimaryActions[i].action instanceof Separator) {
564573
// find group separator
565-
const remainingItems = initialPrimaryActions.slice(i + 1);
574+
const remainingItems = initialPrimaryActions.slice(i + 1).filter(action => action.size !== 0); // todo: need to exclude size 0 items from this
566575
const newTotalSum = sum + (remainingItems.length === 0 ? 0 : (remainingItems.length * ICON_ONLY_ACTION_WIDTH + (remainingItems.length - 1) * ACTION_PADDING));
567576
if (newTotalSum <= leftToolbarContainerMaxWidth) {
568577
lastActionWithLabel = i;
@@ -582,12 +591,12 @@ export function workbenchDynamicCalculateActions(initialPrimaryActions: IActionM
582591
initialPrimaryActions.slice(0, lastActionWithLabel + 1).forEach(action => { action.renderLabel = true; });
583592
initialPrimaryActions.slice(lastActionWithLabel + 1).forEach(action => { action.renderLabel = false; });
584593
return {
585-
primaryActions: initialPrimaryActions.map(action => action.action),
594+
primaryActions: initialPrimaryActions,
586595
secondaryActions: initialSecondaryActions
587596
};
588597
}
589598

590-
function actionOverflowHelper(initialPrimaryActions: IActionModel[], initialSecondaryActions: IAction[], leftToolbarContainerMaxWidth: number, iconOnly: boolean): { primaryActions: IAction[]; secondaryActions: IAction[] } {
599+
function actionOverflowHelper(initialPrimaryActions: IActionModel[], initialSecondaryActions: IAction[], leftToolbarContainerMaxWidth: number, iconOnly: boolean): { primaryActions: IActionModel[]; secondaryActions: IAction[] } {
591600
const renderActions: IActionModel[] = [];
592601
const overflow: IAction[] = [];
593602

@@ -665,7 +674,7 @@ function actionOverflowHelper(initialPrimaryActions: IActionModel[], initialSeco
665674
}
666675

667676
return {
668-
primaryActions: renderActions.map(action => action.action),
677+
primaryActions: renderActions,
669678
secondaryActions: [...overflow, ...initialSecondaryActions]
670679
};
671680
}

‎src/vs/workbench/contrib/notebook/test/browser/notebookWorkbenchToolbar.test.ts

Lines changed: 186 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@
33
* Licensed under the MIT License. See License.txt in the project root for license information.
44
*--------------------------------------------------------------------------------------------*/
55

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

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

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

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

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

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

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

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

@@ -120,7 +120,184 @@ suite('workbenchCalculateActions', () => {
120120
{ action: new Action('action3', 'Action 3'), size: 50, visible: true, renderLabel: true },
121121
];
122122
const result = workbenchCalculateActions(actions, defaultSecondaryActions, 300);
123-
assert.deepEqual(result.primaryActions, [actions[0], actions[1], actions[2], actions[3], actions[5]].map(action => action.action));
123+
assert.deepEqual(result.primaryActions, [actions[0], actions[1], actions[2], actions[3], actions[5]]);
124+
assert.deepEqual(result.secondaryActions, defaultSecondaryActions);
125+
});
126+
});
127+
128+
suite('Workbench Toolbar Dynamic calculateActions (strategy dynamic)', () => {
129+
130+
const actionTemplate = [
131+
new Action('action0', 'Action 0'),
132+
new Action('action1', 'Action 1'),
133+
new Action('action2', 'Action 2'),
134+
new Action('action3', 'Action 3')
135+
];
136+
137+
const defaultSecondaryActionModels: IActionModel[] = [
138+
{ action: new Action('secondaryAction0', 'Secondary Action 0'), size: 50, visible: true, renderLabel: true },
139+
{ action: new Action('secondaryAction1', 'Secondary Action 1'), size: 50, visible: true, renderLabel: true },
140+
{ action: new Action('secondaryAction2', 'Secondary Action 2'), size: 50, visible: true, renderLabel: true },
141+
];
142+
const defaultSecondaryActions: IAction[] = defaultSecondaryActionModels.map(action => action.action);
143+
144+
145+
test('should return empty primary and secondary actions when given empty initial actions', () => {
146+
const result = workbenchDynamicCalculateActions([], [], 100);
147+
assert.deepEqual(result.primaryActions, []);
148+
assert.deepEqual(result.secondaryActions, []);
149+
});
150+
151+
test('should return all primary actions as visiblewhen they fit within the container width', () => {
152+
const constainerSize = 200;
153+
const input: IActionModel[] = [
154+
{ action: actionTemplate[0], size: 50, visible: true, renderLabel: true },
155+
{ action: actionTemplate[1], size: 50, visible: true, renderLabel: true },
156+
{ action: actionTemplate[2], size: 50, visible: true, renderLabel: true },
157+
];
158+
const expected: IActionModel[] = [
159+
{ action: actionTemplate[0], size: 50, visible: true, renderLabel: true },
160+
{ action: actionTemplate[1], size: 50, visible: true, renderLabel: true },
161+
{ action: actionTemplate[2], size: 50, visible: true, renderLabel: true },
162+
];
163+
const result = workbenchDynamicCalculateActions(input, defaultSecondaryActions, constainerSize);
164+
assert.deepEqual(result.primaryActions, expected);
165+
assert.deepEqual(result.secondaryActions, defaultSecondaryActions);
166+
});
167+
168+
test('actions all within a group that cannot all fit, will all be icon only', () => {
169+
const containerSize = 150;
170+
const input: IActionModel[] = [
171+
{ action: actionTemplate[0], size: 50, visible: true, renderLabel: true },
172+
{ action: actionTemplate[1], size: 50, visible: true, renderLabel: true },
173+
{ action: actionTemplate[2], size: 50, visible: true, renderLabel: true },
174+
];
175+
const expected: IActionModel[] = [
176+
{ action: actionTemplate[0], size: 50, visible: true, renderLabel: false },
177+
{ action: actionTemplate[1], size: 50, visible: true, renderLabel: false },
178+
{ action: actionTemplate[2], size: 50, visible: true, renderLabel: false },
179+
];
180+
181+
182+
const result = workbenchDynamicCalculateActions(input, defaultSecondaryActions, containerSize);
183+
assert.deepEqual(result.primaryActions, expected);
184+
assert.deepEqual(result.secondaryActions, [...defaultSecondaryActionModels].map(action => action.action));
185+
});
186+
187+
test('should ignore second separator when two separators are in a row', () => {
188+
const containerSize = 200;
189+
const input: IActionModel[] = [
190+
{ action: actionTemplate[0], size: 50, visible: true, renderLabel: true },
191+
{ action: new Separator(), size: 1, visible: true, renderLabel: true },
192+
{ action: new Separator(), size: 1, visible: true, renderLabel: true },
193+
{ action: actionTemplate[1], size: 50, visible: true, renderLabel: true },
194+
];
195+
const expected: IActionModel[] = [
196+
{ action: actionTemplate[0], size: 50, visible: true, renderLabel: true },
197+
{ action: new Separator(), size: 1, visible: true, renderLabel: true },
198+
{ action: actionTemplate[1], size: 50, visible: true, renderLabel: true },
199+
];
200+
const result = workbenchDynamicCalculateActions(input, defaultSecondaryActions, containerSize);
201+
assert.deepEqual(result.primaryActions, expected);
202+
assert.deepEqual(result.secondaryActions, defaultSecondaryActions);
203+
});
204+
205+
test('check label visibility in different groupings', () => {
206+
const containerSize = 150;
207+
const actions: IActionModel[] = [
208+
{ action: actionTemplate[0], size: 50, visible: true, renderLabel: true },
209+
{ action: new Separator(), size: 1, visible: true, renderLabel: true },
210+
{ action: actionTemplate[1], size: 50, visible: true, renderLabel: true },
211+
{ action: actionTemplate[2], size: 50, visible: true, renderLabel: true },
212+
];
213+
const expectedOutputActions: IActionModel[] = [
214+
{ action: actionTemplate[0], size: 50, visible: true, renderLabel: true },
215+
{ action: new Separator(), size: 1, visible: true, renderLabel: true },
216+
{ action: actionTemplate[1], size: 50, visible: true, renderLabel: false },
217+
{ action: actionTemplate[2], size: 50, visible: true, renderLabel: false },
218+
];
219+
220+
221+
const result = workbenchDynamicCalculateActions(actions, defaultSecondaryActions, containerSize);
222+
assert.deepEqual(result.primaryActions, expectedOutputActions);
223+
assert.deepEqual(result.secondaryActions, defaultSecondaryActions);
224+
});
225+
226+
test('should ignore separators when they are at the end of the resulting primary actions', () => {
227+
const containerSize = 200;
228+
const input: IActionModel[] = [
229+
{ action: actionTemplate[0], size: 50, visible: true, renderLabel: true },
230+
{ action: new Separator(), size: 1, visible: true, renderLabel: true },
231+
{ action: actionTemplate[1], size: 50, visible: true, renderLabel: true },
232+
{ action: new Separator(), size: 1, visible: true, renderLabel: true },
233+
];
234+
const expected: IActionModel[] = [
235+
{ action: actionTemplate[0], size: 50, visible: true, renderLabel: true },
236+
{ action: new Separator(), size: 1, visible: true, renderLabel: true },
237+
{ action: actionTemplate[1], size: 50, visible: true, renderLabel: true },
238+
];
239+
const result = workbenchDynamicCalculateActions(input, defaultSecondaryActions, containerSize);
240+
assert.deepEqual(result.primaryActions, expected);
241+
assert.deepEqual(result.secondaryActions, defaultSecondaryActions);
242+
});
243+
244+
test('should keep actions with size 0 in primary actions', () => {
245+
const containerSize = 170;
246+
const input: IActionModel[] = [
247+
{ action: actionTemplate[0], size: 50, visible: true, renderLabel: true },
248+
{ action: actionTemplate[1], size: 50, visible: true, renderLabel: true },
249+
{ action: new Separator(), size: 1, visible: true, renderLabel: true },
250+
{ action: actionTemplate[2], size: 50, visible: true, renderLabel: true },
251+
{ action: actionTemplate[3], size: 0, visible: true, renderLabel: true },
252+
];
253+
const expected: IActionModel[] = [
254+
{ action: actionTemplate[0], size: 50, visible: true, renderLabel: true },
255+
{ action: actionTemplate[1], size: 50, visible: true, renderLabel: true },
256+
{ action: new Separator(), size: 1, visible: true, renderLabel: true },
257+
{ action: actionTemplate[2], size: 50, visible: true, renderLabel: false },
258+
{ action: actionTemplate[3], size: 0, visible: true, renderLabel: false },
259+
];
260+
const result = workbenchDynamicCalculateActions(input, defaultSecondaryActions, containerSize);
261+
assert.deepEqual(result.primaryActions, expected);
262+
assert.deepEqual(result.secondaryActions, defaultSecondaryActions);
263+
});
264+
265+
test('should not render separator if preceeded by size 0 action(s), but keep size 0 action in primary.', () => {
266+
const containerSize = 116;
267+
const input: IActionModel[] = [
268+
{ action: actionTemplate[0], size: 0, visible: true, renderLabel: true }, // hidden
269+
{ action: new Separator(), size: 1, visible: true, renderLabel: true }, // sep
270+
{ action: actionTemplate[1], size: 50, visible: true, renderLabel: true }, // visible
271+
];
272+
const expected: IActionModel[] = [
273+
{ action: actionTemplate[0], size: 0, visible: true, renderLabel: true }, // hidden
274+
{ action: actionTemplate[1], size: 50, visible: true, renderLabel: true } // visible
275+
];
276+
const result = workbenchDynamicCalculateActions(input, defaultSecondaryActions, containerSize);
277+
assert.deepEqual(result.primaryActions, expected);
278+
assert.deepEqual(result.secondaryActions, defaultSecondaryActions);
279+
});
280+
281+
test('should not render second separator if space between is hidden (size 0) actions.', () => {
282+
const containerSize = 300;
283+
const input: IActionModel[] = [
284+
{ action: actionTemplate[0], size: 50, visible: true, renderLabel: true },
285+
{ action: new Separator(), size: 1, visible: true, renderLabel: true },
286+
{ action: actionTemplate[1], size: 0, visible: true, renderLabel: true },
287+
{ action: actionTemplate[2], size: 0, visible: true, renderLabel: true },
288+
{ action: new Separator(), size: 1, visible: true, renderLabel: true },
289+
{ action: actionTemplate[3], size: 50, visible: true, renderLabel: true },
290+
];
291+
const expected: IActionModel[] = [
292+
{ action: actionTemplate[0], size: 50, visible: true, renderLabel: true },
293+
{ action: new Separator(), size: 1, visible: true, renderLabel: true },
294+
{ action: actionTemplate[1], size: 0, visible: true, renderLabel: true },
295+
{ action: actionTemplate[2], size: 0, visible: true, renderLabel: true },
296+
// remove separator here
297+
{ action: actionTemplate[3], size: 50, visible: true, renderLabel: true },
298+
];
299+
const result = workbenchDynamicCalculateActions(input, defaultSecondaryActions, containerSize);
300+
assert.deepEqual(result.primaryActions, expected);
124301
assert.deepEqual(result.secondaryActions, defaultSecondaryActions);
125302
});
126303
});

0 commit comments

Comments
 (0)