Enable drag-and-drop support for multiple files in the terminal#235672
Enable drag-and-drop support for multiple files in the terminal#235672senyai wants to merge 1 commit intomicrosoft:mainfrom
Conversation
|
@microsoft-github-policy-service agree |
senyai
force-pushed
the
senyai-insert-paths
branch
from
a1f9a84 to
adc9fe1
Compare
|
@Tyriar hi! what do you think about this pull request? Should I just wait for it to be reviewed? |
senyai
force-pushed
the
senyai-insert-paths
branch
from
483464c to
9fcb8b7
Compare
|
Is there an issue that this fixes? Normally changes need to be discussed in an issue, it's not certain we would accept such a change and the issue is required to verify the issue before it reaches stable anyway. |
There was a problem hiding this comment.
Pull request overview
This PR adds support for dragging and dropping multiple files into the terminal, where previously only a single file path would be inserted. The implementation updates the drag-and-drop handlers to collect all dropped files and join their paths with spaces when sending to the terminal.
Key Changes
- Refactored drag-and-drop event handlers to collect and process arrays of file URIs instead of single files
- Introduced
sendPaths()method to handle multiple file paths, joining them with spaces - Renamed
sendPath()toexecutePath()to clarify its purpose (sends path with execution)
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| src/vs/workbench/contrib/terminal/browser/terminalTabsList.ts | Updated _handleExternalDrop to collect multiple file URIs from drag events and call sendPaths() |
| src/vs/workbench/contrib/terminal/browser/terminalInstance.ts | Added sendPaths() method, renamed sendPath() to executePath(), updated drag-and-drop controller to emit URI[] arrays |
| src/vs/workbench/contrib/terminal/browser/terminalActions.ts | Updated call from sendPath() to executePath() for running active file |
| src/vs/workbench/contrib/terminal/browser/terminal.ts | Updated interface definitions with new executePath() and sendPaths() method signatures |
| if (resource) { | ||
| resources.push(URI.file(resource)); | ||
| } | ||
|
|
There was a problem hiding this comment.
[nitpick] There's an extra blank line here that should be removed to maintain consistent code formatting.
| executePath(originalPath: URI): Promise<void>; | ||
|
|
||
| /** | ||
| * Sends a paths to the terminal instance, preparing it as needed based on the detected shell |
There was a problem hiding this comment.
Typo in documentation: "Sends a paths" should be "Sends paths" (remove the article "a").
| * Sends a paths to the terminal instance, preparing it as needed based on the detected shell | |
| * Sends paths to the terminal instance, preparing it as needed based on the detected shell |
| * The character(s) added are \n or \r\n, depending on the platform. This defaults to `true`. | ||
| */ | ||
| sendPath(originalPath: string | URI, shouldExecute: boolean): Promise<void>; | ||
| executePath(originalPath: URI): Promise<void>; |
There was a problem hiding this comment.
The interface signature for executePath specifies originalPath: URI, but the implementation in terminalInstance.ts:1350 accepts originalPath: string | URI. This inconsistency could lead to type errors. The interface should match the implementation by accepting string | URI instead of just URI.
| executePath(originalPath: URI): Promise<void>; | |
| executePath(originalPath: string | URI): Promise<void>; |
| * | ||
| * @param originalPaths The paths to send. | ||
| */ | ||
| sendPaths(originalPaths: URI[]): Promise<void>; |
There was a problem hiding this comment.
The interface signature for sendPaths specifies originalPaths: URI[], but the implementation in terminalInstance.ts:1354 accepts originalPaths: (string | URI)[]. This inconsistency could lead to type errors. The interface should match the implementation by accepting (string | URI)[] instead of just URI[].
| sendPaths(originalPaths: URI[]): Promise<void>; | |
| sendPaths(originalPaths: (string | URI)[]): Promise<void>; |
| } | ||
|
|
||
| if (!resource) { | ||
| if (!resources) { |
There was a problem hiding this comment.
The check if (!resources) will not catch the case where resources is an empty array. After the filesystem file loop (lines 782-788), resources could be an empty array [], which would pass this check but fail when calling instance.sendPaths(resources) with an empty array. Consider changing this to if (!resources || resources.length === 0) to properly handle the empty array case.
| if (!resources) { | |
| if (!resources || resources.length === 0) { |
| async sendPaths(originalPaths: (string | URI)[]): Promise<void> { | ||
| const paths = await Promise.all(originalPaths.map((path) => this.preparePathForShell(path))); | ||
| return this.sendText(paths.join(' '), false); | ||
| } |
There was a problem hiding this comment.
The new sendPaths method lacks test coverage. Since terminalInstance.test.ts has comprehensive tests for other TerminalInstance methods, consider adding tests to verify that sendPaths correctly joins multiple paths with spaces and calls sendText with the expected parameters. This would ensure the multi-file drag-and-drop feature works correctly.
There was a problem hiding this comment.
Nobody tests sendPath, so I could figure out how to test sendPaths
senyai
force-pushed
the
senyai-insert-paths
branch
from
9fcb8b7 to
17d289e
Compare
|
@Tyriar Copilot suggest adding tests, and I really like tests, but nobody tests current So, if anyone can provide some guidance, I will really appreciate it! |
|
For tests I recommend adding a |
Drag and dropping multiple files into the terminal has never worked; only one path was inserted. This pull request fixes the issue. This request is very important to me because I like to
git stashselected files. I want to be able to writegit stash --and drop selected files from the SCM view.Please forgive me for not referencing an issue number here.