-
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
Replace Windows.Storage.Pickers with Common File Dialogs #9760
Conversation
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.
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" }, |
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.
The previous dialog didn't even have this bit of text, so I'm going to elave this unlocalized for now.
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.
follow-up issue #?
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.
(same with "all files" below)
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.
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.
|
||
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)); |
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.
the client GUID is how it remembers the location, so we can have it remember different locations for different types of box!
This comment has been minimized.
This comment has been minimized.
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.
Small things. I don't want to hold up this PR, so I'm ok with follow-up work items on this.
template<typename TLambda> | ||
static winrt::Windows::Foundation::IAsyncOperation<winrt::hstring> OpenFilePicker(TLambda&& customize) |
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.
Should we put a comment here linking 8957 that says why we shouldn't use Windows::Storage::Pickers::FileOpenPicker
?
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.
I guess. We can also simply remain vigilant on PRs that attempt to add it back :p
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" }, |
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.
follow-up issue #?
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" }, |
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.
(same with "all files" below)
} | ||
} | ||
|
||
fire_and_forget Profiles::StartingDirectory_Click(IInspectable const&, RoutedEventArgs const&) | ||
{ | ||
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 |
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.
Can we revert TryPropagateHostingWindow
? PR #8391
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.
Happy to revert it for 1.9, but not for 1.8 or 1.7.
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 <carlos.zamora@microsoft.com>
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.
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)
|
||
StorageFile file = co_await picker.PickSingleFileAsync(); | ||
if (file != nullptr) | ||
auto file = co_await OpenImagePicker(); |
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.
Do we need to hop to the background thread before or during this co_await
? Or does the IFileDialog do that automagically?
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.
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.
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, 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.
Morbid curiosity: will this end up changing the CWD of |
CWD: we actually have disabled the current directory change feature that's built into the dialogs using FOS_NOCHANGEsomething |
Hello @zadjii-msft! 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 (
|
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)
🎉 Handy links: |
🎉 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
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.