Skip to content

Commit

Permalink
Ben/probable-bobcat (#239286)
Browse files Browse the repository at this point in the history 
* Should not change values in environmentService
(fix #171707)

* dialogs - remove `closeOnLinkClick` option

Closing a dialog from anything but a button click is rather unexpected and there is usage of this anymore.

* Code duplication for text input actions (fix #239187)
  • Loading branch information
@bpasero
bpasero authored Jan 31, 2025
1 parent f9c927c commit f2b5597
Show file tree
Hide file tree
Showing 8 changed files with 57 additions and 127 deletions.
15 changes: 0 additions & 15 deletions src/vs/base/browser/ui/dialog/dialog.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,6 @@ export interface IDialogOptions {
readonly icon?: ThemeIcon;
readonly buttonDetails?: string[];
readonly disableCloseAction?: boolean;
readonly closeOnLinkClick?: boolean;
readonly disableDefaultAction?: boolean;
readonly buttonStyles: IButtonStyles;
readonly checkboxStyles: ICheckboxStyles;
Expand Down Expand Up @@ -212,20 +211,6 @@ export class Dialog extends Disposable {
return;
};

if (this.options.closeOnLinkClick) {
for (const el of this.messageContainer.getElementsByTagName('a')) {
this._register(addDisposableListener(el, EventType.CLICK, () => {
setTimeout(close); // HACK to ensure the link action is triggered before the dialog is closed
}));
this._register(addDisposableListener(el, EventType.KEY_DOWN, (e: KeyboardEvent) => {
const evt = new StandardKeyboardEvent(e);
if (evt.equals(KeyCode.Enter)) {
setTimeout(close); // HACK to ensure the link action is triggered before the dialog is closed
}
}));
}
}

const buttonBar = this.buttonBar = this._register(new ButtonBar(this.buttonsContainer));
const buttonMap = this.rearrangeButtons(this.buttons, this.options.cancelId);

Expand Down
1 change: 0 additions & 1 deletion src/vs/platform/dialogs/common/dialogs.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -278,7 +278,6 @@ export interface ICustomDialogOptions {
readonly classes?: string[];
readonly icon?: ThemeIcon;
readonly disableCloseAction?: boolean;
readonly closeOnLinkClick?: boolean;
}

export interface ICustomDialogMarkdown {
Expand Down
2 changes: 1 addition & 1 deletion src/vs/platform/environment/common/environmentService.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -252,7 +252,7 @@ export abstract class AbstractNativeEnvironmentService implements INativeEnviron
return undefined;
}

editSessionId: string | undefined = this.args['editSessionId'];
get editSessionId(): string | undefined { return this.args['editSessionId']; }

get continueOn(): string | undefined {
return this.args['continueOn'];
Expand Down
86 changes: 43 additions & 43 deletions src/vs/workbench/browser/actions/textInputActions.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,11 +16,53 @@ import { StandardMouseEvent } from '../../../base/browser/mouseEvent.js';
import { Event as BaseEvent } from '../../../base/common/event.js';
import { Lazy } from '../../../base/common/lazy.js';

export function createTextInputActions(clipboardService: IClipboardService): IAction[] {
return [

// Undo/Redo
new Action('undo', localize('undo', "Undo"), undefined, true, async () => getActiveDocument().execCommand('undo')),
new Action('redo', localize('redo', "Redo"), undefined, true, async () => getActiveDocument().execCommand('redo')),
new Separator(),

// Cut / Copy / Paste
new Action('editor.action.clipboardCutAction', localize('cut', "Cut"), undefined, true, async () => getActiveDocument().execCommand('cut')),
new Action('editor.action.clipboardCopyAction', localize('copy', "Copy"), undefined, true, async () => getActiveDocument().execCommand('copy')),
new Action('editor.action.clipboardPasteAction', localize('paste', "Paste"), undefined, true, async element => {

// Native: paste is supported
if (isNative) {
getActiveDocument().execCommand('paste');
}

// Web: paste is not supported due to security reasons
else {
const clipboardText = await clipboardService.readText();
if (
isHTMLTextAreaElement(element) ||
isHTMLInputElement(element)
) {
const selectionStart = element.selectionStart || 0;
const selectionEnd = element.selectionEnd || 0;

element.value = `${element.value.substring(0, selectionStart)}${clipboardText}${element.value.substring(selectionEnd, element.value.length)}`;
element.selectionStart = selectionStart + clipboardText.length;
element.selectionEnd = element.selectionStart;
element.dispatchEvent(new Event('input', { bubbles: true, cancelable: true }));
}
}
}),
new Separator(),

// Select All
new Action('editor.action.selectAll', localize('selectAll', "Select All"), undefined, true, async () => getActiveDocument().execCommand('selectAll'))
];
}

export class TextInputActionsProvider extends Disposable implements IWorkbenchContribution {

static readonly ID = 'workbench.contrib.textInputActionsProvider';

private readonly textInputActions = new Lazy<IAction[]>(() => this.createActions());
private readonly textInputActions = new Lazy<IAction[]>(() => createTextInputActions(this.clipboardService));

constructor(
@IWorkbenchLayoutService private readonly layoutService: IWorkbenchLayoutService,
Expand All @@ -32,48 +74,6 @@ export class TextInputActionsProvider extends Disposable implements IWorkbenchCo
this.registerListeners();
}

private createActions(): IAction[] {
return [

// Undo/Redo
new Action('undo', localize('undo', "Undo"), undefined, true, async () => getActiveDocument().execCommand('undo')),
new Action('redo', localize('redo', "Redo"), undefined, true, async () => getActiveDocument().execCommand('redo')),
new Separator(),

// Cut / Copy / Paste
new Action('editor.action.clipboardCutAction', localize('cut', "Cut"), undefined, true, async () => getActiveDocument().execCommand('cut')),
new Action('editor.action.clipboardCopyAction', localize('copy', "Copy"), undefined, true, async () => getActiveDocument().execCommand('copy')),
new Action('editor.action.clipboardPasteAction', localize('paste', "Paste"), undefined, true, async element => {

// Native: paste is supported
if (isNative) {
getActiveDocument().execCommand('paste');
}

// Web: paste is not supported due to security reasons
else {
const clipboardText = await this.clipboardService.readText();
if (
isHTMLTextAreaElement(element) ||
isHTMLInputElement(element)
) {
const selectionStart = element.selectionStart || 0;
const selectionEnd = element.selectionEnd || 0;

element.value = `${element.value.substring(0, selectionStart)}${clipboardText}${element.value.substring(selectionEnd, element.value.length)}`;
element.selectionStart = selectionStart + clipboardText.length;
element.selectionEnd = element.selectionStart;
element.dispatchEvent(new Event('input', { bubbles: true, cancelable: true }));
}
}
}),
new Separator(),

// Select All
new Action('editor.action.selectAll', localize('selectAll', "Select All"), undefined, true, async () => getActiveDocument().execCommand('selectAll'))
];
}

private registerListeners(): void {

// Context menu support in input/textarea
Expand Down
1 change: 0 additions & 1 deletion src/vs/workbench/browser/parts/dialogs/dialogHandler.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -137,7 +137,6 @@ export class BrowserDialogHandler extends AbstractDialogHandler {
renderBody,
icon: customOptions?.icon,
disableCloseAction: customOptions?.disableCloseAction,
closeOnLinkClick: customOptions?.closeOnLinkClick,
buttonDetails: customOptions?.buttonDetails,
checkboxLabel: checkbox?.label,
checkboxChecked: checkbox?.checked,
Expand Down
13 changes: 11 additions & 2 deletions src/vs/workbench/contrib/terminalContrib/find/browser/terminalFindWidget.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,11 +15,12 @@ import { IKeybindingService } from '../../../../../platform/keybinding/common/ke
import { Event } from '../../../../../base/common/event.js';
import type { ISearchOptions } from '@xterm/addon-search';
import { IClipboardService } from '../../../../../platform/clipboard/common/clipboardService.js';
import { openContextMenu } from './textInputContextMenu.js';
import { IDisposable } from '../../../../../base/common/lifecycle.js';
import { IHoverService } from '../../../../../platform/hover/browser/hover.js';
import { TerminalFindCommandId } from '../common/terminal.find.js';
import { TerminalClipboardContribution } from '../../clipboard/browser/terminal.clipboard.contribution.js';
import { StandardMouseEvent } from '../../../../../base/browser/mouseEvent.js';
import { createTextInputActions } from '../../../../browser/actions/textInputActions.js';

const TERMINAL_FIND_WIDGET_INITIAL_WIDTH = 419;

Expand Down Expand Up @@ -74,7 +75,15 @@ export class TerminalFindWidget extends SimpleFindWidget {
}
const findInputDomNode = this.getFindInputDomNode();
this._register(dom.addDisposableListener(findInputDomNode, 'contextmenu', (event) => {
openContextMenu(dom.getWindow(findInputDomNode), event, clipboardService, contextMenuService);
const targetWindow = dom.getWindow(findInputDomNode);
const standardEvent = new StandardMouseEvent(targetWindow, event);
const actions = createTextInputActions(clipboardService);

contextMenuService.showContextMenu({
getAnchor: () => standardEvent,
getActions: () => actions,
getActionsContext: () => event.target,
});
event.stopPropagation();
}));
this._register(themeService.onDidColorThemeChange(() => {
Expand Down
63 changes: 0 additions & 63 deletions src/vs/workbench/contrib/terminalContrib/find/browser/textInputContextMenu.ts
View file

This file was deleted.

3 changes: 2 additions & 1 deletion src/vs/workbench/services/environment/browser/environmentService.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -259,7 +259,8 @@ export class BrowserWorkbenchEnvironmentService implements IBrowserWorkbenchEnvi
@memoize
get profile(): string | undefined { return this.payload?.get('profile'); }

editSessionId: string | undefined = this.options.editSessionId;
@memoize
get editSessionId(): string | undefined { return this.options.editSessionId; }

private payload: Map<string, string> | undefined;

Expand Down

0 comments on commit f2b5597

Please sign in to comment.