Fix unix directory seperator issue #3231
Conversation
This comment has been minimized.
This comment has been minimized.
|
Be a legend 🏆 by adding a before and after screenshot of the changes you made, especially if they are around UI/UX. |
|
@cibere Does this work for you? |
📝 WalkthroughWalkthroughThis pull request adds a new constant, UnixDirectorySeparator, in the Constants class to represent the Unix directory separator. Additionally, the PathSearchAsync method in the SearchManager class now includes a line that converts Unix-style separators to Windows-style separators. These changes ensure that path strings are correctly formatted across different operating systems. No other aspects of the classes were altered. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant SM as SearchManager
participant C as Constants
U->>SM: Request path search
SM->>SM: Receive input path with Unix separators
SM->>C: Access UnixDirectorySeparator constant
SM->>SM: Replace Unix separators with Windows separators
SM->>U: Return formatted search results
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
Plugins/Flow.Launcher.Plugin.Explorer/Search/DirectoryInfo/DirectoryInfoSearch.cs (1)
15-16: LGTM! Consider adding input validation.The changes effectively address the Unix separator issue. However, consider adding validation for edge cases.
Consider adding validation to handle cases like multiple consecutive separators:
// if user uses the unix directory separator, we need to convert it to windows directory separator -search = search.Replace(Constants.UnixDirectorySeparator, Constants.DirectorySeparator); +// Normalize path by replacing consecutive separators with a single separator +while (search.Contains(Constants.UnixDirectorySeparator + Constants.UnixDirectorySeparator) || + search.Contains(Constants.DirectorySeparator + Constants.DirectorySeparator)) +{ + search = search.Replace(Constants.UnixDirectorySeparator + Constants.UnixDirectorySeparator, Constants.UnixDirectorySeparator) + .Replace(Constants.DirectorySeparator + Constants.DirectorySeparator, Constants.DirectorySeparator); +} +// Convert Unix separators to Windows separators +search = search.Replace(Constants.UnixDirectorySeparator, Constants.DirectorySeparator);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Plugins/Flow.Launcher.Plugin.Explorer/Search/Constants.cs(1 hunks)Plugins/Flow.Launcher.Plugin.Explorer/Search/DirectoryInfo/DirectoryInfoSearch.cs(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: gitStream.cm
- GitHub Check: gitStream.cm
- GitHub Check: gitStream.cm
🔇 Additional comments (1)
Plugins/Flow.Launcher.Plugin.Explorer/Search/Constants.cs (1)
34-34: LGTM!The new constant is well-named, appropriately scoped, and logically placed near the related
DirectorySeparatorconstant.
📝 WalkthroughWalkthroughThe changes update the Explorer plugin by introducing a new internal constant for the Unix directory separator in the constants file and modifying the top-level directory search method to replace Unix directory separators with Windows ones. This ensures consistency in handling directory paths across different OS formats. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Caller
participant Search as TopLevelDirectorySearch Method
participant Logic as Directory Search Logic
Caller->>Search: Call TopLevelDirectorySearch(searchString)
Note over Search: Convert Unix '/' to Windows '\\'
Search->>Logic: Process standardized search path
Logic-->>Search: Return search results
Search-->>Caller: Return results
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
Plugins/Flow.Launcher.Plugin.Explorer/Search/DirectoryInfo/DirectoryInfoSearch.cs (1)
15-16: Consider adding null check for robustness.The implementation looks good and directly addresses the Unix directory separator issue. However, consider adding a null check for additional robustness.
internal static IEnumerable<SearchResult> TopLevelDirectorySearch(Query query, string search, CancellationToken token) { + if (string.IsNullOrEmpty(search)) + return Enumerable.Empty<SearchResult>(); + // if user uses the unix directory separator, we need to convert it to windows directory separator search = search.Replace(Constants.UnixDirectorySeparator, Constants.DirectorySeparator);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Plugins/Flow.Launcher.Plugin.Explorer/Search/Constants.cs(1 hunks)Plugins/Flow.Launcher.Plugin.Explorer/Search/DirectoryInfo/DirectoryInfoSearch.cs(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: gitStream.cm
- GitHub Check: gitStream.cm
- GitHub Check: gitStream.cm
🔇 Additional comments (2)
Plugins/Flow.Launcher.Plugin.Explorer/Search/Constants.cs (1)
34-34: LGTM!The new constant is well-named, follows the existing naming convention, and correctly represents the Unix directory separator.
Plugins/Flow.Launcher.Plugin.Explorer/Search/DirectoryInfo/DirectoryInfoSearch.cs (1)
13-29: Add unit tests to verify the directory separator handling.To ensure the directory separator handling remains correct, consider adding unit tests that verify:
- Paths with Unix separators are correctly converted
- Paths with Windows separators remain unchanged
- Mixed separator paths are handled correctly
- Empty/null paths are handled gracefully
Would you like me to help generate the unit test cases?
It indeed works for me, however I'm still able to reproduce the second error I mentioned in the comments. However I'm still unsure how related the two errors are, and if it would be best to split them up |
Sure this path |
It doesn't exist. However, I feel like flow should treat it the same way it does if you only use backslashes (which is to return no results instead of an error)
|
This reverts commit a3cc5e2.
I see. I should handle this in a more external function. Now it works fine for me. |
@check-spelling-bot Report🔴 Please reviewSee the 📂 files view, the 📜action log, or 📝 job summary for details.
See ❌ Event descriptions for more information. If the flagged items are 🤯 false positivesIf items relate to a ...
|
|
🥷 Code experts: jjw24 Jack251970 has most 👩💻 activity in the files. See details
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame: To learn more about /:\ gitStream - Visit our Docs |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
Plugins/Flow.Launcher.Plugin.Explorer/Search/SearchManager.cs (1)
190-192: LGTM! Consider enhancing the comment and adding validation.The implementation correctly handles the conversion of forward slashes to backslashes. However, consider these improvements:
- Make the comment more descriptive:
- // if user uses the unix directory separator, we need to convert it to windows directory separator + // Convert forward slashes to backslashes to handle mixed path formats (e.g., "C:\Test/Test") + // This ensures consistent path handling regardless of the separator style used
- Add validation to ensure it's a Windows path:
+ // Skip conversion for non-Windows paths (e.g., UNC paths) + if (!path.StartsWith("\\\\") && path.Length >= 2 && path[1] == ':') { path = path.Replace(Constants.UnixDirectorySeparator, Constants.DirectorySeparator); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Plugins/Flow.Launcher.Plugin.Explorer/Search/Constants.cs(1 hunks)Plugins/Flow.Launcher.Plugin.Explorer/Search/SearchManager.cs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Plugins/Flow.Launcher.Plugin.Explorer/Search/Constants.cs
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: gitStream.cm
|
@taooceros Ready to merge💪 |
Fix issue when user uses forward slash as directory seperator, like C:\Test/Test.
Related issue in #3229.