implement drag&drop path in '+' button (#10073) #10160

Daniel599 · 2021-05-23T22:19:32Z

Summary of the Pull Request

This PR implements the ability to drop directories/files on the '+' button which in turn will open the tab/pane/window in the given starting path.
In order to do this, I refactored the click's lambda into a method and re-used it
Sadly I wasn't able to add note about the alt/shift feature (any ideas how to do this?)
Also most of the code is "look-a-like" from other places within the project, as I don't have much experience in windows development.

References

implements #10073

PR Checklist

Closes Drag folder/file to '+' button to open new tab in directory #10073
CLA signed. If not, go over here and sign the CLA
Tests added/passed
** tests were done manually both of the old feature (alt/shift+click) on the '+' and on the profiles
Documentation updated. If checked, please file a pull request on our docs repo and link it here: #xxx
** no idea what to add there, if any.
Schema updated.
I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place: #xxx

Detailed Description of the Pull Request / Additional comments

Validation Steps Performed

tested manually.

j4james · 2021-05-23T23:57:51Z

FYI, there's a runformat.cmd script in the tools directory that will apply the correct code formatting to your source, so it'll pass the Code Health check when you push your changes.

Daniel599 · 2021-05-24T05:52:09Z

FYI, there's a runformat.cmd script in the tools directory that will apply the correct code formatting to your source, so it'll pass the Code Health check when you push your changes.

Thanks for the tip, I totally forgot about that script. Hope it will pass now.

skyline75489 · 2021-05-24T06:05:54Z

src/cascadia/TerminalApp/TerminalPage.cpp

+
+            std::wstring pathText = path.wstring();
+
+            // Handle edge case of "C:\\", seems like the "StartingDirectory" doesn't like path which ends with '\'


Wait what? My experience with StartingDirectory tells me this can't be true ...

This edge case happens when you press shift and drop the C: drive on it, you get the following error:

[error 0x8007010b when launching `cmd.exe']
Could not access starting directory "C:\WINDOWS\SysWOW64""

D: drive

[error 0x8007010b when launching `cmd.exe']
Could not access starting directory "D:""

While trying to debug the value of pathText I notice normal folders don't end with \ while C & D drives do.
removing that \ solved the problem...maybe it solved it by avoiding it?
can you please help me understand why it happens?
note that pressing shift is critical since normal drop works just fine.
If you think it should work without this if will trying to debug it further.

skyline75489 · 2021-05-24T06:06:21Z

src/cascadia/TerminalApp/TerminalPage.cpp

+        if (items.Size() == 1)
+        {
+            std::filesystem::path path(items.GetAt(0).Path().c_str());
+            if (!std::filesystem::is_directory(path))


Wonder how well std::filesystem::is_directory handles UNC paths.

tested it with \\HOST\folder and \\HOST\folder\file.txt, std::filesystem::is_directory handles it just fine.
Sadly darg & drop this UNC path only works with windows powershell.
Command Prompt gives the following error:

'\\HOST\folder' CMD.EXE was started with the above path as the current directory. UNC paths are not supported. Defaulting to Windows directory. Microsoft Windows [Version 10.0.19042.985] (c) Microsoft Corporation. All rights reserved. C:\Windows>

while ubuntu just default to the home folder.

skyline75489 · 2021-05-24T06:08:55Z

@Daniel599 Thanks for the contribution! The core developers of the project are probably busy preparing for Build 2021. So it might take a while for them to review this.

Daniel599 · 2021-06-19T13:51:42Z

@skyline75489 any update regarding this PR?

skyline75489 · 2021-06-19T15:44:03Z

@Daniel599 sry about not responding. The engineer who is more eligible to review this is currently out of the office for vacation and won’t have the time until next month. There’s only limited engineering power at the moment. So it might take a while for people to get to this PR.

skyline75489 · 2021-07-13T09:00:59Z

Hi @Daniel599 sorry for not responding for a long time.😅

The team is currently preparing for 1.10 release, and this PR is likely to miss the 1.10 train. Wonder if @zadjii-msft has interest to take a look and probably ship this in 1.11.

miniksa

I'm good with this. Apologies for the delay. We had discussed this at a team meeting and then I think things happened to the major people at that meeting. Like @zadjii-msft going on paternity leave, me visiting my family and then having my son get sick (and then getting sick), and a few other kerfluffles like prepping the 1.10 release.

I'm not the biggest authority on the team at XAML or the Terminal UI... but the code looks fair enough to me and I like the de-duplication of the tab/window opening stuff. So I'll give you one check.

miniksa · 2021-07-15T21:10:17Z

src/cascadia/TerminalApp/TabRowControl.cpp

+    // - Bound in Drag&Drop of the Xaml editor to the [+] button.
+    // Arguments:
+    // <unused>
+    void TabRowControl::OnNewTabButtonDrop(IInspectable const&, winrt::Windows::UI::Xaml::DragEventArgs const&)


Do we have to make and bind this if nothing is going to happen? Is this some XAML trickery?

I think so, I did copy-cat of the other Drag&Drop stuff.
I`ll try to remove it and see what happens

Whichever way it works, I'm fine with it. It was just an interesting curiosity to me.

…/alt+ drop, by refactor the click lambdas into a function

Daniel599 · 2021-07-17T18:43:01Z

I'm good with this. Apologies for the delay. We had discussed this at a team meeting and then I think things happened to the major people at that meeting. Like @zadjii-msft going on paternity leave, me visiting my family and then having my son get sick (and then getting sick), and a few other kerfluffles like prepping the 1.10 release.

I'm not the biggest authority on the team at XAML or the Terminal UI... but the code looks fair enough to me and I like the de-duplication of the tab/window opening stuff. So I'll give you one check.

Thank you for the update, hope you are feeling well now.
I did rebase and now the checks don't pass, any idea why? seems like "unit tests" step but its output is too huge to understand.

zadjii-msft · 2021-07-19T21:27:22Z

sorry for the delay, I'm checking out the code locally right now to give it a review. Probably won't get a full build before EOD but hopefully will early tomorrow.

skyline75489 · 2021-07-20T10:30:00Z

Ah again the ApiRoutinesTests::ApiGetConsoleOriginalTitleA failure. @Daniel599 it's not your fault. There's a PR to fix this.

zadjii-msft

Okay, sorry for the delay reviewing this. The code is nice and straightforward, and I like that it stealthily also works with alt+drag&drop.

I am seeing weird edge cases with C:\, but you know what, I'm just gonna merge this for now and file some follow ups.

Thanks for doing this!

ghost · 2021-07-20T14:05:48Z

Hello @zadjii-msft!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

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 (`@msftbot`) and give me an instruction to get started! Learn more here.

As discussed in team sync. Is this a mysterious dark pattern we didn't know about? * [x] closes #10721 * [x] I work here * [x] doesn't need tests * [x] doesn't need docs * see also #10160

ghost · 2021-08-31T17:14:00Z

🎉Windows Terminal Preview v1.11.2421.0 has been released which incorporates this pull request.:tada:

Handy links:

ghost added Area-UserInterface Issues pertaining to the user interface of the Console or Terminal Issue-Task It's a feature request, but it doesn't really need a major design. Product-Terminal The new Windows Terminal. labels May 23, 2021

skyline75489 reviewed May 24, 2021

View reviewed changes

zadjii-msft added the Needs-Discussion Something that requires a team discussion before we can proceed label Jul 15, 2021

miniksa approved these changes Jul 15, 2021

View reviewed changes

Daniel599 added 2 commits July 17, 2021 18:56

implement drag&drop path in '+' button (#10073) with support of shift…

c9f63e7

…/alt+ drop, by refactor the click lambdas into a function

ran runformat.cmd

f637f1b

zadjii-msft removed the Needs-Discussion Something that requires a team discussion before we can proceed label Jul 19, 2021

zadjii-msft approved these changes Jul 20, 2021

View reviewed changes

zadjii-msft added the AutoMerge Marked for automatic merge by the bot when requirements are met label Jul 20, 2021

ghost merged commit b05a557 into microsoft:main Jul 20, 2021

zadjii-msft mentioned this pull request Jul 20, 2021

Add tracelogging for drag&drop on the new tab button #10726

Merged

4 tasks

ghost mentioned this pull request Aug 31, 2021

Drag folder/file to '+' button to open new tab in directory #10073

Closed

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

implement drag&drop path in '+' button (#10073) #10160

implement drag&drop path in '+' button (#10073) #10160

Daniel599 commented May 23, 2021

j4james commented May 23, 2021

Daniel599 commented May 24, 2021

skyline75489 May 24, 2021

Daniel599 May 24, 2021

skyline75489 May 24, 2021

Daniel599 Jun 5, 2021

skyline75489 commented May 24, 2021

Daniel599 commented Jun 19, 2021

skyline75489 commented Jun 19, 2021

skyline75489 commented Jul 13, 2021

miniksa left a comment •

edited

Loading

miniksa Jul 15, 2021

Daniel599 Jul 16, 2021

miniksa Jul 16, 2021

Daniel599 commented Jul 17, 2021

zadjii-msft commented Jul 19, 2021

skyline75489 commented Jul 20, 2021

zadjii-msft left a comment

ghost commented Jul 20, 2021

ghost commented Aug 31, 2021


		std::wstring pathText = path.wstring();

		// Handle edge case of "C:\\", seems like the "StartingDirectory" doesn't like path which ends with '\'

implement drag&drop path in '+' button (#10073) #10160

implement drag&drop path in '+' button (#10073) #10160

Conversation

Daniel599 commented May 23, 2021

Summary of the Pull Request

References

PR Checklist

Detailed Description of the Pull Request / Additional comments

Validation Steps Performed

j4james commented May 23, 2021

Daniel599 commented May 24, 2021

skyline75489 May 24, 2021

Choose a reason for hiding this comment

Daniel599 May 24, 2021

Choose a reason for hiding this comment

skyline75489 May 24, 2021

Choose a reason for hiding this comment

Daniel599 Jun 5, 2021

Choose a reason for hiding this comment

skyline75489 commented May 24, 2021

Daniel599 commented Jun 19, 2021

skyline75489 commented Jun 19, 2021

skyline75489 commented Jul 13, 2021

miniksa left a comment • edited Loading

Choose a reason for hiding this comment

miniksa Jul 15, 2021

Choose a reason for hiding this comment

Daniel599 Jul 16, 2021

Choose a reason for hiding this comment

miniksa Jul 16, 2021

Choose a reason for hiding this comment

Daniel599 commented Jul 17, 2021

zadjii-msft commented Jul 19, 2021

skyline75489 commented Jul 20, 2021

zadjii-msft left a comment

Choose a reason for hiding this comment

ghost commented Jul 20, 2021

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 (@msftbot) and give me an instruction to get started! Learn more here.

ghost commented Aug 31, 2021

miniksa left a comment •

edited

Loading

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 (`@msftbot`) and give me an instruction to get started! Learn more here.