Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[vscode] support TaskPresentationOptions close #82

Closed
wants to merge 4 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,9 @@

## v1.40.0 -

- Show command shortcuts in toolbar item tooltips. #12660 (https://github.com/eclipse-theia/theia/pull/12660) - Contributed on behalf of STMicroelectronics
- Show command shortcuts in toolbar item tooltips. [#12660](https://github.com/eclipse-theia/theia/pull/12660) - Contributed on behalf of STMicroelectronics
- [cli] added `check:theia-extensions` which checks the uniqueness of Theia extension versions [#12596](https://github.com/eclipse-theia/theia/pull/12596) - Contributed on behalf of STMicroelectronics
- [vscode] Add support for the TaskPresentationOptions close property [#12749](https://github.com/eclipse-theia/theia/pull/12749) - Contributed on behalf of STMicroelectronics

<a name="breaking_changes_1.40.0">[Breaking Changes:](#breaking_changes_1.40.0)</a>

Expand Down
8 changes: 6 additions & 2 deletions packages/core/src/browser/style/menus.css
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,11 @@
min-height: var(--theia-private-menu-item-height);
max-height: var(--theia-private-menu-item-height);
padding: 0px;
line-height: var(--theia-private-menu-item-height);
/**
* FireFox adds 0.5px to all menu items for some reason
* Subtracting that amount fixes the behavior on FireFox and doesn't introduce issues on Chromium
*/
line-height: calc(var(--theia-private-menu-item-height) - 0.5px);
overflow: hidden;
}

Expand Down Expand Up @@ -158,7 +162,7 @@
.p-Menu-itemIcon {
width: 21px;
padding: 0px 2px 0px 4px;
margin-top: -2px;
height: var(--theia-private-menu-item-height);
}

.p-Menu-itemLabel {
Expand Down
1 change: 1 addition & 0 deletions packages/filesystem/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
"minimatch": "^5.1.0",
"multer": "1.4.4-lts.1",
"rimraf": "^2.6.2",
"stat-mode": "^1.0.0",
"tar-fs": "^1.16.2",
"trash": "^7.2.0",
"uuid": "^8.0.0",
Expand Down
109 changes: 109 additions & 0 deletions packages/filesystem/src/node/disk-file-system-provider.spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,109 @@
// *****************************************************************************
// Copyright (C) 2023 Arduino SA and others.
//
// This program and the accompanying materials are made available under the
// terms of the Eclipse Public License v. 2.0 which is available at
// http://www.eclipse.org/legal/epl-2.0.
//
// This Source Code may also be made available under the following Secondary
// Licenses when the conditions for such availability set forth in the Eclipse
// Public License v. 2.0 are satisfied: GNU General Public License, version 2
// with the GNU Classpath Exception which is available at
// https://www.gnu.org/software/classpath/license.html.
//
// SPDX-License-Identifier: EPL-2.0 OR GPL-2.0-only WITH Classpath-exception-2.0
// *****************************************************************************

import { Disposable, DisposableCollection } from '@theia/core/lib/common/disposable';
import { EncodingService } from '@theia/core/lib/common/encoding-service';
import { ILogger } from '@theia/core/lib/common/logger';
import { MockLogger } from '@theia/core/lib/common/test/mock-logger';
import { FileUri } from '@theia/core/lib/node/file-uri';
import { IPCConnectionProvider } from '@theia/core/lib/node/messaging/ipc-connection-provider';
import { Container, ContainerModule } from '@theia/core/shared/inversify';
import { equal, fail } from 'assert';
import { promises as fs } from 'fs';
import { join } from 'path';
import * as temp from 'temp';
import { v4 } from 'uuid';
import { FilePermission, FileSystemProviderError, FileSystemProviderErrorCode } from '../common/files';
import { DiskFileSystemProvider } from './disk-file-system-provider';
import { bindFileSystemWatcherServer } from './filesystem-backend-module';

const tracked = temp.track();

describe('disk-file-system-provider', () => {
let toDisposeAfter: DisposableCollection;
let fsProvider: DiskFileSystemProvider;

before(() => {
fsProvider = createContainer().get<DiskFileSystemProvider>(
DiskFileSystemProvider
);
toDisposeAfter = new DisposableCollection(
fsProvider,
Disposable.create(() => tracked.cleanupSync())
);
});

after(() => {
toDisposeAfter.dispose();
});

describe('stat', () => {
it("should omit the 'permissions' property of the stat if the file can be both read and write", async () => {
const tempDirPath = tracked.mkdirSync();
const tempFilePath = join(tempDirPath, `${v4()}.txt`);
await fs.writeFile(tempFilePath, 'some content', { encoding: 'utf8' });

let content = await fs.readFile(tempFilePath, { encoding: 'utf8' });
equal(content, 'some content');

await fs.writeFile(tempFilePath, 'other content', { encoding: 'utf8' });

content = await fs.readFile(tempFilePath, { encoding: 'utf8' });
equal(content, 'other content');

const stat = await fsProvider.stat(FileUri.create(tempFilePath));
equal(stat.permissions, undefined);
});

it("should set the 'permissions' property to `Readonly` if the file can be read but not write", async () => {
const tempDirPath = tracked.mkdirSync();
const tempFilePath = join(tempDirPath, `${v4()}.txt`);
await fs.writeFile(tempFilePath, 'readonly content', {
encoding: 'utf8',
});

await fs.chmod(tempFilePath, '444'); // read-only for owner/group/world

try {
await fsProvider.writeFile(FileUri.create(tempFilePath), new Uint8Array(), { create: false, overwrite: true });
fail('Expected an EACCES error for readonly (chmod 444) files');
} catch (err) {
equal(err instanceof FileSystemProviderError, true);
equal((<FileSystemProviderError>err).code, FileSystemProviderErrorCode.NoPermissions);
}

const content = await fs.readFile(tempFilePath, { encoding: 'utf8' });
equal(content, 'readonly content');

const stat = await fsProvider.stat(FileUri.create(tempFilePath));
equal(stat.permissions, FilePermission.Readonly);
});
});

function createContainer(): Container {
const container = new Container({ defaultScope: 'Singleton' });
const module = new ContainerModule(bind => {
bind(DiskFileSystemProvider).toSelf().inSingletonScope();
bind(EncodingService).toSelf().inSingletonScope();
bindFileSystemWatcherServer(bind);
bind(MockLogger).toSelf().inSingletonScope();
bind(ILogger).toService(MockLogger);
bind(IPCConnectionProvider).toSelf().inSingletonScope();
});
container.load(module);
return container;
}
});
8 changes: 5 additions & 3 deletions packages/filesystem/src/node/disk-file-system-provider.ts
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ import {
FileSystemProviderError,
FileChange,
WatchOptions,
FileUpdateOptions, FileUpdateResult, FileReadStreamOptions
FileUpdateOptions, FileUpdateResult, FileReadStreamOptions, FilePermission
} from '../common/files';
import { FileSystemWatcherServer } from '../common/filesystem-watcher-protocol';
import trash = require('trash');
Expand All @@ -65,6 +65,7 @@ import { BinaryBuffer } from '@theia/core/lib/common/buffer';
import { ReadableStreamEvents, newWriteableStream } from '@theia/core/lib/common/stream';
import { CancellationToken } from '@theia/core/lib/common/cancellation';
import { readFileIntoStream } from '../common/io';
import { Mode } from 'stat-mode';

export namespace DiskFileSystemProvider {
export interface StatAndLink {
Expand Down Expand Up @@ -151,12 +152,13 @@ export class DiskFileSystemProvider implements Disposable,
async stat(resource: URI): Promise<Stat> {
try {
const { stat, symbolicLink } = await this.statLink(this.toFilePath(resource)); // cannot use fs.stat() here to support links properly

const mode = new Mode(stat);
return {
type: this.toType(stat, symbolicLink),
ctime: stat.birthtime.getTime(), // intentionally not using ctime here, we want the creation time
mtime: stat.mtime.getTime(),
size: stat.size
size: stat.size,
permissions: !mode.owner.write ? FilePermission.Readonly : undefined,
};
} catch (error) {
throw this.toFileSystemProviderError(error);
Expand Down
1 change: 1 addition & 0 deletions packages/plugin-ext/src/common/plugin-api-rpc.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1510,6 +1510,7 @@ export interface TaskPresentationOptionsDTO {
panel?: number;
showReuseMessage?: boolean;
clear?: boolean;
close?: boolean;
}

export interface TaskExecutionDto {
Expand Down
3 changes: 3 additions & 0 deletions packages/plugin/src/theia.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12627,6 +12627,9 @@ export module '@theia/plugin' {

/** Controls whether the terminal is cleared before executing the task. */
clear?: boolean;

/** Controls whether the terminal is closed after executing the task. */
close?: boolean;
}

/**
Expand Down
15 changes: 9 additions & 6 deletions packages/task/src/browser/task-terminal-widget-manager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,8 @@ export class TaskTerminalWidgetManager {
for (const terminal of this.getTaskTerminalWidgets()) {
if (terminal.taskId === finishedTaskId) {
const showReuseMessage = !!event.config && TaskOutputPresentation.shouldShowReuseMessage(event.config);
this.notifyTaskFinished(terminal, showReuseMessage);
const closeOnFinish = !!event.config && TaskOutputPresentation.shouldCloseTerminalOnFinish(event.config);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems to me that if closeOnFinish defaults to true in TaskOutputPresentation.shouldCloseTerminalOnFinish. Is that the right default?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the default was supposed to be 'false' and was badly implemented. Fixed with new patch.

this.updateTerminalOnTaskExit(terminal, showReuseMessage, closeOnFinish);
break;
}
}
Expand All @@ -113,11 +114,11 @@ export class TaskTerminalWidgetManager {
terminal.taskConfig = taskConfig;
terminal.busy = true;
} else {
this.notifyTaskFinished(terminal, true);
this.updateTerminalOnTaskExit(terminal, true, false);
}
});
const didConnectFailureListener = terminal.onDidOpenFailure(async () => {
this.notifyTaskFinished(terminal, true);
this.updateTerminalOnTaskExit(terminal, true, false);
});
terminal.onDidDispose(() => {
didConnectListener.dispose();
Expand Down Expand Up @@ -211,10 +212,12 @@ export class TaskTerminalWidgetManager {
return this.terminalService.all.filter(TaskTerminalWidget.is);
}

protected notifyTaskFinished(terminal: TaskTerminalWidget, showReuseMessage: boolean): void {
protected updateTerminalOnTaskExit(terminal: TaskTerminalWidget, showReuseMessage: boolean, closeOnFinish: boolean): void {
terminal.busy = false;
terminal.scrollToBottom();
if (showReuseMessage) {
if (closeOnFinish) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like that the terminal is closed in a message that is names notifyTaskFinished. A reader would not expect that from the name. Why not just do this at the (single) call site?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I vaguely remember that we reuse terminals for running tasks in some cases. If my recollection is correct, shouldn't we check if this is a shared terminal and keep it open in that case?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, thanks for pointing out!
I renamed to a more generic term on handling the terminal on task exit, not only notifying the task was finished. That should be more readable from a developer point of view.

For the reuse terminal options, I tend to think that closing the terminal has a higher priority than sharing it with other tasks. I checked on VS Code, the behavior is coherent. Their close option prevails over the shared/dedicated option.

terminal.close();
} else if (showReuseMessage) {
terminal.scrollToBottom();
terminal.writeLine('\x1b[1m\n\rTerminal will be reused by tasks. \x1b[0m\n');
}
}
Expand Down
11 changes: 9 additions & 2 deletions packages/task/src/common/task-protocol.ts
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ export interface TaskOutputPresentation {
panel?: PanelKind;
showReuseMessage?: boolean;
clear?: boolean;
close?: boolean;
// eslint-disable-next-line @typescript-eslint/no-explicit-any
[name: string]: any;
}
Expand All @@ -59,7 +60,8 @@ export namespace TaskOutputPresentation {
focus: false,
panel: PanelKind.Shared,
showReuseMessage: true,
clear: false
clear: false,
close: false
};
}

Expand Down Expand Up @@ -90,7 +92,8 @@ export namespace TaskOutputPresentation {
echo: task.presentation.echo === undefined || task.presentation.echo,
focus: shouldSetFocusToTerminal(task),
showReuseMessage: shouldShowReuseMessage(task),
clear: shouldClearTerminalBeforeRun(task)
clear: shouldClearTerminalBeforeRun(task),
close: shouldCloseTerminalOnFinish(task)
};
}
return outputPresentation;
Expand All @@ -108,6 +111,10 @@ export namespace TaskOutputPresentation {
return !!task.presentation && !!task.presentation.clear;
}

export function shouldCloseTerminalOnFinish(task: TaskCustomization): boolean {
return !!task.presentation && !!task.presentation.close;
}

export function shouldShowReuseMessage(task: TaskCustomization): boolean {
return !task.presentation || task.presentation.showReuseMessage === undefined || !!task.presentation.showReuseMessage;
}
Expand Down
5 changes: 5 additions & 0 deletions yarn.lock
Original file line number Diff line number Diff line change
Expand Up @@ -10222,6 +10222,11 @@ ssri@9.0.1, ssri@^9.0.0:
dependencies:
minipass "^3.1.1"

stat-mode@^1.0.0:
version "1.0.0"
resolved "https://registry.yarnpkg.com/stat-mode/-/stat-mode-1.0.0.tgz#68b55cb61ea639ff57136f36b216a291800d1465"
integrity sha512-jH9EhtKIjuXZ2cWxmXS8ZP80XyC3iasQxMDV8jzhNJpfDb7VbQLVW4Wvsxz9QZvzV+G4YoSfBUVKDOyxLzi/sg==

statuses@2.0.1:
version "2.0.1"
resolved "https://registry.yarnpkg.com/statuses/-/statuses-2.0.1.tgz#55cb000ccf1d48728bd23c685a063998cf1a1b63"
Expand Down
Loading