-
Notifications
You must be signed in to change notification settings - Fork 5
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
Changes from all commits
5b69857
34eba25
d07fe93
43b3de3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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; | ||
} | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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); | ||
this.updateTerminalOnTaskExit(terminal, showReuseMessage, closeOnFinish); | ||
break; | ||
} | ||
} | ||
|
@@ -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(); | ||
|
@@ -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) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Indeed, thanks for pointing out! 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'); | ||
} | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -10222,6 +10222,11 @@ [email protected], 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== | ||
|
||
[email protected]: | ||
version "2.0.1" | ||
resolved "https://registry.yarnpkg.com/statuses/-/statuses-2.0.1.tgz#55cb000ccf1d48728bd23c685a063998cf1a1b63" | ||
|
There was a problem hiding this comment.
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 inTaskOutputPresentation.shouldCloseTerminalOnFinish
. Is that the right default?There was a problem hiding this comment.
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.