Fix issue 14624: [PropertyGrid] FolderNameEditor does not use the modern FolderBrowserDialog#14635
Fix issue 14624: [PropertyGrid] FolderNameEditor does not use the modern FolderBrowserDialog#14635SimonZhao888 wants to merge 1 commit into
Conversation
…ern FolderBrowserDialog
There was a problem hiding this comment.
Pull request overview
This PR updates the WinForms design-time FolderNameEditor used by PropertyGrid so it uses System.Windows.Forms.FolderBrowserDialog (which supports the modern/Vista-style folder picker when AutoUpgradeEnabled is enabled), and adjusts related design-time editors and tests accordingly.
Changes:
- Replaced the legacy
FolderNameEditor.FolderBrowserusage withFolderBrowserDialog, and updated returned path handling to useSelectedPath. - Updated
SelectedPathEditor/InitialDirectoryEditoroverrides to match the newInitializeDialogsignature. - Refactored/updated unit + UI integration tests and adjusted shipped public API text for the signature change.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/System.Windows.Forms.Design/src/System/Windows/Forms/Design/FolderNameEditor.cs | Switches the editor implementation to FolderBrowserDialog and updates InitializeDialog signature. |
| src/System.Windows.Forms.Design/src/System/Windows/Forms/Design/SelectedPathEditor.cs | Updates override to configure the new dialog type. |
| src/System.Windows.Forms.Design/src/System/Windows/Forms/Design/InitialDirectoryEditor.cs | Updates override to configure the new dialog type. |
| src/System.Windows.Forms.Design/src/PublicAPI.Shipped.txt | Updates the shipped API entry for FolderNameEditor.InitializeDialog. |
| src/System.Windows.Forms.Design/tests/UnitTests/System/Windows/Forms/Design/FolderNameEditorTests.cs | Refactors unit tests around the new dialog type/signature. |
| src/test/integration/UIIntegrationTests/FolderNameEditorTests.cs | Updates UI integration test to use FolderBrowserDialog / SelectedPath. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /// <summary> | ||
| /// Initializes the folder browser dialog when it is created. This gives you an opportunity | ||
| /// to configure the dialog as you please. The default implementation provides a generic folder browser. | ||
| /// </summary> | ||
| protected virtual void InitializeDialog(FolderBrowser folderBrowser) | ||
| protected virtual void InitializeDialog(FolderBrowserDialog folderBrowserDialog) | ||
| { |
| private class TestFolderNameEditor : FolderNameEditor | ||
| { | ||
| private FolderBrowser? _folderBrowser; | ||
| private FolderBrowserDialog? _folderBrowser; |
LeafShi1
left a comment
LeafShi1
left a comment
There was a problem hiding this comment.
Thanks for the PR! I have a few concerns — see inline comments.
| virtual System.Windows.Forms.Design.DesignerOptions.UseSnapLines.set -> void | ||
| virtual System.Windows.Forms.Design.FileNameEditor.InitializeDialog(System.Windows.Forms.OpenFileDialog! openFileDialog) -> void | ||
| virtual System.Windows.Forms.Design.FolderNameEditor.InitializeDialog(System.Windows.Forms.Design.FolderNameEditor.FolderBrowser! folderBrowser) -> void | ||
| virtual System.Windows.Forms.Design.FolderNameEditor.InitializeDialog(System.Windows.Forms.FolderBrowserDialog! folderBrowserDialog) -> void |
There was a problem hiding this comment.
This is a binary-breaking change to a shipped public API. Any downstream code that overrides InitializeDialog(FolderBrowser) will fail at both compile time and runtime after this change.
Typically for shipped APIs in dotnet, the old method signature should be kept (marked [Obsolete] or [EditorBrowsable(Never)]) and a new overload with the FolderBrowserDialog parameter should be introduced. The old entry should remain in PublicAPI.Shipped.txt and the new one added to PublicAPI.Unshipped.txt.
Alternatively, if the team has decided this breaking change is acceptable, it should be tracked in the breaking-changes documentation.
| private FolderBrowserDialog? _folderBrowserDialog; | ||
|
|
||
| public override object? EditValue(ITypeDescriptorContext? context, IServiceProvider provider, object? value) | ||
| { |
There was a problem hiding this comment.
FolderBrowserDialog implements IDisposable (inherits from CommonDialog → Component). Since _folderBrowserDialog is cached as a field but FolderNameEditor does not implement IDisposable, this dialog will never be disposed, leaking native handles.
Consider either:
- Making
FolderNameEditorimplementIDisposableto clean up the dialog, or - Creating the dialog locally in
EditValue(inside ausingblock) instead of caching it as a field — this also avoids stale state between invocations.
| { | ||
| return _folderBrowser.DirectoryPath; | ||
| return _folderBrowser.SelectedPath; | ||
| } |
There was a problem hiding this comment.
The integration test's TestFolderNameEditor diverges from the updated base class — it doesn't set SelectedPath from the incoming value before calling ShowDialog. The base class now does:
csharp _folderBrowserDialog.SelectedPath = value as string ?? string.Empty;
Should this override replicate that behavior so it tests the same flow?
Fixes #14624
Proposed changes
Customer Impact
Regression?
Risk
Screenshots
Before
14624-before.mp4
After
14624.mp4
Test methodology
Test environment(s)