Skip to content

Add OpenFolderDialog #3967

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

Closed
wants to merge 2 commits into from
Closed

Add OpenFolderDialog #3967

wants to merge 2 commits into from

Conversation

Symbai
Copy link
Contributor

@Symbai Symbai commented Jan 9, 2021

Fixes #438

Usage:

var dialog = new OpenFolderDialog();
if (dialog.ShowDialog() != true) return;
MessageBox.Show(dialog.FileName);
var dialog = new OpenFolderDialog { Multiselect = true };
if (dialog.ShowDialog() != true) return;
foreach (var directory in dialog.FileNames)
{
    MessageBox.Show(directory);
}

@Symbai Symbai requested a review from a team as a code owner January 9, 2021 01:38
@ghost ghost added the PR metadata: Label to tag PRs, to facilitate with triage label Jan 9, 2021
@ghost ghost requested review from fabiant3, ryalanms and SamBent January 9, 2021 01:38
@ryalanms
Copy link
Member

ryalanms commented Jan 9, 2021

/cc @fabiant3 @SamBent

@miloush
Copy link
Contributor

miloush commented Jan 9, 2021

Sorry, this is just a bad copy&paste implementation that does not fit into the framework.

@Symbai
Copy link
Contributor Author

Symbai commented Jan 9, 2021

Sorry, this is just a bad copy&paste implementation that does not fit into the framework.

This was requested since nearly 2 years. Nobody bothered to add a PR for it, including yourself. Instead of downvoting my PR and calling it "bad" without giving any argument, you should make it better then. At least I'm trying to solve the problem, what's your excuse?

@ryalanms
Copy link
Member

ryalanms commented Jan 9, 2021

Thanks, @Symbai and @miloush. We will review this during the next triage session.

if (folder != null)
dialog.SetFolder(folder);
}
catch
Copy link
Member

Choose a reason for hiding this comment

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

Should we catch all exception?

/// Gets or sets a value that controls whether to show or hide the list of places where the user has recently opened or saved items.
/// Default value is true.
/// </summary>
public bool AddToMruList { get; set; } = true;
Copy link
Member

Choose a reason for hiding this comment

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

May I know what "Mru" is?

Copy link
Contributor

Choose a reason for hiding this comment

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

most recently used

Copy link
Member

Choose a reason for hiding this comment

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

Thank you. But I don’t think abbreviations are suitable here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Names were taken from WindowsAPICodePack

@miloush
Copy link
Contributor

miloush commented Jan 10, 2021

@Symbai My excuse is quality expectations. I am not submitting a PR if I cannot test the code is even working. If there is anything worse than a project not accepting PRs it is a project accepting anything. The folder dialog seems to be a heated topic and there is a pressure from the community to demonstrate commitment, and since it was flagged for review I wanted to note that this implementation does not meet the expectations of at least some of us.

That said, I could have been more constructive in the reasoning, so please accept my apologies, here are some of the reasons I am unhappy with this PR (in no particular order):

  • It is a file dialog (and common dialog), but the class is not in the file dialog hierarchy.
  • It is in different namespace than the other common dialogs, and you are introducing a new root namespace that is not used for anything else.
  • Introducing enormous number of types that already exist in the framework (and worryingly with different signatures), have you seen e.g. ShellProvider.cs?
  • All types in single file
  • Every property has an automatic backing field, taking unnecessary amount of memory for what is effectively a flag number.
  • Inconsistent type accessibility (internal vs unspecified), and naming conventions differ from the existing code
  • No support for custom places
  • No hooks to process the dialog notifications
  • No security checks, no threading checks, no error handling.

Here is an alternative implementation with minimal impact on the existing codebase: master...miloush:PickFolderFileDialog

@Symbai
Copy link
Contributor Author

Symbai commented Jan 11, 2021

Here is an alternative implementation with minimal impact on the existing codebase: master...miloush:PickFolderFileDialog

Great, go ahead and make a PR then.

@Symbai Symbai closed this Jan 11, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Apr 10, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
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