Skip to content

Commit b0a03e5

Browse files
meganroggemeganrogge
authored
improve & simplify quick fix info (microsoft#167473)
1 parent ab7ccc9 commit b0a03e5

File tree

3 files changed

+40
-75
lines changed

3 files changed

+40
-75
lines changed

‎src/vs/workbench/contrib/terminal/browser/terminalQuickFixBuiltinActions.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -102,7 +102,7 @@ export function freePort(terminalInstance?: Partial<ITerminalInstance>): IIntern
102102
return {
103103
class: TerminalQuickFixType.Port,
104104
tooltip: label,
105-
id: 'terminal.freePort',
105+
id: 'Free Port',
106106
label,
107107
enabled: true,
108108
run: async () => {

‎src/vs/workbench/contrib/terminal/browser/xterm/quickFixAddon.ts

Lines changed: 24 additions & 59 deletions
Original file line numberDiff line numberDiff line change
@@ -34,14 +34,12 @@ import { URI } from 'vs/base/common/uri';
3434
const quickFixTelemetryTitle = 'terminal/quick-fix';
3535
type QuickFixResultTelemetryEvent = {
3636
quickFixId: string;
37-
fixesShown: boolean;
38-
ranQuickFixCommand?: boolean;
37+
ranQuickFix: boolean;
3938
};
4039
type QuickFixClassification = {
4140
owner: 'meganrogge';
4241
quickFixId: { classification: 'SystemMetaData'; purpose: 'FeatureInsight'; comment: 'The quick fix ID' };
43-
fixesShown: { classification: 'SystemMetaData'; purpose: 'FeatureInsight'; comment: 'Whether the fixes were shown by the user' };
44-
ranQuickFixCommand?: { classification: 'SystemMetaData'; purpose: 'FeatureInsight'; comment: 'If the command that was executed matched a quick fix suggested one. Undefined if no command is expected.' };
42+
ranQuickFix: { classification: 'SystemMetaData'; purpose: 'FeatureInsight'; comment: 'Whether the quick fix was run' };
4543
comment: 'Terminal quick fixes';
4644
};
4745
const quickFixSelectors = [DecorationSelector.QuickFix, DecorationSelector.LightBulb, DecorationSelector.Codicon, DecorationSelector.CommandDecoration, DecorationSelector.XtermDecoration];
@@ -68,11 +66,10 @@ export class TerminalQuickFixAddon extends Disposable implements ITerminalAddon,
6866

6967
private _decoration: IDecoration | undefined;
7068

71-
private _fixesShown: boolean = false;
72-
private _expectedCommands: string[] | undefined;
73-
private _fixId: string | undefined;
7469
private _currentRenderContext: { quickFixes: ITerminalAction[]; anchor: IAnchor; parentElement: HTMLElement } | undefined;
7570

71+
private _lastQuickFixId: string | undefined;
72+
7673
constructor(
7774
private readonly _aliases: string[][] | undefined,
7875
private readonly _capabilities: ITerminalCapabilityStore,
@@ -112,7 +109,7 @@ export class TerminalQuickFixAddon extends Disposable implements ITerminalAddon,
112109
if (!this._currentRenderContext) {
113110
return;
114111
}
115-
this._fixesShown = true;
112+
116113
// TODO: What's documentation do? Need a vscode command?
117114
const actions = this._currentRenderContext.quickFixes.map(f => new TerminalQuickFix(f, f.class || TerminalQuickFixType.Command, f.source, f.label));
118115
const documentation = this._currentRenderContext.quickFixes.map(f => { return { id: f.source, title: f.label, tooltip: f.source }; });
@@ -128,6 +125,7 @@ export class TerminalQuickFixAddon extends Disposable implements ITerminalAddon,
128125
onSelect: async (fix: TerminalQuickFix) => {
129126
fix.action?.run();
130127
this._actionWidgetService.hide();
128+
this._disposeQuickFix(fix.action.id, true);
131129
},
132130
onHide: () => {
133131
this._terminal?.focus();
@@ -165,26 +163,7 @@ export class TerminalQuickFixAddon extends Disposable implements ITerminalAddon,
165163
if (!terminal || !commandDetection) {
166164
return;
167165
}
168-
this._register(commandDetection.onCommandFinished(async command => {
169-
if (this._expectedCommands) {
170-
const quickFixId = this._fixId || '';
171-
const ranQuickFixCommand = this._expectedCommands.includes(command.command);
172-
this._logService.debug(quickFixTelemetryTitle, {
173-
quickFixId,
174-
fixesShown: this._fixesShown,
175-
ranQuickFixCommand
176-
});
177-
this._telemetryService?.publicLog2<QuickFixResultTelemetryEvent, QuickFixClassification>(quickFixTelemetryTitle, {
178-
quickFixId,
179-
fixesShown: this._fixesShown,
180-
ranQuickFixCommand
181-
});
182-
this._expectedCommands = undefined;
183-
this._fixId = undefined;
184-
}
185-
await this._resolveQuickFixes(command, this._aliases);
186-
this._fixesShown = false;
187-
}));
166+
this._register(commandDetection.onCommandFinished(async command => await this._resolveQuickFixes(command, this._aliases)));
188167

189168
// The buffer is not ready by the time command finish
190169
// is called. Add the decoration on command start if there are corresponding quick fixes
@@ -203,8 +182,8 @@ export class TerminalQuickFixAddon extends Disposable implements ITerminalAddon,
203182
if (!terminal) {
204183
return;
205184
}
206-
if (command.command !== '') {
207-
this._disposeQuickFix();
185+
if (command.command !== '' && this._lastQuickFixId) {
186+
this._disposeQuickFix(this._lastQuickFixId, false);
208187
}
209188
const resolver = async (selector: ITerminalQuickFixOptions, lines?: string[]) => {
210189
const id = selector.id;
@@ -220,31 +199,24 @@ export class TerminalQuickFixAddon extends Disposable implements ITerminalAddon,
220199
if (!result) {
221200
return;
222201
}
223-
const { fixes, onDidRunQuickFix, expectedCommands } = result;
224-
this._expectedCommands = expectedCommands;
225-
this._fixId = fixes.map(f => f.id).join('');
226-
this._quickFixes = fixes;
227-
this._register(onDidRunQuickFix((quickFixId) => {
228-
const ranQuickFixCommand = (this._expectedCommands?.includes(command.command) || false);
229-
this._logService.debug(quickFixTelemetryTitle, {
230-
quickFixId,
231-
fixesShown: this._fixesShown,
232-
ranQuickFixCommand
233-
});
234-
this._telemetryService?.publicLog2<QuickFixResultTelemetryEvent, QuickFixClassification>(quickFixTelemetryTitle, {
235-
quickFixId,
236-
fixesShown: this._fixesShown,
237-
ranQuickFixCommand
238-
});
239-
this._disposeQuickFix();
240-
this._fixesShown = false;
241-
}));
202+
203+
this._quickFixes = result;
204+
this._lastQuickFixId = this._quickFixes[0].id;
242205
}
243206

244-
private _disposeQuickFix(): void {
207+
private _disposeQuickFix(id: string, ranQuickFix: boolean): void {
208+
this._logService.debug(quickFixTelemetryTitle, {
209+
quickFixId: id,
210+
ranQuickFix
211+
});
212+
this._telemetryService?.publicLog2<QuickFixResultTelemetryEvent, QuickFixClassification>(quickFixTelemetryTitle, {
213+
quickFixId: id,
214+
ranQuickFix
215+
});
245216
this._decoration?.dispose();
246217
this._decoration = undefined;
247218
this._quickFixes = undefined;
219+
this._lastQuickFixId = undefined;
248220
}
249221

250222
/**
@@ -310,9 +282,7 @@ export async function getQuickFixesForCommand(
310282
openerService: IOpenerService,
311283
onDidRequestRerunCommand?: Emitter<{ command: string; addNewLine?: boolean }>,
312284
getResolvedFixes?: (selector: ITerminalQuickFixOptions, lines?: string[]) => Promise<ITerminalQuickFix | ITerminalQuickFix[] | undefined>
313-
): Promise<{ fixes: ITerminalAction[]; onDidRunQuickFix: Event<string>; expectedCommands?: string[] } | undefined> {
314-
const onDidRunQuickFixEmitter = new Emitter<string>();
315-
const onDidRunQuickFix = onDidRunQuickFixEmitter.event;
285+
): Promise<ITerminalAction[] | undefined> {
316286
const fixes: ITerminalAction[] = [];
317287
const newCommand = terminalCommand.command;
318288
const expectedCommands = [];
@@ -321,7 +291,6 @@ export async function getQuickFixesForCommand(
321291
if (option.exitStatus === (terminalCommand.exitCode !== 0)) {
322292
continue;
323293
}
324-
const id = option.id;
325294
let quickFixes;
326295
if (option.type === 'resolved') {
327296
quickFixes = await (option as IResolvedExtensionOptions).getQuickFixes(terminalCommand, getLinesForCommand(terminal.buffer.active, terminalCommand, terminal.cols, option.outputMatcher), option, new CancellationTokenSource().token);
@@ -394,9 +363,6 @@ export async function getQuickFixesForCommand(
394363
return;
395364
}
396365
openerService.open(uri);
397-
// since no command gets run here, need to
398-
// clear the decoration and quick fix
399-
onDidRunQuickFixEmitter.fire(id);
400366
},
401367
tooltip: label,
402368
uri: fix.uri
@@ -414,7 +380,6 @@ export async function getQuickFixesForCommand(
414380
enabled: fix.enabled,
415381
run: () => {
416382
fix.run();
417-
onDidRunQuickFixEmitter.fire(id);
418383
},
419384
tooltip: fix.tooltip
420385
};
@@ -426,7 +391,7 @@ export async function getQuickFixesForCommand(
426391
}
427392
}
428393
}
429-
return fixes.length > 0 ? { fixes, onDidRunQuickFix, expectedCommands } : undefined;
394+
return fixes.length > 0 ? fixes : undefined;
430395
}
431396

432397
function convertToQuickFixOptions(selectorProvider: ITerminalQuickFixProviderSelector): IResolvedExtensionOptions {

‎src/vs/workbench/contrib/terminal/test/browser/quickFixAddon.test.ts

Lines changed: 15 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -87,15 +87,15 @@ suite('QuickFixAddon', () => {
8787
});
8888
suite('returns undefined when', () => {
8989
test('expected unix exit code', async () => {
90-
assertMatchOptions((await getQuickFixesForCommand([], terminal, createCommand(command, output, GitSimilarOutputRegex, exitCode), expectedMap, openerService))?.fixes, actions);
90+
assertMatchOptions((await getQuickFixesForCommand([], terminal, createCommand(command, output, GitSimilarOutputRegex, exitCode), expectedMap, openerService)), actions);
9191
});
9292
test('matching exit status', async () => {
93-
assertMatchOptions((await getQuickFixesForCommand([], terminal, createCommand(command, output, GitSimilarOutputRegex, 2), expectedMap, openerService))?.fixes, actions);
93+
assertMatchOptions((await getQuickFixesForCommand([], terminal, createCommand(command, output, GitSimilarOutputRegex, 2), expectedMap, openerService)), actions);
9494
});
9595
});
9696
suite('returns match', () => {
9797
test('returns match', async () => {
98-
assertMatchOptions((await getQuickFixesForCommand([], terminal, createCommand(command, output, GitSimilarOutputRegex), expectedMap, openerService))?.fixes, actions);
98+
assertMatchOptions((await getQuickFixesForCommand([], terminal, createCommand(command, output, GitSimilarOutputRegex), expectedMap, openerService)), actions);
9999
});
100100

101101
test('returns multiple match', async () => {
@@ -117,14 +117,14 @@ suite('QuickFixAddon', () => {
117117
tooltip: 'Run: git push',
118118
command: 'git push'
119119
}];
120-
assertMatchOptions((await getQuickFixesForCommand([], terminal, createCommand('git pu', output, GitSimilarOutputRegex), expectedMap, openerService))?.fixes, actions);
120+
assertMatchOptions((await getQuickFixesForCommand([], terminal, createCommand('git pu', output, GitSimilarOutputRegex), expectedMap, openerService)), actions);
121121
});
122122
test('passes any arguments through', async () => {
123123
output = `git: 'checkoutt' is not a git command. See 'git --help'.
124124
125125
The most similar commands are
126126
checkout`;
127-
assertMatchOptions((await getQuickFixesForCommand([], terminal, createCommand('git checkoutt .', output, GitSimilarOutputRegex), expectedMap, openerService))?.fixes, [{
127+
assertMatchOptions((await getQuickFixesForCommand([], terminal, createCommand('git checkoutt .', output, GitSimilarOutputRegex), expectedMap, openerService)), [{
128128
id: 'Git Similar',
129129
enabled: true,
130130
label: 'Run: git checkout .',
@@ -161,10 +161,10 @@ suite('QuickFixAddon', () => {
161161
});
162162
suite('returns undefined when', () => {
163163
test('expected unix exit code', async () => {
164-
assertMatchOptions((await getQuickFixesForCommand([], terminal, createCommand(command, output, GitTwoDashesRegex, exitCode), expectedMap, openerService))?.fixes, actions);
164+
assertMatchOptions((await getQuickFixesForCommand([], terminal, createCommand(command, output, GitTwoDashesRegex, exitCode), expectedMap, openerService)), actions);
165165
});
166166
test('matching exit status', async () => {
167-
assertMatchOptions((await getQuickFixesForCommand([], terminal, createCommand(command, output, GitTwoDashesRegex, 2), expectedMap, openerService))?.fixes, actions);
167+
assertMatchOptions((await getQuickFixesForCommand([], terminal, createCommand(command, output, GitTwoDashesRegex, 2), expectedMap, openerService)), actions);
168168
});
169169
});
170170
});
@@ -187,7 +187,7 @@ suite('QuickFixAddon', () => {
187187
error Command failed with exit code 1.
188188
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.`;
189189
const actionOptions = [{
190-
id: 'terminal.freePort',
190+
id: 'Free Port',
191191
label: 'Free port 3000',
192192
run: true,
193193
tooltip: 'Free port 3000',
@@ -200,11 +200,11 @@ suite('QuickFixAddon', () => {
200200
});
201201
suite('returns undefined when', () => {
202202
test('output does not match', async () => {
203-
strictEqual((await getQuickFixesForCommand([], terminal, createCommand(portCommand, `invalid output`, FreePortOutputRegex), expectedMap, openerService))?.fixes, undefined);
203+
strictEqual((await getQuickFixesForCommand([], terminal, createCommand(portCommand, `invalid output`, FreePortOutputRegex), expectedMap, openerService)), undefined);
204204
});
205205
});
206206
test('returns actions', async () => {
207-
assertMatchOptions((await getQuickFixesForCommand([], terminal, createCommand(portCommand, output, FreePortOutputRegex), expectedMap, openerService))?.fixes, actionOptions);
207+
assertMatchOptions((await getQuickFixesForCommand([], terminal, createCommand(portCommand, output, FreePortOutputRegex), expectedMap, openerService)), actionOptions);
208208
});
209209
});
210210
}
@@ -239,10 +239,10 @@ suite('QuickFixAddon', () => {
239239
});
240240
suite('returns actions when', () => {
241241
test('expected unix exit code', async () => {
242-
assertMatchOptions((await getQuickFixesForCommand([], terminal, createCommand(command, output, GitPushOutputRegex, exitCode), expectedMap, openerService))?.fixes, actions);
242+
assertMatchOptions((await getQuickFixesForCommand([], terminal, createCommand(command, output, GitPushOutputRegex, exitCode), expectedMap, openerService)), actions);
243243
});
244244
test('matching exit status', async () => {
245-
assertMatchOptions((await getQuickFixesForCommand([], terminal, createCommand(command, output, GitPushOutputRegex, 2), expectedMap, openerService))?.fixes, actions);
245+
assertMatchOptions((await getQuickFixesForCommand([], terminal, createCommand(command, output, GitPushOutputRegex, 2), expectedMap, openerService)), actions);
246246
});
247247
});
248248
});
@@ -283,7 +283,7 @@ suite('QuickFixAddon', () => {
283283
});
284284
suite('returns actions when', () => {
285285
test('expected unix exit code', async () => {
286-
assertMatchOptions((await getQuickFixesForCommand([], terminal, createCommand(command, output, GitCreatePrOutputRegex, exitCode), expectedMap, openerService))?.fixes, actions);
286+
assertMatchOptions((await getQuickFixesForCommand([], terminal, createCommand(command, output, GitCreatePrOutputRegex, exitCode), expectedMap, openerService)), actions);
287287
});
288288
});
289289
});
@@ -319,10 +319,10 @@ suite('QuickFixAddon', () => {
319319
});
320320
suite('returns actions when', () => {
321321
test('expected unix exit code', async () => {
322-
assertMatchOptions((await getQuickFixesForCommand([], terminal, createCommand(command, output, GitPushOutputRegex, exitCode), expectedMap, openerService))?.fixes, actions);
322+
assertMatchOptions((await getQuickFixesForCommand([], terminal, createCommand(command, output, GitPushOutputRegex, exitCode), expectedMap, openerService)), actions);
323323
});
324324
test('matching exit status', async () => {
325-
assertMatchOptions((await getQuickFixesForCommand([], terminal, createCommand(command, output, GitPushOutputRegex, 2), expectedMap, openerService))?.fixes, actions);
325+
assertMatchOptions((await getQuickFixesForCommand([], terminal, createCommand(command, output, GitPushOutputRegex, 2), expectedMap, openerService)), actions);
326326
});
327327
});
328328
});

0 commit comments

Comments
 (0)