Skip to content

Commit 468f6fc

Browse files
Don't use disposable Action and use IAction instead (microsoft#201337)
Turns out all of these disposables were not getting cleaned up... but they really don't need to be disposables in this context. ref microsoft#201320
1 parent b964ed9 commit 468f6fc

File tree

3 files changed

+44
-45
lines changed

3 files changed

+44
-45
lines changed

src/vs/platform/quickinput/browser/quickInput.ts

Lines changed: 17 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,6 @@ import { IListRenderer, IListVirtualDelegate } from 'vs/base/browser/ui/list/lis
1515
import { IListOptions, IListStyles, List } from 'vs/base/browser/ui/list/listWidget';
1616
import { IProgressBarStyles, ProgressBar } from 'vs/base/browser/ui/progressbar/progressbar';
1717
import { IToggleStyles, Toggle } from 'vs/base/browser/ui/toggle/toggle';
18-
import { Action } from 'vs/base/common/actions';
1918
import { equals } from 'vs/base/common/arrays';
2019
import { TimeoutTimer } from 'vs/base/common/async';
2120
import { Codicon } from 'vs/base/common/codicons';
@@ -30,7 +29,7 @@ import { localize } from 'vs/nls';
3029
import { IInputBox, IKeyMods, IQuickInput, IQuickInputButton, IQuickInputHideEvent, IQuickInputToggle, IQuickNavigateConfiguration, IQuickPick, IQuickPickDidAcceptEvent, IQuickPickItem, IQuickPickItemButtonEvent, IQuickPickSeparator, IQuickPickSeparatorButtonEvent, IQuickPickWillAcceptEvent, IQuickWidget, ItemActivation, NO_KEY_MODS, QuickInputHideReason } from 'vs/platform/quickinput/common/quickInput';
3130
import { QuickInputBox } from './quickInputBox';
3231
import { QuickInputList, QuickInputListFocus } from './quickInputList';
33-
import { getIconClass, renderQuickInputDescription } from './quickInputUtils';
32+
import { quickInputButtonToAction, renderQuickInputDescription } from './quickInputUtils';
3433

