[API Proposal]: OpenFolderDialog #7689
Comments
pchaurasia14
added
the
API suggestion
|
@miloush Should the OpenFolderDialog use the |
|
@lindexi We discussed this in the PR, I originally had I can try to look into it again, extracting the path handling into separate helper class perhaps, I don't exactly remember what the problems where (it was half a year ago!). I would really like to have the |
|
@miloush, this proposal looks good. Adding a new base class is not a breaking change ( except for reflection, which is not an issue ) and we will like to proceed with this proposal. Although our target here is to enable folder selection, however IMO we should consider adding the public properties / commonly used options to this proposal that are provided by other options, so that there is no loss of functionality for the developers who want to switch to OpenFolderDialog. Thoughts? |
|
@dipeshmsft can you suggest which properties / options are missing? |
|
Here is the list of properties that I think we should include in this proposal In CommonItemDialog:
In OpenFolderDialog, we can look into |
|
Oh! I am all up for getting in as much as we can from #7248. I kind of thought we have to have a PR already for the API review. If that is not the case, we can get API surface approved first and then deal with the implementation, then no problem. I will update the issue. Do you want me to include API for the controls? (#7256) |
I am not a big fan of |
I don't have issue with including this one, but note that it does not hide pinned places only, it makes the left pane completely empty. Not sure how useful it is. |
Yeah, I think we should get the API Proposal approved first and then we can later deal with the implementation. I am currently going through all the available flags, and will checkout the API for controls.
Agreed, that there will be fewer developers using this flag. I would like to take other community members' opinion here. |
|
@miloush, after a look at the FOS_* flags and IFileDialog methods, I think we should include these four properties in CommonItemDialog for now Also, in which scenarios |
When you have shortcuts (i.e. lnk files) pointing to folders. Dereferencing them would return the target folder path, not doing so would return a path to the .lnk file. |
|
@miloush, what I meant by that question is, can we enable this property by default for OpenFolderDialog? Because when we set this to False, we cannot use navigation via the .lnk file, and since it is a file, it cannot be selected. Although it is not a non-functioning API for OpenFolderDialog and we can have it in CommonItemDialog. Thoughts, about this ? |
|
I see it in the PR but I don't see where its used. Anyway I created two new projects, one with Winforms (.NET 7) and the other is WPF which uses the WinAPI Code pack (which almost all WPF developers use if they dont use the winforms folder dialolg) just to see their behavior on shortcuts. Both with all options left on their default value. WPF code: private void Button_Click(object sender, RoutedEventArgs e)
{
var dlg = new CommonOpenFileDialog
{
IsFolderPicker = true
};
if (dlg.ShowDialog() == CommonFileDialogResult.Ok)
{
MessageBox.Show(dlg.FileName);
}
}Winforms: private void NewButton_Click(object? sender, EventArgs e)
{
var dlg = new FolderBrowserDialog();
if (dlg.ShowDialog() == DialogResult.OK)
{
MessageBox.Show(dlg.SelectedPath);
}
}Both are showing shortcuts by default. Selecting the shortcut and clicking on the OK button will print out the TARGET folder path, not the shortcut path. This is something that I personally think is expected and wanted. I dont think that developers would want getting a path to a FILE (shortcut). I also haven't seen there is an option to disable the behavior on these two as well. Either we remove |
|
@dipeshmsft my point was it is a functioning API, you can see .lnk files in the open folder dialog. This property is already on by deafult, see |
|
I have updated the API with additional new properties that I believe are relatively easy to implement (i.e. just a flag or interface call). I didn't add:
Please have a look and let me know your thoughts, or if you want me to add the default values to the API diff. |
|
I would, however, not want to delay the open folder feature only because of discussion about new unrelated file dialog properties that can be added later. Note that 8.0 is a long-term support release. |
terrajobst
added
api-ready-for-review
|
Marked as ready for review, so we'll take a look next Tuesday 4/18 |
terrajobst
added
API suggestion
|
@miloush, I looked at the updated members and here are my thoughts:
|
|
Since
|
|
@lindexi asked about the folder dialog having a
The downsides of doing that would be:
At this point I prefer shared properties as proposed. @lindexi do you have any arguments why using FolderName rather than FileName would be beneficial? |
namespace Microsoft.Win32;
public abstract partial class CommonDialog
{
protected CommonDialog();
public object Tag { get; set; }
protected virtual void CheckPermissionsToShowDialog();
protected virtual IntPtr HookProc(IntPtr hwnd, int msg, IntPtr wParam, IntPtr lParam);
public abstract void Reset();
protected abstract bool RunDialog(IntPtr hwndOwner);
public virtual bool? ShowDialog();
public bool? ShowDialog(Window owner);
}
+ public abstract partial class CommonItemDialog : CommonDialog
+ {
+ private protected CommonItemDialog();
+
+ // lifted up from FileDialog
+ public IList<FileDialogCustomPlace> CustomPlaces { get; set; }
+ public bool DereferenceLinks { get; set; }
+ public string InitialDirectory { get; set; }
+ public string FileName { get; set; }
+ public string[] FileNames { get; }
+ public bool RestoreDirectory { get; set; }
+ public string SafeFileName { get; }
+ public string[] SafeFileNames { get; }
+ public string Title { get; set; }
+ public bool ValidateNames { get; set; }
+ public event CancelEventHandler FileOk { add; remove; }
+ protected void OnFileOk(CancelEventArgs e);
+ protected override bool RunDialog(IntPtr hwndOwner};
+ public override void Reset();
+ public override string ToString();
+
+ // new members
+ public bool AddToRecent { get; set; } // = true; (FOS_DONTADDTORECENT)
+ public bool AllowReadOnlyItems { get; set; } // = false; (FOS_NOREADONLYRETURN)
+ public Guid? ClientGuid { get; set; } // (IFileDialog::SetClientGuid)
+ public string DefaultDirectory { get; set; } // (IFileDialog::SetDefaultFolder)
+ public bool ShowHiddenFiles { get; set; } // = false; (FOS_FORCESHOWHIDDEN )
+ public string RootDirectory { get; set; } // (IFileDialog2::SetNavigationRoot)
+ }
- // unless noted otherwise, all removed members lifted up to CommonItemDialog
-
- public abstract partial class FileDialog : CommonDialog
+ public abstract partial class FileDialog : CommonItemDialog
{
- protected FileDialog();
+ private protected FileDialog();
public bool AddExtension { get; set; }
public virtual bool CheckFileExists { get; set; }
public bool CheckPathExists { get; set; } // WAS VIRTUAL
- public IList<FileDialogCustomPlace> CustomPlaces { get; set; }
public string DefaultExt { get; set; }
- public bool DereferenceLinks { get; set; }
- public string InitialDirectory { get; set; }
- public string FileName { get; set; }
- public string[] FileNames { get; }
public string Filter { get; set; }
public int FilterIndex { get; set; }
- protected int Options { get; } // REMOVED
- public bool RestoreDirectory { get; set; }
- public string SafeFileName { get; }
- public string[] SafeFileNames { get; }
- public string Title { get; set; }
- public bool ValidateNames { get; set; }
- public event CancelEventHandler FileOk { add; remove; }
- protected override IntPtr HookProc(IntPtr hwnd, int msg, IntPtr wParam, IntPtr lParam); // OVERRIDE REMOVED
- protected void OnFileOk(CancelEventArgs e);
public override void Reset();
protected override bool RunDialog(IntPtr hwndOwner);
public override string ToString();
}
public sealed partial class OpenFileDialog : FileDialog
{
public OpenFileDialog();
+ public bool ForcePreviewPane { get; set; } // = false; (FOS_FORCEPREVIEWPANEON)
public bool Multiselect { get; set; }
public bool ReadOnlyChecked { get; set; }
public bool ShowReadOnly { get; set; }
protected override void CheckPermissionsToShowDialog();
public System.IO.Stream OpenFile();
public System.IO.Stream[] OpenFiles();
public override void Reset();
}
+ public sealed partial class OpenFolderDialog : CommonItemDialog
+ {
+ public OpenFolderDialog();
+ public bool Multiselect { get; set; }
+ public override void Reset();
+ }
public sealed partial class SaveFileDialog : FileDialog
{
public SaveFileDialog();
+ public bool CreateTestFile { get; set; } // = true; (FOS_NOTESTFILECREATE)
public bool CreatePrompt { get; set; }
public bool OverwritePrompt { get; set; }
public System.IO.Stream OpenFile();
public override void Reset();
} |
bartonjs
added
api-approved
|
@miloush Or |
|
@lindexi |
|
Added some basic tests for |
|
On further investigation and reviewing the PR, there are a few things that I want to point out - OpenFolderDialog, does not return the path of the .lnk filled. When DereferenceLinks is true, the path pointed by the .lnk file is returned. On the other hand, when DereferenceLinks = false, nothing is returned, and the dialog shows a prompt with the message that the folder name is not valid. Considering this, we should move back FileName and similar properties to FileDialog class and have FolderName for the folder dialog. For this, I think @miloush suggestion of having internal properties in CommonItemDialog is a good approach.
|
|
/cc @miloush @lindexi namespace Microsoft.Win32;
public abstract partial class CommonDialog
{
protected CommonDialog();
public object Tag { get; set; }
protected virtual void CheckPermissionsToShowDialog();
protected virtual IntPtr HookProc(IntPtr hwnd, int msg, IntPtr wParam, IntPtr lParam);
public abstract void Reset();
protected abstract bool RunDialog(IntPtr hwndOwner);
public virtual bool? ShowDialog();
public bool? ShowDialog(Window owner);
}
+ public abstract partial class CommonItemDialog : CommonDialog
+ {
+ private protected CommonItemDialog();
+
+ // lifted up from FileDialog
+ public IList<FileDialogCustomPlace> CustomPlaces { get; set; }
+ public bool DereferenceLinks { get; set; }
+ public string InitialDirectory { get; set; }
+ public string Title { get; set; }
+ public bool ValidateNames { get; set; }
+ protected virtual void OnItemOk(CancelEventArgs e);
+ protected override bool RunDialog(IntPtr hwndOwner);
+ public override void Reset();
+ public override string ToString();
+
+ // new members
+ public bool AddToRecent { get; set; } // = true; (FOS_DONTADDTORECENT)
+ public Guid? ClientGuid { get; set; } // (IFileDialog::SetClientGuid)
+ public string DefaultDirectory { get; set; } // (IFileDialog::SetDefaultFolder)
+ public bool ShowHiddenItems { get; set; } // = false; (FOS_FORCESHOWHIDDEN )
+ public string RootDirectory { get; set; } // (IFileDialog2::SetNavigationRoot)
+ }
- // unless noted otherwise, all removed members lifted up to CommonItemDialog
-
- public abstract partial class FileDialog : CommonDialog
+ public abstract partial class FileDialog : CommonItemDialog
{
- protected FileDialog();
+ private protected FileDialog();
public bool AddExtension { get; set; }
- public virtual bool CheckFileExists { get; set; }
+ public bool CheckFileExists { get; set; } // WAS VIRTUAL
public bool CheckPathExists { get; set; }
- public IList<FileDialogCustomPlace> CustomPlaces { get; set; }
public string DefaultExt { get; set; }
- public bool DereferenceLinks { get; set; }
- public string InitialDirectory { get; set; }
public string FileName { get; set; }
public string[] FileNames { get; }
public string Filter { get; set; }
public int FilterIndex { get; set; }
- protected int Options { get; } // REMOVED
public bool RestoreDirectory { get; set; }
public string SafeFileName { get; }
public string[] SafeFileNames { get; }
- public string Title { get; set; }
- public bool ValidateNames { get; set; }
public event CancelEventHandler FileOk { add; remove; }
+ protected override void OnItemOk(CancelEventArgs e);
- protected override IntPtr HookProc(IntPtr hwnd, int msg, IntPtr wParam, IntPtr lParam); // OVERRIDE REMOVED
- protected void OnFileOk(CancelEventArgs e);
public override void Reset();
- protected override bool RunDialog(IntPtr hwndOwner);
public override string ToString();
}
public sealed partial class OpenFileDialog : FileDialog
{
public OpenFileDialog();
+ public bool ForcePreviewPane { get; set; } // = false; (FOS_FORCEPREVIEWPANEON)
public bool Multiselect { get; set; }
public bool ReadOnlyChecked { get; set; }
public bool ShowReadOnly { get; set; }
- protected override void CheckPermissionsToShowDialog();
public System.IO.Stream OpenFile();
public System.IO.Stream[] OpenFiles();
public override void Reset();
}
+ public sealed partial class OpenFolderDialog : CommonItemDialog
+ {
+ public OpenFolderDialog();
+ public bool Multiselect { get; set; }
+ public override void Reset();
+ public string FolderName { get; }
+ public string[] FolderNames { get; }
+ public event CancelEventHandler FolderOk { add; remove; }
+ protected override void OnItemOk(CancelEventArgs e);
+ public string SafeFolderName { get; }
+ public string[] SafeFolderNames { get; }
+ public override string ToString();
+ }
public sealed partial class SaveFileDialog : FileDialog
{
public SaveFileDialog();
+ public bool CreateTestFile { get; set; } // = true; (FOS_NOTESTFILECREATE)
public bool CreatePrompt { get; set; }
public bool OverwritePrompt { get; set; }
public System.IO.Stream OpenFile();
public override void Reset();
}The above API has been reviewed and approved internally ( over email ). We will be proceeding with the implementation. |
|
Thank you @dipeshmsft Looks good to me |
|
Closing as completed. |
|
Awesome |
ghost
locked as resolved and limited conversation to collaborators
Background and motivation
I was asked to create an API Proposal issue for the OpenFolderDialog, PR #7244
The purpose of the API is to enable folder selection using the common folder dialogs in WPF. #438 is the top voted issue in the repository.
This proposal involves splitting a hierarchy of classes and removing support for Windows XP dialogs (they fallback to Vista dialogs).
API Proposal
The proposed API is a compromise on perceived compatibility requirements.
Note that users cannot inherit from
FileDialogdue to internal abstract members, soprotectedandabstract/virtualchanges are non-breaking.Class diagram in PR. Public members diff:
namespace Microsoft.Win32; public abstract partial class CommonDialog { protected CommonDialog(); public object Tag { get; set; } protected virtual void CheckPermissionsToShowDialog(); protected virtual IntPtr HookProc(IntPtr hwnd, int msg, IntPtr wParam, IntPtr lParam); public abstract void Reset(); protected abstract bool RunDialog(IntPtr hwndOwner); public virtual bool? ShowDialog(); public bool? ShowDialog(Window owner); } + public abstract partial class CommonItemDialog : CommonDialog + { + private protected CommonItemDialog(); + + // lifted up from FileDialog + public IList<FileDialogCustomPlace> CustomPlaces { get; set; } + public bool DereferenceLinks { get; set; } + public string InitialDirectory { get; set; } + public string FileName { get; set; } + public string[] FileNames { get; } + public bool RestoreDirectory { get; set; } + public string SafeFileName { get; } + public string[] SafeFileNames { get; } + public string Title { get; set; } + public bool ValidateNames { get; set; } + public event CancelEventHandler FileOk { add; remove; } + protected void OnFileOk(CancelEventArgs e); + protected override bool RunDialog(IntPtr hwndOwner}; + public override void Reset(); + public override string ToString(); + + // new members + public bool AddToRecent { get; set; } // = true; (FOS_DONTADDTORECENT) + public bool AllowReadOnlyItems { get; set; } // = false; (FOS_NOREADONLYRETURN) + public Guid? ClientGuid { get; set; } // (IFileDialog::SetClientGuid) + public string DefaultDirectory { get; set; } // (IFileDialog::SetDefaultFolder) + public bool ShowHiddenFiles { get; set; } // = false; (FOS_FORCESHOWHIDDEN ) + public string RootDirectory { get; set; } // (IFileDialog2::SetNavigationRoot) + } - // unless noted otherwise, all removed members lifted up to CommonItemDialog - - public abstract partial class FileDialog : CommonDialog + public abstract partial class FileDialog : CommonItemDialog { protected FileDialog(); public bool AddExtension { get; set; } public virtual bool CheckFileExists { get; set; } public bool CheckPathExists { get; set; } // WAS VIRTUAL - public IList<FileDialogCustomPlace> CustomPlaces { get; set; } public string DefaultExt { get; set; } - public bool DereferenceLinks { get; set; } - public string InitialDirectory { get; set; } - public string FileName { get; set; } - public string[] FileNames { get; } public string Filter { get; set; } public int FilterIndex { get; set; } - protected int Options { get; } // REMOVED - public bool RestoreDirectory { get; set; } - public string SafeFileName { get; } - public string[] SafeFileNames { get; } - public string Title { get; set; } - public bool ValidateNames { get; set; } - public event CancelEventHandler FileOk { add; remove; } - protected override IntPtr HookProc(IntPtr hwnd, int msg, IntPtr wParam, IntPtr lParam); // OVERRIDE REMOVED - protected void OnFileOk(CancelEventArgs e); public override void Reset(); protected override bool RunDialog(IntPtr hwndOwner); public override string ToString(); } public sealed partial class OpenFileDialog : FileDialog { public OpenFileDialog(); + public bool ForcePreviewPane { get; set; } // = false; (FOS_FORCEPREVIEWPANEON) public bool Multiselect { get; set; } public bool ReadOnlyChecked { get; set; } public bool ShowReadOnly { get; set; } protected override void CheckPermissionsToShowDialog(); public System.IO.Stream OpenFile(); public System.IO.Stream[] OpenFiles(); public override void Reset(); } + public sealed partial class OpenFolderDialog : CommonItemDialog + { + public OpenFolderDialog(); + public bool Multiselect { get; set; } + public override void Reset(); + } public sealed partial class SaveFileDialog : FileDialog { public SaveFileDialog(); + public bool CreateTestFile { get; set; } // = true; (FOS_NOTESTFILECREATE) public bool CreatePrompt { get; set; } public bool OverwritePrompt { get; set; } public System.IO.Stream OpenFile(); public override void Reset(); }API Usage
Alternative Designs
There are several ways how the dialog could be introduced.
Pile on the technical debt and just add another flag property on
OpenFileDialog. This is PickFolders option for OpenFileDialog (minimal) #6374.Pros: Minimal risk, no breaking changes, reflects native API design.
Cons: When using
OpenFileDialogfor folders, some of its properties would become meaningless, some would start throwing. Any future enhancements to the API would become more and more difficult.Inherit directly from
CommonDialog, or do not inherit form anything.Pros: No backward compatibility requirements, can have clean design.
Cons: Does not reflect the fact the underlying API is a file dialog. User code that prepares the dialog would have to be duplicated across different types. Vast majority of the code that is currently in
FileDialogwould have to be duplicated (and any future enhancements to it).Moving the members around and/or splitting the inheritance chain seems to be the only way how to achieve a reasonably clean design and utilize existing code.
Ideally, the situation would be like this:
OpenFolderDialog : CommonDialogOpenFileDialog : FileDialog : CommonDialogwhere
CommonDialogwould specifically mean common file dialog (dropping the current design whereCommonDialogwas expected to be used for other system dialogs like color, font, etc.), and all the members that are shared between file and folder dialogs would be lifted up toCommonDialog. Unfortunately touchingCommonDialogis a compatibility concern, because unlikeFileDialog, people can inherit directly fromCommonDialogand even if it mostly wouldn't be a breaking change, we would pollute it with file-related members even for unrelated inheritors.Having to keep
CommonDialogwhat it is, we need to introduce another class in the inheritance chain for the shared members for both files and folders.However, introducing a class in-between, e.g.
OpenFolderDialog : FileDialog : CommonDialogOpenFileDialog : CommonFileDialog : FileDialog : CommonDialogwould mean some of the members would have to be moved down from
FileDialogto a derived class. That is a breaking change for when a variable is declared asFileDialog, it would be missing the moved members.The remaining options is to introduce a new base class (i.e. this proposal):
OpenFolderDialog : CommonItemDialog : CommonDialogOpenFileDialog : FileDialog : CommonItemDialog : CommonDialogand lift the members up from
FileDialogto a parent class. This will keep variables declared asFileDialogworking like before, the existing members would become inherited.(There is also always the possibility to throw away the current implementation and create a clean design in a different namespace.)
Risks
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 will still work, however, the dialogs will use the
IFileDialogAPI rather than the legacy dialogs.The text was updated successfully, but these errors were encountered: