Skip to content
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 single quotes to WSL drag and drop #16214

Merged
merged 2 commits into from Oct 24, 2023

Conversation

js324
Copy link
Contributor

@js324 js324 commented Oct 23, 2023

Wrap single quotes to drag and dropped paths in WSL

References and Relevant Issues

#15646 , #8109

Detailed Description of the Pull Request / Additional comments

First time contributor, from what I understand from reading #15646 and #8109 , issue is asking for single quotes added to a drag and dropped path always, regardless of whitespace and special characters, in WSL.

Validation Steps Performed

Tested drag and drop changes in WSL and non WSL sources.

Closes #15646

@microsoft-github-policy-service microsoft-github-policy-service bot added Issue-Bug It either shouldn't be doing this or needs an investigation. Area-User Interface Issues pertaining to the user interface of the Console or Terminal Priority-3 A description (P3) Product-Terminal The new Windows Terminal. labels Oct 23, 2023
@js324 js324 marked this pull request as ready for review October 23, 2023 05:32
Comment on lines 2943 to 2944
fullPath.insert(0, L"\'");
fullPath += L"\'";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you don't mind doing it in this PR, the below code is kinda whack and could need an improvement. .insert(0, L"\""); is an O(n) operation that is entirely avoidable, because the fullPath is being appended to allPathsString anyways, so we can instead just append the quotes to allPathsString directly. Here's what I mean:

const auto isWSL = _interactivity.ManglePathsForWsl();

if (isWSL)
{
    // ...
    // the above WSL specific code block but without your new addition
    // ...
}

const auto quotesNeeded = isWSL || fullPath.find(L' ') != decltype(fullPath)::npos;
const auto quotesChar = isWSL ? L'\'' : L'"';

// Append fullPath and also wrap it in quotes if needed
if (needsQuotes)
{
    allPathsString.push_back(quotesChar);
}
allPathsString.append(fullPath);
if (needsQuotes)
{
    allPathsString.push_back(quotesChar);
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the feedback! Implemented and tested the changes

@js324
Copy link
Contributor Author

js324 commented Oct 24, 2023

@microsoft-github-policy-service agree

Copy link
Member

@carlos-zamora carlos-zamora left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for doing this!

@carlos-zamora carlos-zamora merged commit d0d3039 into microsoft:main Oct 24, 2023
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-User Interface Issues pertaining to the user interface of the Console or Terminal Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-3 A description (P3) Product-Terminal The new Windows Terminal.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Drag & drop in WSL should use single quotes
3 participants