-
Notifications
You must be signed in to change notification settings - Fork 8.4k
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
Automatically convert paths dropped on WSL instances #11625
Conversation
@check-spelling-bot ReportUnrecognized words, please review:
Previously acknowledged words that are now absentcarlos dpg sid SPACEBAR Unregister Urxvt vcvarsall xIcon yIcon zamoraTo accept these unrecognized words as correct (and remove the previously acknowledged and now absent words), run the following commands... in a clone of the git@github.com:petrsnm/terminal.git repository
✏️ Contributor please read thisBy default the command suggestion will generate a file named based on your commit. That's generally ok as long as you add the file to your commit. Someone can reorganize it later.
If the listed items are:
See the 🔬 You can test your commits without appending to a PR by creating a new branch with that extra change and pushing it to your fork. The check-spelling action will run in response to your push -- it doesn't require an open pull request. By using such a branch, you can limit the number of typos your peers see you make. 😉 🗜️ If you see a bunch of garbageIf it relates to a ... well-formed patternSee if there's a pattern that would match it. If not, try writing one and adding it to a Patterns are Perl 5 Regular Expressions - you can test yours before committing to verify it will match your lines. Note that patterns can't match multiline strings. binary-ish stringPlease add a file path to the File paths are Perl 5 Regular Expressions - you can test yours before committing to verify it will match your files.
|
Wow, this is elegant. It informs the control as to the source of the profile, and then the control will convert the path if it already knows it's a WSL profile. That's pretty neat. It obviously won't work for the "run We had a similar idea in mind for the final resolution to #3158, so I'm cool with this. |
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.
Does this work with paths with spaces in them too? e.x. C:\Program Files (x86)
// Remove \\wsl.localhost from the string (leaving the raw Linux filesystem path) | ||
else if (fullPath.find(L"//wsl.localhost") != std::string::npos) | ||
{ | ||
fullPath.erase(0, fullPath.find('/', fullPath.find('/', fullPath.find('/', fullPath.find('/') + 1) + 1) + 1)); |
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.
uh, wat
If we're looking for the literal //wsl.localhost
string, wouldn't it be easier to just hardcode erase(0, 15)
(+/-1)?
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.
oh the path is \\wsl.localhost\Ubuntu-18.04\foo\bar
, got it, I see now.
Co-authored-by: Leonard Hecker <leonard@hecker.io>
@check-spelling-bot ReportUnrecognized words, please review:
Previously acknowledged words that are now absentcarlos dpg sid SPACEBAR Unregister Urxvt vcvarsall xIcon yIcon zamoraTo accept these unrecognized words as correct (and remove the previously acknowledged and now absent words), run the following commands... in a clone of the git@github.com:petrsnm/terminal.git repository
✏️ Contributor please read thisBy default the command suggestion will generate a file named based on your commit. That's generally ok as long as you add the file to your commit. Someone can reorganize it later.
If the listed items are:
See the 🔬 You can test your commits without appending to a PR by creating a new branch with that extra change and pushing it to your fork. The check-spelling action will run in response to your push -- it doesn't require an open pull request. By using such a branch, you can limit the number of typos your peers see you make. 😉 🗜️ If you see a bunch of garbageIf it relates to a ... well-formed patternSee if there's a pattern that would match it. If not, try writing one and adding it to a Patterns are Perl 5 Regular Expressions - you can test yours before committing to verify it will match your lines. Note that patterns can't match multiline strings. binary-ish stringPlease add a file path to the File paths are Perl 5 Regular Expressions - you can test yours before committing to verify it will match your files.
|
// Fix path for WSL | ||
if (_settings.ProfileSource() == L"Windows.Terminal.Wsl") | ||
{ | ||
static constexpr std::wstring_view wslPathPrefix{L"//wsl.localhost/"}; |
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.
this won't work on windows 10 because the WSL path prefix is //wsl$/
there
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.
In that case we can just run the prefix check twice and pick the std::wstring_view
that matched. 🙂
(Sorry for the surprise comment) What might happen if you have multiple WSL distributions and you copy and paste in a \\wsl.localhost path from a different distribution from the one in the Terminal tab?
|
This isn't a copy/paste fix. Only works for drag and drop. But yes, the drag and drop from \wsl.localhost<distro>\ only works if you're dragging into a terminal of the same distro. If you drag it to a terminal with a different distro, then the path may not exist. Currently (without this PR), none of the paths that you drag and drop will exist in WSL (due to slashes and rooting). |
Sorry, I meant to say drag and drop not copy and paste. Did you intend to close the PR? |
@petrsnm you need to run a |
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.
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.
ugh... looks like I made a commit from a WSL2 console with automount options that fix up file permissions (umask=22, fmask=11). I'll fix the commits |
@DHowett, sorry about the fileMode mess. I was goofing around with WSL automount umask and fmask for another project and didn't notice the filemode change in the commit. Lesson learned. Rather than re-add the files I just committed |
Thanks for working on this, @petrsnm! Sorry for the delay in my reviewing it. I love the direction it's headed, and this is definitely a feature that people will be happy about. I'm a little iffy on the design, however, for one reason: I accept that this design is not obvious if you're jumping into the code out of nowhere, and that we should for sure make sure our documentation mentions our guiding principles. It's also a bit of a pain for a contributor to have to wrap their head around. Because of that, I'm not going to block your pull request over it. If you're not comfortable making those changes, or if me asking is overstaying my "maintainer welcome", or if you feel like you've done enough: what is here is perfectly acceptable and something that we can build on later. Let me know 😄 |
@DHowett, I saw that there was already pre-paste fixup code for quoting paths with spaces in It looks like it would not be that difficult to move the fix to I'm currently running a dev copy of Terminal because this PR makes a huge productivity difference for me. Would it be possible to merge this PR , and then maybe I can come back and move the fix into |
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.
FWIW we're cool merging this as-is. From an "architectural purity" standpoint, we should maybe move some logic around, but that's not something we'd totally expect an external contributor to do. The code is good, and the feature works, I can come back around later and move it 😋 Thanks again for doing this!
Notes moved from PR body: windows.terminal.drag.and.drop.for.wsl.mp4 |
Hello @DHowett! Because this pull request has the p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (
|
Thanks so much for doing this! I'm writing up the overall architecture plan for another comment, but I wanted to make sure I said thanks foremost 😄 |
🎉 Handy links: |
My Terminal has 2 Ubuntu profiles. (I installed the Ubuntu app from Windows Store). This fix works well with the Ubuntu profile that is "hidden" by default (profile with a Tux icon and a command line of I haven't researched the concrete difference between |
Basically, the |
Drag and drop does not work for WSL because paths are pasted as windows
paths having incorrect path separator and path root. This PR adds code
to correct the path in TerminalControl before pasting to WSL terminals.
One problem with this approach is that it assumes the default WSL
automount root of "/mnt". It would be possible to add a setting like
"WslDragAndDropMountRoot"... but I decided it if someone wants to change
automount location it would be simple enough just to create the "/mnt"
symlink in WSL.
Validation
Couldn't find an obvious place to add a test. Manually tested
cut-n-paste from following paths:
Closes #331