Fix copy to clipboard STA thread issue & Add retry for copy #3314
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. |
📝 WalkthroughWalkthroughThis pull request updates several components in the application. It adds two new Windows API entries ( Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application
participant API as PublicAPIInstance
participant RT as RetryActionOnSTAThreadAsync
participant STA as Win32Helper (STA Task)
participant Clip as Clipboard API
App->>API: CopyToClipboard(data)
API->>API: Validate input
API->>RT: Invoke retry mechanism with clipboard action
RT->>STA: Call StartSTATaskAsync(action)
STA->>STA: OleInitialize & Execute clipboard action
STA->>Clip: Call Clipboard.SetText / SetFileDropList
Clip-->>STA: Return success/failure
STA-->>RT: Return result
RT-->>API: Propagate outcome
API-->>App: Notify success or error
sequenceDiagram
participant Caller as Calling Method
participant STA as New STA Thread
participant OLE as OLE Functions
Caller->>STA: Invoke StartSTATaskAsync(Action/Func)
STA->>OLE: OleInitialize()
STA->>STA: Execute provided action/function
STA->>OLE: OleUninitialize()
STA-->>Caller: Return task result
Assessment against linked issues
Possibly related PRs
Suggested labels
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
🔇 Additional comments (4)
✨ Finishing Touches
🪧 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 (
|
|
I am very against this. This is the core, most used, functionality, and it should be available to everyone. Plugins shouldn't need to reinvent the wheel every time. Especially considering Node and Python don't provide a way to do this. I remember seeing one plugin that packed another binary with itself just so it could copy text, which is absolutely ridiculous. |
Well, could you please give me the sample to test? |
Jack251970
changed the title
Jack251970
changed the title
Jack251970
changed the title
|
After installing this plugin, type |
Many thanks! Sorry, I don't see this as a bug unique to the V2 plugin. In that case, the bug could be fixed. |
This reverts commit 9f8e829.
Sorry I cannot open your demo plugin. I have used 1.19.5 release version of FL (master branch) and downloaded embedded node.js environment from FL but I got this issue:
I want to check it by myself, but I really know little about JavaScript. Could you please fix it? |
|
Could you please install a newer version of NodeJS (v22), specify it in settings (General — Node.js Path) and try again? Flow currently uses very old version that has reached end-of-life already. |
This comment has been minimized.
This comment has been minimized.
Jack251970
changed the title
Thanks, I think I have fixed it. Now could you please help me check if copy to clipboard (directory) with JsonRPCV2 plugins can work? |
Nvm, I have checked it. |
![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
🧹 Nitpick comments (5)
Flow.Launcher.Infrastructure/Win32Helper.cs (2)
122-125: Consider propagating exceptions rather than suppressing them.The current implementation catches all exceptions but doesn't propagate them to the caller, making it difficult to diagnose issues when they occur.
- catch (System.Exception) - { - taskCompletionSource.SetResult(); - } + catch (System.Exception ex) + { + taskCompletionSource.SetException(ex); + }
154-157: Consider propagating exceptions rather than returning default values.Similar to the non-generic method, this implementation catches all exceptions but doesn't propagate them to the caller. Instead, it returns a default value which could mask errors.
- catch (System.Exception) - { - taskCompletionSource.SetResult(default); - } + catch (System.Exception ex) + { + taskCompletionSource.SetException(ex); + }Flow.Launcher/PublicAPIInstance.cs (3)
131-156: Consolidate repetitive clipboard copy logic.The logic for direct file/folder copying (lines 131–156) and text copying (lines 160–181) mostly duplicates the retry mechanism and message handling. Consider extracting common retry logic into a shared helper or unify this code block to improve maintainability and clarity.
Also applies to: 160-181
164-164: Fix minor typo in comment.Change "shouold" to "should" for clarity.
- // We shouold use SetText instead of SetDataObject to avoid the clipboard being locked by other applications + // We should use SetText instead of SetDataObject to avoid the clipboard being locked by other applications
185-204: Reconsider returning the exception object fromRetryActionOnSTAThreadAsync.Returning an
Exceptionobject directly is functional, but throwing an exception or returning a status (e.g.,bool) might simplify the calling code. The current design requires the caller to check fornullvs. non-nullto determine success/failure, which can be error-prone or less conventional.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
Flow.Launcher.Infrastructure/NativeMethods.txt(1 hunks)Flow.Launcher.Infrastructure/UserSettings/Settings.cs(1 hunks)Flow.Launcher.Infrastructure/Win32Helper.cs(2 hunks)Flow.Launcher/Languages/en.xaml(1 hunks)Flow.Launcher/PublicAPIInstance.cs(1 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 (4)
Flow.Launcher.Infrastructure/NativeMethods.txt (1)
21-22: Good addition of required OLE function declarations.These OLE function declarations are necessary for properly initializing and uninitializing the clipboard operations on STA threads, ensuring proper COM interop which is critical for clipboard functionality.
Flow.Launcher/Languages/en.xaml (1)
339-339: Good addition of error message for clipboard operations.This new string resource will help provide better user feedback when clipboard operations fail, improving the overall user experience.
Flow.Launcher.Infrastructure/UserSettings/Settings.cs (1)
251-251:✅ Verification successful
Ensure clipboard operations run on STA thread.
Good change to use the new
StartSTATaskAsyncmethod for clipboard text retrieval. This ensures proper threading model for COM operations.Note that using
.Resultcould potentially lead to deadlocks in certain scenarios, but since this is only creating a model with a function reference to be executed later, it should be fine in this context.
🏁 Script executed:
#!/bin/bash # Check if this pattern is used elsewhere in the codebase where it might cause problems rg "StartSTATaskAsync.*\.Result" --type csLength of output: 248
Verified STA Thread Usage for Clipboard Operations
After reviewing the code and verifying the pattern usage in the codebase, it’s clear that the call to
Win32Helper.StartSTATaskAsync(Clipboard.GetText).Resultis only present inFlow.Launcher.Infrastructure/UserSettings/Settings.cs. Since this function is used to defer clipboard text retrieval until later execution and is correctly encapsulated within a model creation, the use of.Resulthere is acceptable and does not pose a deadlock risk in this context.
- Verified that the snippet is uniquely located in the intended file.
- Confirmed that clipboard operations are safely executed on an STA thread.
Flow.Launcher.Infrastructure/Win32Helper.cs (1)
104-174: Good implementation of STA thread execution helpers.The implementation of these helper methods provides a clean way to execute actions on an STA thread, which is necessary for clipboard and other COM operations. Properly initializing and uninitializing OLE ensures resources are managed correctly.
|
🥷 Code experts: onesounds Jack251970, onesounds have 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 |
Jack251970
changed the title
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
Flow.Launcher/PublicAPIInstance.cs (1)
123-123: 🛠️ Refactor suggestionUse
Taskinstead ofasync voidto better handle exceptions and caller awareness.Returning a
Taskrather than usingasync voidis a standard best practice. Doing so allows the caller to await (or track) the completion and capture any unhandled exceptions within calling code.- public async void CopyToClipboard(string stringToCopy, bool directCopy = false, bool showDefaultNotification = true) + public async Task CopyToClipboard(string stringToCopy, bool directCopy = false, bool showDefaultNotification = true)
🧹 Nitpick comments (4)
Flow.Launcher.Infrastructure/Win32Helper.cs (1)
325-328: Add comprehensive documentation for STA thread methods.The comment only indicates the source of the code but doesn't explain the purpose of STA threads, when to use them, or why OLE initialization is necessary.
Consider enhancing the documentation:
/* - Found on https://github.com/files-community/Files + Found on https://github.com/files-community/Files + + These methods execute actions on Single Threaded Apartment (STA) threads. + STA is required for certain Windows operations, particularly those involving + the clipboard, COM objects, or drag-and-drop functionality. OLE initialization + is necessary for these operations to work correctly in the STA context. */Flow.Launcher/PublicAPIInstance.cs (3)
166-166: Fix typo in comment.There's a typo in the comment: "shouold" should be "should".
- // We shouold use SetText instead of SetDataObject to avoid the clipboard being locked by other applications + // We should use SetText instead of SetDataObject to avoid the clipboard being locked by other applications
187-206: Extract magic numbers to named constants and improve error reporting.The retry count and delay are hardcoded as magic numbers. Additionally, the method returns only the exception without additional context.
Consider improving the implementation:
+private const int DefaultClipboardRetryCount = 6; +private const int DefaultClipboardRetryDelayMs = 150; -private static async Task<Exception> RetryActionOnSTAThreadAsync(Action action, int retryCount = 6, int retryDelay = 150) +private static async Task<Exception> RetryActionOnSTAThreadAsync( + Action action, + int retryCount = DefaultClipboardRetryCount, + int retryDelay = DefaultClipboardRetryDelayMs) { for (var i = 0; i < retryCount; i++) { try { await Win32Helper.StartSTATaskAsync(action); break; } catch (Exception e) { if (i == retryCount - 1) { + Log.Debug(nameof(PublicAPIInstance), $"Clipboard operation failed after {retryCount} attempts"); return e; } await Task.Delay(retryDelay); } } return null; }
123-185: Consider adding cancellation support to the clipboard operations.The clipboard operations are now asynchronous but don't support cancellation, which could be useful for long-running operations or when the user navigates away.
Consider adding an optional
CancellationTokenparameter:- public async void CopyToClipboard(string stringToCopy, bool directCopy = false, bool showDefaultNotification = true) + public async Task CopyToClipboard(string stringToCopy, bool directCopy = false, bool showDefaultNotification = true, CancellationToken cancellationToken = default) { if (string.IsNullOrEmpty(stringToCopy)) { return; } var isFile = File.Exists(stringToCopy); if (directCopy && (isFile || Directory.Exists(stringToCopy))) { // Sometimes the clipboard is locked and cannot be accessed, // we need to retry a few times before giving up - var exception = await RetryActionOnSTAThreadAsync(() => + var exception = await RetryActionOnSTAThreadAsync(() => { var paths = new StringCollection { stringToCopy }; Clipboard.SetFileDropList(paths); - }); + }, cancellationToken: cancellationToken); // Rest of the method... } } - private static async Task<Exception> RetryActionOnSTAThreadAsync(Action action, int retryCount = 6, int retryDelay = 150) + private static async Task<Exception> RetryActionOnSTAThreadAsync(Action action, int retryCount = 6, int retryDelay = 150, CancellationToken cancellationToken = default) { for (var i = 0; i < retryCount; i++) { + cancellationToken.ThrowIfCancellationRequested(); try { await Win32Helper.StartSTATaskAsync(action); break; } catch (Exception e) { if (i == retryCount - 1) { return e; } - await Task.Delay(retryDelay); + await Task.Delay(retryDelay, cancellationToken); } } return null; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
Flow.Launcher.Infrastructure/NativeMethods.txt(1 hunks)Flow.Launcher.Infrastructure/UserSettings/Settings.cs(1 hunks)Flow.Launcher.Infrastructure/Win32Helper.cs(2 hunks)Flow.Launcher/Languages/en.xaml(1 hunks)Flow.Launcher/PublicAPIInstance.cs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- Flow.Launcher/Languages/en.xaml
- Flow.Launcher.Infrastructure/UserSettings/Settings.cs
- Flow.Launcher.Infrastructure/NativeMethods.txt
🧰 Additional context used
🧬 Code Definitions (1)
Flow.Launcher/PublicAPIInstance.cs (3)
Flow.Launcher.Core/Plugin/JsonRPCV2Models/JsonRPCPublicAPI.cs (1) (1)
CopyToClipboard(37-40)Flow.Launcher.Infrastructure/Win32Helper.cs (3) (3)
Task(329-359)Task(361-391)Win32Helper(17-394)Flow.Launcher.Core/Plugin/PluginManager.cs (4) (4)
Task(87-95)Task(97-104)Task(106-113)Task(115-122)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: gitStream.cm
| @@ -253,7 +253,7 @@ public SearchPrecisionScore QuerySearchPrecision | |||
| [JsonIgnore] | |||
| public ObservableCollection<BuiltinShortcutModel> BuiltinShortcuts { get; set; } = new() | |||
| { | |||
| new BuiltinShortcutModel("{clipboard}", "shortcut_clipboard_description", Clipboard.GetText), | |||
| new BuiltinShortcutModel("{clipboard}", "shortcut_clipboard_description", () => Win32Helper.StartSTATaskAsync(Clipboard.GetText).Result), | |||
There was a problem hiding this comment.
I think it's better to use GetAwaiter().GetResult().
There was a problem hiding this comment.
Maybe async void is better? I am not sure.
There was a problem hiding this comment.
I think either is ok?
There was a problem hiding this comment.
I think it's better to use GetAwaiter().GetResult().
This cannot work here, so I choose to keep as it was
There was a problem hiding this comment.
what do you mean by not work here? It is almost semantically equal to Task.Result, except that the exception is handled in a different way?
There was a problem hiding this comment.
Changing () => Win32Helper.StartSTATaskAsync(Clipboard.GetText).Result to Win32Helper.StartSTATaskAsync(Clipboard.GetText).GetAwaiter().GetResult cannot work, the result is always empty
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
Flow.Launcher/PublicAPIInstance.cs (2)
166-166: Fix typo in comment.There's a typo in the comment.
- // We shouold use SetText instead of SetDataObject to avoid the clipboard being locked by other applications + // We should use SetText instead of SetDataObject to avoid the clipboard being locked by other applications
187-206: Well-implemented retry pattern.The
RetryActionOnSTAThreadAsyncmethod provides a robust implementation of the retry pattern. Consider adding a comment explaining the reasoning behind the default values forretryCountandretryDelay.-private static async Task<Exception> RetryActionOnSTAThreadAsync(Action action, int retryCount = 6, int retryDelay = 150) +/// <summary> +/// Executes an action on an STA thread with retry capability. +/// </summary> +/// <param name="action">The action to execute.</param> +/// <param name="retryCount">Number of retry attempts (default: 6).</param> +/// <param name="retryDelay">Delay between retries in milliseconds (default: 150).</param> +/// <returns>The exception if all retries fail, or null if successful.</returns> +private static async Task<Exception> RetryActionOnSTAThreadAsync(Action action, int retryCount = 6, int retryDelay = 150)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Flow.Launcher/Languages/en.xaml(1 hunks)Flow.Launcher/PublicAPIInstance.cs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Flow.Launcher/Languages/en.xaml
🔇 Additional comments (5)
Flow.Launcher/PublicAPIInstance.cs (5)
123-123: Changeasync voidtoasync Taskto handle exceptions properly.Using
async voidcan lead to unhandled exceptions that crash the application. It's a best practice to useasync Taskinstead, which allows callers to await the operation and handle exceptions properly.-public async void CopyToClipboard(string stringToCopy, bool directCopy = false, bool showDefaultNotification = true) +public async Task CopyToClipboard(string stringToCopy, bool directCopy = false, bool showDefaultNotification = true)
125-128: LGTM! Good defensive programming.Adding a null/empty check as an early return is a good practice that prevents unnecessary processing.
162-183: Good improvement for text copying with retry mechanism.Using
Clipboard.SetTextinstead ofSetDataObjectis a good choice to prevent clipboard locking issues. The retry logic with proper error handling and user feedback enhances reliability.
133-158:❓ Verification inconclusive
Improved file/folder copy with retry mechanism.
The retry mechanism for clipboard operations is a good addition that addresses potential clipboard locking issues.
Don't forget to update interface definitions in other files to maintain consistency with the new async signature.
🏁 Script executed:
#!/bin/bash # Check if the IPublicAPI interface has been updated to match the new async signature echo "Checking if interfaces need updating for async method:" rg "interface IPublicAPI" -A 20 --include "*.cs"Length of output: 611
Interface Async Update Verification:
The new retry mechanism implementation for clipboard operations looks robust. However, the automatic verification of the async signature update in the
IPublicAPIinterface was inconclusive due to the search command error. Please manually verify that all interface definitions (e.g.,IPublicAPI) have been updated to use the new async signature to ensure consistency across the codebase.
- Action Required:
- Manually check the interface definitions (in files where
IPublicAPIis declared) to confirm they now include the updated async method signature.- Ensure that any related interface changes are propagated throughout the codebase.
193-193:❓ Verification inconclusive
Ensure Win32Helper.StartSTATaskAsync is properly implemented.
The code relies on
Win32Helper.StartSTATaskAsync, which should ensure clipboard operations run on an STA thread as required by the Windows Clipboard API.
🏁 Script executed:
#!/bin/bash # Check the implementation of Win32Helper.StartSTATaskAsync echo "Showing implementation of Win32Helper.StartSTATaskAsync:" rg "StartSTATaskAsync" -A 15 --include "*.cs"Length of output: 610
Action Required: Manually Verify STA Implementation in Win32Helper.StartSTATaskAsync
The call to
Win32Helper.StartSTATaskAsyncon line 193 ofFlow.Launcher/PublicAPIInstance.csis critical to ensure that clipboard operations execute on an STA thread, as required by the Windows Clipboard API. Our initial attempt to auto-locate its implementation failed due to an argument error with the search command. Please verify manually—or re-run an updated search script—to confirm that the method indeed wraps clipboard calls within an STA context.
- Location to check:
Flow.Launcher/PublicAPIInstance.csat line 193.- Action: Confirm that
Win32Helper.StartSTATaskAsyncis implemented to marshal clipboard operations onto an STA thread.
If needed, you can use the following revised shell script to try searching for the method implementation:
#!/bin/bash echo "Searching for Win32Helper.StartSTATaskAsync implementation using corrected command:" rg "StartSTATaskAsync" -A 30 .
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
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 ...
|
Fix copy to clipboard STA thread issue
Fix CopyToClipboard function in JsonRPCAPI for JsonRPCV2 plugins: we should use STA thread to execute clipboard options.
Close #3071.
Add retry for clipboard copy
Sometimes clipboard is locked, we need to wait and retry.
Test