Skip to content

Replace Windows.Storage.Pickers with Common File Dialogs #9760

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.

Sign up for GitHub

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

Merged
3 commits merged into from
Apr 12, 2021
Merged

Replace Windows.Storage.Pickers with Common File Dialogs #9760

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
92a9d77
Replace Windows.Storage.Pickers with Common File Dialogs
DHowett Apr 9, 2021
d5e4198
and, spell
DHowett Apr 9, 2021
e989005
Update src/cascadia/TerminalSettingsEditor/Profiles.cpp
DHowett Apr 9, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 9 additions & 2 deletions .github/actions/spelling/dictionary/apis.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,15 +6,19 @@ bitfields
CLASSNOTAVAILABLE
cmdletbinding
COLORPROPERTY
COMDLG
CXICON
CYICON
D2DERR_SHADER_COMPILE_FAILED
dataobject
DERR
dlldata
DONTADDTORECENT
environstrings
EXPCMDFLAGS
EXPCMDSTATE
FILTERSPEC
FORCEFILESYSTEM
FORCEMINIMIZE
frac
fullkbd
Expand All @@ -32,12 +36,13 @@ IAsync
IBind
IBox
IClass
IConnection
IComparable
IConnection
ICustom
IDialog
IDirect
IExplorer
IFile
IInheritable
IMap
IObject
Expand All @@ -63,6 +68,7 @@ NCLBUTTONDBLCLK
NCRBUTTONDBLCLK
NOAGGREGATION
NOASYNC
NOCHANGEDIR
NOPROGRESS
NOREDIRECTIONBITMAP
ntprivapi
Expand All @@ -72,10 +78,11 @@ otms
OUTLINETEXTMETRICW
overridable
PAGESCROLL
PICKFOLDERS
pmr
REGCLS
RETURNCMD
REGCLS
RETURNCMD
rfind
roundf
RSHIFT
Expand Down
1 change: 1 addition & 0 deletions .github/actions/spelling/expect/expect.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2397,6 +2397,7 @@ titlebar
TITLEISLINKNAME
TJson
tl
TLambda
TLEN
Tlg
Tlgdata
Expand Down
6 changes: 6 additions & 0 deletions src/cascadia/TerminalSettingsEditor/Microsoft.Terminal.Settings.Editor.vcxproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -300,6 +300,12 @@
</Reference>
</ItemGroup>

<ItemDefinitionGroup>
<Link>
<AdditionalDependencies>shell32.lib;%(AdditionalDependencies)</AdditionalDependencies>
</Link>
</ItemDefinitionGroup>

<Import Project="$(OpenConsoleDir)src\cppwinrt.build.post.props" />
<Import Project="..\..\..\packages\Microsoft.UI.Xaml.2.5.0-prerelease.201202003\build\native\Microsoft.UI.Xaml.targets" Condition="Exists('..\..\..\packages\Microsoft.UI.Xaml.2.5.0-prerelease.201202003\build\native\Microsoft.UI.Xaml.targets')" />
<Target Name="EnsureNuGetPackageBuildImports" BeforeTargets="PrepareForBuild">
Expand Down
147 changes: 101 additions & 46 deletions src/cascadia/TerminalSettingsEditor/Profiles.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,16 +15,71 @@ using namespace winrt::Windows::UI::Xaml::Data;
using namespace winrt::Windows::UI::Xaml::Navigation;
using namespace winrt::Windows::Foundation;
using namespace winrt::Windows::Foundation::Collections;
using namespace winrt::Windows::Storage;
using namespace winrt::Windows::Storage::AccessCache;
using namespace winrt::Windows::Storage::Pickers;
using namespace winrt::Microsoft::Terminal::Settings::Model;

static const std::array<winrt::guid, 2> InBoxProfileGuids{
winrt::guid{ 0x61c54bbd, 0xc2c6, 0x5271, { 0x96, 0xe7, 0x00, 0x9a, 0x87, 0xff, 0x44, 0xbf } }, // Windows Powershell
winrt::guid{ 0x0caa0dad, 0x35be, 0x5f56, { 0xa8, 0xff, 0xaf, 0xce, 0xee, 0xaa, 0x61, 0x01 } } // Command Prompt
};

// Function Description:
// - This function presents a File Open "common dialog" and returns its selected file asynchronously.
// Parameters:
// - customize: A lambda that receives an IFileDialog* to customize.
// Return value:
// (async) path to the selected item.
template<typename TLambda>
static winrt::Windows::Foundation::IAsyncOperation<winrt::hstring> OpenFilePicker(TLambda&& customize)
Comment on lines +31 to +32
Copy link
Member

@carlos-zamora carlos-zamora Apr 9, 2021

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?

Copy link
Member Author

