Skip to content

Commit 1f94559

Browse files
martin-fleck-atmartin-fleck-at
authoredAug 24, 2023
Properly re-render problem widget and fix problem matching (#12802)
- Ensure we re-render the tree model if the problem widget is activated - Update Ansi stripping to catch Windows 'clear screen' and other codes - Consistently use Theia URI in problem matcher protocol - Report exit status to terminal widget to avoid resize warning #12724
·
v1.59.0v1.41.0
1 parent 0184f16 commit 1f94559

File tree

10 files changed

+136
-52
lines changed

10 files changed

+136
-52
lines changed
 

‎packages/markers/src/browser/problem/problem-widget.tsx

+6-1
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,7 @@ import { ProblemTreeModel } from './problem-tree-model';
2121
import { MarkerInfoNode, MarkerNode, MarkerRootNode } from '../marker-tree';
2222
import {
2323
TreeWidget, TreeProps, ContextMenuRenderer, TreeNode, NodeProps, TreeModel,
24-
ApplicationShell, Navigatable, ExpandableTreeNode, SelectableTreeNode, TREE_NODE_INFO_CLASS, codicon
24+
ApplicationShell, Navigatable, ExpandableTreeNode, SelectableTreeNode, TREE_NODE_INFO_CLASS, codicon, Message
2525
} from '@theia/core/lib/browser';
2626
import { DiagnosticSeverity } from '@theia/core/shared/vscode-languageserver-protocol';
2727
import * as React from '@theia/core/shared/react';
@@ -73,6 +73,11 @@ export class ProblemWidget extends TreeWidget {
7373
}));
7474
}
7575