3534
export interface IQuickInputOptions {
3635
idPrefix: string;
@@ -388,23 +387,23 @@ class QuickInput extends Disposable implements IQuickInput {
388387
if (this.buttonsUpdated) {
389388
this.buttonsUpdated = false;
390389
this.ui.leftActionBar.clear();
391-
const leftButtons = this.buttons.filter(button => button === backButton);
392-
this.ui.leftActionBar.push(leftButtons.map((button, index) => {
393-
const action = new Action(`id-${index}`, '', button.iconClass || getIconClass(button.iconPath), true, async () => {
394-
this.onDidTriggerButtonEmitter.fire(button);
395-
});
396-
action.tooltip = button.tooltip || '';
397-
return action;
398-
}), { icon: true, label: false });
390+
const leftButtons = this.buttons
391+
.filter(button => button === backButton)
392+
.map((button, index) => quickInputButtonToAction(
393+
button,
394+
`id-${index}`,
395+
async () => this.onDidTriggerButtonEmitter.fire(button)
396+
));
397+
this.ui.leftActionBar.push(leftButtons, { icon: true, label: false });
399398
this.ui.rightActionBar.clear();
400-
const rightButtons = this.buttons.filter(button => button !== backButton);
401-
this.ui.rightActionBar.push(rightButtons.map((button, index) => {
402-
const action = new Action(`id-${index}`, '', button.iconClass || getIconClass(button.iconPath), true, async () => {
403-
this.onDidTriggerButtonEmitter.fire(button);
404-
});
405-
action.tooltip = button.tooltip || '';
406-
return action;
407-
}), { icon: true, label: false });
399+
const rightButtons = this.buttons
400+
.filter(button => button !== backButton)
401+
.map((button, index) => quickInputButtonToAction(
402+
button,
403+
`id-${index}`,
404+
async () => this.onDidTriggerButtonEmitter.fire(button)
405+
));
406+
this.ui.rightActionBar.push(rightButtons, { icon: true, label: false });
408407
}
409408
if (this.togglesUpdated) {
410409
this.togglesUpdated = false;

src/vs/platform/quickinput/browser/quickInputList.ts

Lines changed: 8 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,6 @@ import { IconLabel, IIconLabelValueOptions } from 'vs/base/browser/ui/iconLabel/
1313
import { KeybindingLabel } from 'vs/base/browser/ui/keybindingLabel/keybindingLabel';
1414
import { IListRenderer, IListVirtualDelegate } from 'vs/base/browser/ui/list/list';
1515
import { IListAccessibilityProvider, IListOptions, IListStyles, List } from 'vs/base/browser/ui/list/listWidget';
16-
import { IAction } from 'vs/base/common/actions';
1716
import { range } from 'vs/base/common/arrays';
1817
import { ThrottledDelayer } from 'vs/base/common/async';
1918
import { compareAnything } from 'vs/base/common/comparers';
@@ -30,7 +29,7 @@ import { ltrim } from 'vs/base/common/strings';
3029
import 'vs/css!./media/quickInput';
3130
import { localize } from 'vs/nls';
3231
import { IQuickInputOptions } from 'vs/platform/quickinput/browser/quickInput';
33-
import { getIconClass } from 'vs/platform/quickinput/browser/quickInputUtils';
32+
import { quickInputButtonToAction } from 'vs/platform/quickinput/browser/quickInputUtils';
3433
import { IQuickPickItem, IQuickPickItemButtonEvent, IQuickPickSeparator, IQuickPickSeparatorButtonEvent, QuickPickItem } from 'vs/platform/quickinput/common/quickInput';
3534
import { Lazy } from 'vs/base/common/lazy';
3635
import { URI } from 'vs/base/common/uri';
@@ -355,30 +354,13 @@ class ListElementRenderer implements IListRenderer<IListElement, IListElementTem
355354
// Actions
356355
const buttons = mainItem.buttons;
357356
if (buttons && buttons.length) {
358-
data.actionBar.push(buttons.map((button, index): IAction => {
359-
let cssClasses = button.iconClass || (button.iconPath ? getIconClass(button.iconPath) : undefined);
360-
if (button.alwaysVisible) {
361-
cssClasses = cssClasses ? `${cssClasses} always-visible` : 'always-visible';
362-
}
363-
return {
364-
id: `id-${index}`,
365-
class: cssClasses,
366-
enabled: true,
367-
label: '',
368-
tooltip: button.tooltip || '',
369-
run: () => {
370-
mainItem.type !== 'separator'
371-
? element.fireButtonTriggered({
372-
button,
373-
item: mainItem
374-
})
375-
: element.fireSeparatorButtonTriggered({
376-
button,
377-
separator: mainItem
378-
});
379-
}
380-
};
381-
}), { icon: true, label: false });
357+
data.actionBar.push(buttons.map((button, index) => quickInputButtonToAction(
358+
button,
359+
`id-${index}`,
360+
() => mainItem.type !== 'separator'
361+
? element.fireButtonTriggered({ button, item: mainItem })
362+
: element.fireSeparatorButtonTriggered({ button, separator: mainItem })
363+
)), { icon: true, label: false });
382364
data.entry.classList.add('has-actions');
383365
} else {
384366
data.entry.classList.remove('has-actions');

src/vs/platform/quickinput/browser/quickInputUtils.ts

Lines changed: 19 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -16,11 +16,13 @@ import { URI } from 'vs/base/common/uri';
1616
import 'vs/css!./media/quickInput';
1717
import { localize } from 'vs/nls';
1818
import { DisposableStore } from 'vs/base/common/lifecycle';
19+
import { IQuickInputButton } from 'vs/platform/quickinput/common/quickInput';
20+
import { IAction } from 'vs/base/common/actions';
1921

2022
const iconPathToClass: Record<string, string> = {};
2123
const iconClassGenerator = new IdGenerator('quick-input-button-icon-');
2224

23-
export function getIconClass(iconPath: { dark: URI; light?: URI } | undefined): string | undefined {
25+
function getIconClass(iconPath: { dark: URI; light?: URI } | undefined): string | undefined {
2426
if (!iconPath) {
2527
return undefined;
2628
}
@@ -39,6 +41,22 @@ export function getIconClass(iconPath: { dark: URI; light?: URI } | undefined):
3941
return iconClass;
4042
}
4143

44+
export function quickInputButtonToAction(button: IQuickInputButton, id: string, run: () => unknown): IAction {
45+
let cssClasses = button.iconClass || getIconClass(button.iconPath);
46+
if (button.alwaysVisible) {
47+
cssClasses = cssClasses ? `${cssClasses} always-visible` : 'always-visible';
48+
}
49+
50+
return {
51+
id,
52+
label: '',
53+
tooltip: button.tooltip || '',
54+
class: cssClasses,
55+
enabled: true,
56+
run
57+
};
58+
}
59+
4260
export function renderQuickInputDescription(description: string, container: HTMLElement, actionHandler: { callback: (content: string) => void; disposables: DisposableStore }) {
4361
dom.reset(container);
4462
const parsed = parseLinkedText(description);

0 commit comments

Comments
 (0)