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

Allow partial enumeration of directory entries #6083

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

ILW8
Copy link

@ILW8 ILW8 commented Dec 21, 2023

Closes #6082

Due to DirectoryInfo.GetDirectories pre-populating some DirectoryInfo properties, TryGetEntriesForPath may fail and return false despite there being valid subdirectories for the user to safely pick.

Copy link
Member

@Susko3 Susko3 left a comment

Choose a reason for hiding this comment

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

Found an interesting overload that could be used to improve code quality and (probably and very slightly) performance.

{
// Don't fail enumeration if we fail getting attributes for a single entry
Logger.Log($"Directory {directoryName} is inaccessible", LoggingTarget.Information, LogLevel.Debug);
}
}

return true;
Copy link
Member

Choose a reason for hiding this comment

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

To keep in line with the intentions of the method, I think it should return false (like it previously did) if enumerating all directories failed. I.e. if the inner catch got triggered, and items.Count == 0, false should be returned.

Empty directories should return true as it currently works.

If you opt for IgnoreInaccessible I'm fine with returning true even if all the entries couldn't be opened.

Copy link
Author

Choose a reason for hiding this comment

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

You're right, I overlooked the scenario you described.

@@ -204,10 +205,19 @@ protected virtual bool TryGetEntriesForPath(DirectoryInfo path, out ICollection<

try
{
foreach (var dir in path.GetDirectories().OrderBy(d => d.Name))
Copy link
Member

Choose a reason for hiding this comment

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

Looking into this, I found an EnumerateDirectories(String, EnumerationOptions) overload that can take in EnumerationOptions, which has the option IgnoreInaccessible -- exactly what you're looking for (there's also AttributesToSkip that could be used for hidden items).

The same should also apply to FileSelector.

Copy link
Author

Choose a reason for hiding this comment

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

Unfortunately EnumerateDirectories, even with IgnoreInaccessible, throws an exception. I've tried many of the other overloads that allow ignoring certain failures with no success; enumerating using a string of the path really was a last-resort.

2023-12-27_04-41-41.mp4

@pull-request-size pull-request-size bot added size/M and removed size/S labels Dec 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DirectorySelector fails to enumerate ~/Library/Application Support directories
2 participants