Enhancement: Rename Quick Access Links #3586
Conversation
This change is necessary for quick access settings window
This comment has been minimized.
This comment has been minimized.
|
🥷 Code experts: jjw24 Jack251970, jjw24 have most 🧠 knowledge in the files. See details
Knowledge based on git-blame:
Knowledge based on git-blame:
Knowledge based on git-blame:
Knowledge based on git-blame:
Knowledge based on git-blame:
Knowledge based on git-blame: To learn more about /:\ gitStream - Visit our Docs |
📝 WalkthroughWalkthroughThis update introduces explicit naming for quick access links in the Explorer plugin. It adds a helper for deriving names from paths, refactors the settings UI and commands for clearer management of quick access links and excluded paths, and provides a dedicated dialog for editing quick access links with validation and localization support. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant SettingsViewModel
participant QuickAccessLinkSettings (Dialog)
participant AccessLinkCollection
User->>SettingsViewModel: Clicks "Add/Edit Quick Access Link"
SettingsViewModel->>QuickAccessLinkSettings (Dialog): Opens dialog with current collection/link
User->>QuickAccessLinkSettings (Dialog): Edits name/path, selects folder
QuickAccessLinkSettings (Dialog)->>QuickAccessLinkSettings (Dialog): Validates input (name/path, duplicates)
QuickAccessLinkSettings (Dialog)-->>SettingsViewModel: On "Done", updates/creates AccessLink
SettingsViewModel->>AccessLinkCollection: Adds or updates AccessLink with explicit Name
Suggested labels
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
⏰ Context from checks skipped due to timeout of 90000ms (7)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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: 7
🧹 Nitpick comments (4)
Plugins/Flow.Launcher.Plugin.Explorer/Helper/PathHelper.cs (1)
12-20: Consider consistency in directory separator usage.The method uses
Constants.DirectorySeparatorfor checking trailing separators butPath.DirectorySeparatorCharfor splitting. This could lead to inconsistencies on different platforms.Apply this diff to use consistent separator handling:
-var path = selectedPath.EndsWith(Constants.DirectorySeparator) ? selectedPath[0..^1] : selectedPath; +var path = selectedPath.TrimEnd(Path.DirectorySeparatorChar, Path.AltDirectorySeparatorChar); -return path.Split(new[] { Path.DirectorySeparatorChar }, StringSplitOptions.None) +return path.Split(new[] { Path.DirectorySeparatorChar, Path.AltDirectorySeparatorChar }, StringSplitOptions.RemoveEmptyEntries) .Last();This approach:
- Uses
TrimEndto handle both directory separators uniformly- Removes empty entries to handle cases with multiple consecutive separators
- Handles both primary and alternate directory separators (e.g., '/' and '' on Windows)
Plugins/Flow.Launcher.Plugin.Explorer/Main.cs (1)
101-110: Well-implemented backward compatibility method.This method effectively handles the migration of legacy quick access links that lack explicit names. The logic correctly identifies links with missing names and applies the path-based naming convention using the
GetPathName()helper method.Consider adding a null check for safety:
private void FixLegacyQuickAccessLinkNames() { - foreach (var link in Settings.QuickAccessLinks) + foreach (var link in Settings.QuickAccessLinks ?? Enumerable.Empty<AccessLink>()) { if (string.IsNullOrWhiteSpace(link.Name)) { link.Name = link.Path.GetPathName(); } } }Plugins/Flow.Launcher.Plugin.Explorer/Views/QuickAccessLinkSettings.xaml (1)
79-80: Consider adding input validation.The Name TextBox should consider adding validation to prevent empty or duplicate names that could cause confusion.
<TextBox Margin="10 0 0 0" VerticalAlignment="Center" FontSize="12" Width="250" + MaxLength="100" Text="{Binding SelectedName, Mode=TwoWay}" />Plugins/Flow.Launcher.Plugin.Explorer/ViewModels/SettingsViewModel.cs (1)
330-422: Consider adding duplicate validation across all methods.The current implementation doesn't prevent duplicate paths from being added to collections. Consider adding validation to check for existing paths before adding new
AccessLinkinstances.private bool IsPathAlreadyExists(ICollection<AccessLink> collection, string path) { return collection.Any(link => string.Equals(link.Path, path, StringComparison.OrdinalIgnoreCase)); }Then use this validation in add/edit methods before modifying collections.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
Plugins/Flow.Launcher.Plugin.Explorer/ContextMenu.cs(1 hunks)Plugins/Flow.Launcher.Plugin.Explorer/Helper/PathHelper.cs(1 hunks)Plugins/Flow.Launcher.Plugin.Explorer/Languages/en.xaml(3 hunks)Plugins/Flow.Launcher.Plugin.Explorer/Main.cs(3 hunks)Plugins/Flow.Launcher.Plugin.Explorer/Search/QuickAccessLinks/AccessLink.cs(1 hunks)Plugins/Flow.Launcher.Plugin.Explorer/ViewModels/SettingsViewModel.cs(2 hunks)Plugins/Flow.Launcher.Plugin.Explorer/Views/ExplorerSettings.xaml(3 hunks)Plugins/Flow.Launcher.Plugin.Explorer/Views/QuickAccessLinkSettings.xaml(1 hunks)Plugins/Flow.Launcher.Plugin.Explorer/Views/QuickAccessLinkSettings.xaml.cs(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
Plugins/Flow.Launcher.Plugin.Explorer/ContextMenu.cs (1)
Plugins/Flow.Launcher.Plugin.Explorer/Helper/PathHelper.cs (1)
GetPathName(10-20)
Plugins/Flow.Launcher.Plugin.Explorer/Main.cs (1)
Plugins/Flow.Launcher.Plugin.Explorer/Helper/PathHelper.cs (1)
GetPathName(10-20)
🪛 GitHub Actions: Check Spelling
Plugins/Flow.Launcher.Plugin.Explorer/ContextMenu.cs
[warning] 73-73: Spell check warning: quickaccess is not a recognized word. (unrecognized-spelling)
[warning] 84-84: Spell check warning: addfilefoldersuccess is not a recognized word. (unrecognized-spelling)
[warning] 92-92: Spell check warning: contextmenu is not a recognized word. (unrecognized-spelling)
[warning] 92-92: Spell check warning: titletooltip is not a recognized word. (unrecognized-spelling)
[warning] 101-101: Spell check warning: quickaccess is not a recognized word. (unrecognized-spelling)
[warning] 102-102: Spell check warning: quickaccess is not a recognized word. (unrecognized-spelling)
[warning] 108-108: Spell check warning: removefilefoldersuccess is not a recognized word. (unrecognized-spelling)
[warning] 115-115: Spell check warning: contextmenu is not a recognized word. (unrecognized-spelling)
[warning] 115-115: Spell check warning: titletooltip is not a recognized word. (unrecognized-spelling)
[warning] 116-116: Spell check warning: contextmenu is not a recognized word. (unrecognized-spelling)
[warning] 116-116: Spell check warning: titletooltip is not a recognized word. (unrecognized-spelling)
[warning] 125-125: Spell check warning: copypath is not a recognized word. (unrecognized-spelling)
[warning] 148-148: Spell check warning: copyname is not a recognized word. (unrecognized-spelling)
[warning] 218-218: Spell check warning: deletefilefoldersuccess is not a recognized word. (unrecognized-spelling)
[warning] 240-240: Spell check warning: contextmenu is not a recognized word. (unrecognized-spelling)
[warning] 259-259: Spell check warning: runasdifferentuser is not a recognized word. (unrecognized-spelling)
[warning] 328-328: Spell check warning: opencontainingfolder is not a recognized word. (unrecognized-spelling)
[warning] 372-372: Spell check warning: openwitheditor is not a recognized word. (unrecognized-spelling)
[warning] 405-405: Spell check warning: openwithshell is not a recognized word. (unrecognized-spelling)
[warning] 453-453: Spell check warning: openindexingoptions is not a recognized word. (unrecognized-spelling)
[warning] 470-470: Spell check warning: openindexingoptions is not a recognized word. (unrecognized-spelling)
[warning] 486-486: Spell check warning: openwith is not a recognized word. (unrecognized-spelling)
Plugins/Flow.Launcher.Plugin.Explorer/Views/ExplorerSettings.xaml
[warning] 491-491: Spell check warning: previewpanel is not a recognized word. (unrecognized-spelling)
[warning] 498-498: Spell check warning: previewpanel is not a recognized word. (unrecognized-spelling)
[warning] 502-502: Spell check warning: previewpanel is not a recognized word. (unrecognized-spelling)
[warning] 506-506: Spell check warning: previewpanel is not a recognized word. (unrecognized-spelling)
[warning] 719-719: Spell check warning: lbx is not a recognized word. (unrecognized-spelling)
[warning] 794-794: Spell check warning: lbx is not a recognized word. (unrecognized-spelling)
[warning] 797-797: Spell check warning: lbx is not a recognized word. (unrecognized-spelling)
[warning] 798-798: Spell check warning: Lbx is not a recognized word. (unrecognized-spelling)
Plugins/Flow.Launcher.Plugin.Explorer/ViewModels/SettingsViewModel.cs
[warning] 260-260: Spell check warning: actionkeywordview is not a recognized word. (unrecognized-spelling)
[warning] 262-262: Spell check warning: actionkeywordview is not a recognized word. (unrecognized-spelling)
[warning] 264-264: Spell check warning: actionkeywordview is not a recognized word. (unrecognized-spelling)
[warning] 266-266: Spell check warning: actionkeywordview is not a recognized word. (unrecognized-spelling)
[warning] 266-266: Spell check warning: quickaccess is not a recognized word. (unrecognized-spelling)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: gitStream.cm
- GitHub Check: gitStream.cm
- GitHub Check: build
🔇 Additional comments (16)
Plugins/Flow.Launcher.Plugin.Explorer/Search/QuickAccessLinks/AccessLink.cs (1)
9-9: LGTM!The change from a computed property to a settable property correctly supports the new explicit naming feature for quick access links. This aligns well with the use of
PathHelper.GetPathName()for name assignment.Plugins/Flow.Launcher.Plugin.Explorer/Views/QuickAccessLinkSettings.xaml.cs (1)
37-52: 🛠️ Refactor suggestionPotential infinite recursion in SelectedName getter.
The
SelectedNamegetter calls_selectedPath.GetPathName()when_selectedNameis null or empty, but if_selectedPathis also null, this could cause issues. Additionally, this logic might not behave as expected in all scenarios.Consider simplifying the logic:
public string SelectedName { get { - if (string.IsNullOrEmpty(_selectedName)) return _selectedPath.GetPathName(); - return _selectedName; + return _selectedName ?? string.Empty; } set { if (_selectedName != value) { _selectedName = value; OnPropertyChanged(); } } }The automatic name generation is already handled in the setter of
SelectedPath(line 29), so the getter doesn't need this logic.Likely an incorrect or invalid review comment.
Plugins/Flow.Launcher.Plugin.Explorer/ContextMenu.cs (1)
78-81: LGTM!The addition of the
Nameproperty assignment usingGetPathName()correctly implements the new explicit naming feature for quick access links. This change aligns perfectly with the refactoring of theAccessLinkclass and the introduction of thePathHelperutility.The pipeline spelling warnings for translation keys like "quickaccess", "addfilefoldersuccess", etc. are false positives since these are intentionally compound words used as localization keys.
Plugins/Flow.Launcher.Plugin.Explorer/Main.cs (2)
9-9: LGTM - Required import for LINQ operations.The
System.Linqimport is necessary for theforeachloop in the newFixLegacyQuickAccessLinkNamesmethod.
39-39: Good placement for legacy data migration.Calling
FixLegacyQuickAccessLinkNames()here is appropriate - it runs after settings are loaded but before the view model is created, ensuring legacy quick access links get proper names.Plugins/Flow.Launcher.Plugin.Explorer/Views/ExplorerSettings.xaml (4)
722-722: Improved binding mode for better selection handling.Changing to
Mode=TwoWayensures proper bidirectional communication between the UI and view model for selected item tracking.
723-741: Excellent UI enhancement with GridView layout.The transition from a simple DataTemplate to a GridView with separate Name and Path columns significantly improves the user experience. The layout provides clear visual separation and makes it easy to distinguish between the customizable name and the actual path. The ellipsis trimming on the path column is a nice touch for handling long paths.
759-759: Good command naming specificity.Renaming from generic
EditLinkCommandandAddLinkCommandtoEditQuickAccessLinkCommandandAddQuickAccessLinkCommandimproves code clarity and maintainability by explicitly indicating the purpose of each command.Also applies to: 765-765
819-819: Consistent command naming pattern.The renaming to
EditIndexSearchExcludePathsCommandandAddIndexSearchExcludePathsCommandmaintains consistency with the quick access link command naming pattern and clearly separates functionality.Also applies to: 825-825
Plugins/Flow.Launcher.Plugin.Explorer/Languages/en.xaml (2)
8-9: Helpful validation messages for user guidance.These new strings provide clear and actionable feedback to users when managing quick access links. The messages guide users to select a folder path or choose different values when conflicts arise.
32-32: Well-crafted UI labels for the new dialog.The new header "Customise Quick Access" and label "Name:" appropriately support the new QuickAccessLinkSettings dialog interface, maintaining consistency with the existing localization patterns.
Also applies to: 98-98
Plugins/Flow.Launcher.Plugin.Explorer/Views/QuickAccessLinkSettings.xaml (4)
1-19: Well-configured window properties for modal dialog.The window configuration is appropriate for a modal dialog:
ResizeMode="NoResize"prevents unintended resizingWindowStartupLocation="CenterScreen"ensures proper positioningSizeToContent="Width"with fixed height provides optimal sizing- Dynamic resource bindings maintain theme consistency
35-55: Professional title bar implementation.The custom title bar with close button follows modern WPF patterns. The path styling with opacity changes based on window activation state provides good visual feedback to the user.
67-108: Logical input field layout with good UX patterns.The horizontal arrangement of label and input fields is clean and intuitive. Making the path field read-only with a dedicated "Select" button is a good UX pattern that prevents manual path entry errors and guides users to use proper folder selection.
112-136: Standard dialog button layout.The bottom button area follows Windows dialog conventions with Cancel and Done buttons. The accent styling on the Done button appropriately emphasizes the primary action.
Plugins/Flow.Launcher.Plugin.Explorer/ViewModels/SettingsViewModel.cs (1)
418-422: Good implementation of helper method.The
ShowUnselectedMessagemethod is well-implemented with proper localization support and clear responsibility separation.
Plugins/Flow.Launcher.Plugin.Explorer/Views/QuickAccessLinkSettings.xaml.cs
Outdated
Show resolved
Hide resolved
Plugins/Flow.Launcher.Plugin.Explorer/Views/QuickAccessLinkSettings.xaml.cs
Outdated
Show resolved
Hide resolved
| if (QuickAccessLinks.Any(x => x.Path == SelectedPath && x.Name == SelectedName)) | ||
| { | ||
| var warning = Main.Context.API.GetTranslation("plugin_explorer_quick_access_link_select_different_folder"); | ||
| Main.Context.API.ShowMsgBox(warning); | ||
| return; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve duplicate link validation logic.
The current validation only checks if both name and path match exactly, but it should also prevent duplicate paths with different names, as this could confuse users.
Apply this diff to improve validation:
-if (QuickAccessLinks.Any(x => x.Path == SelectedPath && x.Name == SelectedName))
+if (QuickAccessLinks.Any(x => x.Path.Equals(SelectedPath, StringComparison.OrdinalIgnoreCase) && !ReferenceEquals(x, SelectedAccessLink)))
{
- var warning = Main.Context.API.GetTranslation("plugin_explorer_quick_access_link_select_different_folder");
+ var warning = Main.Context.API.GetTranslation("plugin_explorer_quick_access_link_path_already_exists");
Main.Context.API.ShowMsgBox(warning);
return;
}This change:
- Uses case-insensitive path comparison
- Excludes the currently edited link from the duplicate check
- Focuses on path uniqueness rather than name+path combination
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (QuickAccessLinks.Any(x => x.Path == SelectedPath && x.Name == SelectedName)) | |
| { | |
| var warning = Main.Context.API.GetTranslation("plugin_explorer_quick_access_link_select_different_folder"); | |
| Main.Context.API.ShowMsgBox(warning); | |
| return; | |
| } | |
| if (QuickAccessLinks.Any(x => | |
| x.Path.Equals(SelectedPath, StringComparison.OrdinalIgnoreCase) | |
| && !ReferenceEquals(x, SelectedAccessLink))) | |
| { | |
| var warning = Main.Context.API.GetTranslation("plugin_explorer_quick_access_link_path_already_exists"); | |
| Main.Context.API.ShowMsgBox(warning); | |
| return; | |
| } |
🤖 Prompt for AI Agents
In Plugins/Flow.Launcher.Plugin.Explorer/Views/QuickAccessLinkSettings.xaml.cs
around lines 93 to 98, the duplicate link validation only checks for exact
matches of both path and name, which misses cases where paths are duplicated
with different names. Update the validation to check for duplicate paths only,
using a case-insensitive comparison, and exclude the currently edited link from
this check to avoid false positives. This will ensure path uniqueness and
prevent user confusion.
Plugins/Flow.Launcher.Plugin.Explorer/ViewModels/SettingsViewModel.cs
Outdated
Show resolved
Hide resolved
Plugins/Flow.Launcher.Plugin.Explorer/ViewModels/SettingsViewModel.cs
Outdated
Show resolved
Hide resolved
Plugins/Flow.Launcher.Plugin.Explorer/ViewModels/SettingsViewModel.cs
Outdated
Show resolved
Hide resolved
| private void EditIndexSearchExcludePaths(object commandParameter) | ||
| { | ||
| var (selectedLink, collection) = commandParameter switch | ||
| { | ||
| "QuickAccessLink" => (SelectedQuickAccessLink, Settings.QuickAccessLinks), | ||
| "IndexSearchExcludedPaths" => (SelectedIndexSearchExcludedPath, Settings.IndexSearchExcludedSubdirectoryPaths), | ||
| _ => throw new ArgumentOutOfRangeException(nameof(commandParameter)) | ||
| }; | ||
|
|
||
| if (selectedLink is null) | ||
| var collection = Settings.IndexSearchExcludedSubdirectoryPaths; | ||
|
|
||
| if (SelectedIndexSearchExcludedPath is null) | ||
| { | ||
| ShowUnselectedMessage(); | ||
| return; | ||
| } | ||
|
|
||
| var path = PromptUserSelectPath(selectedLink.Type, | ||
| selectedLink.Type == ResultType.Folder | ||
| ? selectedLink.Path | ||
| : Path.GetDirectoryName(selectedLink.Path)); | ||
| var path = PromptUserSelectPath(SelectedIndexSearchExcludedPath.Type, | ||
| SelectedIndexSearchExcludedPath.Type == ResultType.Folder | ||
| ? SelectedIndexSearchExcludedPath.Path | ||
| : Path.GetDirectoryName(SelectedIndexSearchExcludedPath.Path)); | ||
|
|
||
| if (path is null) | ||
| return; | ||
|
|
||
| collection.Remove(selectedLink); | ||
| var selectedType = SelectedIndexSearchExcludedPath.Type; | ||
| collection.Remove(SelectedIndexSearchExcludedPath); | ||
| collection.Add(new AccessLink | ||
| { | ||
| Path = path, Type = selectedLink.Type, | ||
| Path = path, Type = selectedType, | ||
| }); | ||
| } |
There was a problem hiding this comment.
Missing Name property assignment for AccessLink.
The method creates a new AccessLink but only sets Path and Type properties. According to the AI summary, the AccessLink class now requires explicit Name assignment instead of using a computed property.
collection.Add(new AccessLink
{
- Path = path, Type = selectedType,
+ Path = path,
+ Type = selectedType,
+ Name = PathHelper.GetPathName(path)
});Also consider adding validation to prevent duplicate paths and unnecessary operations when the path hasn't changed.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private void EditIndexSearchExcludePaths(object commandParameter) | |
| { | |
| var (selectedLink, collection) = commandParameter switch | |
| { | |
| "QuickAccessLink" => (SelectedQuickAccessLink, Settings.QuickAccessLinks), | |
| "IndexSearchExcludedPaths" => (SelectedIndexSearchExcludedPath, Settings.IndexSearchExcludedSubdirectoryPaths), | |
| _ => throw new ArgumentOutOfRangeException(nameof(commandParameter)) | |
| }; | |
| if (selectedLink is null) | |
| var collection = Settings.IndexSearchExcludedSubdirectoryPaths; | |
| if (SelectedIndexSearchExcludedPath is null) | |
| { | |
| ShowUnselectedMessage(); | |
| return; | |
| } | |
| var path = PromptUserSelectPath(selectedLink.Type, | |
| selectedLink.Type == ResultType.Folder | |
| ? selectedLink.Path | |
| : Path.GetDirectoryName(selectedLink.Path)); | |
| var path = PromptUserSelectPath(SelectedIndexSearchExcludedPath.Type, | |
| SelectedIndexSearchExcludedPath.Type == ResultType.Folder | |
| ? SelectedIndexSearchExcludedPath.Path | |
| : Path.GetDirectoryName(SelectedIndexSearchExcludedPath.Path)); | |
| if (path is null) | |
| return; | |
| collection.Remove(selectedLink); | |
| var selectedType = SelectedIndexSearchExcludedPath.Type; | |
| collection.Remove(SelectedIndexSearchExcludedPath); | |
| collection.Add(new AccessLink | |
| { | |
| Path = path, Type = selectedLink.Type, | |
| Path = path, Type = selectedType, | |
| }); | |
| } | |
| var selectedType = SelectedIndexSearchExcludedPath.Type; | |
| collection.Remove(SelectedIndexSearchExcludedPath); | |
| collection.Add(new AccessLink | |
| { | |
| Path = path, | |
| Type = selectedType, | |
| Name = PathHelper.GetPathName(path) | |
| }); |
🤖 Prompt for AI Agents
In Plugins/Flow.Launcher.Plugin.Explorer/ViewModels/SettingsViewModel.cs lines
332 to 357, the new AccessLink instance is missing the required Name property
assignment. Update the code to explicitly set the Name property when creating
the new AccessLink. Additionally, add validation to check if the new path is
different from the existing one and ensure no duplicate paths are added to the
collection to avoid unnecessary operations.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Jack251970
changed the title
|
Thanks for your PR! I will check it after 1.20.0 release. |
@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 ...
|
Issue: #2207
Screenshots
Notes
"Name"is not provided, it will be derived from the file or folder name in the given path.