Support More Information for Files, Folders and Volumes Tooltips & Add Custom Preview Panel Support for Folder Results #3631
Conversation
This comment has been minimized.
This comment has been minimized.
|
🥷 Code experts: no user but you matched threshold 10 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:
Activity based on git-commit:
Knowledge based on git-blame:
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 |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
Warning Rate limit exceeded@Jack251970 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 23 minutes and 39 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis change introduces a new setting to the Explorer plugin allowing users to display more detailed information—such as size, creation date, and modification date—in file, folder, and volume tooltips. It updates UI resources, settings, tooltip logic, and metadata retrieval methods to support and conditionally display this enhanced tooltip information. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ExplorerPlugin
participant ResultManager
participant PreviewPanel
User->>ExplorerPlugin: Search for files/folders
ExplorerPlugin->>ResultManager: Build search results
ResultManager->>Settings: Check DisplayMoreInformationInToolTip
alt If enabled
ResultManager->>PreviewPanel: Get size, creation, modification info
PreviewPanel-->>ResultManager: Return formatted details
ResultManager->>ExplorerPlugin: Set detailed tooltip
else If disabled
ResultManager->>ExplorerPlugin: Set plain path as tooltip
end
ExplorerPlugin-->>User: Display search results with tooltips
Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes found. Possibly related PRs
Suggested labels
Suggested reviewers
Poem
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: 4
🧹 Nitpick comments (1)
Plugins/Flow.Launcher.Plugin.Explorer/Views/PreviewPanel.xaml.cs (1)
94-105: Consider refactoring to reduce code duplication.The file and folder date formatting methods share significant logic. Consider extracting the common formatting logic into a shared helper method.
+private static string FormatDateTime(DateTime dateTime, string dateFormat, string timeFormat, bool showAge) +{ + var formattedDate = dateTime.ToString($"{dateFormat} {timeFormat}", CultureInfo.CurrentCulture); + return showAge ? $"{GetFileAge(dateTime)} - {formattedDate}" : formattedDate; +} public static string GetFileCreatedAt(string filePath, string previewPanelDateFormat, string previewPanelTimeFormat, bool showFileAgeInPreviewPanel) { try { var createdDate = File.GetCreationTime(filePath); - var formattedDate = createdDate.ToString( - $"{previewPanelDateFormat} {previewPanelTimeFormat}", - CultureInfo.CurrentCulture - ); - - var result = formattedDate; - if (showFileAgeInPreviewPanel) result = $"{GetFileAge(createdDate)} - {formattedDate}"; - return result; + return FormatDateTime(createdDate, previewPanelDateFormat, previewPanelTimeFormat, showFileAgeInPreviewPanel); } catch (Exception) { return Main.Context.API.GetTranslation("plugin_explorer_plugin_tooltip_more_info_unknown"); } }Also applies to: 107-118, 138-149, 151-162
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
Plugins/Flow.Launcher.Plugin.Explorer/Languages/en.xaml(2 hunks)Plugins/Flow.Launcher.Plugin.Explorer/Search/ResultManager.cs(5 hunks)Plugins/Flow.Launcher.Plugin.Explorer/Settings.cs(1 hunks)Plugins/Flow.Launcher.Plugin.Explorer/Views/ExplorerSettings.xaml(11 hunks)Plugins/Flow.Launcher.Plugin.Explorer/Views/PreviewPanel.xaml.cs(3 hunks)
🧰 Additional context used
🪛 GitHub Actions: Check Spelling
Plugins/Flow.Launcher.Plugin.Explorer/Views/ExplorerSettings.xaml
[warning] 501-501: previewpanel is not a recognized word. (unrecognized-spelling)
[warning] 508-508: previewpanel is not a recognized word. (unrecognized-spelling)
[warning] 512-512: previewpanel is not a recognized word. (unrecognized-spelling)
[warning] 516-516: previewpanel is not a recognized word. (unrecognized-spelling)
[warning] 729-729: lbx is not a recognized word. (unrecognized-spelling)
[warning] 786-786: lbx is not a recognized word. (unrecognized-spelling)
[warning] 789-789: lbx is not a recognized word. (unrecognized-spelling)
[warning] 790-790: Lbx is not a recognized word. (unrecognized-spelling)
Plugins/Flow.Launcher.Plugin.Explorer/Search/ResultManager.cs
[warning] 134-134: opendir is not a recognized word. (unrecognized-spelling)
[warning] 149-149: opendir is not a recognized word. (unrecognized-spelling)
[warning] 174-174: drv is not a recognized word. (unrecognized-spelling)
[warning] 175-175: drv is not a recognized word. (unrecognized-spelling)
[warning] 176-176: totalspace is not a recognized word. (unrecognized-spelling)
[warning] 177-177: drv is not a recognized word. (unrecognized-spelling)
[warning] 177-177: drv is not a recognized word. (unrecognized-spelling)
[warning] 245-245: openresultfolder is not a recognized word. (unrecognized-spelling)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: gitStream workflow automation
- GitHub Check: gitStream.cm
- GitHub Check: build
🔇 Additional comments (10)
Plugins/Flow.Launcher.Plugin.Explorer/Settings.cs (1)
40-40: LGTM! Clean implementation of the new setting.The property follows established patterns in the class with a sensible default value and descriptive name.
Plugins/Flow.Launcher.Plugin.Explorer/Views/ExplorerSettings.xaml (2)
201-201: LGTM! Proper XAML implementation for the new setting.The Grid layout is correctly maintained by adding a new RowDefinition and shifting subsequent elements. The CheckBox is properly bound and uses appropriate localization.
Also applies to: 226-234
235-403: Grid row indices properly adjusted.All subsequent controls have been correctly shifted down by one row to accommodate the new setting.
Plugins/Flow.Launcher.Plugin.Explorer/Search/ResultManager.cs (3)
102-102: LGTM! Proper PreviewPanel integration with lazy initialization.The changes correctly replace the Preview property with PreviewPanel and pass the necessary parameters (Settings, path, ResultType).
Also applies to: 286-286
163-163: Good conditional tooltip implementation.The logic properly checks the setting and falls back to the simple path when detailed information is disabled.
Also applies to: 317-317
348-367:❓ Verification inconclusive
Verify PreviewPanel static methods exist and add error handling.
The implementation looks good but needs verification that the static methods are available and proper error handling for IO operations.
Please run this script to verify the PreviewPanel static methods exist:
Consider adding try-catch blocks around the PreviewPanel method calls to handle potential IO exceptions gracefully:
private static string GetMoreInfoToolTip(string filePath, ResultType type) { + try + { switch (type) { case ResultType.Folder: var folderSize = PreviewPanel.GetFolderSize(filePath); // ... rest of implementation case ResultType.File: var fileSize = PreviewPanel.GetFileSize(filePath); // ... rest of implementation default: return filePath; } + } + catch (Exception) + { + return filePath; // Fallback to simple path on error + } }
🏁 Script executed:
#!/bin/bash # Verify PreviewPanel static methods are implemented echo "Searching for PreviewPanel static methods..." # Check for GetFileSize, GetFolderSize methods ast-grep --pattern $'class PreviewPanel { $$$ static $_ GetFileSize($_) { $$$ } $$$ }' # Check for date/time methods ast-grep --pattern $'class PreviewPanel { $$$ static $_ GetFileCreatedAt($$$) { $$$ } $$$ }' # Check for GetFolderSize method ast-grep --pattern $'class PreviewPanel { $$$ static $_ GetFolderSize($_) { $$$ } $$$ }'Length of output: 406
🏁 Script executed:
#!/bin/bash echo "Locating PreviewPanel class definition..." preview_file=$(rg -l "^class PreviewPanel") if [ -z "$preview_file" ]; then echo "❌ PreviewPanel class not found in repository." exit 0 fi echo "✅ Found PreviewPanel in: $preview_file" echo echo "Listing static methods in PreviewPanel starting with 'Get':" grep -n "static .*Get" "$preview_file"Length of output: 265
Ensure PreviewPanel methods exist and guard against IO errors
Before merging, please confirm that all the
PreviewPanelstatic helpers you’re calling are exposed by the plugin or a referenced assembly—e.g.
PreviewPanel.GetFileSize(string)PreviewPanel.GetFolderSize(string)PreviewPanel.GetFileCreatedAt(string, …)PreviewPanel.GetFolderCreatedAt(string, …)PreviewPanel.GetFileLastModifiedAt(string, …)PreviewPanel.GetFolderLastModifiedAt(string, …)If any of these live in another project or assembly, make sure you’ve added the correct reference or NuGet package.
Then wrap the calls in a try/catch around the switch so that any unexpected IO errors fall back to returning the path:
private static string GetMoreInfoToolTip(string filePath, ResultType type) { + try + { switch (type) { case ResultType.Folder: var folderSize = PreviewPanel.GetFolderSize(filePath); // …same as before case ResultType.File: var fileSize = PreviewPanel.GetFileSize(filePath); // …same as before default: return filePath; } + } + catch (IOException) + { + // Fallback if file/folder info can’t be read + return filePath; + } }Plugins/Flow.Launcher.Plugin.Explorer/Languages/en.xaml (2)
46-46: LGTM! Descriptive and user-friendly setting label.The localization string clearly describes what the setting does.
83-84: Well-structured tooltip format strings.The format string properly uses positional parameters that match the usage in
GetMoreInfoToolTipmethod. The "Unknown" string provides a good fallback for unavailable data.Plugins/Flow.Launcher.Plugin.Explorer/Views/PreviewPanel.xaml.cs (2)
63-63: Good refactoring to separate file and folder logic.The conditional logic based on
ResultTypeappropriately delegates to specialized static methods, improving code organization and maintainability.Also applies to: 68-70, 75-77
53-53:✅ Verification successful
Verify the constructor signature change across the codebase.
The constructor now requires a new
ResultType typeparameter. Ensure all instantiations ofPreviewPanelhave been updated to include this parameter.
🏁 Script executed:
#!/bin/bash # Description: Find all PreviewPanel constructor calls to verify they include the new parameter # Expected: All constructor calls should include the ResultType parameter # Search for PreviewPanel constructor calls rg -A 3 "new PreviewPanel\(" --type csLength of output: 1023
Constructor signature change verified
All instantiations of
PreviewPanelnow include the newResultTypeparameter:
- Plugins/Flow.Launcher.Plugin.Explorer/Search/ResultManager.cs:
new PreviewPanel(Settings, path, ResultType.Folder)new PreviewPanel(Settings, filePath, ResultType.File)No further updates required.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
@coderabbitai Review. |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain@Jack251970, I'll review this PR comprehensively. This is a nice enhancement that adds detailed tooltip information for files and folders with proper user control. 🏁 Script executed: Length of output: 27977 🏁 Script executed: Length of output: 28021 🏁 Script executed: Length of output: 44123 Excellent work on this feature enhancement! The implementation is well-structured with good separation of concerns and proper user choice. Here's my comprehensive review: ✅ Strengths
🔧 Areas for Improvement1. Performance - Folder Size CalculationThe folder size calculation has potential performance issues: 🏁 Script executed: Length of output: 45683 Current Issues: // ❌ Problem: Synchronous, blocks UI thread
public static string GetFolderSize(string folderPath)
{
// This runs on UI thread and can freeze interface
var cancellationTokenSource = new CancellationTokenSource(TimeSpan.FromSeconds(3));
foreach (var file in directoryInfo.GetFiles("*", SearchOption.AllDirectories))
{
// Timeout check doesn't actually cancel file enumeration
if (cancellationTokenSource.Token.IsCancellationRequested)
return "Unknown";
size += file.Length;
}
}Recommendations:
public static async Task<string> GetFolderSizeAsync(string folderPath, CancellationToken cancellationToken = default)
{
return await Task.Run(() =>
{
using var timeoutCts = new CancellationTokenSource(TimeSpan.FromSeconds(3));
using var combinedCts = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken, timeoutCts.Token);
try
{
// Use parallel enumeration for better performance
var directoryInfo = new DirectoryInfo(folderPath);
return directoryInfo.EnumerateFiles("*", SearchOption.AllDirectories)
.AsParallel()
.WithCancellation(combinedCts.Token)
.Sum(file => file.Length);
}
catch (OperationCanceledException)
{
return -1; // Indicate timeout/cancellation
}
}, cancellationToken);
}
private static readonly ConcurrentDictionary<string, (DateTime lastWrite, string size)> _folderSizeCache = new();
public static string GetFolderSize(string folderPath)
{
var lastWrite = Directory.GetLastWriteTime(folderPath);
if (_folderSizeCache.TryGetValue(folderPath, out var cached) && cached.lastWrite >= lastWrite)
return cached.size;
// Calculate and cache result
var size = CalculateFolderSize(folderPath);
_folderSizeCache[folderPath] = (lastWrite, size);
return size;
}2. Resource Management Issue// ❌ Current: CancellationTokenSource not disposed
var cancellationTokenSource = new CancellationTokenSource(TimeSpan.FromSeconds(3));
// ✅ Should be:
using var cancellationTokenSource = new CancellationTokenSource(TimeSpan.FromSeconds(3));3. UI/UX ConsiderationsThe tooltip generation should be fast. Consider showing basic info immediately and updating with detailed info asynchronously: SubTitleToolTip = Settings.DisplayMoreInformationInToolTip
? $"{filePath}\nLoading detailed info..."
: filePath
// Then update asynchronously when detailed info is available🎯 Specific Suggestions
📝 Minor Issues
🔧 Performance Test SuggestionTest the feature with:
Ensure the UI remains responsive in all scenarios. Overall, this is a valuable feature with solid implementation. The main concern is performance optimization for folder operations. Would you like me to help create an optimized version of the folder size calculation? |
Co-authored-by: Copilot <[email protected]>
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
@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 ...
|
|
Note: This is currently only shown as a tooltip in the path area. It might be better to remove the Ctrl+Enter hint and apply the tooltip to the entire area instead. Although it would be nice to have a setting for tooltip display speed, I’d prefer not to add one. It might be a good idea to adjust the tooltip speed dynamically—for example, make it faster when mouse interaction is detected. Let’s consider this for later. |
3 participants
Changes
Resolve #3518.
Test
Change option here:
And search for files & folders:
Add MMM support for date format: