Quick Switch #1018
Quick Switch #1018
Conversation
|
Some threading issues seem appearing. Not sure the detailed reason. |
|
Okay, my main questions would be
|
I think the major use case is to sync the path of an opened explorer to a open file dialog to select a file easily.
Sry I don't get the idea. |
|
Okay, I understand what this is used for now. I'd have to dig a lot deeper into what the IUIAutomation can do to be able to improve this. I think the rule of thumb is to avoid sending keyboard events, and instead always use an API if one exists. Keyboard events can be delayed and whatnot. |
Yeah that's what I would like to see. It is possible to use PInvoke directly without IUIAutomation though, so it will be cool if you are familiar with that as well. Another thing is the original listary seems implement this feature without changing the textbox and sending an enter signal, so I wonder whether you may have some clues about that. |
|
I tried searching for what I could, but that's apparently quite tricky to hook into. So I don't really have a better solution at the moment. |
okay thanks🤣 |
|
There might be a alternate design: So the file manager has the "quick access" sidebar. Flow could add its own entry there, and that entry always redirects to the currently open folder. An additional advantage might be that it's easier to discover this, compared to a keyboard shortcut. Screenshot for context:
(Note: I have no idea how hard that would be to efficiently pull that off.) |
|
So you mean to add a entry that redirect to the most recent opened explorer path?🤔Interesting |
|
Yep, spot-on. |
If that's the case, we may be able to create a plugin for it. |
|
Do you have any docs for that? |
|
@taooceros I haven't looked into this all that much (just a few cursory google searches) Programmatic accessApparently there's a way of programmatically adding folders to the quick access area. Special Links folderhttps://blogs.msmvps.com/kenlin/2017/06/14/537/ Steps:
Symbolic links or HardlinkI bet there's some trickery that could be done with those Extra harddriveWe could add an in-memory harddrive, mount it and provide a single shortcut in there. |
|
Could this be done? I really love this feature. |
|
Yet another option would be to add a "switch to" context menu entry Sort of like how 7zip has a dynamic context menu, except that we'd populate it with the titles of other explorer windows. |
|
Apparently Windows 11 can add files to quick access. That might let us pin a program to quick access Such a program could then update the list of files in the quick access window. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (6)
Flow.Launcher/ViewModel/MainViewModel.cs (6)
373-386:⚠️ Potential issueUnhandled exceptions in async navigation task.
The Quick Switch navigation task is launched without error handling, causing any exceptions to be silently dropped.
Apply this fix to properly handle exceptions during navigation:
- _ = Task.Run(() => QuickSwitch.JumpToPathAsync(DialogWindowHandle, quickSwitchResult.QuickSwitchPath)); + _ = Task.Run(async () => + { + try + { + await QuickSwitch.JumpToPathAsync(DialogWindowHandle, quickSwitchResult.QuickSwitchPath) + .ConfigureAwait(false); + } + catch (Exception ex) + { + App.API.LogException(ClassName, "Quick switch navigation failed", ex); + } + });
467-477:⚠️ Potential issueSame unhandled exception issue in left-click handler.
Similar to the right-click handler, this fire-and-forget task doesn't handle exceptions.
Apply the same fix pattern here:
- _ = Task.Run(() => QuickSwitch.JumpToPathAsync(DialogWindowHandle, quickSwitchResult.QuickSwitchPath)); + _ = Task.Run(async () => + { + try + { + await QuickSwitch.JumpToPathAsync(DialogWindowHandle, quickSwitchResult.QuickSwitchPath) + .ConfigureAwait(false); + } + catch (Exception ex) + { + App.API.LogException(ClassName, "Quick switch navigation failed", ex); + } + });
1766-1769:⚠️ Potential issueAdd thread-safety to Quick Switch state variables.
These properties are accessed from multiple threads (UI thread, background tasks) without synchronization.
+private readonly object _quickSwitchLock = new(); +private nint _dialogWindowHandle = nint.Zero; +private bool _isQuickSwitch = false; -public nint DialogWindowHandle { get; private set; } = nint.Zero; +public nint DialogWindowHandle +{ + get + { + lock (_quickSwitchLock) + { + return _dialogWindowHandle; + } + } + private set + { + lock (_quickSwitchLock) + { + _dialogWindowHandle = value; + } + } +} -private bool _isQuickSwitch = false; +private bool IsQuickSwitch +{ + get + { + lock (_quickSwitchLock) + { + return _isQuickSwitch; + } + } + set + { + lock (_quickSwitchLock) + { + _isQuickSwitch = value; + } + } +}
1784-1870:⚠️ Potential issueFix token capture race condition in SetupQuickSwitchAsync.
Creating a new CancellationTokenSource and using its token inside a Task.Run lambda creates a race condition if another dialog arrives before the task runs.
// Create a new cancellation token source _quickSwitchSource = new CancellationTokenSource(); +// Capture token locally to avoid race conditions +var token = _quickSwitchSource.Token; _ = Task.Run(() => { try { // Check task cancellation - if (_quickSwitchSource.Token.IsCancellationRequested) return; + if (token.IsCancellationRequested) return; // Check dialog handle if (DialogWindowHandle == nint.Zero) return; // Wait 150ms to check if quick switch window gets the focus var timeOut = !SpinWait.SpinUntil(() => !Win32Helper.IsForegroundWindow(DialogWindowHandle), 150); if (timeOut) return; // Bring focus back to the the dialog Win32Helper.SetForegroundWindow(DialogWindowHandle); } catch (Exception e) { App.API.LogException(ClassName, "Failed to focus on dialog window", e); } });
1874-1914:⚠️ Potential issueChange async void to Task return type.
Async void methods can cause unhandled exceptions and are harder to test and use properly.
-public async void ResetQuickSwitch() +public async Task ResetQuickSwitch() { if (DialogWindowHandle == nint.Zero) return; DialogWindowHandle = nint.Zero; _isQuickSwitch = false; if (_previousMainWindowVisibilityStatus != MainWindowVisibilityStatus) { // Show or hide to change visibility if (_previousMainWindowVisibilityStatus) { Show(); _ = ResetWindowAsync(); } else { await ResetWindowAsync(); Hide(false); } } else { if (_previousMainWindowVisibilityStatus) { // Only update the position Application.Current?.Dispatcher.Invoke(() => { (Application.Current?.MainWindow as MainWindow).UpdatePosition(); }); _ = ResetWindowAsync(); } else { _ = ResetWindowAsync(); } } }
1918-1933:⚠️ Potential issueVerify window handle validity in HideQuickSwitch.
The method assumes the DialogWindowHandle is valid if non-zero, but Windows handles can be reused or invalidated.
public void HideQuickSwitch() { if (DialogWindowHandle != nint.Zero) { + // Verify the handle is still valid + if (!Win32Helper.IsWindow(DialogWindowHandle)) + { + DialogWindowHandle = nint.Zero; + _isQuickSwitch = false; + return; + } + if (QuickSwitch.QuickSwitchWindowPosition == QuickSwitchWindowPositions.UnderDialog) { // Warning: Main window is already in foreground // This is because if you click popup menus in other applications to hide quick switch window, // they can steal focus before showing main window if (MainWindowVisibilityStatus) { Hide(); } } } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Flow.Launcher/ViewModel/MainViewModel.cs(16 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (4)
Flow.Launcher/ViewModel/MainViewModel.cs (4)
56-56: LGTM: Added empty QuickSwitchResult list.A read-only list of empty QuickSwitchResult objects - a good practice to avoid creating new collections unnecessarily.
1525-1540: LGTM: Well-structured ClearResults method.The method properly handles the three main UI aspects: clearing results, resetting plugin icon, and hiding the progress bar.
1936-1956: LGTM: Well-structured ResetWindowAsync method.This method properly handles all aspects of window reset: resetting history, closing previews, selecting results, and clearing query text.
2177-2177: Good resource management for QuickSwitch.Properly disposing the QuickSwitch cancellation token source in the Dispose method prevents potential memory leaks.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
@Jack251970 I haven't looked at the code but for Files Explorer feature, can we separate it into another PR? Try to keep PR focused on one feature (ideally smaller chunks of a large PR so it's easier for review but I know that's not always applicable) |
|
|
Well, I can remove all changes in |
This comment has been minimized.
This comment has been minimized.
Well, removed all changes for Files explorer support. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (7)
Flow.Launcher/ViewModel/MainViewModel.cs (7)
518-519:⚠️ Potential issueAdd type checking to prevent casting exceptions
Casting directly to QuickSwitchResult without checking the type first could lead to InvalidCastException.
- var resultCopy = ((QuickSwitchResult)result).Clone(); - resultsCopy.Add(resultCopy); + if (result is QuickSwitchResult quickSwitchResult) + { + var resultCopy = quickSwitchResult.Clone(); + resultsCopy.Add(resultCopy); + } + else + { + App.API.LogWarning(ClassName, $"Expected QuickSwitchResult but got {result.GetType().Name}"); + var resultCopy = result.Clone(); + resultsCopy.Add(resultCopy); + }
1782-1798: 🛠️ Refactor suggestionReplace async void with async Task and add null check for Application.Current
You should avoid async void methods for better exception handling, and add null check for Application.Current to prevent potential NullReferenceException.
- public async Task SetupQuickSwitchAsync(nint handle) + public async Task SetupQuickSwitchAsync(nint handle) { if (handle == nint.Zero) return; // Only set flag & reset window once for one file dialog var dialogWindowHandleChanged = false; if (DialogWindowHandle != handle) { DialogWindowHandle = handle; _previousMainWindowVisibilityStatus = MainWindowVisibilityStatus; _isQuickSwitch = true; dialogWindowHandleChanged = true; // If don't give a time, Positioning will be weird await Task.Delay(300); } }
1846-1866:⚠️ Potential issueCapture cancellation token locally to avoid race conditions
Accessing
_quickSwitchSource.Tokeninside the lambda could lead to race conditions if_quickSwitchSourceis modified by another thread.// Create a new cancellation token source _quickSwitchSource = new CancellationTokenSource(); +var token = _quickSwitchSource.Token; _ = Task.Run(() => { try { // Check task cancellation - if (_quickSwitchSource.Token.IsCancellationRequested) return; + if (token.IsCancellationRequested) return; // Check dialog handle if (DialogWindowHandle == nint.Zero) return; // Wait 150ms to check if quick switch window gets the focus var timeOut = !SpinWait.SpinUntil(() => !Win32Helper.IsForegroundWindow(DialogWindowHandle), 150); if (timeOut) return; // Bring focus back to the the dialog Win32Helper.SetForegroundWindow(DialogWindowHandle); } catch (Exception e) { App.API.LogException(ClassName, "Failed to focus on dialog window", e); } });
1872-1912: 🛠️ Refactor suggestionChange ResetQuickSwitch from async void to async Task
Using async void methods can lead to unhandled exceptions and make the method harder to test.
-public async void ResetQuickSwitch() +public async Task ResetQuickSwitch() { if (DialogWindowHandle == nint.Zero) return; DialogWindowHandle = nint.Zero; _isQuickSwitch = false; // Rest of the method remains the same }
1918-1925: 🛠️ Refactor suggestionVerify dialog window handle is valid in HideQuickSwitch
The method should check if the handle is still valid before attempting to use it.
public void HideQuickSwitch() { if (DialogWindowHandle != nint.Zero) { + // Verify the handle is still valid + if (!Win32Helper.IsWindow(DialogWindowHandle)) + { + DialogWindowHandle = nint.Zero; + _isQuickSwitch = false; + return; + } if (QuickSwitch.QuickSwitchWindowPosition == QuickSwitchWindowPositions.UnderDialog) { // Rest of the method remains the same } } }
476-477:⚠️ Potential issueAdd error handling for path navigation in OpenResultAsync
Similar to the previous comment, this Task.Run call lacks error handling.
- _ = Task.Run(() => QuickSwitch.JumpToPathAsync(DialogWindowHandle, quickSwitchResult.QuickSwitchPath)); + _ = Task.Run(async () => + { + try + { + await QuickSwitch.JumpToPathAsync(DialogWindowHandle, quickSwitchResult.QuickSwitchPath); + } + catch (Exception ex) + { + App.API.LogException(ClassName, "Failed to navigate to path", ex); + } + });
374-386:⚠️ Potential issueAdd error handling for path navigation in LoadContextMenu
The
Task.Run()call is a fire-and-forget operation without any error handling. If navigation fails, exceptions will be lost.- _ = Task.Run(() => QuickSwitch.JumpToPathAsync(DialogWindowHandle, quickSwitchResult.QuickSwitchPath)); + _ = Task.Run(async () => + { + try + { + await QuickSwitch.JumpToPathAsync(DialogWindowHandle, quickSwitchResult.QuickSwitchPath); + } + catch (Exception ex) + { + App.API.LogException(ClassName, "Failed to navigate to path", ex); + } + });
🧹 Nitpick comments (3)
Flow.Launcher/ViewModel/MainViewModel.cs (3)
55-56: Consider adding XML documentation to the new _emptyQuickSwitchResult fieldSimilar to the _emptyResult field above it, adding a comment would help explain its purpose and usage.
- private readonly IReadOnlyList<QuickSwitchResult> _emptyQuickSwitchResult = new List<QuickSwitchResult>(); + /// <summary> + /// Empty result list for QuickSwitch queries that return no results + /// </summary> + private readonly IReadOnlyList<QuickSwitchResult> _emptyQuickSwitchResult = new List<QuickSwitchResult>();
1478-1482: Consider extracting query method selection to a separate methodThe conditional expression is complex and could be more readable as a separate method.
- IReadOnlyList<Result> results = currentIsQuickSwitch ? - await PluginManager.QueryQuickSwitchForPluginAsync(plugin, query, token) : - currentIsHomeQuery ? - await PluginManager.QueryHomeForPluginAsync(plugin, query, token) : - await PluginManager.QueryForPluginAsync(plugin, query, token); + IReadOnlyList<Result> results = await GetPluginResultsAsync(plugin, query, token, currentIsQuickSwitch, currentIsHomeQuery); + // Add this method elsewhere in the class + private async Task<IReadOnlyList<Result>> GetPluginResultsAsync( + PluginPair plugin, + Query query, + CancellationToken token, + bool isQuickSwitch, + bool isHomeQuery) + { + if (isQuickSwitch) + { + return await PluginManager.QueryQuickSwitchForPluginAsync(plugin, query, token); + } + else if (isHomeQuery) + { + return await PluginManager.QueryHomeForPluginAsync(plugin, query, token); + } + else + { + return await PluginManager.QueryForPluginAsync(plugin, query, token); + } + }
1934-1949: Simplify ResetWindowAsync using ConfigureAwaitFor improved performance when continuing the task, add ConfigureAwait(false) to the Task.Delay call.
private async Task ResetWindowAsync() { lastHistoryIndex = 1; if (ExternalPreviewVisible) { - await CloseExternalPreviewAsync(); + await CloseExternalPreviewAsync().ConfigureAwait(false); } if (!QueryResultsSelected()) { SelectedResults = Results; } - await ChangeQueryTextAsync(string.Empty, true); + await ChangeQueryTextAsync(string.Empty, true).ConfigureAwait(false); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Flow.Launcher/ViewModel/MainViewModel.cs(17 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Report (PR)
- GitHub Check: build
🔇 Additional comments (1)
Flow.Launcher/ViewModel/MainViewModel.cs (1)
1317-1321: Good decision to skip home page for quick switch windowThis is a good UX decision - the home page would be confusing in a quick switch context where users are looking to navigate somewhere specific.
| Application.Current?.Dispatcher.Invoke(() => | ||
| { | ||
| (Application.Current?.MainWindow as MainWindow).UpdatePosition(); | ||
| }); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add null check for mainWindow
After getting the mainWindow from Application.Current?.MainWindow, you should check if it's not null before using it.
Application.Current?.Dispatcher.Invoke(() =>
{
- (Application.Current?.MainWindow as MainWindow).UpdatePosition();
+ var mainWindow = Application.Current?.MainWindow as MainWindow;
+ mainWindow?.UpdatePosition();
});📝 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.
| Application.Current?.Dispatcher.Invoke(() => | |
| { | |
| (Application.Current?.MainWindow as MainWindow).UpdatePosition(); | |
| }); | |
| Application.Current?.Dispatcher.Invoke(() => | |
| { | |
| var mainWindow = Application.Current?.MainWindow as MainWindow; | |
| mainWindow?.UpdatePosition(); | |
| }); |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (9)
Flow.Launcher/ViewModel/MainViewModel.cs (9)
467-493:⚠️ Potential issueApply the same error handling pattern to this navigation task.
Similar to the previous comment, this navigation task needs proper error handling to prevent silent failures.
- _ = Task.Run(() => QuickSwitch.JumpToPathAsync(DialogWindowHandle, quickSwitchResult.QuickSwitchPath)); + _ = Task.Run(async () => + { + try + { + await QuickSwitch.JumpToPathAsync(DialogWindowHandle, quickSwitchResult.QuickSwitchPath).ConfigureAwait(false); + } + catch (Exception ex) + { + App.API.LogException(ClassName, "Failed to jump to path", ex); + } + });
503-526:⚠️ Potential issueAdd type checking to prevent InvalidCastException.
The direct cast to
QuickSwitchResultassumes all results are of that type, which could throw an exception if misused.if (isQuickSwitch) { foreach (var result in results.ToList()) { if (token.IsCancellationRequested) break; - var resultCopy = ((QuickSwitchResult)result).Clone(); - resultsCopy.Add(resultCopy); + if (result is QuickSwitchResult quickSwitchResult) + { + var resultCopy = quickSwitchResult.Clone(); + resultsCopy.Add(resultCopy); + } + else + { + App.API.LogWarning(ClassName, $"Expected QuickSwitchResult but got {result.GetType().Name}"); + var resultCopy = result.Clone(); + resultsCopy.Add(resultCopy); + } } }
1764-1770:⚠️ Potential issueAdd thread-safe access to QuickSwitch properties.
These properties are accessed from multiple threads (UI thread, Task.Run threads) without proper synchronization, which could lead to race conditions and unexpected behavior.
+ private readonly object _quickSwitchLock = new(); - public nint DialogWindowHandle { get; private set; } = nint.Zero; + private nint _dialogWindowHandle = nint.Zero; + public nint DialogWindowHandle + { + get + { + lock (_quickSwitchLock) + { + return _dialogWindowHandle; + } + } + private set + { + lock (_quickSwitchLock) + { + _dialogWindowHandle = value; + } + } + } - private bool _isQuickSwitch = false; + private bool _isQuickSwitch = false; + private bool IsQuickSwitch + { + get + { + lock (_quickSwitchLock) + { + return _isQuickSwitch; + } + } + set + { + lock (_quickSwitchLock) + { + _isQuickSwitch = value; + } + } + }
1782-1798:⚠️ Potential issueChange the async void method to return Task.
Async void methods can lead to unhandled exceptions that crash the application. Return a Task to allow callers to handle exceptions.
- public async Task SetupQuickSwitchAsync(nint handle) + public async Task SetupQuickSwitchAsync(nint handle)Also, consider adding more comprehensive logging to track the quick switch initialization process:
if (handle == nint.Zero) return; + App.API.LogDebug(ClassName, $"Setting up quick switch for handle: {handle}"); // Only set flag & reset window once for one file dialog var dialogWindowHandleChanged = false; if (DialogWindowHandle != handle) { DialogWindowHandle = handle; _previousMainWindowVisibilityStatus = MainWindowVisibilityStatus; _isQuickSwitch = true; dialogWindowHandleChanged = true; + App.API.LogDebug(ClassName, "Dialog window handle changed, resetting quick switch window"); // If don't give a time, Positioning will be weird await Task.Delay(300); }
1809-1813:⚠️ Potential issueAdd null check before using mainWindow.
After getting the mainWindow from Application.Current?.MainWindow, you should check if it's not null before using it.
Application.Current?.Dispatcher.Invoke(() => { - (Application.Current?.MainWindow as MainWindow).UpdatePosition(); + var mainWindow = Application.Current?.MainWindow as MainWindow; + mainWindow?.UpdatePosition(); });
1837-1867:⚠️ Potential issueCapture cancellation token locally to avoid race conditions.
The lambda captures the token source directly, which could lead to race conditions if another dialog arrives and replaces
_quickSwitchSource.// Create a new cancellation token source _quickSwitchSource = new CancellationTokenSource(); + var token = _quickSwitchSource.Token; _ = Task.Run(() => { try { // Check task cancellation - if (_quickSwitchSource.Token.IsCancellationRequested) return; + if (token.IsCancellationRequested) return; // Check dialog handle if (DialogWindowHandle == nint.Zero) return; // Wait 150ms to check if quick switch window gets the focus var timeOut = !SpinWait.SpinUntil(() => !Win32Helper.IsForegroundWindow(DialogWindowHandle), 150); if (timeOut) return; // Bring focus back to the the dialog Win32Helper.SetForegroundWindow(DialogWindowHandle); } catch (Exception e) { App.API.LogException(ClassName, "Failed to focus on dialog window", e); } });
1872-1912:⚠️ Potential issueChange async void method to return Task for proper exception handling.
Async void methods can lead to unhandled exceptions that crash the application. Return a Task to allow proper exception handling.
- public async void ResetQuickSwitch() + public async Task ResetQuickSwitch() { if (DialogWindowHandle == nint.Zero) return; + App.API.LogDebug(ClassName, "Resetting quick switch"); DialogWindowHandle = nint.Zero; _isQuickSwitch = false; if (_previousMainWindowVisibilityStatus != MainWindowVisibilityStatus) { // Show or hide to change visibility if (_previousMainWindowVisibilityStatus) { Show(); _ = ResetWindowAsync(); } else { await ResetWindowAsync(); Hide(false); } } else { if (_previousMainWindowVisibilityStatus) { // Only update the position Application.Current?.Dispatcher.Invoke(() => { - (Application.Current?.MainWindow as MainWindow).UpdatePosition(); + var mainWindow = Application.Current?.MainWindow as MainWindow; + mainWindow?.UpdatePosition(); }); _ = ResetWindowAsync(); } else { _ = ResetWindowAsync(); } } }
1916-1931:⚠️ Potential issueAdd window handle validity check in HideQuickSwitch.
The method assumes that if DialogWindowHandle is not zero, it's still valid, but Windows handles can be reused or invalidated.
public void HideQuickSwitch() { if (DialogWindowHandle != nint.Zero) { + // Verify the handle is still valid + if (!Win32Helper.IsWindow(DialogWindowHandle)) + { + DialogWindowHandle = nint.Zero; + _isQuickSwitch = false; + return; + } if (QuickSwitch.QuickSwitchWindowPosition == QuickSwitchWindowPositions.UnderDialog) { // Warning: Main window is already in foreground // This is because if you click popup menus in other applications to hide quick switch window, // they can steal focus before showing main window if (MainWindowVisibilityStatus) { Hide(); } } } }
372-386:⚠️ Potential issueAdd error handling to the path navigation task.
The navigation task is fire-and-forget, meaning exceptions won't be caught, which could lead to silent failures when jumping to paths.
- _ = Task.Run(() => QuickSwitch.JumpToPathAsync(DialogWindowHandle, quickSwitchResult.QuickSwitchPath)); + _ = Task.Run(async () => + { + try + { + await QuickSwitch.JumpToPathAsync(DialogWindowHandle, quickSwitchResult.QuickSwitchPath).ConfigureAwait(false); + } + catch (Exception ex) + { + App.API.LogException(ClassName, "Failed to jump to path", ex); + } + });
🧹 Nitpick comments (3)
Flow.Launcher/ViewModel/MainViewModel.cs (3)
55-56: Add a method for creating an empty quick switch result list.Consider adding a method to create this empty list to maintain symmetry with
_emptyResultusage. This would make the code more maintainable if the creation logic changes in the future.- private readonly IReadOnlyList<QuickSwitchResult> _emptyQuickSwitchResult = new List<QuickSwitchResult>(); + private readonly IReadOnlyList<QuickSwitchResult> _emptyQuickSwitchResult => EmptyQuickSwitchResultList(); + + private static IReadOnlyList<QuickSwitchResult> EmptyQuickSwitchResultList() + { + return new List<QuickSwitchResult>(); + }
1474-1478: Consider using a more explicit conditional structure for readability.The nested ternary operation is hard to read. A more explicit
if-elsestructure would make the logic clearer.- IReadOnlyList<Result> results = currentIsQuickSwitch ? - await PluginManager.QueryQuickSwitchForPluginAsync(plugin, query, token) : - currentIsHomeQuery ? - await PluginManager.QueryHomeForPluginAsync(plugin, query, token) : - await PluginManager.QueryForPluginAsync(plugin, query, token); + IReadOnlyList<Result> results; + if (currentIsQuickSwitch) + { + results = await PluginManager.QueryQuickSwitchForPluginAsync(plugin, query, token); + } + else if (currentIsHomeQuery) + { + results = await PluginManager.QueryHomeForPluginAsync(plugin, query, token); + } + else + { + results = await PluginManager.QueryForPluginAsync(plugin, query, token); + }
1934-1949: Consider adding cancellation handling to ResetWindowAsync.The ResetWindowAsync method could be interrupted by application termination or other events. Adding cancellation support would make it more robust.
- private async Task ResetWindowAsync() + private async Task ResetWindowAsync(CancellationToken cancellationToken = default) { + try + { lastHistoryIndex = 1; if (ExternalPreviewVisible) { - await CloseExternalPreviewAsync(); + await CloseExternalPreviewAsync().ConfigureAwait(false); } if (!QueryResultsSelected()) { SelectedResults = Results; } - await ChangeQueryTextAsync(string.Empty, true); + await ChangeQueryTextAsync(string.Empty, true).ConfigureAwait(false); + } + catch (Exception ex) when (!(ex is OperationCanceledException)) + { + App.API.LogException(ClassName, "Error resetting window", ex); + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Flow.Launcher/MainWindow.xaml.cs(11 hunks)Flow.Launcher/ViewModel/MainViewModel.cs(16 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Flow.Launcher/MainWindow.xaml.cs
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (1)
Flow.Launcher/ViewModel/MainViewModel.cs (1)
2170-2170: Ensure cancellation token source disposal.This change correctly disposes the QuickSwitch cancellation token source.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (9)
Flow.Launcher/ViewModel/MainViewModel.cs (9)
371-384:⚠️ Potential issueAdd error handling for fire-and-forget JumpToPath task
The JumpToPathAsync task is launched without any error handling, which could lead to unhandled exceptions.
Win32Helper.SetForegroundWindow(DialogWindowHandle); -_ = Task.Run(() => QuickSwitch.JumpToPathAsync(DialogWindowHandle, quickSwitchResult.QuickSwitchPath)); +_ = Task.Run(async () => { + try + { + await QuickSwitch.JumpToPathAsync(DialogWindowHandle, quickSwitchResult.QuickSwitchPath) + .ConfigureAwait(false); + } + catch (Exception ex) + { + App.API.LogException(ClassName, "Quick switch navigation failed", ex); + } +});
464-476:⚠️ Potential issueAdd error handling for JumpToPath in OpenResultAsync method
Similar to the previous comment, the JumpToPathAsync task is launched without error handling.
Win32Helper.SetForegroundWindow(DialogWindowHandle); -_ = Task.Run(() => QuickSwitch.JumpToPathAsync(DialogWindowHandle, quickSwitchResult.QuickSwitchPath)); +_ = Task.Run(async () => { + try + { + await QuickSwitch.JumpToPathAsync(DialogWindowHandle, quickSwitchResult.QuickSwitchPath) + .ConfigureAwait(false); + } + catch (Exception ex) + { + App.API.LogException(ClassName, "Quick switch navigation failed", ex); + } +});
501-527:⚠️ Potential issueAdd type checking to prevent InvalidCastException in DeepCloneResults
Direct casting to
QuickSwitchResultcould throw an exception if the result is not of that type.if (isQuickSwitch) { foreach (var result in results.ToList()) { if (token.IsCancellationRequested) break; - var resultCopy = ((QuickSwitchResult)result).Clone(); - resultsCopy.Add(resultCopy); + if (result is QuickSwitchResult quickSwitchResult) + { + var resultCopy = quickSwitchResult.Clone(); + resultsCopy.Add(resultCopy); + } + else + { + App.API.LogWarning(ClassName, $"Expected QuickSwitchResult but got {result.GetType().Name}"); + var resultCopy = result.Clone(); + resultsCopy.Add(resultCopy); + } } }
1766-1774:⚠️ Potential issueMake shared state variables thread-safe
The fields
DialogWindowHandleand_isQuickSwitchare accessed from multiple threads without synchronization, which could lead to race conditions.+private readonly object _quickSwitchLock = new(); +private nint _dialogWindowHandleValue = nint.Zero; +private bool _isQuickSwitchValue = false; -public nint DialogWindowHandle { get; private set; } = nint.Zero; +public nint DialogWindowHandle +{ + get + { + lock (_quickSwitchLock) + { + return _dialogWindowHandleValue; + } + } + private set + { + lock (_quickSwitchLock) + { + _dialogWindowHandleValue = value; + } + } +} -private bool _isQuickSwitch = false; +private bool _isQuickSwitch +{ + get + { + lock (_quickSwitchLock) + { + return _isQuickSwitchValue; + } + } + set + { + lock (_quickSwitchLock) + { + _isQuickSwitchValue = value; + } + } +}
1841-1872:⚠️ Potential issueFix potential race condition with cancellation token
There's a potential race condition with the cancellation token if a new task is started before the previous one is cancelled.
// Cancel the previous quick switch task _quickSwitchSource?.Cancel(); // Create a new cancellation token source _quickSwitchSource = new CancellationTokenSource(); +var token = _quickSwitchSource.Token; _ = Task.Run(() => { try { // Check task cancellation - if (_quickSwitchSource.Token.IsCancellationRequested) return; + if (token.IsCancellationRequested) return; // Check dialog handle if (DialogWindowHandle == nint.Zero) return; // Wait 150ms to check if quick switch window gets the focus var timeOut = !SpinWait.SpinUntil(() => !Win32Helper.IsForegroundWindow(DialogWindowHandle), 150); if (timeOut) return; // Bring focus back to the the dialog Win32Helper.SetForegroundWindow(DialogWindowHandle); } catch (Exception e) { App.API.LogException(ClassName, "Failed to focus on dialog window", e); } });
1813-1817:⚠️ Potential issueAdd null check for MainWindow before calling UpdatePosition
There's no null check before accessing MainWindow, which could lead to a NullReferenceException.
Application.Current?.Dispatcher.Invoke(() => { - (Application.Current?.MainWindow as MainWindow).UpdatePosition(); + var mainWindow = Application.Current?.MainWindow as MainWindow; + mainWindow?.UpdatePosition(); });
1876-1877: 🛠️ Refactor suggestionUse Task return type instead of async void
Using async void is not recommended for non-event handlers as it can lead to unhandled exceptions.
-public async void ResetQuickSwitch() +public async Task ResetQuickSwitch()
1904-1908:⚠️ Potential issueAdd null check for MainWindow in ResetQuickSwitch
Similar to the previous issue, there's no null check before accessing MainWindow.
Application.Current?.Dispatcher.Invoke(() => { - (Application.Current?.MainWindow as MainWindow).UpdatePosition(); + var mainWindow = Application.Current?.MainWindow as MainWindow; + mainWindow?.UpdatePosition(); });
1920-1935: 🛠️ Refactor suggestionAdd checks for window handle validity in HideQuickSwitch
The method only checks if the handle is non-zero, but not if it's still valid. Windows handles can be reused or invalidated.
public void HideQuickSwitch() { if (DialogWindowHandle != nint.Zero) { + // Verify the handle is still valid + if (!Win32Helper.IsWindow(DialogWindowHandle)) + { + DialogWindowHandle = nint.Zero; + _isQuickSwitch = false; + return; + } + if (QuickSwitch.QuickSwitchWindowPosition == QuickSwitchWindowPositions.UnderDialog) { // Warning: Main window is already in foreground // This is because if you click popup menus in other applications to hide quick switch window, // they can steal focus before showing main window if (MainWindowVisibilityStatus) { Hide(); } } } }
🧹 Nitpick comments (2)
Flow.Launcher/ViewModel/MainViewModel.cs (2)
55-55: Unused field_emptyQuickSwitchResultThis field is defined but never used in the codebase. Consider removing it to avoid dead code.
-private readonly IReadOnlyList<QuickSwitchResult> _emptyQuickSwitchResult = new List<QuickSwitchResult>();
1472-1476: Consider improving readability of nested ternary operatorsThe nested ternary operators make this code harder to read and maintain.
-IReadOnlyList<Result> results = currentIsQuickSwitch ? - await PluginManager.QueryQuickSwitchForPluginAsync(plugin, query, token) : - currentIsHomeQuery ? - await PluginManager.QueryHomeForPluginAsync(plugin, query, token) : - await PluginManager.QueryForPluginAsync(plugin, query, token); +IReadOnlyList<Result> results; +if (currentIsQuickSwitch) +{ + results = await PluginManager.QueryQuickSwitchForPluginAsync(plugin, query, token); +} +else if (currentIsHomeQuery) +{ + results = await PluginManager.QueryHomeForPluginAsync(plugin, query, token); +} +else +{ + results = await PluginManager.QueryForPluginAsync(plugin, query, token); +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
Flow.Launcher.Core/Plugin/PluginManager.cs(2 hunks)Flow.Launcher.Infrastructure/UserSettings/Settings.cs(4 hunks)Flow.Launcher/MainWindow.xaml.cs(11 hunks)Flow.Launcher/ViewModel/MainViewModel.cs(16 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- Flow.Launcher.Core/Plugin/PluginManager.cs
- Flow.Launcher.Infrastructure/UserSettings/Settings.cs
- Flow.Launcher/MainWindow.xaml.cs
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
@check-spelling-bot Report🔴 Please reviewSee the 📂 files view, the 📜action log, or 📝 job summary for details.
See ❌ Event descriptions for more information. Forbidden patterns 🙅 (2)In order to address this, you could change the content to not match the forbidden patterns (comments before forbidden patterns may help explain why they're forbidden), add patterns for acceptable instances, or adjust the forbidden patterns themselves. These forbidden patterns matched content: s.b. workaround(s)Reject duplicate wordsIf the flagged items are 🤯 false positivesIf items relate to a ...
|
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
Flow.Launcher/ViewModel/MainViewModel.cs (4)
464-477: 🛠️ Refactor suggestionAdd error handling for the QuickSwitch path navigation.
Similar to the issue in LoadContextMenu, this Task.Run for navigation is also fire-and-forget with no error handling.
_ = Task.Run(() => QuickSwitch.JumpToPathAsync(DialogWindowHandle, quickSwitchResult.QuickSwitchPath)); + _ = Task.Run(async () => + { + try + { + await QuickSwitch.JumpToPathAsync(DialogWindowHandle, quickSwitchResult.QuickSwitchPath); + } + catch (Exception ex) + { + App.API.LogException(ClassName, "Failed to navigate to path in OpenResultAsync", ex); + } + });
511-512:⚠️ Potential issueAdd type checking before casting to QuickSwitchResult.
The direct cast to
QuickSwitchResultassumes all items in the collection are of that type. This could throwInvalidCastExceptionif misused.-var resultCopy = ((QuickSwitchResult)result).Clone(); -resultsCopy.Add(resultCopy); +if (result is QuickSwitchResult quickSwitchResult) +{ + var resultCopy = quickSwitchResult.Clone(); + resultsCopy.Add(resultCopy); +} +else +{ + App.API.LogWarning(ClassName, $"Expected QuickSwitchResult in DeepCloneResults but got {result.GetType().Name}"); + // Add regular clone as fallback + var resultCopy = result.Clone(); + resultsCopy.Add(resultCopy); +}
1908-1948:⚠️ Potential issueChange async void to return Task for proper exception handling.
Using async void for the ResetQuickSwitch method prevents proper exception handling and can lead to application crashes if exceptions occur.
-public async void ResetQuickSwitch() +public async Task ResetQuickSwitch() { if (DialogWindowHandle == nint.Zero) return; DialogWindowHandle = nint.Zero; _isQuickSwitch = false; // Rest of the method... }
371-384: 🛠️ Refactor suggestionAdd error handling for the QuickSwitch path navigation.
The Task.Run for navigation is fire-and-forget with no error handling, which could lead to unhandled exceptions if the navigation fails.
_ = Task.Run(() => QuickSwitch.JumpToPathAsync(DialogWindowHandle, quickSwitchResult.QuickSwitchPath)); + _ = Task.Run(async () => + { + try + { + await QuickSwitch.JumpToPathAsync(DialogWindowHandle, quickSwitchResult.QuickSwitchPath); + } + catch (Exception ex) + { + App.API.LogException(ClassName, "Failed to navigate to path in LoadContextMenu", ex); + } + });
🧹 Nitpick comments (1)
Flow.Launcher/ViewModel/MainViewModel.cs (1)
1818-1845: Consider simplifying window positioning delay logic.The 300ms delay before updating the position is used to ensure the window is positioned correctly, but this is a brittle approach that might not work consistently across different systems.
Consider using a more robust window positioning strategy:
- Use window message hooks to detect when the dialog has finished positioning
- Use a callback approach instead of a fixed delay
- Implement a progressive retry mechanism with shorter delays
Alternatively, document the reason for this specific delay value with a comment if it's based on testing across different systems.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
Flow.Launcher.Infrastructure/UserSettings/Settings.cs(4 hunks)Flow.Launcher/MainWindow.xaml.cs(11 hunks)Flow.Launcher/ViewModel/MainViewModel.cs(16 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- Flow.Launcher/MainWindow.xaml.cs
- Flow.Launcher.Infrastructure/UserSettings/Settings.cs
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (4)
Flow.Launcher/ViewModel/MainViewModel.cs (4)
55-55: Good addition to support QuickSwitchResult objects.Adding a dedicated empty list for QuickSwitchResult objects is consistent with the existing pattern for regular Result objects.
1309-1315: Good addition to skip home page for quick switch window.This is a sensible special case handling - showing the home page during quick switch would likely confuse users and interfere with the focused path switching experience.
1477-1482: Great implementation of plugin querying logic for different modes.The conditional expression effectively handles three distinct query modes (quick switch, home, and standard queries) with clear branching logic that routes to the appropriate plugin manager method.
2209-2209: Proper disposal of QuickSwitch resources.Good practice to dispose of the cancellation token source when the object is disposed.
Nothing more than quickswitch. We may integrate flow's path system to this feature instead of relying explorer.
Setup Quick Switch
Use Quick Switch
Open explorer -> Open file dialog -> Use hotkey to navigate to that path.
Open file dialog -> Query window (quick switch window) fixed under file dialog -> Click results to navigate to the selected path
Quick Switch API
Implement new api interfaces to let plugin be queried on quick switch window for dotnet plugins only.
Additionally,
Explorerplugin already supports quick switch.Develop third party explorer & dialog
Check
Flow.Launcher.Infrastructure.QuickSwitch.Interface.IQuickSwitchExplorerandFlow.Launcher.Infrastructure.QuickSwitch.Interface.IQuickSwitchDialogfor more info.Todos
However, for the Save As dialog, the path does not apply immediately when the dialog is opened. It only works after switching focus to File Explorer and then returning.