-
Notifications
You must be signed in to change notification settings - Fork 29k
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
Add smart paste terminal feature #231178
base: main
Are you sure you want to change the base?
Add smart paste terminal feature #231178
Conversation
Hi @Tyriar , I've opened this PR but still need some clarity on a few aspects regarding smart paste:
Thank you for your help with these questions! 😊 |
@microsoft-github-policy-service agree |
dadcc47
to
8c0e9f1
Compare
@@ -83,6 +83,7 @@ export const enum TerminalSettingId { | |||
EnableVisualBell = 'terminal.integrated.enableVisualBell', | |||
CommandsToSkipShell = 'terminal.integrated.commandsToSkipShell', | |||
AllowChords = 'terminal.integrated.allowChords', | |||
AllowSmartPaste = 'terminal.integrated.allowSmartPaste', |
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 think enableSmartPaste
is more appropriate than allow*
@@ -165,6 +165,7 @@ export interface ITerminalConfiguration { | |||
scrollback: number; | |||
commandsToSkipShell: string[]; | |||
allowChords: boolean; | |||
allowSmartPaste: boolean; |
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.
We want this to live entirely within terminalContrib/clipboard
, including the configuration setup
@@ -96,8 +96,17 @@ export class TerminalClipboardContribution extends Disposable implements ITermin | |||
return; | |||
} | |||
|
|||
let shellType = ''; | |||
|
|||
shellType = (this._ctx.instance as ITerminalInstance).shellType ?? ''; |
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.
We should not be using as
here, I think the fix here is to add shellType
to IBaseTerminalInstance
@@ -9,14 +9,84 @@ import { IDialogService } from '../../../../../platform/dialogs/common/dialogs.j | |||
import { ServicesAccessor } from '../../../../../platform/instantiation/common/instantiation.js'; | |||
import { TerminalSettingId } from '../../../../../platform/terminal/common/terminal.js'; | |||
|
|||
export async function shouldPasteTerminalText(accessor: ServicesAccessor, text: string, bracketedPasteMode: boolean | undefined): Promise<boolean | { modifiedText: string }> { | |||
class SmartPasteUtils { |
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.
namespace smartPaste
is preferable to a class containing only static
s. However just moving these into a separate file inside terminalContrib/clipboard
would be best
* @param string | ||
* @returns true if the string is a path | ||
*/ | ||
static isPath(string: string): boolean { |
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.
isPathLike
? This will help hint that it's not actually verifying that it's an actual path, but something that likely is.
static isPath(string: string): boolean { | ||
// Regex to detect common path formats | ||
const windowsPathPattern = /^[a-zA-Z]:(\\|\/)/; // Windows absolute path | ||
const uncPathPattern = /^\\\\/; // Windows UNC path |
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.
windowsUncPattern
?
* | ||
* @param string | ||
* @param shellType | ||
* @returns func to handle smart paste, escape chars or wrap inside double quotes |
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.
Same as above
// Escape backslashes and wrap in double quotes if necessary | ||
const escapedPath = string.replace(/\\/g, '\\\\'); | ||
if (string.includes(' ')) { | ||
return `"${escapedPath}"`; | ||
} | ||
return escapedPath; | ||
} | ||
|
||
case 'bash': // Linux/macOS Bash | ||
// Wrap in quotes if spaces or special characters exist | ||
if (string.includes(' ')) { | ||
return `"${string}"`; | ||
} | ||
return string; | ||
|
||
case 'pwsh': | ||
// Simply wrap in quotes if spaces are present | ||
if (string.includes(' ')) { | ||
return `"${string}"`; | ||
} |
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.
You also need to ensure that the quote you're using is escaped properly within string
* @param shellType | ||
* @returns func to handle smart paste, escape chars or wrap inside double quotes | ||
*/ | ||
static handleSmartPaste(string: string, shellType: string): string { |
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.
Same as above, we want to avoid a "string
" var
function setConfigValue(multiLinePaste: unknown, smartPaste: boolean) { | ||
configurationService = new TestConfigurationService({ | ||
[TerminalSettingId.EnableMultiLinePasteWarning]: value | ||
[TerminalSettingId.EnableMultiLinePasteWarning]: multiLinePaste, | ||
[TerminalSettingId.AllowSmartPaste]: smartPaste | ||
}); | ||
instantiationService.stub(IConfigurationService, configurationService); | ||
} |
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.
Since there's another one now, tests would be easier to read if this was an object:
values: { enableMultiLinePaste?: unknown, enableSmartPaste?: boolean }
They could also default to false?
@Tyriar while I go through your comments and incorporate them, could you please check these queries? 🙂 |
You could experiment with using
We could have a default path, and then tweak for shell-specific behavior as necessary.
Pasting in PowerShell is actually handled a little specially, there's a custom keybinding which sends ^V to PSReadLine which then handles the pasting. vscode/src/vs/workbench/contrib/terminal/browser/terminal.contribution.ts Lines 141 to 150 in 8b7a797
As you see in the comment this is done to properly support multi-line pasting. Not sure the best way to handle this, maybe we should only be doing that when the string is multi-line? 🤔
I think it makes the most sense only if the entire text is a path. Otherwise it would be pretty unexpected imo. |
8c0e9f1
to
c775b6e
Compare
c775b6e
to
82ed483
Compare
@Parasaran-Python fyi it's a little easier to review your change if you don't force push. Force pushing breaks diffs so I can't diff when you've newly done over what you did that i already reviewed |
Sure @Tyriar will not force push going forward, force push had become necessary as I prefer rebase over merge to have a clean linear commit history, Fyi, have incorporated the changes, just the relative paths thing is pending. |
@Tyriar , I experimented with detectLinks and noticed an issue: it ignores anything after a space in paths that contain spaces. A relative path could be mistaken for another keyword. For example, if I have a folder named "cd dir" in my current working directory, and I try to paste "cd dir" in the terminal to access the folder "dir," the command gets wrapped in quotes. This leads to unintended behavior. If it was just for cd we could even check if the path string exists (on device) before wrapping but this could be used with a command like mkdir as well. I hope I was clear. Need some clarity in this aspect, as I seem to be approaching a dead end 😕 May be the scope has to be limited to a safe extent (for relative paths). |
Yeah you're right, I thought it handled space paths too but that's tackled here: vscode/src/vs/workbench/contrib/terminalContrib/links/browser/terminalLocalLinkDetector.ts Lines 37 to 59 in 6c8fa55
Perhaps use terminalLinkParsing first and then fallback to the simpler matching? |
Thanks @Tyriar that works, the only thing that's pending now is the ctrl+v on powershell, smart paste not working on ctrl+v on PS might be a pain, I would love to have smart paste working even with a ctrl+v action on a power shell instance 🙂 While I explore ways to achieve this please do drop suggestions if any.😊 Thanks again. |
…oard | Made smart paste work even for strings pasted using ctrl+v action on pwsh
// reader. This works even when clipboard.readText is not supported. | ||
if (isWindows) { | ||
registerSendSequenceKeybinding(String.fromCharCode('V'.charCodeAt(0) - Constants.CtrlLetterOffset), { // ctrl+v | ||
when: ContextKeyExpr.and(TerminalContextKeys.focus, ContextKeyExpr.equals(TerminalContextKeyStrings.ShellType, GeneralShellType.PowerShell), CONTEXT_ACCESSIBILITY_MODE_ENABLED.negate()), |
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.
@Tyriar I have removed this and handled the ctrl+v action in "src\vs\workbench\contrib\terminalContrib\clipboard\browser\terminalClipboard.ts" itself,
this used to handle only ctrl+v action on pwsh, if the user tries to paste using right mouse click the multi line paste didn't used to work properly (Not sure if this is a bug or an expected behavior),
Now with the changes I've made the right mouse click also works as expected
It's just that we get the multi line paste alert on ctrl+v as well, which in my opinion shouldn't be an issue as this brings in consistency in the behavior.
@Tyriar have made some changes, requesting you to re-review. |
|
||
export const terminalClipboardConfiguration: IStringDictionary<IConfigurationPropertySchema> = { | ||
[TerminalClipboardSettingId.EnableSmartPaste]: { | ||
markdownDescription: localize('terminal.integrated.enableSmartPaste', "Whether or not to allow smart paste to automatically wrap file path with double quotes"), |
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.
We dont just wrap the path with ", we just escape the \ if there are no spaces, need a text that's more clear
fix #229280
Added a new terminal feature setting to control the feature, we check for spaces or back slashes and escape or wrap in double-quotes accordingly, the unit tests have been modified accordingly.