-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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 EnvironmentVariableCollection description #12696 #12838
Changes from 2 commits
f45ef07
10df2a8
c27c87d
b80c556
81de14d
bab13b9
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 |
---|---|---|
|
@@ -20,10 +20,12 @@ import { RPCProtocol } from '../common/rpc-protocol'; | |
import { Event, Emitter } from '@theia/core/lib/common/event'; | ||
import { Deferred } from '@theia/core/lib/common/promise-util'; | ||
import * as theia from '@theia/plugin'; | ||
import * as Converter from './type-converters'; | ||
import { Disposable, EnvironmentVariableMutatorType, TerminalExitReason, ThemeIcon } from './types-impl'; | ||
import { SerializableEnvironmentVariableCollection } from '@theia/terminal/lib/common/base-terminal-protocol'; | ||
import { ProvidedTerminalLink } from '../common/plugin-api-rpc-model'; | ||
import { ThemeIcon as MonacoThemeIcon } from '@theia/monaco-editor-core/esm/vs/platform/theme/common/themeService'; | ||
import { MarkdownString as MarkdownStringDTO } from '@theia/core/lib/common/markdown-rendering'; | ||
|
||
export function getIconUris(iconPath: theia.TerminalOptions['iconPath']): { id: string } | undefined { | ||
if (ThemeIcon.is(iconPath)) { | ||
|
@@ -313,7 +315,18 @@ export class TerminalServiceExtImpl implements TerminalServiceExt { | |
|
||
private syncEnvironmentVariableCollection(extensionIdentifier: string, collection: EnvironmentVariableCollection): void { | ||
const serialized = [...collection.map.entries()]; | ||
this.proxy.$setEnvironmentVariableCollection(extensionIdentifier, collection.persistent, serialized.length === 0 ? undefined : serialized); | ||
this.proxy.$setEnvironmentVariableCollection(extensionIdentifier, collection.persistent, serialized.length === 0 ? undefined : serialized, | ||
this.descriptionToDTO(collection.description)); | ||
} | ||
|
||
private descriptionToDTO(value: string | theia.MarkdownString | undefined): string | MarkdownStringDTO | undefined { | ||
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. Does it make sense to move this to the Converter? Are there other locations that do the same conversion? 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. Moved to converter as fromMarkdownOrString. |
||
if (value === undefined) { | ||
return undefined; | ||
} else if (typeof value === 'string') { | ||
return value; | ||
} else { | ||
return Converter.fromMarkdown(value); | ||
} | ||
} | ||
|
||
private setEnvironmentVariableCollection(extensionIdentifier: string, collection: EnvironmentVariableCollection): void { | ||
|
@@ -339,8 +352,15 @@ export class TerminalServiceExtImpl implements TerminalServiceExt { | |
|
||
export class EnvironmentVariableCollection implements theia.EnvironmentVariableCollection { | ||
readonly map: Map<string, theia.EnvironmentVariableMutator> = new Map(); | ||
private _description?: string | theia.MarkdownString; | ||
private _persistent: boolean = true; | ||
|
||
public get description(): string | theia.MarkdownString | undefined { return this._description; } | ||
public set description(value: string | theia.MarkdownString | undefined) { | ||
this._description = value; | ||
this.onDidChangeCollectionEmitter.fire(); | ||
} | ||
|
||
public get persistent(): boolean { return this._persistent; } | ||
public set persistent(value: boolean) { | ||
this._persistent = value; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -20,6 +20,7 @@ import { CommandLineOptions } from '@theia/process/lib/common/shell-command-buil | |
import { TerminalSearchWidget } from '../search/terminal-search-widget'; | ||
import { TerminalProcessInfo, TerminalExitReason } from '../../common/base-terminal-protocol'; | ||
import URI from '@theia/core/lib/common/uri'; | ||
import { MarkdownString } from '@theia/core/lib/common/markdown-rendering/markdown-string'; | ||
|
||
export interface TerminalDimensions { | ||
cols: number; | ||
|
@@ -58,6 +59,9 @@ export abstract class TerminalWidget extends BaseWidget { | |
*/ | ||
abstract processInfo: Promise<TerminalProcessInfo>; | ||
|
||
/** The extensions contributing to the environment of this terminal */ | ||
abstract contributingExtensions: Promise<Map<string, string | MarkdownString | undefined>>; | ||
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 think the name is quite non-descriptive here: what is being held in this map? 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. Went with |
||
|
||
/** Terminal kind that indicates whether a terminal is created by a user or by some extension for a user */ | ||
abstract readonly kind: 'user' | string; | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -32,7 +32,7 @@ import { | |
import { | ||
ApplicationShell, KeybindingContribution, KeyCode, Key, WidgetManager, PreferenceService, | ||
KeybindingRegistry, LabelProvider, WidgetOpenerOptions, StorageService, QuickInputService, | ||
codicon, CommonCommands, FrontendApplicationContribution, OnWillStopAction, Dialog, ConfirmDialog, FrontendApplication, PreferenceScope, Widget | ||
codicon, CommonCommands, FrontendApplicationContribution, OnWillStopAction, Dialog, ConfirmDialog, FrontendApplication, PreferenceScope, Widget, HoverService | ||
} from '@theia/core/lib/browser'; | ||
import { TabBarToolbarContribution, TabBarToolbarRegistry } from '@theia/core/lib/browser/shell/tab-bar-toolbar'; | ||
import { TERMINAL_WIDGET_FACTORY_ID, TerminalWidgetFactoryOptions, TerminalWidgetImpl } from './terminal-widget-impl'; | ||
|
@@ -60,6 +60,8 @@ import { nls } from '@theia/core/lib/common/nls'; | |
import { Profiles, TerminalPreferences } from './terminal-preferences'; | ||
import { ShellTerminalProfile } from './shell-terminal-profile'; | ||
import { VariableResolverService } from '@theia/variable-resolver/lib/browser'; | ||
import { TerminalInfoToolbarItem } from './terminal-info-toolbar-item'; | ||
import { MarkdownRenderer, MarkdownRendererFactory } from '@theia/core/lib/browser/markdown-rendering/markdown-renderer'; | ||
|
||
export namespace TerminalMenus { | ||
export const TERMINAL = [...MAIN_MENU_BAR, '7_terminal']; | ||
|
@@ -216,6 +218,17 @@ export class TerminalFrontendContribution implements FrontendApplicationContribu | |
@inject(TerminalPreferences) | ||
protected terminalPreferences: TerminalPreferences; | ||
|
||
@inject(HoverService) | ||
protected readonly hoverService: HoverService; | ||
|
||
@inject(MarkdownRendererFactory) protected readonly markdownRendererFactory: MarkdownRendererFactory; | ||
|
||
protected _markdownRenderer: MarkdownRenderer | undefined; | ||
protected get markdownRenderer(): MarkdownRenderer { | ||
this._markdownRenderer ||= this.markdownRendererFactory(); | ||
return this._markdownRenderer; | ||
} | ||
|
||
protected mergePreferencesPromise: Promise<void> = Promise.resolve(); | ||
|
||
protected readonly onDidCreateTerminalEmitter = new Emitter<TerminalWidget>(); | ||
|
@@ -250,7 +263,7 @@ export class TerminalFrontendContribution implements FrontendApplicationContribu | |
this.storageService.getData<string>(ENVIRONMENT_VARIABLE_COLLECTIONS_KEY).then(data => { | ||
if (data) { | ||
const collectionsJson: SerializableExtensionEnvironmentVariableCollection[] = JSON.parse(data); | ||
collectionsJson.forEach(c => this.shellTerminalServer.setCollection(c.extensionIdentifier, true, c.collection)); | ||
collectionsJson.forEach(c => this.shellTerminalServer.setCollection(c.extensionIdentifier, true, c.collection, undefined)); | ||
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. If we make the description part of the env var collection, we can serialize it at the same time, right? 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've added the description to |
||
} | ||
}); | ||
}); | ||
|
@@ -731,6 +744,7 @@ export class TerminalFrontendContribution implements FrontendApplicationContribu | |
} | ||
|
||
registerToolbarItems(toolbar: TabBarToolbarRegistry): void { | ||
toolbar.registerItem(new TerminalInfoToolbarItem(this.hoverService, this.markdownRenderer)); | ||
toolbar.registerItem({ | ||
id: TerminalCommands.SPLIT.id, | ||
command: TerminalCommands.SPLIT.id, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,109 @@ | ||
// ***************************************************************************** | ||
// Copyright (C) 2023 STMicroelectronics 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 { ReactTabBarToolbarItem } from '@theia/core/lib/browser/shell/tab-bar-toolbar'; | ||
import { ReactNode } from '@theia/core/shared/react'; | ||
import React = require('@theia/core/shared/react'); | ||
import { HoverService, Widget } from '@theia/core/lib/browser'; | ||
import { TerminalWidget } from './base/terminal-widget'; | ||
import { MarkdownRenderer } from '@theia/core/lib/browser/markdown-rendering/markdown-renderer'; | ||
import { MarkdownStringImpl } from '@theia/core/lib/common/markdown-rendering/markdown-string'; | ||
import { DisposableCollection } from '@theia/core'; | ||
|
||
export class TerminalInfoToolbarItem implements ReactTabBarToolbarItem { | ||
readonly id = 'terminal:info'; | ||
|
||
constructor( | ||
protected readonly hoverService: HoverService, | ||
protected readonly markdownRenderer: MarkdownRenderer | ||
) {} | ||
|
||
render(widget?: Widget): ReactNode { | ||
const toDispose = new DisposableCollection(); | ||
return ( | ||
<div | ||
id={this.id} | ||
className='codicon codicon-terminal-bash action-label' | ||
onMouseEnter={e => this.onMouseEnter(e, toDispose, widget)} | ||
onMouseLeave={e => this.onMouseLeave(toDispose)} | ||
></div> | ||
); | ||
} | ||
|
||
protected async onMouseEnter( | ||
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. It seems odd that we have to implement our own "show hover on mouse-over" support. Dont' we have this somewhere else already? 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. Not that I am aware of. I think the reusable functionality is all in the 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. Code removed because of usage of enhanced preview |
||
event: React.MouseEvent<HTMLElement, MouseEvent>, toDispose: DisposableCollection, currentTerminal?: Widget | ||
): Promise<void> { | ||
const currentTarget = event.currentTarget; | ||
if (currentTerminal instanceof TerminalWidget) { | ||
const extensions = await currentTerminal.contributingExtensions; | ||
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. Wouldn't it make more sense to let the TerminalWidget render (and cache) a Markdown string that we can just render with the markdow renderer instead of manipulating divs ourselves here? 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. When adapting the enhanced preview this definitely makes sense. So I will change this. |
||
const processId = await currentTerminal.processId; | ||
const processInfo = await currentTerminal.processInfo; | ||
|
||
const mainDiv = document.createElement('div'); | ||
|
||
const pid = document.createElement('div'); | ||
pid.textContent = 'Process ID: ' + processId; | ||
mainDiv.appendChild(pid); | ||
|
||
const commandLine = document.createElement('div'); | ||
commandLine.textContent = | ||
'Command line: ' + | ||
processInfo.executable + | ||
' ' + | ||
processInfo.arguments.join(' '); | ||
mainDiv.appendChild(commandLine); | ||
|
||
mainDiv.appendChild(document.createElement('hr')); | ||
|
||
const header = document.createElement('div'); | ||
header.textContent = | ||
'The following extensions have contributed to this terminal\'s environment:'; | ||
mainDiv.appendChild(header); | ||
|
||
const list = document.createElement('ul'); | ||
mainDiv.appendChild(list); | ||
|
||
extensions.forEach((value, key) => { | ||
const item = document.createElement('li'); | ||
let markdown; | ||
if (value === undefined) { | ||
markdown = new MarkdownStringImpl(''); | ||
markdown.appendText(key); | ||
} else if (typeof value === 'string') { | ||
markdown = new MarkdownStringImpl(''); | ||
markdown.appendText(key + ': ' + value); | ||
} else { | ||
markdown = new MarkdownStringImpl('', value); | ||
markdown.appendText(key + ': '); | ||
markdown.appendMarkdown(value.value); | ||
} | ||
const result = this.markdownRenderer.render(markdown); | ||
toDispose.push(result); | ||
item.appendChild(result.element); | ||
list.appendChild(item); | ||
}); | ||
|
||
this.hoverService.requestHover({ | ||
content: mainDiv, | ||
target: currentTarget, | ||
position: 'right', | ||
}); | ||
} | ||
} | ||
|
||
protected async onMouseLeave(toDispose: DisposableCollection): Promise<void> { | ||
toDispose.dispose(); | ||
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.
|
||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -16,6 +16,7 @@ | |
|
||
import { RpcServer } from '@theia/core/lib/common/messaging/proxy-factory'; | ||
import { Disposable } from '@theia/core'; | ||
import { MarkdownString } from '@theia/core/lib/common/markdown-rendering/markdown-string'; | ||
|
||
export interface TerminalProcessInfo { | ||
executable: string | ||
|
@@ -28,6 +29,7 @@ export interface IBaseTerminalServer extends RpcServer<IBaseTerminalClient> { | |
create(IBaseTerminalServerOptions: object): Promise<number>; | ||
getProcessId(id: number): Promise<number>; | ||
getProcessInfo(id: number): Promise<TerminalProcessInfo>; | ||
getContributingExtensions(id: number): Promise<Map<string, string | MarkdownString | undefined>>; | ||
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.
|
||
getCwdURI(id: number): Promise<string>; | ||
resize(id: number, cols: number, rows: number): Promise<void>; | ||
attach(id: number): Promise<number>; | ||
|
@@ -48,7 +50,7 @@ export interface IBaseTerminalServer extends RpcServer<IBaseTerminalClient> { | |
/** | ||
* Sets an extension's environment variable collection. | ||
*/ | ||
setCollection(extensionIdentifier: string, persistent: boolean, collection: SerializableEnvironmentVariableCollection): void; | ||
setCollection(extensionIdentifier: string, persistent: boolean, collection: SerializableEnvironmentVariableCollection, description: string | MarkdownString | undefined): void; | ||
/** | ||
* Deletes an extension's environment variable collection. | ||
*/ | ||
|
@@ -154,6 +156,7 @@ export interface EnvironmentVariableCollection { | |
|
||
export interface EnvironmentVariableCollectionWithPersistence extends EnvironmentVariableCollection { | ||
readonly persistent: boolean; | ||
readonly description: string | MarkdownString | undefined; | ||
} | ||
|
||
export enum EnvironmentVariableMutatorType { | ||
|
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.
The descriptions seems part of the
SerializableEnvironmentCollection
to me. Why not change the typing to be a proper object?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.
I am not sure if I understand this, because there is no
SerializableEnvironmentCollection
. Are you referring toSerializableEnvironmentVariableCollection
? This type is also used used inSerializableExtensionEnvironmentVariableCollection
, which is actually persisted using the storageService. So I think changing this type/interface would break reading existing data.But I guess we could add the
description
toSerializableExtensionEnvironmentVariableCollection
and use$setEnvironmentVariableCollection(persistent: boolean, collection: SerializableExtensionEnvironmentVariableCollection): void;
as a signature, if this does not count as an API break.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.
I've went with
$setEnvironmentVariableCollection(persistent: boolean, collection: SerializableExtensionEnvironmentVariableCollection): void
in the updated PR. Please let me know what you think.