-
-
Notifications
You must be signed in to change notification settings - Fork 548
Improve code quality #4257
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
Improve code quality #4257
Changes from all commits
f87b115
0de552f
f3f9d62
7b3216b
c864d9f
0acda2c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -48,14 +48,14 @@ | |
| foreach (FileInfo file in files) | ||
| { | ||
| string temppath = Path.Combine(targetPath, file.Name); | ||
| file.CopyTo(temppath, false); | ||
| } | ||
|
|
||
| // Recursively copy subdirectories by calling itself on each subdirectory until there are no more to copy | ||
| foreach (DirectoryInfo subdir in dirs) | ||
| { | ||
| string temppath = Path.Combine(targetPath, subdir.Name); | ||
| CopyAll(subdir.FullName, temppath, messageBoxExShow); | ||
| } | ||
| } | ||
| catch (Exception) | ||
|
|
@@ -130,6 +130,119 @@ | |
| } | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Attempts to delete a directory robustly with retry logic for locked files. | ||
| /// This method tries to delete files individually with retries, then removes empty directories. | ||
| /// Returns true if the directory was completely deleted, false if some files/folders remain. | ||
| /// </summary> | ||
| /// <param name="path">The directory path to delete</param> | ||
| /// <param name="maxRetries">Maximum number of retry attempts for locked files (default: 3)</param> | ||
| /// <param name="retryDelayMs">Delay in milliseconds between retries (default: 100ms)</param> | ||
| /// <returns>True if directory was fully deleted, false if some items remain</returns> | ||
| public static bool TryDeleteDirectoryRobust(string path, int maxRetries = 3, int retryDelayMs = 100) | ||
| { | ||
| if (!Directory.Exists(path)) | ||
| return true; | ||
|
|
||
| bool fullyDeleted = true; | ||
|
|
||
| try | ||
| { | ||
| // First, try to delete all files in the directory tree | ||
| var files = Directory.GetFiles(path, "*", SearchOption.AllDirectories); | ||
| foreach (var file in files) | ||
|
Comment on lines
+151
to
+153
|
||
| { | ||
| bool fileDeleted = false; | ||
| for (int attempt = 0; attempt <= maxRetries; attempt++) | ||
| { | ||
|
Comment on lines
+155
to
+157
|
||
| try | ||
| { | ||
| // Remove read-only attribute if present | ||
| var fileInfo = new FileInfo(file); | ||
| if (fileInfo.Exists && fileInfo.IsReadOnly) | ||
| { | ||
| fileInfo.IsReadOnly = false; | ||
| } | ||
|
|
||
| File.Delete(file); | ||
| fileDeleted = true; | ||
| break; | ||
| } | ||
| catch (UnauthorizedAccessException) | ||
| { | ||
| // File is in use or access denied, wait and retry | ||
| if (attempt < maxRetries) | ||
| { | ||
| System.Threading.Thread.Sleep(retryDelayMs); | ||
| } | ||
| } | ||
| catch (IOException) | ||
| { | ||
| // File is in use, wait and retry | ||
| if (attempt < maxRetries) | ||
| { | ||
| System.Threading.Thread.Sleep(retryDelayMs); | ||
| } | ||
| } | ||
| catch | ||
| { | ||
| // Other exceptions, don't retry | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| if (!fileDeleted) | ||
| { | ||
| fullyDeleted = false; | ||
| } | ||
| } | ||
|
|
||
| // Then, try to delete all empty directories (from deepest to shallowest) | ||
| var directories = Directory.GetDirectories(path, "*", SearchOption.AllDirectories) | ||
| .OrderByDescending(d => d.Length) // Delete deeper directories first | ||
| .ToArray(); | ||
|
|
||
| foreach (var directory in directories) | ||
| { | ||
| try | ||
| { | ||
| if (Directory.Exists(directory) && !Directory.EnumerateFileSystemEntries(directory).Any()) | ||
| { | ||
| Directory.Delete(directory, false); | ||
| } | ||
| } | ||
| catch | ||
| { | ||
| // If we can't delete an empty directory, mark as not fully deleted | ||
| fullyDeleted = false; | ||
| } | ||
| } | ||
|
|
||
| // Finally, try to delete the root directory itself | ||
| try | ||
| { | ||
| if (Directory.Exists(path) && !Directory.EnumerateFileSystemEntries(path).Any()) | ||
| { | ||
| Directory.Delete(path, false); | ||
| } | ||
| else if (Directory.Exists(path)) | ||
| { | ||
| fullyDeleted = false; | ||
| } | ||
| } | ||
| catch | ||
| { | ||
| fullyDeleted = false; | ||
| } | ||
| } | ||
| catch | ||
| { | ||
| fullyDeleted = false; | ||
| } | ||
|
|
||
| return fullyDeleted; | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Checks if a directory exists | ||
| /// </summary> | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,7 @@ | ||
| using Flow.Launcher.Plugin.SharedCommands; | ||
| using NUnit.Framework; | ||
| using NUnit.Framework.Legacy; | ||
| using System.IO; | ||
|
|
||
| namespace Flow.Launcher.Test | ||
| { | ||
|
|
@@ -32,8 +33,8 @@ | |
| [TestCase(@"c:\foobar\", @"c:\foo\a.txt", false)] | ||
| // Edge case | ||
| [TestCase(@"c:\foo", @"c:\foo\..\bar\baz", false)] | ||
| [TestCase(@"c:\bar", @"c:\foo\..\bar\baz", true)] | ||
| [TestCase(@"c:\barr", @"c:\foo\..\bar\baz", false)] | ||
| public void GivenTwoPaths_WhenCheckPathContains_ThenShouldBeExpectedResult(string parentPath, string path, bool expectedResult) | ||
| { | ||
| ClassicAssert.AreEqual(expectedResult, FilesFolders.PathContains(parentPath, path)); | ||
|
|
@@ -50,5 +51,89 @@ | |
| { | ||
| ClassicAssert.AreEqual(expectedResult, FilesFolders.PathContains(parentPath, path, allowEqual: expectedResult)); | ||
| } | ||
|
|
||
| [Test] | ||
| public void TryDeleteDirectoryRobust_WhenDirectoryDoesNotExist_ReturnsTrue() | ||
| { | ||
| // Arrange | ||
| string nonExistentPath = Path.Combine(Path.GetTempPath(), Path.GetRandomFileName()); | ||
|
|
||
| // Act | ||
| bool result = FilesFolders.TryDeleteDirectoryRobust(nonExistentPath); | ||
|
|
||
| // Assert | ||
| ClassicAssert.IsTrue(result); | ||
| } | ||
|
|
||
| [Test] | ||
| public void TryDeleteDirectoryRobust_WhenDirectoryIsEmpty_DeletesSuccessfully() | ||
| { | ||
| // Arrange | ||
| string tempDir = Path.Combine(Path.GetTempPath(), Path.GetRandomFileName()); | ||
| Directory.CreateDirectory(tempDir); | ||
|
|
||
| // Act | ||
| bool result = FilesFolders.TryDeleteDirectoryRobust(tempDir); | ||
|
|
||
| // Assert | ||
| ClassicAssert.IsTrue(result); | ||
| ClassicAssert.IsFalse(Directory.Exists(tempDir)); | ||
| } | ||
|
Comment on lines
+69
to
+81
|
||
|
|
||
| [Test] | ||
| public void TryDeleteDirectoryRobust_WhenDirectoryHasFiles_DeletesSuccessfully() | ||
| { | ||
| // Arrange | ||
| string tempDir = Path.Combine(Path.GetTempPath(), Path.GetRandomFileName()); | ||
| Directory.CreateDirectory(tempDir); | ||
| File.WriteAllText(Path.Combine(tempDir, "test.txt"), "test content"); | ||
|
|
||
| // Act | ||
| bool result = FilesFolders.TryDeleteDirectoryRobust(tempDir); | ||
|
|
||
| // Assert | ||
| ClassicAssert.IsTrue(result); | ||
| ClassicAssert.IsFalse(Directory.Exists(tempDir)); | ||
| } | ||
|
|
||
| [Test] | ||
| public void TryDeleteDirectoryRobust_WhenDirectoryHasNestedStructure_DeletesSuccessfully() | ||
| { | ||
| // Arrange | ||
| string tempDir = Path.Combine(Path.GetTempPath(), Path.GetRandomFileName()); | ||
| Directory.CreateDirectory(tempDir); | ||
| string subDir1 = Path.Combine(tempDir, "SubDir1"); | ||
| string subDir2 = Path.Combine(tempDir, "SubDir2"); | ||
| Directory.CreateDirectory(subDir1); | ||
| Directory.CreateDirectory(subDir2); | ||
| File.WriteAllText(Path.Combine(subDir1, "file1.txt"), "content1"); | ||
| File.WriteAllText(Path.Combine(subDir2, "file2.txt"), "content2"); | ||
| File.WriteAllText(Path.Combine(tempDir, "root.txt"), "root content"); | ||
|
|
||
| // Act | ||
| bool result = FilesFolders.TryDeleteDirectoryRobust(tempDir); | ||
|
|
||
| // Assert | ||
| ClassicAssert.IsTrue(result); | ||
| ClassicAssert.IsFalse(Directory.Exists(tempDir)); | ||
| } | ||
|
|
||
| [Test] | ||
| public void TryDeleteDirectoryRobust_WhenFileIsReadOnly_RemovesAttributeAndDeletes() | ||
| { | ||
| // Arrange | ||
| string tempDir = Path.Combine(Path.GetTempPath(), Path.GetRandomFileName()); | ||
| Directory.CreateDirectory(tempDir); | ||
| string filePath = Path.Combine(tempDir, "readonly.txt"); | ||
| File.WriteAllText(filePath, "readonly content"); | ||
| File.SetAttributes(filePath, FileAttributes.ReadOnly); | ||
|
|
||
| // Act | ||
| bool result = FilesFolders.TryDeleteDirectoryRobust(tempDir); | ||
|
|
||
| // Assert | ||
| ClassicAssert.IsTrue(result); | ||
| ClassicAssert.IsFalse(Directory.Exists(tempDir)); | ||
| } | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When deletion is not fully successful, the code attempts to recreate the marker file under
directorywithout first ensuring the directory still exists. If the root directory was actually deleted (or moved) despitefullyDeletedbeing false (e.g., due to an exception after deletion),File.WriteAllTextwill throwDirectoryNotFoundExceptionand the marker won’t be recreated. Guard withDirectory.Exists(directory)before writing the marker (and skip/re-log if it no longer exists).