@DHowett DHowett Apr 9, 2021

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

{
auto fileDialog{ winrt::create_instance<IFileDialog>(CLSID_FileOpenDialog) };
DWORD flags{};
THROW_IF_FAILED(fileDialog->GetOptions(&flags));
THROW_IF_FAILED(fileDialog->SetOptions(flags | FOS_FORCEFILESYSTEM | FOS_NOCHANGEDIR | FOS_DONTADDTORECENT)); // filesystem objects only; no recent places
customize(fileDialog.get());

auto hr{ fileDialog->Show(NULL) };
if (!SUCCEEDED(hr))
{
if (hr == HRESULT_FROM_WIN32(ERROR_CANCELLED))
{
co_return winrt::hstring{};
}
THROW_HR(hr);
}

winrt::com_ptr<IShellItem> result;
THROW_IF_FAILED(fileDialog->GetResult(result.put()));

wil::unique_cotaskmem_string filePath;
THROW_IF_FAILED(result->GetDisplayName(SIGDN_FILESYSPATH, &filePath));

co_return winrt::hstring{ filePath.get() };
}

// Function Description:
// - Helper that opens a file picker pre-seeded with image file types.
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" },
Copy link
Member Author

@DHowett DHowett Apr 9, 2021

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.

Copy link
Member

@carlos-zamora carlos-zamora Apr 9, 2021

Choose a reason for hiding this comment

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

follow-up issue #?

Copy link
Member

@carlos-zamora carlos-zamora Apr 9, 2021

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)

Copy link
Member Author

@DHowett DHowett Apr 9, 2021

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.

{ L"All Files (*.*)", L"*.*" }
};

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));
Copy link
Member Author

@DHowett DHowett Apr 9, 2021

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!

try
{
auto pictureFolderShellItem{ winrt::capture<IShellItem>(&SHGetKnownFolderItem, FOLDERID_PicturesLibrary, KF_FLAG_DEFAULT, nullptr) };
dialog->SetDefaultFolder(pictureFolderShellItem.get());
}
CATCH_LOG(); // non-fatal
THROW_IF_FAILED(dialog->SetFileTypes(ARRAYSIZE(supportedImageFileTypes), supportedImageFileTypes));
THROW_IF_FAILED(dialog->SetFileTypeIndex(1)); // the array is 1-indexed
THROW_IF_FAILED(dialog->SetDefaultExtension(L"jpg;jpeg;png;bmp;gif;tiff;ico"));
});
}

namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
{
Windows::Foundation::Collections::IObservableVector<Editor::Font> ProfileViewModel::_MonospaceFontList{ nullptr };
Expand Down Expand Up @@ -582,74 +637,74 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
{
auto lifetime = get_strong();

FileOpenPicker picker;

_State.WindowRoot().TryPropagateHostingWindow(picker); // if we don't do this, there's no HWND for it to attach to
picker.ViewMode(PickerViewMode::Thumbnail);
picker.SuggestedStartLocation(PickerLocationId::PicturesLibrary);

// Converted into a BitmapImage. This list of supported image file formats is from BitmapImage documentation
// https://docs.microsoft.com/en-us/uwp/api/Windows.UI.Xaml.Media.Imaging.BitmapImage?view=winrt-19041#remarks
picker.FileTypeFilter().ReplaceAll({ L".jpg", L".jpeg", L".png", L".bmp", L".gif", L".tiff", L".ico" });

StorageFile file = co_await picker.PickSingleFileAsync();
if (file != nullptr)
auto file = co_await OpenImagePicker();
Copy link
Member

@zadjii-msft zadjii-msft Apr 12, 2021

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?

Copy link
Member Author

@DHowett DHowett Apr 12, 2021

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.

Copy link
Member

@zadjii-msft zadjii-msft Apr 12, 2021

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.

if (!file.empty())
{
_State.Profile().BackgroundImagePath(file.Path());
_State.Profile().BackgroundImagePath(file);
}
}

fire_and_forget Profiles::Icon_Click(IInspectable const&, RoutedEventArgs const&)
{
auto lifetime = get_strong();

FileOpenPicker picker;

_State.WindowRoot().TryPropagateHostingWindow(picker); // if we don't do this, there's no HWND for it to attach to
picker.ViewMode(PickerViewMode::Thumbnail);
picker.SuggestedStartLocation(PickerLocationId::PicturesLibrary);

// Converted into a BitmapIconSource. This list of supported image file formats is from BitmapImage documentation
// https://docs.microsoft.com/en-us/uwp/api/Windows.UI.Xaml.Media.Imaging.BitmapImage?view=winrt-19041#remarks
picker.FileTypeFilter().ReplaceAll({ L".jpg", L".jpeg", L".png", L".bmp", L".gif", L".tiff", L".ico" });

StorageFile file = co_await picker.PickSingleFileAsync();
if (file != nullptr)
auto file = co_await OpenImagePicker();
if (!file.empty())
{
_State.Profile().Icon(file.Path());
_State.Profile().Icon(file);
}
}

fire_and_forget Profiles::Commandline_Click(IInspectable const&, RoutedEventArgs const&)
{
auto lifetime = get_strong();

FileOpenPicker picker;
static constexpr COMDLG_FILTERSPEC supportedFileTypes[] = {
{ L"Executable Files (*.exe, *.cmd, *.bat)", L"*.exe;*.cmd;*.bat" },
{ L"All Files (*.*)", L"*.*" }
};

_State.WindowRoot().TryPropagateHostingWindow(picker); // if we don't do this, there's no HWND for it to attach to
picker.ViewMode(PickerViewMode::Thumbnail);
picker.SuggestedStartLocation(PickerLocationId::ComputerFolder);
picker.FileTypeFilter().ReplaceAll({ L".bat", L".exe", L".cmd" });
static constexpr winrt::guid clientGuidExecutables{ 0x2E7E4331, 0x0800, 0x48E6, { 0xB0, 0x17, 0xA1, 0x4C, 0xD8, 0x73, 0xDD, 0x58 } };
auto path = co_await OpenFilePicker([](auto&& dialog) {
THROW_IF_FAILED(dialog->SetClientGuid(clientGuidExecutables));
try
{
auto folderShellItem{ winrt::capture<IShellItem>(&SHGetKnownFolderItem, FOLDERID_ComputerFolder, KF_FLAG_DEFAULT, nullptr) };
dialog->SetDefaultFolder(folderShellItem.get());
}
CATCH_LOG(); // non-fatal
THROW_IF_FAILED(dialog->SetFileTypes(ARRAYSIZE(supportedFileTypes), supportedFileTypes));
THROW_IF_FAILED(dialog->SetFileTypeIndex(1)); // the array is 1-indexed
THROW_IF_FAILED(dialog->SetDefaultExtension(L"exe;cmd;bat"));
});

StorageFile file = co_await picker.PickSingleFileAsync();
if (file != nullptr)
if (!path.empty())
{
_State.Profile().Commandline(file.Path());
_State.Profile().Commandline(path);
}
}

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
Copy link
Member

@carlos-zamora carlos-zamora Apr 9, 2021

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

Copy link
Member Author

@DHowett DHowett Apr 9, 2021

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.

picker.SuggestedStartLocation(PickerLocationId::DocumentsLibrary);
picker.FileTypeFilter().ReplaceAll({ L"*" });
StorageFolder folder = co_await picker.PickSingleFolderAsync();
if (folder != nullptr)
{
StorageApplicationPermissions::FutureAccessList().AddOrReplace(L"PickedFolderToken", folder);
_State.Profile().StartingDirectory(folder.Path());
auto folder = co_await OpenFilePicker([](auto&& dialog) {
static constexpr winrt::guid clientGuidFolderPicker{ 0xAADAA433, 0xB04D, 0x4BAE, { 0xB1, 0xEA, 0x1E, 0x6C, 0xD1, 0xCD, 0xA6, 0x8B } };
THROW_IF_FAILED(dialog->SetClientGuid(clientGuidFolderPicker));
try
{
auto folderShellItem{ winrt::capture<IShellItem>(&SHGetKnownFolderItem, FOLDERID_ComputerFolder, KF_FLAG_DEFAULT, nullptr) };
dialog->SetDefaultFolder(folderShellItem.get());
}
CATCH_LOG(); // non-fatal

DWORD flags{};
THROW_IF_FAILED(dialog->GetOptions(&flags));
THROW_IF_FAILED(dialog->SetOptions(flags | FOS_PICKFOLDERS)); // folders only
});

if (!folder.empty())
{
_State.Profile().StartingDirectory(folder);
}
}

Expand Down
4 changes: 2 additions & 2 deletions src/cascadia/TerminalSettingsEditor/pch.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,6 @@
#include <winrt/Windows.Foundation.Collections.h>
#include <winrt/Windows.Storage.h>
#include <winrt/Windows.Storage.AccessCache.h>
#include <winrt/Windows.Storage.Pickers.h>

#include <winrt/Windows.UI.h>
#include <winrt/Windows.UI.Core.h>
Expand All @@ -54,7 +53,8 @@
#include <winrt/Microsoft.Terminal.Control.h>
#include <winrt/Microsoft.Terminal.Settings.Model.h>

#include "shobjidl_core.h"
#include <shlobj.h>
#include <shobjidl_core.h>
#include <dwrite.h>
#include <dwrite_1.h>

Expand Down