76+
protected override onActivateRequest(msg: Message): void {
77+
super.onActivateRequest(msg);
78+
this.update();
79+
}
80+
7681
protected updateFollowActiveEditor(): void {
7782
this.toDisposeOnCurrentWidgetChanged.dispose();
7883
this.toDispose.push(this.toDisposeOnCurrentWidgetChanged);

‎packages/process/src/node/terminal-process.ts

+66-2
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,7 @@
1515
// *****************************************************************************
1616

1717
import { injectable, inject, named } from '@theia/core/shared/inversify';
18-
import { isWindows } from '@theia/core';
18+
import { Disposable, DisposableCollection, Emitter, Event, isWindows } from '@theia/core';
1919
import { ILogger } from '@theia/core/lib/common';
2020
import { Process, ProcessType, ProcessOptions, /* ProcessErrorEvent */ } from './process';
2121
import { ProcessManager } from './process-manager';
@@ -54,6 +54,8 @@ export enum NodePtyErrors {
5454
export class TerminalProcess extends Process {
5555

5656
protected readonly terminal: IPty | undefined;
57+
private _delayedResizer: DelayedResizer | undefined;
58+
private _exitCode: number | undefined;
5759

5860
readonly outputStream = this.createOutputStream();
5961
readonly errorStream = new DevNullStream({ autoDestroy: true });
@@ -79,6 +81,19 @@ export class TerminalProcess extends Process {
7981
}
8082
this.logger.debug('Starting terminal process', JSON.stringify(options, undefined, 2));
8183

84+
// Delay resizes to avoid conpty not respecting very early resize calls
85+
// see https://github.com/microsoft/vscode/blob/a1c783c/src/vs/platform/terminal/node/terminalProcess.ts#L177
86+
if (isWindows) {
87+
this._delayedResizer = new DelayedResizer();
88+
this._delayedResizer.onTrigger(dimensions => {
89+
this._delayedResizer?.dispose();
90+
this._delayedResizer = undefined;
91+
if (dimensions.cols && dimensions.rows) {
92+
this.resize(dimensions.cols, dimensions.rows);
93+
}
94+
});
95+
}
96+
8297
const startTerminal = (command: string): { terminal: IPty | undefined, inputStream: Writable } => {
8398
try {
8499
return this.createPseudoTerminal(command, options, ringBuffer);
@@ -148,6 +163,7 @@ export class TerminalProcess extends Process {
148163
// signal value). If it was terminated because of a signal, the
149164
// signal parameter will hold the signal number and code should
150165
// be ignored.
166+
this._exitCode = exitCode;
151167
if (signal === undefined || signal === 0) {
152168
this.onTerminalExit(exitCode, undefined);
153169
} else {
@@ -208,8 +224,32 @@ export class TerminalProcess extends Process {
208224
}
209225

210226
resize(cols: number, rows: number): void {
227+
if (typeof cols !== 'number' || typeof rows !== 'number' || isNaN(cols) || isNaN(rows)) {
228+
return;
229+
}
211230
this.checkTerminal();
212-
this.terminal!.resize(cols, rows);
231+
try {
232+
// Ensure that cols and rows are always >= 1, this prevents a native exception in winpty.
233+
cols = Math.max(cols, 1);
234+
rows = Math.max(rows, 1);
235+
236+
// Delay resize if needed
237+
if (this._delayedResizer) {
238+
this._delayedResizer.cols = cols;
239+
this._delayedResizer.rows = rows;
240+
return;
241+
}
242+
243+
this.terminal!.resize(cols, rows);
244+
} catch (error) {
245+
// swallow error if the pty has already exited
246+
// see also https://github.com/microsoft/vscode/blob/a1c783c/src/vs/platform/terminal/node/terminalProcess.ts#L549
247+
if (this._exitCode !== undefined &&
248+
error.message !== 'ioctl(2) failed, EBADF' &&
249+
error.message !== 'Cannot resize a pty that has already exited') {
250+
throw error;
251+
}
252+
}
213253
}
214254

215255
write(data: string): void {
@@ -224,3 +264,27 @@ export class TerminalProcess extends Process {
224264
}
225265

226266
}
267+
268+
/**
269+
* Tracks the latest resize event to be trigger at a later point.
270+
*/
271+
class DelayedResizer extends DisposableCollection {
272+
rows: number | undefined;
273+
cols: number | undefined;
274+
private _timeout: NodeJS.Timeout;
275+
276+
private readonly _onTrigger = new Emitter<{ rows?: number; cols?: number }>();
277+
get onTrigger(): Event<{ rows?: number; cols?: number }> { return this._onTrigger.event; }
278+
279+
constructor() {
280+
super();
281+
this.push(this._onTrigger);
282+
this._timeout = setTimeout(() => this._onTrigger.fire({ rows: this.rows, cols: this.cols }), 1000);
283+
this.push(Disposable.create(() => clearTimeout(this._timeout)));
284+
}
285+
286+
override dispose(): void {
287+
super.dispose();
288+
clearTimeout(this._timeout);
289+
}
290+
}

‎packages/task/src/browser/task-service.ts

+1-1
Original file line numberDiff line numberDiff line change
@@ -249,7 +249,7 @@ export class TaskService implements TaskConfigurationClient {
249249
}
250250
}
251251
}
252-
const uri = new URI(problem.resource.path).withScheme(problem.resource.scheme);
252+
const uri = problem.resource.withScheme(problem.resource.scheme);
253253
const document = this.monacoWorkspace.getTextDocument(uri.toString());
254254
if (problem.description.applyTo === ApplyToKind.openDocuments && !!document ||
255255
problem.description.applyTo === ApplyToKind.closedDocuments && !document ||

‎packages/task/src/common/problem-matcher-protocol.ts

+1-2
Original file line numberDiff line numberDiff line change
@@ -21,10 +21,9 @@
2121
* Licensed under the MIT License. See License.txt in the project root for license information.
2222
*--------------------------------------------------------------------------------------------*/
2323

24+
import { URI } from '@theia/core';
2425
import { Severity } from '@theia/core/lib/common/severity';
2526
import { Diagnostic } from '@theia/core/shared/vscode-languageserver-protocol';
26-
// TODO use URI from `@theia/core` instead
27-
import { URI } from '@theia/core/shared/vscode-uri';
2827

2928
export enum ApplyToKind {
3029
allDocuments,

‎packages/task/src/node/process/process-task.ts

+7-10
Original file line numberDiff line numberDiff line change
@@ -27,21 +27,18 @@ import { TaskManager } from '../task-manager';
2727
import { ProcessType, ProcessTaskInfo } from '../../common/process/task-protocol';
2828
import { TaskExitedEvent } from '../../common/task-protocol';
2929

30-
// copied from https://github.com/Microsoft/vscode/blob/1.33.1/src/vs/base/common/strings.ts
31-
// Escape codes
32-
// http://en.wikipedia.org/wiki/ANSI_escape_code
33-
const EL = /\x1B\x5B[12]?K/g; // Erase in line
34-
const COLOR_START = /\x1b\[\d+(;\d+)*m/g; // Color
35-
const COLOR_END = /\x1b\[0?m/g; // Color
30+
// copied from https://github.com/microsoft/vscode/blob/1.79.0/src/vs/base/common/strings.ts#L736
31+
const CSI_SEQUENCE = /(:?\x1b\[|\x9B)[=?>!]?[\d;:]*["$#'* ]?[a-zA-Z@^`{}|~]/g;
32+
33+
// Plus additional markers for custom `\x1b]...\x07` instructions.
34+
const CSI_CUSTOM_SEQUENCE = /\x1b\].*?\x07/g;
3635

3736
export function removeAnsiEscapeCodes(str: string): string {
3837
if (str) {
39-
str = str.replace(EL, '');
40-
str = str.replace(COLOR_START, '');
41-
str = str.replace(COLOR_END, '');
38+
str = str.replace(CSI_SEQUENCE, '').replace(CSI_CUSTOM_SEQUENCE, '');
4239
}
4340

44-
return str.trimRight();
41+
return str.trimEnd();
4542
}
4643

4744
export const TaskProcessOptions = Symbol('TaskProcessOptions');

‎packages/task/src/node/task-abstract-line-matcher.ts

+4-9
Original file line numberDiff line numberDiff line change
@@ -26,10 +26,9 @@ import {
2626
ProblemMatch, ProblemMatchData, ProblemLocationKind
2727
} from '../common/problem-matcher-protocol';
2828
import URI from '@theia/core/lib/common/uri';
29-
// TODO use only URI from '@theia/core'
30-
import { URI as vscodeURI } from '@theia/core/shared/vscode-uri';
3129
import { Severity } from '@theia/core/lib/common/severity';
3230
import { MAX_SAFE_INTEGER } from '@theia/core/lib/common/numbers';
31+
import { join } from 'path';
3332

3433
const endOfLine: string = EOL;
3534

@@ -247,7 +246,7 @@ export abstract class AbstractLineMatcher {
247246
return Severity.toDiagnosticSeverity(result);
248247
}
249248

250-
private getResource(filename: string, matcher: ProblemMatcher): vscodeURI {
249+
private getResource(filename: string, matcher: ProblemMatcher): URI {
251250
const kind = matcher.fileLocation;
252251
let fullPath: string | undefined;
253252
if (kind === FileLocationKind.Absolute) {
@@ -257,19 +256,15 @@ export abstract class AbstractLineMatcher {
257256
if (relativeFileName.startsWith('./')) {
258257
relativeFileName = relativeFileName.slice(2);
259258
}
260-
fullPath = new URI(matcher.filePrefix).resolve(relativeFileName).path.toString();
259+
fullPath = join(matcher.filePrefix, relativeFileName);
261260
}
262261
if (fullPath === undefined) {
263262
throw new Error('FileLocationKind is not actionable. Does the matcher have a filePrefix? This should never happen.');
264263
}
265-
fullPath = fullPath.replace(/\\/g, '/');
266-
if (fullPath[0] !== '/') {
267-
fullPath = '/' + fullPath;
268-
}
269264
if (matcher.uriProvider !== undefined) {
270265
return matcher.uriProvider(fullPath);
271266
} else {
272-
return vscodeURI.file(fullPath);
267+
return URI.fromFilePath(fullPath);
273268
}
274269
}
275270

‎packages/task/src/node/task-problem-collector.spec.ts

+14-14
Original file line numberDiff line numberDiff line change
@@ -14,11 +14,11 @@
1414
// SPDX-License-Identifier: EPL-2.0 OR GPL-2.0-only WITH Classpath-exception-2.0
1515
// *****************************************************************************
1616

17-
import { expect } from 'chai';
17+
import { Severity } from '@theia/core/lib/common/severity';
1818
import { DiagnosticSeverity } from '@theia/core/shared/vscode-languageserver-protocol';
19-
import { ProblemCollector } from './task-problem-collector';
19+
import { expect } from 'chai';
2020
import { ApplyToKind, FileLocationKind, ProblemLocationKind, ProblemMatch, ProblemMatchData, ProblemMatcher } from '../common/problem-matcher-protocol';
21-
import { Severity } from '@theia/core/lib/common/severity';
21+
import { ProblemCollector } from './task-problem-collector';
2222

2323
const startStopMatcher1: ProblemMatcher = {
2424
owner: 'test1',
@@ -130,23 +130,23 @@ describe('ProblemCollector', () => {
130130

131131
expect(allMatches.length).to.eq(3);
132132

133-
expect((allMatches[0] as ProblemMatchData).resource!.path).eq('/home/test/hello.go');
133+
expect((allMatches[0] as ProblemMatchData).resource!.path.toString()).eq('/home/test/hello.go');
134134
expect((allMatches[0] as ProblemMatchData).marker).deep.equal({
135135
range: { start: { line: 8, character: 1 }, end: { line: 8, character: 1 } },
136136
severity: DiagnosticSeverity.Error,
137137
source: 'test1',
138138
message: 'undefined: fmt.Pntln'
139139
});
140140

141-
expect((allMatches[1] as ProblemMatchData).resource!.path).eq('/home/test/hello.go');
141+
expect((allMatches[1] as ProblemMatchData).resource!.path.toString()).eq('/home/test/hello.go');
142142
expect((allMatches[1] as ProblemMatchData).marker).deep.equal({
143143
range: { start: { line: 9, character: 5 }, end: { line: 9, character: 5 } },
144144
severity: DiagnosticSeverity.Error,
145145
source: 'test1',
146146
message: 'undefined: numb'
147147
});
148148

149-
expect((allMatches[2] as ProblemMatchData).resource!.path).eq('/home/test/hello.go');
149+
expect((allMatches[2] as ProblemMatchData).resource!.path.toString()).eq('/home/test/hello.go');
150150
expect((allMatches[2] as ProblemMatchData).marker).deep.equal({
151151
range: { start: { line: 14, character: 8 }, end: { line: 14, character: 8 } },
152152
severity: DiagnosticSeverity.Error,
@@ -176,7 +176,7 @@ describe('ProblemCollector', () => {
176176

177177
expect(allMatches.length).to.eq(4);
178178

179-
expect((allMatches[0] as ProblemMatchData).resource!.path).eq('/home/test/test-dir.js');
179+
expect((allMatches[0] as ProblemMatchData).resource!.path.toString()).eq('/home/test/test-dir.js');
180180
expect((allMatches[0] as ProblemMatchData).marker).deep.equal({
181181
range: { start: { line: 13, character: 20 }, end: { line: 13, character: 20 } },
182182
severity: DiagnosticSeverity.Warning,
@@ -185,7 +185,7 @@ describe('ProblemCollector', () => {
185185
code: 'semi'
186186
});
187187

188-
expect((allMatches[1] as ProblemMatchData).resource!.path).eq('/home/test/test-dir.js');
188+
expect((allMatches[1] as ProblemMatchData).resource!.path.toString()).eq('/home/test/test-dir.js');
189189
expect((allMatches[1] as ProblemMatchData).marker).deep.equal({
190190
range: { start: { line: 14, character: 22 }, end: { line: 14, character: 22 } },
191191
severity: DiagnosticSeverity.Warning,
@@ -194,15 +194,15 @@ describe('ProblemCollector', () => {
194194
code: 'semi'
195195
});
196196

197-
expect((allMatches[2] as ProblemMatchData).resource!.path).eq('/home/test/test-dir.js');
197+
expect((allMatches[2] as ProblemMatchData).resource!.path.toString()).eq('/home/test/test-dir.js');
198198
expect((allMatches[2] as ProblemMatchData).marker).deep.equal({
199199
range: { start: { line: 102, character: 8 }, end: { line: 102, character: 8 } },
200200
severity: DiagnosticSeverity.Error,
201201
source: 'test2',
202202
message: 'Parsing error: Unexpected token inte'
203203
});
204204

205-
expect((allMatches[3] as ProblemMatchData).resource!.path).eq('/home/test/more-test.js');
205+
expect((allMatches[3] as ProblemMatchData).resource!.path.toString()).eq('/home/test/more-test.js');
206206
expect((allMatches[3] as ProblemMatchData).marker).deep.equal({
207207
range: { start: { line: 12, character: 8 }, end: { line: 12, character: 8 } },
208208
severity: DiagnosticSeverity.Error,
@@ -232,7 +232,7 @@ describe('ProblemCollector', () => {
232232

233233
expect(allMatches.length).to.eq(4);
234234

235-
expect((allMatches[0] as ProblemMatchData).resource?.path).eq('/home/test/test-dir.js');
235+
expect((allMatches[0] as ProblemMatchData).resource?.path.toString()).eq('/home/test/test-dir.js');
236236
expect((allMatches[0] as ProblemMatchData).marker).deep.equal({
237237
range: { start: { line: 0, character: 0 }, end: { line: 0, character: 0 } },
238238
severity: DiagnosticSeverity.Warning,
@@ -241,7 +241,7 @@ describe('ProblemCollector', () => {
241241
code: 'semi'
242242
});
243243

244-
expect((allMatches[1] as ProblemMatchData).resource?.path).eq('/home/test/test-dir.js');
244+
expect((allMatches[1] as ProblemMatchData).resource?.path.toString()).eq('/home/test/test-dir.js');
245245
expect((allMatches[1] as ProblemMatchData).marker).deep.equal({
246246
range: { start: { line: 0, character: 0 }, end: { line: 0, character: 0 } },
247247
severity: DiagnosticSeverity.Warning,
@@ -250,15 +250,15 @@ describe('ProblemCollector', () => {
250250
code: 'semi'
251251
});
252252

253-
expect((allMatches[2] as ProblemMatchData).resource?.path).eq('/home/test/test-dir.js');
253+
expect((allMatches[2] as ProblemMatchData).resource?.path.toString()).eq('/home/test/test-dir.js');
254254
expect((allMatches[2] as ProblemMatchData).marker).deep.equal({
255255
range: { start: { line: 0, character: 0 }, end: { line: 0, character: 0 } },
256256
severity: DiagnosticSeverity.Error,
257257
source: 'test2',
258258
message: 'Parsing error: Unexpected token inte'
259259
});
260260

261-
expect((allMatches[3] as ProblemMatchData).resource?.path).eq('/home/test/more-test.js');
261+
expect((allMatches[3] as ProblemMatchData).resource?.path.toString()).eq('/home/test/more-test.js');
262262
expect((allMatches[3] as ProblemMatchData).marker).deep.equal({
263263
range: { start: { line: 0, character: 0 }, end: { line: 0, character: 0 } },
264264
severity: DiagnosticSeverity.Error,

‎packages/terminal/src/browser/terminal-widget-impl.ts

+13-5
Original file line numberDiff line numberDiff line change
@@ -206,21 +206,25 @@ export class TerminalWidgetImpl extends TerminalWidget implements StatefulWidget
206206
});
207207
this.toDispose.push(titleChangeListenerDispose);
208208

209-
this.toDispose.push(this.terminalWatcher.onTerminalError(({ terminalId, error }) => {
209+
this.toDispose.push(this.terminalWatcher.onTerminalError(({ terminalId, error, attached }) => {
210210
if (terminalId === this.terminalId) {
211211
this.exitStatus = { code: undefined, reason: TerminalExitReason.Process };
212-
this.dispose();
213212
this.logger.error(`The terminal process terminated. Cause: ${error}`);
213+
if (!attached) {
214+
this.dispose();
215+
}
214216
}
215217
}));
216-
this.toDispose.push(this.terminalWatcher.onTerminalExit(({ terminalId, code, reason }) => {
218+
this.toDispose.push(this.terminalWatcher.onTerminalExit(({ terminalId, code, reason, attached }) => {
217219
if (terminalId === this.terminalId) {
218220
if (reason) {
219221
this.exitStatus = { code, reason };
220222
} else {
221223
this.exitStatus = { code, reason: TerminalExitReason.Process };
222224
}
223-
this.dispose();
225+
if (!attached) {
226+
this.dispose();
227+
}
224228
}
225229
}));
226230
this.toDispose.push(this.toDisposeOnConnect);
@@ -502,6 +506,8 @@ export class TerminalWidgetImpl extends TerminalWidget implements StatefulWidget
502506
protected async attachTerminal(id: number): Promise<number> {
503507
const terminalId = await this.shellTerminalServer.attach(id);
504508
if (IBaseTerminalServer.validateId(terminalId)) {
509+
// reset exit status if a new terminal process is attached
510+
this.exitStatus = undefined;
505511
return terminalId;
506512
}
507513
this.logger.warn(`Failed attaching to terminal id ${id}, the terminal is most likely gone. Starting up a new terminal instead.`);
@@ -776,7 +782,9 @@ export class TerminalWidgetImpl extends TerminalWidget implements StatefulWidget
776782
return;
777783
}
778784
if (!IBaseTerminalServer.validateId(this.terminalId)
779-
|| !this.terminalService.getById(this.id)) {
785+
|| this.exitStatus
786+
|| !this.terminalService.getById(this.id)
787+
) {
780788
return;
781789
}
782790
const { cols, rows } = this.term;

‎packages/terminal/src/common/base-terminal-protocol.ts

+4-1
Original file line numberDiff line numberDiff line change
@@ -67,6 +67,8 @@ export interface IBaseTerminalExitEvent {
6767
code?: number;
6868
reason?: TerminalExitReason;
6969
signal?: string;
70+
71+
attached?: boolean;
7072
}
7173

7274
export enum TerminalExitReason {
@@ -79,7 +81,8 @@ export enum TerminalExitReason {
7981

8082
export interface IBaseTerminalErrorEvent {
8183
terminalId: number;
82-
error: Error
84+
error: Error;
85+
attached?: boolean;
8386
}
8487

8588
export interface IBaseTerminalClient {

‎packages/terminal/src/node/base-terminal-server.ts

+20-7
Original file line numberDiff line numberDiff line change
@@ -77,6 +77,8 @@ export abstract class BaseTerminalServer implements IBaseTerminalServer {
7777
// Didn't execute `unregisterProcess` on terminal `exit` event to enable attaching task output to terminal,
7878
// Fixes https://github.com/eclipse-theia/theia/issues/2961
7979
terminal.unregisterProcess();
80+
} else {
81+
this.postAttachAttempted(terminal);
8082
}
8183
}
8284
}
@@ -142,31 +144,42 @@ export abstract class BaseTerminalServer implements IBaseTerminalServer {
142144
this.client.updateTerminalEnvVariables();
143145
}
144146

145-
protected postCreate(term: TerminalProcess): void {
147+
protected notifyClientOnExit(term: TerminalProcess): DisposableCollection {
146148
const toDispose = new DisposableCollection();
147149

148150
toDispose.push(term.onError(error => {
149151
this.logger.error(`Terminal pid: ${term.pid} error: ${error}, closing it.`);
150152

151153
if (this.client !== undefined) {
152154
this.client.onTerminalError({
153-
'terminalId': term.id,
154-
'error': new Error(`Failed to execute terminal process (${error.code})`),
155+
terminalId: term.id,
156+
error: new Error(`Failed to execute terminal process (${error.code})`),
157+
attached: term instanceof TaskTerminalProcess && term.attachmentAttempted
155158
});
156159
}
157160
}));
158161

159162
toDispose.push(term.onExit(event => {
160163
if (this.client !== undefined) {
161164
this.client.onTerminalExitChanged({
162-
'terminalId': term.id,
163-
'code': event.code,
164-
'reason': TerminalExitReason.Process,
165-
'signal': event.signal
165+
terminalId: term.id,
166+
code: event.code,
167+
reason: TerminalExitReason.Process,
168+
signal: event.signal,
169+
attached: term instanceof TaskTerminalProcess && term.attachmentAttempted
166170
});
167171
}
168172
}));
173+
return toDispose;
174+
}
175+
176+
protected postCreate(term: TerminalProcess): void {
177+
const toDispose = this.notifyClientOnExit(term);
178+
this.terminalToDispose.set(term.id, toDispose);
179+
}
169180

181+
protected postAttachAttempted(term: TaskTerminalProcess): void {
182+
const toDispose = this.notifyClientOnExit(term);
170183
this.terminalToDispose.set(term.id, toDispose);
171184
}
172185

0 commit comments

Comments
 (0)
Please sign in to comment.