Set DragData when directory is dragged - #243656
Conversation
Previously, it was not set for directory
|
Couldn't reproduce on the latest version. |
Are you using ubuntu ? |
|
I'm using Windows 11, what's the directory structure ? I created a folder and dragged it to an md file, it appeared fine. |
|
Ted Vu (@TedVu) The issue occurs on Ubuntu but works fine on Windows. When dragging a directory into a Markdown file, it inserts the absolute path instead of the relative path and does not generate a link. |
| const { selection, uri } = extractSelection(resourceOrEditor); | ||
| editor = { resource: uri, options: selection ? { selection } : undefined }; | ||
| } else if (!resourceOrEditor.isDirectory) { | ||
| } else { |
There was a problem hiding this comment.
A lot of the code below assumes that resource will be a file, not a directory
Instead I think this check should happen down where we set the Mimes.uriList data. That logic should be updated to also include directories in the url-list
There was a problem hiding this comment.
Sure, will update accordingly.
Can you describe some scenarios, through which I can test the change?
There was a problem hiding this comment.
It should work the same as with the PR but using a more correct data-transfer type instead of the editors type. That could potentially result in VS Code trying to open a folder as an editor, while url-list should work for any type of file or folder
There was a problem hiding this comment.
Got it. 👍
Thank you for your prompt reply.
There was a problem hiding this comment.
I reviewed the code in detail and found that the flow is identical for both files and directories. To ensure that the if (draggedEditors.length) condition is met (which is required for setting the MIME type), we need to add the editor to the draggedEditors list.
Additionally, textFileModel is undefined for both files and directories:
vscode/src/vs/workbench/browser/dnd.ts
Lines 291 to 292 in 8785777
The most reasonable approach seems to be adding a separate condition for directories, like this:
} else if (!resourceOrEditor.isDirectory) {
editor = {
resource: resourceOrEditor.resource,
options: { selection: resourceOrEditor.selection }
};
} else if (resourceOrEditor.isDirectory) {
editor = {
resource: resourceOrEditor.resource,
options: { selection: undefined } // or resourceOrEditor.selection
};
}Since this is my first contribution to this repo, I’d appreciate any guidance on whether this approach aligns with the expected changes or if there are additional considerations I should address.
There was a problem hiding this comment.
It may be easier to add a new draggedDirectories variable to go with with draggedEditors. A directory should never end up in draggedEditors
Then update the code where we build the uriListEntries to loop through both the draggedEditors and draggedDirectories
There was a problem hiding this comment.
Sure, will update the code soon.
Head branch was pushed to by a user without write access
|
|
||
| // Text: allows to paste into text-capable areas | ||
| const lineDelimiter = isWindows ? '\r\n' : '\n'; | ||
| event.dataTransfer.setData(DataTransfers.TEXT, fileSystemResources.map(({ resource }) => labelService.getUriLabel(resource, { noPrefix: true })).join(lineDelimiter)); |
There was a problem hiding this comment.
We want to write both files and dirs in this case
It looks like maybe a few too many lines got touch in this change. Maybe instead of adding draggedDirectories and draggedFiles, you can make the fix clearer by focusing on the part below where we built the url-list. You can use use fileSystemResources to get the directories and combine those with the dragged editors. Something like:
if (draggedEditors.length) {
event.dataTransfer.setData(CodeDataTransfers.EDITORS, stringify(draggedEditors));
}
const allDraggedResources = [
...draggedEditors.map(editor => resource),
...fileSystemResources.filter(({ isDirectory }) => isDirectory).map(dir => dir.resource)
]
if (allDraggedResources.length) {
// set url-list
}
Matt Bierner (mjbvz)
left a comment
Matt Bierner (mjbvz)
left a comment
There was a problem hiding this comment.
Just a minor note but otherwise looks good. Thanks for following up!
|
|
||
| const draggedDirectories: URI[] = [ | ||
| ...fileSystemResources.filter(({ isDirectory }) => isDirectory).map(({ resource }) => resource), | ||
| ]; |
There was a problem hiding this comment.
Since filter creates a copy of the array, you can simplify this to: const draggedDirectories = fileSystemResources .filter(...) without using spread
There was a problem hiding this comment.
Sure.
It is required to show the markdown link for directory
| const draggedDirectories: URI[] = fileSystemResources.filter(({ isDirectory }) => isDirectory).map(({ resource }) => resource); | ||
|
|
||
| if (draggedEditors.length || draggedDirectories.length) { | ||
| event.dataTransfer.setData(CodeDataTransfers.EDITORS, stringify([...draggedEditors, ...draggedDirectories])); |
There was a problem hiding this comment.
If we don't set directories here, it does not solve the bug.
|
Matt Bierner (@mjbvz) I have updated the PR based on the requested changes. Please review it when you have time. |
|
Muhammad Khuzaima Umair (@mkhuzaima) I think there was a bug with how |
4 participants
Fixes #241007
Bug
drag-directory-issue.webm
After Fix
drag-directory-fix.webm