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

OpenFolderDialog #7244

Merged
merged 13 commits into from Jul 7, 2023
Merged

Conversation

miloush
Copy link
Contributor

@miloush miloush commented Nov 1, 2022

Fixes #438. Further discussion in #4039. Previous attempt: #6351

This PR contains runtime breaking changes. For minimal implementation of #438 without breaking changes see #6374. EDIT: Introducing a base class is not breaking.

Description

Allows developers to ask the user to select a folder, a feature known as FolderBrowserDialog in WinForms. This PR

  1. removes all legacy (Windows XP) dialogs
  2. splits the existing inheritance and moves common members up the hierarchy, so that members only available or working for files are not present for the folder browsing.

Public members before:
Before

Public members after:
After

Note that this abstraction does not exist in the native code, there is only a flag to pick folders on FileOpenDialog (akin to #6374).

What used to be FileDialog.Options is now strictly for the native API usage, so there is no longer need to mix and mask them.

Example usage

OpenFolderDialog dialog = new OpenFolderDialog();
dialog.Multiselect = true;
dialog.ShowDialog();
string[] folders = dialog.FileNames;

Compatibility requirements and limitations

The goal for this PR was to preserve compile-time compatibility, but not run-time compatibility. Old PresentationFramework.dll assembly cannot be replaced by a new one, the application using it has to be recompiled (since members moved around types).

Anything that compiled before should still compile without changes. Most notably, while developers cannot derive from FileDialog due to internal abstract members, deriving from CommonDialog is possible. If this scenario was not required to be supported, I would further remove the following members as part of removing support for legacy dialogs:

  • protected CommonDialog.HookProc
  • private CommonDialog.MoveToScreenCenter

Not that inserting a class before FileDialog is the only place where class can be inserted if compatibility with .NET Framework API is to be preserved. If that was not a requirement, it would be possible to insert a class after FileDialog, which would be a bit more natural. Finally, if .NET Framework API surface was not a requirement AND we would be happy polluting the base class of possible inheritors of CommonDialog, we could treat CommonDialog newly as for IFileDialog API only and not insert any new classes in the hierarchy.

.NET Framework baseline contains changes to FileDialog that would be breaking if developers could inherit from the dialog, but as noted above, that is not allowed.

Naming considerations

  • CommonItemDialog is a name coming from the documentation. The native dialog works with any IShellItems, not only file-based ones. I envision in the future if the code was extended to support arbitrary shell items, CommonItemDialog could provide the selection as shell items and the derived classes would utilize those for FileNames resp. FolderNames instead of doing their own conversion. EDIT: FileNames were now lifted up to CommonItemDialog, shared for both files and folders.

  • OpenFolderDialog is for parity with OpenFileDialog. Note that the save dialog no longer supports folder selection, so there is no need for an extra empty FolderDialog class in-between.

  • FileName(s) makes the design much simpler (than having separate FolderNames) and reflects the native API. Note also that when dereferencing is disabled, results may contain shortcut files pointing to folders.

Customer Impact

Without this fix, users have to either use WinForms' FolderBrowserDialog, resort to 3rd party libraries or re-implement the whole IFileDialog API. #438 is currently the top voted issue in the repository.

Regression

No.

Known issues in the already existing code that are not addressed by this PR:

  • read-only check box not available OpenFileDialog.ShowReadOnly does not work #6346
  • ValidateNames is used to do several unrelated things - alleged invalid filename checking (that does not seem to be happening or necessary); processing file names, adding extensions, showing prompts etc.; and as a flag to native API to try create a dummy file to ensure user has rights to access the selected path.
  • AddExtension works differently from the native API extension enforcements, so it is not realized as an API flag in the PR
  • CreatePrompt and OverwritePrompt are also implemented differently than the native FOS_CREATEPROMPT resp. FOS_OVERWRITEPROMPT, notably create prompt only works for open dialogs, not save dialogs where it is defined in the WPF API, so they are not realized as API flags in the PR

Testing

Compiled and manually tested on 7.0.0-rc.2.22472.3. Ensured overwrite and create prompts did not regress, cancelling the dialog did not regress restoring file names and filter index.

Risk

Medium risk - No public API removed, a new base class introduced. Current applications using the types will keep working without recompilation. Code using reflection even on public types might break if it is not flattening them.

Applications with Windows XP compatibility flag when recompiled will still work, however, the dialogs will use the IFileDialog API rather than the legacy dialogs.

/cc @Symbai @ThomasGoulet73 @fabiant3 @batzen @lindexi

Microsoft Reviewers: Open in CodeFlow

@miloush miloush requested a review from a team as a code owner November 1, 2022 14:18
@ghost ghost assigned miloush Nov 1, 2022
@ghost ghost added the PR metadata: Label to tag PRs, to facilitate with triage label Nov 1, 2022
@ghost ghost requested review from dipeshmsft and singhashish-wpf November 1, 2022 14:19
@ghost ghost added the Community Contribution A label for all community Contributions label Nov 1, 2022
@miloush miloush force-pushed the FileDialog-PickFolders-Breaking branch from d14730b to f74b091 Compare November 1, 2022 18:13
@Symbai
Copy link
Contributor

Symbai commented Nov 1, 2022

Previously many WPF users had used Windows Api Codec Pack's folder dialog including myself. This dialog allowed users to optionally select multiple folders. Your PR only allows selecting a single folder. Selecting multiple can be quite helpful in any applications where you would have to add multiple folders. With your PR an end user would have to open the dialog over and over again just to select multiple sub folders.

The implementation is relatively easy as you only need to call SetOption and set the flag AllowMultiSelect. See Api Codec Pack

So I suggest adding the following members to the folder dialog:

MultiSelect
FolderNames
SafeFolderNames

Also CheckIfPathExists is quite important. Without that a user could enter anything in the folder dialog bottom path field and click on 'OK'. Thus, developers would always have to check the result.

@miloush
Copy link
Contributor Author

miloush commented Nov 1, 2022

Great point on multiselect, will fix that! It's the save dialog that used to support both multiselect and pick folders, but does neither anymore.

Without that a user could enter anything in the folder dialog bottom path field and click on 'OK'.

Have you tried that with Windows Api Codec Pack? Because the FOS_PICKFOLDERS implies FOS_FILEMUSTEXIST which implies FOS_PATHMUSTEXIST, my guess is since Vista SP1.

@Symbai
Copy link
Contributor

Symbai commented Nov 2, 2022

Because the FOS_PICKFOLDERS implies FOS_FILEMUSTEXIST which implies FOS_PATHMUSTEXIST

You are correct, forget about my comment on CheckIfPathExists.

@miloush
Copy link
Contributor Author

miloush commented Nov 2, 2022

As for multiselect, the complexity with lifting up file names to CommonItemDialog is in the file dialog needing to revert filter index if the OK event is cancelled (and also mutating the results during filenames processing). There is an architectural struggle between clean separation and knowing the API will not be extended anymore. I might also keep FileName(s) for both file and folder dialogs rather than separate FolderName(s).

@miloush
Copy link
Contributor Author

miloush commented Apr 19, 2023

I believe this PR now conforms to the API review.

@miloush miloush force-pushed the FileDialog-PickFolders-Breaking branch from 28fdbf3 to 5f6e180 Compare June 11, 2023 22:23
@miloush
Copy link
Contributor Author

miloush commented Jun 11, 2023

Reverted the FileNames unification to separate FolderNames.

Note that there is FileOk event. Are we happy with that one or should it also be disunified to FolderOk?

@dipeshmsft

@miloush
Copy link
Contributor Author

miloush commented Jul 2, 2023

Since the review related to the new properties as well, all fixes have been pushed to #7916. Thanks @dipeshmsft for the review!

@miloush
Copy link
Contributor Author

miloush commented Jul 3, 2023

@dipeshmsft backported

@dipeshmsft dipeshmsft merged commit f068d67 into dotnet:main Jul 7, 2023
9 checks passed
@dotnet dotnet locked as resolved and limited conversation to collaborators Aug 6, 2023
@dotnet dotnet unlocked this conversation Aug 10, 2023
pomianowski added a commit to lepoco/wpfui that referenced this pull request Sep 6, 2023
@dotnet dotnet locked as resolved and limited conversation to collaborators Sep 9, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Community Contribution A label for all community Contributions PR metadata: Label to tag PRs, to facilitate with triage
Projects
None yet
Development

Successfully merging this pull request may close these issues.

WPF alternative for WinForms FolderBrowserDialog
4 participants