Replace Windows.Storage.Pickers with Common File Dialogs by DHowett · Pull Request #9760 · microsoft/terminal

DHowett · 2021-04-09T17:56:22Z

Using Pickers from an elevated application yields an
ERROR_ACCESS_DENIED. Of course it does: it was designed for the modern
app platform.

Using the common dialog infrastructure has some downsides¹, but it
doesn't crash and is just as flexible.

I've added some fun templated functions that help us with the
complexity.

Fixes #8957

¹You've got to use raw COM, and it runs in-proc instead of out-of-proc.

Validation Steps Performed

I tested every picker.

Using Pickers from an elevated application yields an ERROR_ACCESS_DENIED. Of course it does: it was designed for the modern app platform. Using the common dialog infrastructure has some downsides¹, but it doesn't crash and is just as flexible. Fixes #8957 ¹ You've got to use raw COM, and it runs in-proc instead of out-of-proc.

DHowett · 2021-04-09T17:59:40Z

src/cascadia/TerminalSettingsEditor/Profiles.cpp

+static winrt::Windows::Foundation::IAsyncOperation<winrt::hstring> OpenImagePicker()
+{
+    static constexpr COMDLG_FILTERSPEC supportedImageFileTypes[] = {
+        { L"All Supported Bitmap Types (*.jpg, *.jpeg, *.png, *.bmp, *.gif, *.tiff, *.ico)", L"*.jpg;*.jpeg;*.png;*.bmp;*.gif;*.tiff;*.ico" },


The previous dialog didn't even have this bit of text, so I'm going to elave this unlocalized for now.

follow-up issue #?

(same with "all files" below)

It is my suspicion that this text does not need to be localized, as it is a file picker that filters file types and the *.bmp, *.jpg is directly understandable by anyone who might try to read the string.

DHowett · 2021-04-09T18:00:01Z

src/cascadia/TerminalSettingsEditor/Profiles.cpp

+
+    static constexpr winrt::guid clientGuidImagePicker{ 0x55675F54, 0x74A1, 0x4552, { 0xA3, 0x9D, 0x94, 0xAE, 0x85, 0xD8, 0xF2, 0x7A } };
+    return OpenFilePicker([](auto&& dialog) {
+        THROW_IF_FAILED(dialog->SetClientGuid(clientGuidImagePicker));


the client GUID is how it remembers the location, so we can have it remember different locations for different types of box!

carlos-zamora

Small things. I don't want to hold up this PR, so I'm ok with follow-up work items on this.

carlos-zamora · 2021-04-09T18:06:25Z

src/cascadia/TerminalSettingsEditor/Profiles.cpp

+template<typename TLambda>
+static winrt::Windows::Foundation::IAsyncOperation<winrt::hstring> OpenFilePicker(TLambda&& customize)


Should we put a comment here linking 8957 that says why we shouldn't use Windows::Storage::Pickers::FileOpenPicker?

I guess. We can also simply remain vigilant on PRs that attempt to add it back :p

carlos-zamora · 2021-04-09T18:12:45Z

src/cascadia/TerminalSettingsEditor/Profiles.cpp

+static winrt::Windows::Foundation::IAsyncOperation<winrt::hstring> OpenImagePicker()
+{
+    static constexpr COMDLG_FILTERSPEC supportedImageFileTypes[] = {
+        { L"All Supported Bitmap Types (*.jpg, *.jpeg, *.png, *.bmp, *.gif, *.tiff, *.ico)", L"*.jpg;*.jpeg;*.png;*.bmp;*.gif;*.tiff;*.ico" },


follow-up issue #?

carlos-zamora · 2021-04-09T18:16:39Z

src/cascadia/TerminalSettingsEditor/Profiles.cpp

+static winrt::Windows::Foundation::IAsyncOperation<winrt::hstring> OpenImagePicker()
+{
+    static constexpr COMDLG_FILTERSPEC supportedImageFileTypes[] = {
+        { L"All Supported Bitmap Types (*.jpg, *.jpeg, *.png, *.bmp, *.gif, *.tiff, *.ico)", L"*.jpg;*.jpeg;*.png;*.bmp;*.gif;*.tiff;*.ico" },


(same with "all files" below)

src/cascadia/TerminalSettingsEditor/Profiles.cpp

carlos-zamora · 2021-04-09T18:21:13Z

src/cascadia/TerminalSettingsEditor/Profiles.cpp

    {
        auto lifetime = get_strong();
-        FolderPicker picker;
-        _State.WindowRoot().TryPropagateHostingWindow(picker); // if we don't do this, there's no HWND for it to attach to


Can we revert TryPropagateHostingWindow? PR #8391

Happy to revert it for 1.9, but not for 1.8 or 1.7.

carlos-zamora · 2021-04-09T18:25:05Z

Also, you tested this in as Admin, right?

DHowett · 2021-04-09T18:50:38Z

Also, you tested this in as Admin, right?

It would not have been a worthwhile change had I not tested it 😉

Co-authored-by: Carlos Zamora <[email protected]>

zadjii-msft

I'm only holding the approval to make sure we don't end up blocking the UI thread entirely (or if we do, then it's intentional)

zadjii-msft · 2021-04-12T09:41:24Z

src/cascadia/TerminalSettingsEditor/Profiles.cpp

-
-        StorageFile file = co_await picker.PickSingleFileAsync();
-        if (file != nullptr)
+        auto file = co_await OpenImagePicker();


Do we need to hop to the background thread before or during this co_await? Or does the IFileDialog do that automagically?

It's actually the co_await that splits off to the right thread! However: we actually do want to block the UI thread- the dialog is naturally a UI thing, and we don't want the user navigating to another profile page while the dialog is up. Yes, that sucks, but I don't believe this is a difference from Pickers.

Oh, wait really? I always assumed that until you resume_* then you're still on the main thread.

I guess blocking the UI thread is in fact the right behavior, I guess I just didn't want the window to do the "Window unresponsive" thing, but I guess Windows knows how to deal with that already.

zadjii-msft · 2021-04-12T09:44:08Z

Morbid curiosity: will this end up changing the CWD of wt.exe, such that commandlines with "startingDirectory": "." set would behave differently after using this dialog?

DHowett · 2021-04-12T12:32:59Z

CWD: we actually have disabled the current directory change feature that's built into the dialogs using FOS_NOCHANGEsomething

ghost · 2021-04-12T13:11:49Z

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.

This is required to maintain the modality of the dialogs, which we lost when we moved from Pickers to IFileDialog. The HWND hosting Window API we dreamed up is incompatible with IModalDialog, because IModalDialog requires the HWND immediately upon `Show`. We're smuggling it in a uint64, as is tradition. zadjii-msft noticed this in #9760.

Using Pickers from an elevated application yields an ERROR_ACCESS_DENIED. Of course it does: it was designed for the modern app platform. Using the common dialog infrastructure has some downsides¹, but it doesn't crash and is just as flexible. I've added some fun templated functions that help us with the complexity. Fixes #8957 ¹You've got to use raw COM, and it runs in-proc instead of out-of-proc. I tested every picker. (cherry picked from commit 959c423)

This is required to maintain the modality of the dialogs, which we lost when we moved from Pickers to IFileDialog. The HWND hosting Window API we dreamed up is incompatible with IModalDialog, because IModalDialog requires the HWND immediately upon `Show`. We're smuggling it in a uint64, as is tradition. zadjii-msft noticed this in #9760. (cherry picked from commit 8f79f7c)

ghost · 2021-04-14T17:03:33Z

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

Handy links:

ghost · 2021-04-14T17:06:54Z

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

Handy links:

Just like in #9760, we can't actually use the UWP file picker API, because it will absolutely not work at all when the Terminal is running elevated. That would prevent the picker from appearing at all. So instead, we'll just use the shell32 one manually. This also gets rid of the confirmation dialog, since the team felt we didn't really need that. We could maybe replace it with a Toast (#8592), but _meh_ * [x] closes #11356 * [x] closes #11358 * This is a lot like #9760 * introduced in #11062 * megathread: #9700

ghost added Area-SettingsUI Anything specific to the SUI Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-1 A description (P1) Product-Terminal The new Windows Terminal. Severity-Crash Crashes are real bad news. labels Apr 9, 2021

DHowett requested review from carlos-zamora and zadjii-msft and removed request for carlos-zamora April 9, 2021 17:56

DHowett commented Apr 9, 2021

View reviewed changes

This comment has been minimized.

Sign in to view

and, spell

d5e4198

carlos-zamora approved these changes Apr 9, 2021

View reviewed changes

DHowett added the zPreview-Service-Consider label Apr 9, 2021

Update src/cascadia/TerminalSettingsEditor/Profiles.cpp

e989005

Co-authored-by: Carlos Zamora <[email protected]>

zadjii-msft reviewed Apr 12, 2021

View reviewed changes

zadjii-msft approved these changes Apr 12, 2021

View reviewed changes

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

ghost merged commit 959c423 into main Apr 12, 2021

ghost deleted the dev/duhowett/pickers branch April 12, 2021 13:12

DHowett mentioned this pull request Apr 12, 2021

Propagate the hosting HWND to the new IFileDialogs #9789

Merged

DHowett removed the zPreview-Service-Consider label Apr 13, 2021

ghost mentioned this pull request Apr 14, 2021

Browse buttons crash when running as admin #8957

Closed

zadjii-msft mentioned this pull request Aug 30, 2021

Allow exporting terminal buffer into file via tab context menu #11062

Merged

zadjii-msft mentioned this pull request Sep 29, 2021

Replace the UWP file export with the shell32 one #11365

Merged

2 tasks

AndromedaMelody mentioned this pull request Dec 16, 2022

Use new folder browser M2Team/NanaZip#251

Closed

This pull request was closed.

		template<typename TLambda>
		static winrt::Windows::Foundation::IAsyncOperation<winrt::hstring> OpenFilePicker(TLambda&& customize)

Conversation

DHowett commented Apr 9, 2021

Validation Steps Performed

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

carlos-zamora left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

carlos-zamora commented Apr 9, 2021

Uh oh!

DHowett commented Apr 9, 2021

Uh oh!

zadjii-msft left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zadjii-msft commented Apr 12, 2021

Uh oh!

DHowett commented Apr 12, 2021

Uh oh!

ghost commented Apr 12, 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.

Uh oh!

ghost commented Apr 14, 2021

Uh oh!

ghost commented Apr 14, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

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.