fix paste performance issue on windows and linux #249
Conversation
📝 WalkthroughWalkthroughThis pull request introduces comprehensive changes to clipboard and console input handling across multiple components of the Consolonia framework. The modifications span various files and include the implementation of clipboard functionality for different platforms (Windows, macOS, Linux, and WSL), enhancements to console input processing, and improvements to text input event handling. Key changes include adding platform-specific clipboard implementations, introducing text input events, modifying console input buffers, and updating user interface components to support scrolling. The changes aim to improve text input performance, clipboard interactions, and overall user experience in console and terminal-based applications. Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/Consolonia.PlatformSupport/Win32Console.cs (1)
137-140: Add error handling for HandleKeyInput.The simulation of CTRL+V events should include error handling to ensure all events are processed correctly.
// simulate CTRL_V foreach (var keyEvent in CtrlVKeyEvents) { + try { HandleKeyInput(keyEvent); + } catch (Exception ex) { + Debug.WriteLine($"Failed to handle CTRL+V key event: {ex.Message}"); + // Consider whether to re-throw or continue + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Consolonia.GuiCS/WindowsDriver.cs(1 hunks)src/Consolonia.PlatformSupport/Win32Console.cs(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (2)
src/Consolonia.GuiCS/WindowsDriver.cs (1)
45-46: Consider validating buffer size against system limits.The static buffer size of 65535 is reasonable but should be validated against GetConsoleInfo limits to ensure it doesn't exceed system capabilities.
✅ Verification successful
Buffer size of 65535 is safe and doesn't require validation
The chosen buffer size is well within Windows console input buffer capabilities. The Windows API automatically handles partial reads and provides built-in overflow protection, making additional validation unnecessary.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if similar buffer sizes are used elsewhere in the codebase rg -g '!*.{md,txt}' '65535|0xFFFF' .Length of output: 116
src/Consolonia.PlatformSupport/Win32Console.cs (1)
69-75: Consider keyboard layout compatibility.The hard-coded scan codes (29, 47) in CtrlVKeyEvents might not work correctly across different keyboard layouts.
Consider using virtual key codes exclusively or implementing keyboard layout detection:
private static readonly KEY_EVENT_RECORD[] CtrlVKeyEvents = [ - new KEY_EVENT_RECORD {bKeyDown = true,wVirtualKeyCode = 17, wVirtualScanCode = 29, dwControlKeyState = CONTROL_KEY_STATE.LEFT_CTRL_PRESSED}, - new KEY_EVENT_RECORD {bKeyDown = true,wVirtualKeyCode = 86, wVirtualScanCode = 47, uChar = '\u0016', dwControlKeyState = CONTROL_KEY_STATE.LEFT_CTRL_PRESSED}, - new KEY_EVENT_RECORD {bKeyDown = false,wVirtualKeyCode = 86,wVirtualScanCode = 47, uChar = '\u0016', dwControlKeyState = CONTROL_KEY_STATE.LEFT_CTRL_PRESSED}, - new KEY_EVENT_RECORD {bKeyDown = false,wVirtualKeyCode = 17,wVirtualScanCode = 29, dwControlKeyState = 0 } + new KEY_EVENT_RECORD {bKeyDown = true,wVirtualKeyCode = 17, dwControlKeyState = CONTROL_KEY_STATE.LEFT_CTRL_PRESSED}, + new KEY_EVENT_RECORD {bKeyDown = true,wVirtualKeyCode = 86, uChar = '\u0016', dwControlKeyState = CONTROL_KEY_STATE.LEFT_CTRL_PRESSED}, + new KEY_EVENT_RECORD {bKeyDown = false,wVirtualKeyCode = 86, uChar = '\u0016', dwControlKeyState = CONTROL_KEY_STATE.LEFT_CTRL_PRESSED}, + new KEY_EVENT_RECORD {bKeyDown = false,wVirtualKeyCode = 17, dwControlKeyState = 0 }✅ Verification successful
Scan code usage is limited to paste optimization
The scan codes are only used in the CTRL+V simulation and won't affect regular keyboard input handling. While keyboard layout compatibility could be improved by removing scan codes, the current implementation is acceptable as:
- Virtual key codes (17 for CTRL, 86 for V) ensure basic functionality across layouts
- Regular keyboard input handling remains unaffected
- Paste operation would fallback to regular input if simulation fails
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other scan code usage in the codebase rg "wVirtualScanCode" .Length of output: 833
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Consolonia.Core/Infrastructure/ConsoleWindow.cs(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (1)
src/Consolonia.Core/Infrastructure/ConsoleWindow.cs (1)
401-419: Verify the impact on key handling logicThe early return after handling the 'r' key could skip important key handling logic for modifiers and text input.
Let's verify if any key handling logic is being skipped:
✅ Verification successful
Early return after 'r' key handling is safe and intentional
The early return is part of a specific feature that injects test data when 'r' is pressed. It doesn't skip any critical key handling as the method separately processes key up events after this block. Key modifiers and text input are properly handled through the regular flow for all other keys.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for references to ConsoleOnKeyEvent to understand the expected key handling flow rg -A 10 "ConsoleOnKeyEvent.*=.*"Length of output: 35
Script:
#!/bin/bash # Find the class definition and surrounding context ast-grep --pattern 'class ConsoleWindow { $$$ }' # Look for similar key handling patterns rg -B 5 -A 10 "if \(keyChar is '[a-z]'\)" # Search for key-related method names rg -B 2 -A 10 "OnKey(Down|Up|Press)"Length of output: 17136
Script:
#!/bin/bash # Get the complete ConsoleOnKeyEvent method implementation rg -B 5 -A 30 "private.*void ConsoleOnKeyEvent.*\(" src/Consolonia.Core/Infrastructure/ConsoleWindow.csLength of output: 1889
Co-authored-by: <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/Consolonia.PlatformSupport/Win32Console.cs (1)
146-146:⚠️ Potential issueImprove paste detection logic.
The current implementation assumes any multiple key events indicate a paste operation, which could lead to false positives with fast typing or other key combinations.
🧹 Nitpick comments (2)
src/Consolonia.PlatformSupport/Win32Console.cs (2)
69-87: Document key codes and consider using constants.The key sequence simulation is correct, but consider:
- Adding XML documentation to explain the CTRL+V simulation sequence
- Using constants for virtual key codes (17, 86) and scan codes (29, 47) for better maintainability
Example improvement:
+ // Virtual key codes for CTRL+V simulation + private const ushort VK_CONTROL = 17; // CTRL key + private const ushort VK_V = 86; // V key + private const ushort SC_CONTROL = 29; // CTRL scan code + private const ushort SC_V = 47; // V scan code + + /// <summary> + /// Simulates a complete CTRL+V key sequence with both key press and release events. + /// The sequence: CTRL down → V down → V up → CTRL up + /// </summary> private static readonly KEY_EVENT_RECORD[] CtrlVKeyEvents = [ new() { - bKeyDown = true, wVirtualKeyCode = 17, wVirtualScanCode = 29, + bKeyDown = true, wVirtualKeyCode = VK_CONTROL, wVirtualScanCode = SC_CONTROL, dwControlKeyState = CONTROL_KEY_STATE.LEFT_CTRL_PRESSED },
143-146: Document Windows console behavior being worked around.Add comments explaining:
- Why the Windows console generates multiple key events for paste
- Why this needs to be converted back to CTRL+V
+ // Windows console translates CTRL+V paste operations into a sequence of individual keystrokes, + // simulating as if each character was typed manually. This behavior can cause performance issues + // with large amounts of text. We detect this case and convert it back to a single CTRL+V event + // which will be handled more efficiently by the terminal. bool isPaste = inputRecords.Where(ir => ir.EventType == EVENT_TYPE.KEY_EVENT).Count() > 1;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Consolonia.Core/Infrastructure/ConsoleWindow.cs(2 hunks)src/Consolonia.PlatformSupport/Win32Console.cs(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Consolonia.Core/Infrastructure/ConsoleWindow.cs
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (1)
src/Consolonia.PlatformSupport/Win32Console.cs (1)
147-173: Consider adding error handling for key simulation.The code that simulates CTRL+V should handle potential failures in the key event processing.
Add try-catch block to handle potential failures:
if (isPaste) + { + try + { // simulate CTRL_V foreach (KEY_EVENT_RECORD keyEvent in CtrlVKeyEvents) HandleKeyInput(keyEvent); + } + catch (Exception ex) + { + // If CTRL+V simulation fails, fall back to processing individual key events + foreach (INPUT_RECORD inputRecord in inputRecords) + HandleKeyInput(inputRecord.Event.KeyEvent); + } + }Run this script to verify the robustness of key event handling:
make textbox elements scale to panel.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/Consolonia.Gallery/View/ControlsListView.axaml (1)
47-49: LGTM! Consider adding ScrollViewer properties for better accessibility.The addition of ScrollViewer is a good improvement for handling variable content sizes. Consider adding
HorizontalScrollBarVisibilityandVerticalScrollBarVisibilityproperties for better control over scrolling behavior.- <ScrollViewer Content="{Binding ElementName=GalleryGrid,Path=SelectedItem, Converter={StaticResource GalleryItemConverter}}"/> + <ScrollViewer HorizontalScrollBarVisibility="Auto" + VerticalScrollBarVisibility="Auto" + Content="{Binding ElementName=GalleryGrid,Path=SelectedItem, Converter={StaticResource GalleryItemConverter}}"/>src/Consolonia.Gallery/Gallery/GalleryViews/GalleryTextBox.axaml (1)
65-71: Consider making the TextBox height configurable.The height increase from 4-5 to 10 provides more space for pasted content, which is good. However, consider making this value configurable through a property or style for better reusability.
- Height="10" + Height="{DynamicResource MultilineTextBoxHeight}"Add to resources:
<UserControl.Resources> <system:Double x:Key="MultilineTextBoxHeight">10</system:Double> </UserControl.Resources>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Consolonia.Gallery/Gallery/GalleryViews/GalleryTextBox.axaml(1 hunks)src/Consolonia.Gallery/View/ControlsListView.axaml(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (1)
src/Consolonia.Gallery/Gallery/GalleryViews/GalleryTextBox.axaml (1)
9-19: Good improvement in layout flexibility!Removing the fixed width constraints and switching to
HorizontalAlignment="Stretch"is a better approach for handling pasted content of varying lengths. This change aligns well with the PR's objective of improving paste performance.
Co-authored-by: <[email protected]>
…into tomlm/fixPaste
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/Consolonia.PlatformSupport/Win32Console.cs (3)
142-154: Refine paste detection logic.The current implementation might have false positives as it assumes any sequence of multiple key events is a paste operation. Consider additional validation by checking the actual key sequence.
- if (clipboard != null && inputRecords.Where(evt => evt.EventType == EVENT_TYPE.KEY_EVENT).Skip(1).Any()) + if (clipboard != null && + inputRecords.Count(evt => + evt.EventType == EVENT_TYPE.KEY_EVENT && + evt.Event.KeyEvent.bKeyDown && + ((evt.Event.KeyEvent.wVirtualKeyCode == 17 && evt.Event.KeyEvent.dwControlKeyState.HasFlag(CONTROL_KEY_STATE.LEFT_CTRL_PRESSED)) || + evt.Event.KeyEvent.wVirtualKeyCode == 86) + ) >= 2)
202-214: Consider more flexible string comparison for clipboard text.The current implementation might be too strict with whitespace handling. Consider normalizing both strings for a more flexible comparison.
- if (clipboardText.Trim() == currentBufferText.Trim()) + if (string.Equals( + clipboardText.Trim().Replace(" ", ""), + currentBufferText.Trim().Replace(" ", ""), + StringComparison.Ordinal))
236-254: Add default case to handle unknown event types.The switch statement silently ignores unknown event types. Consider adding a default case for better error handling and logging.
switch (inputRecord.EventType) { // ... existing cases ... + default: + Debug.WriteLine($"Unhandled console event type: {inputRecord.EventType}"); + break; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/Consolonia.Core/Infrastructure/ConsoleWindow.cs(1 hunks)src/Consolonia.Gallery/Gallery/GalleryViews/GalleryTextBox.axaml(2 hunks)src/Consolonia.Gallery/View/ControlsListView.axaml(1 hunks)src/Consolonia.GuiCS/WindowsDriver.cs(1 hunks)src/Consolonia.PlatformSupport/Win32Console.cs(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- src/Consolonia.Gallery/View/ControlsListView.axaml
- src/Consolonia.Gallery/Gallery/GalleryViews/GalleryTextBox.axaml
- src/Consolonia.GuiCS/WindowsDriver.cs
- src/Consolonia.Core/Infrastructure/ConsoleWindow.cs
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (1)
src/Consolonia.PlatformSupport/Win32Console.cs (1)
72-90: LGTM! Well-structured key event simulation.The
CtrlVKeyEventsarray correctly simulates the CTRL+V key sequence with proper virtual key codes, scan codes, and control states.
Co-authored-by: <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/Consolonia.PlatformSupport/Win32Console.cs (3)
137-153: Add cancellation support and improve paste detection.The infinite loop should support cancellation for proper cleanup. Additionally, the paste detection logic could be more precise.
Consider these improvements:
-Task.Run(async () => +Task.Run(async (CancellationToken ct) => { - while (!Disposed) + while (!Disposed && !ct.IsCancellationRequested) { PauseTask?.Wait(); var inputRecords = _windowsConsole.ReadConsoleInput(); var clipboard = AvaloniaLocator.Current.GetService<IClipboard>(); if (clipboard != null && - inputRecords.Where(evt => evt.EventType == EVENT_TYPE.KEY_EVENT).Skip(1).Any()) + inputRecords.Count(evt => + evt.EventType == EVENT_TYPE.KEY_EVENT && + evt.Event.KeyEvent.bKeyDown) > 1)
161-229: Consider breaking down the complex method.The
ProcessClipboardInputmethod is quite long and handles multiple responsibilities.Consider extracting these methods:
TryMatchClipboardText(string clipboardText, StringBuilder buffer)ProcessBufferedKeyEvents(List<INPUT_RECORD> events)EmitCtrlVSequence()This would improve readability and maintainability.
231-252: LGTM! Consider adding XML documentation.The event handling implementation is clean and well-structured. Consider adding XML documentation for better maintainability.
Add documentation for the method:
+/// <summary> +/// Processes a single input record and dispatches it to the appropriate handler. +/// </summary> +/// <param name="inputRecord">The input record to process.</param> private void HandleInputRecord(INPUT_RECORD inputRecord)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Consolonia.PlatformSupport/Win32Console.cs(3 hunks)
🔇 Additional comments (1)
src/Consolonia.PlatformSupport/Win32Console.cs (1)
72-90: LGTM! Well-structured key event sequence.The
CtrlVKeyEventsarray correctly simulates the CTRL+V key sequence with proper virtual key codes and control states.
This should work on linux and macos as well
…into tomlm/fixPaste
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/Consolonia.Core/Infrastructure/ClipboardImpl.cs (1)
20-23: Document unimplemented methods with XML comments.The unimplemented methods should include XML documentation explaining why they're not implemented and potential future plans.
+/// <summary> +/// Not implemented in the current version. Will be implemented when full clipboard data support is added. +/// </summary> +/// <exception cref="NotImplementedException">Always thrown as this method is not yet implemented.</exception> public Task<object> GetDataAsync(string format)Also applies to: 25-28, 33-36
src/Consolonia.Core/Infrastructure/ConsoloniaPlatform.cs (1)
70-92: Remove commented-out code.Since we've moved to a platform-agnostic clipboard implementation, this commented-out code should be removed to improve maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/Consolonia.Core/Consolonia.Core.csproj(1 hunks)src/Consolonia.Core/Infrastructure/ClipboardImpl.cs(1 hunks)src/Consolonia.Core/Infrastructure/ConsoloniaPlatform.cs(1 hunks)
🧰 Additional context used
🪛 GitHub Check: build
src/Consolonia.Core/Infrastructure/ClipboardImpl.cs
[failure] 14-14:
Either replace the suffix 'Impl' in member name 'ClipboardImpl' with the suggested alternate 'Core' or remove the suffix completely (https://learn.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca1711)
[failure] 14-14:
Either replace the suffix 'Impl' in member name 'ClipboardImpl' with the suggested alternate 'Core' or remove the suffix completely (https://learn.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca1711)
🪛 GitHub Actions: General Build
src/Consolonia.Core/Infrastructure/ClipboardImpl.cs
[error] 14-14: CA1711: Either replace the suffix 'Impl' in member name 'ClipboardImpl' with the suggested alternate 'Core' or remove the suffix completely
🪛 GitHub Actions: Code Inspections
src/Consolonia.Core/Infrastructure/ClipboardImpl.cs
[warning] 45-45: Possible 'null' assignment to non-nullable entity
[warning] 15-15: Either replace the suffix 'Impl' in member name 'ClipboardImpl' with the suggested alternate 'Core' or remove the suffix completely
🔇 Additional comments (4)
src/Consolonia.Core/Infrastructure/ClipboardImpl.cs (1)
8-13: Consider adding timeline for replacing temporary solution.The TODO comment indicates this is a temporary solution. Consider adding a timeline or tracking issue for implementing the proper Avalonia platform implementation.
Would you like me to create a GitHub issue to track the implementation of a proper platform-specific clipboard solution?
src/Consolonia.Core/Infrastructure/ConsoloniaPlatform.cs (1)
62-62: LGTM! Simplified clipboard initialization.The direct binding to ClipboardImpl simplifies the code and removes potential performance overhead from dynamic assembly loading.
src/Consolonia.Core/Consolonia.Core.csproj (2)
9-9: Clarify reason for SkiaSharp update.The update to SkiaSharp.NativeAssets.Linux.NoDependencies seems unrelated to the clipboard performance fix. Consider moving this change to a separate PR.
10-10: Verify TextCopy package version compatibility.Please verify that version 6.2.1 of TextCopy is the latest stable version and is compatible with the project's target framework.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/Consolonia.Core/Helpers/Extensions.cs(1 hunks)src/Consolonia.Core/Infrastructure/ClipboardImpl.cs(1 hunks)src/Experimental/Consolonia.Editor/AvaloniaEdit.Demo.csproj(0 hunks)src/Experimental/Consolonia.Editor/CEdit.csproj(0 hunks)src/Experimental/Consolonia.Editor/CEdit.sln(0 hunks)
💤 Files with no reviewable changes (3)
- src/Experimental/Consolonia.Editor/AvaloniaEdit.Demo.csproj
- src/Experimental/Consolonia.Editor/CEdit.csproj
- src/Experimental/Consolonia.Editor/CEdit.sln
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Consolonia.Core/Infrastructure/ClipboardImpl.cs
🧰 Additional context used
🪛 GitHub Check: build
src/Consolonia.Core/Helpers/Extensions.cs
[failure] 17-17:
The type name Extensions conflicts in whole or in part with the namespace name 'Microsoft.Extensions'. Change either name to eliminate the conflict. (https://learn.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca1724)
[failure] 17-17:
The type name Extensions conflicts in whole or in part with the namespace name 'Microsoft.Extensions'. Change either name to eliminate the conflict. (https://learn.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca1724)
🪛 GitHub Actions: General Build
src/Consolonia.Core/Helpers/Extensions.cs
[error] 17-17: The type name Extensions conflicts in whole or in part with the namespace name 'Microsoft.Extensions'. Change either name to eliminate the conflict.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (1)
src/Consolonia.Core/Helpers/Extensions.cs (1)
16-18: Verify relevance of these changes to PR objectives.The changes in this file appear unrelated to fixing paste performance on Windows. Could you clarify how these changes contribute to addressing the performance issue described in the PR objectives?
🧰 Tools
🪛 GitHub Check: build
[failure] 17-17:
The type name Extensions conflicts in whole or in part with the namespace name 'Microsoft.Extensions'. Change either name to eliminate the conflict. (https://learn.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca1724)
[failure] 17-17:
The type name Extensions conflicts in whole or in part with the namespace name 'Microsoft.Extensions'. Change either name to eliminate the conflict. (https://learn.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca1724)🪛 GitHub Actions: General Build
[error] 17-17: The type name Extensions conflicts in whole or in part with the namespace name 'Microsoft.Extensions'. Change either name to eliminate the conflict.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/Consolonia.Core/Infrastructure/ConsoloniaPlatform.cs (1)
62-68: Consider cleaning up commented-out code.Since we're moving to a new clipboard implementation, these commented-out bindings could be removed to improve code maintainability. If they're kept for reference, consider moving them to documentation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Consolonia.Core/Infrastructure/ClipboardImpl.cs(1 hunks)src/Consolonia.Core/Infrastructure/ConsoloniaPlatform.cs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Consolonia.Core/Infrastructure/ClipboardImpl.cs
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (3)
src/Consolonia.Core/Infrastructure/ConsoloniaPlatform.cs (3)
61-61: Verify clipboard implementation across platforms.The new platform-agnostic
ClipboardImplapproach might need validation across different operating systems. While this change addresses the Windows paste performance issue, we should ensure it works correctly on other platforms.Let's verify if there are any platform-specific clipboard implementations or tests:
✅ Verification successful
Clipboard implementation is intentionally simplified for cross-platform support
The new
ClipboardImplis an intentional temporary solution that:
- Uses the TextCopy library to handle platform differences
- Implements basic text operations while deferring advanced features
- Removes complex Windows-specific event processing that caused performance issues
The trade-offs are well-documented in the implementation's TODO comment, acknowledging this as an interim solution until a proper Avalonia platform implementation can be integrated without the full rendering subsystem dependencies.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for clipboard-related implementations and tests echo "Searching for clipboard implementations..." rg -l "IClipboard|ClipboardImpl" --type cs echo -e "\nSearching for clipboard-related tests..." rg -l "clipboard|paste|copy" --glob "*test*.cs"Length of output: 643
Script:
#!/bin/bash # Examine clipboard implementations echo "=== ClipboardImpl.cs ===" cat src/Consolonia.Core/Infrastructure/ClipboardImpl.cs echo -e "\n=== Win32Console.cs clipboard-related code ===" rg "clipboard|paste|copy" -i -C 5 src/Consolonia.PlatformSupport/Win32Console.cs echo -e "\n=== Looking for tests with correct pattern ===" fd -e cs -g "*test*" -x rg -l "clipboard|paste|copy" {}Length of output: 6046
61-90: Ensure comprehensive testing of the new clipboard implementation.While the simplified clipboard handling should improve paste performance on Windows, please ensure:
- The changes are tested on all supported platforms
- Performance metrics are collected to verify the improvement
- Edge cases (large pastes, special characters, etc.) are covered
Let's check for existing clipboard-related tests:
69-90: Document the architectural decision to remove platform-specific clipboard handling.The removal of platform-specific clipboard initialization, especially the dynamic loading of
Avalonia.Win32, should improve performance. However, consider:
- Adding a comment explaining why platform-specific handling was removed
- Documenting any limitations of the new approach
- Creating an issue to track potential platform-specific requirements for macOS and Linux
Let's check if there are any remaining references to platform-specific clipboard handling:
✅ Verification successful
Platform-specific clipboard handling removal is justified
The removal of platform-specific clipboard code is a valid architectural improvement:
- The new
ClipboardImplprovides a unified, simpler implementation usingClipboardService- Performance is improved by eliminating dynamic assembly loading
- The codebase is more maintainable with a single implementation
The commented-out code can be safely removed as it's been replaced with a better solution.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for remaining platform-specific clipboard references echo "Searching for Win32 clipboard references..." rg "Avalonia\.Win32.*Clipboard" --type cs echo -e "\nSearching for X11 clipboard references..." rg "X11.*Clipboard" --type cs echo -e "\nSearching for platform checks in clipboard context..." rg "(?i)(iswindows|islinux|ismacos).*clipboard" --type csLength of output: 716
Script:
#!/bin/bash # Search for clipboard implementations and documentation echo "Searching for clipboard implementations..." rg "class.*Clipboard" -A 5 --type cs echo -e "\nSearching for clipboard interface..." rg "interface.*Clipboard" -A 5 --type cs echo -e "\nSearching for clipboard documentation..." fd README.md CHANGELOG.md -x cat {} \; | rg -i "clipboard"Length of output: 1052
Script:
#!/bin/bash # Check ClipboardService implementation echo "Searching for ClipboardService..." rg "class.*ClipboardService" -A 10 --type cs echo -e "\nSearching for ClipboardService usage..." ast-grep --pattern 'ClipboardService.$_($$$)'Length of output: 619
…into tomlm/fixPaste
Co-authored-by: <[email protected]>
…into tomlm/fixPaste
Pull request was converted to draft
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
src/Consolonia.Core/Infrastructure/NaiveClipboard.cs (2)
8-8: Add implementation timeline to TODO commentThe TODO comment suggests this is a temporary solution, but doesn't indicate when it should be replaced.
Consider adding more context:
- // TODO: Replace this with avalonia platform implementation + // TODO: Replace this with avalonia platform implementation when Windows-specific clipboard handling is implemented (Issue #239)
19-27: Document unsupported operationsMethods throwing NotImplementedException should be documented to indicate they're not supported.
Add XML documentation:
+ /// <summary> + /// Not supported in the naive implementation. + /// </summary> + /// <exception cref="NotImplementedException">This operation is not supported.</exception> public async Task<object> GetDataAsync(string format)🧰 Tools
🪛 GitHub Check: build
[failure] 19-19:
This async method lacks 'await' operators and will run synchronously. Consider using the 'await' operator to await non-blocking API calls, or 'await Task.Run(...)' to do CPU-bound work on a background thread.
[failure] 24-24:
This async method lacks 'await' operators and will run synchronously. Consider using the 'await' operator to await non-blocking API calls, or 'await Task.Run(...)' to do CPU-bound work on a background thread.
[failure] 19-19:
This async method lacks 'await' operators and will run synchronously. Consider using the 'await' operator to await non-blocking API calls, or 'await Task.Run(...)' to do CPU-bound work on a background thread.
[failure] 24-24:
This async method lacks 'await' operators and will run synchronously. Consider using the 'await' operator to await non-blocking API calls, or 'await Task.Run(...)' to do CPU-bound work on a background thread.src/Tests/Consolonia.Gallery.Tests/TextBlockTests.cs (1)
46-47: Document the rationale for text content changesThe test's expected text content has been significantly modified. Consider adding a comment explaining why these specific text changes were necessary.
Add a comment before the test:
+ // Text content adjusted to align with new clipboard handling implementation [Test] public async Task HandlesMultilineText()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/Consolonia.Core/Consolonia.Core.csproj(1 hunks)src/Consolonia.Core/Infrastructure/ConsoloniaPlatform.cs(2 hunks)src/Consolonia.Core/Infrastructure/NaiveClipboard.cs(1 hunks)src/Tests/Consolonia.Gallery.Tests/TextBlockTests.cs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/Consolonia.Core/Consolonia.Core.csproj
- src/Consolonia.Core/Infrastructure/ConsoloniaPlatform.cs
🧰 Additional context used
🪛 GitHub Check: build
src/Consolonia.Core/Infrastructure/NaiveClipboard.cs
[failure] 14-14:
This async method lacks 'await' operators and will run synchronously. Consider using the 'await' operator to await non-blocking API calls, or 'await Task.Run(...)' to do CPU-bound work on a background thread.
[failure] 19-19:
This async method lacks 'await' operators and will run synchronously. Consider using the 'await' operator to await non-blocking API calls, or 'await Task.Run(...)' to do CPU-bound work on a background thread.
[failure] 24-24:
This async method lacks 'await' operators and will run synchronously. Consider using the 'await' operator to await non-blocking API calls, or 'await Task.Run(...)' to do CPU-bound work on a background thread.
[failure] 29-29:
This async method lacks 'await' operators and will run synchronously. Consider using the 'await' operator to await non-blocking API calls, or 'await Task.Run(...)' to do CPU-bound work on a background thread.
[failure] 34-34:
This async method lacks 'await' operators and will run synchronously. Consider using the 'await' operator to await non-blocking API calls, or 'await Task.Run(...)' to do CPU-bound work on a background thread.
[failure] 39-39:
This async method lacks 'await' operators and will run synchronously. Consider using the 'await' operator to await non-blocking API calls, or 'await Task.Run(...)' to do CPU-bound work on a background thread.
[failure] 14-14:
This async method lacks 'await' operators and will run synchronously. Consider using the 'await' operator to await non-blocking API calls, or 'await Task.Run(...)' to do CPU-bound work on a background thread.
[failure] 19-19:
This async method lacks 'await' operators and will run synchronously. Consider using the 'await' operator to await non-blocking API calls, or 'await Task.Run(...)' to do CPU-bound work on a background thread.
[failure] 24-24:
This async method lacks 'await' operators and will run synchronously. Consider using the 'await' operator to await non-blocking API calls, or 'await Task.Run(...)' to do CPU-bound work on a background thread.
[failure] 29-29:
This async method lacks 'await' operators and will run synchronously. Consider using the 'await' operator to await non-blocking API calls, or 'await Task.Run(...)' to do CPU-bound work on a background thread.
🪛 GitHub Actions: General Build
src/Consolonia.Core/Infrastructure/NaiveClipboard.cs
[error] 14-14: This async method lacks 'await' operators and will run synchronously. Consider using the 'await' operator to await non-blocking API calls, or 'await Task.Run(...)' to do CPU-bound work on a background thread.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (1)
src/Tests/Consolonia.Gallery.Tests/TextBlockTests.cs (1)
46-47: Verify text wrapping behavior changesThe test expectations have been modified to show different text content and wrapping. Please ensure these changes are intentional and align with the new clipboard handling implementation.
Run the following to check for related text wrapping changes:
✅ Verification successful
Test changes are safe and intentional
The modifications only update the test data content while maintaining the same test purpose - verifying multiline text handling. No implementation changes affecting text wrapping behavior were found in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for text wrapping related changes rg -l "TextWrapping|WordWrapping"Length of output: 285
Script:
#!/bin/bash # Get test file content for context cat src/Tests/Consolonia.Gallery.Tests/TextBlockTests.cs # Look for text wrapping related changes in implementation rg -B2 -A2 "TextWrapping|WordWrapping" --type csLength of output: 1771
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (7)
src/Consolonia.PlatformSupport/Clipboard/X11Clipboard.cs (3)
18-22: Simplify asynchronous methods that lackawaitoperatorsThe
ClearAsyncmethod does not contain anyawaitstatements. You can simplify it by removing theasyncmodifier and returning a completed task directly.Apply this diff:
-public async Task ClearAsync() +public Task ClearAsync() { - await Task.CompletedTask; Medo.X11.X11Clipboard.Clipboard.Clear(); + return Task.CompletedTask; }Similarly, consider applying this change to other methods like
SetTextAsyncandGetTextAsyncthat do not useawait.
34-38: SimplifyGetTextAsyncmethod by removing unnecessaryasyncmodifierThe
GetTextAsyncmethod does not perform any asynchronous operations. Removing theasyncmodifier and returning the result usingTask.FromResultcan improve code clarity.Apply this diff:
-public async Task<string> GetTextAsync() +public Task<string> GetTextAsync() { - await Task.CompletedTask; - return Medo.X11.X11Clipboard.Clipboard.GetText(); + return Task.FromResult(Medo.X11.X11Clipboard.Clipboard.GetText()); }
45-49: SimplifySetTextAsyncmethod by removing unnecessaryasyncmodifierSince
SetTextAsyncdoes not contain anyawaitstatements, you can remove theasyncmodifier and return a completed task.Apply this diff:
-public async Task SetTextAsync(string text) +public Task SetTextAsync(string text) { - await Task.CompletedTask; Medo.X11.X11Clipboard.Clipboard.SetText(text); + return Task.CompletedTask; }src/Consolonia.PlatformSupport/Clipboard/WSLClipboard.cs (1)
55-71: Add retry mechanism for clipboard operationsClipboard operations can fail transiently, especially in WSL environment.
Consider implementing a retry mechanism with exponential backoff for clipboard operations:
+private async Task<string> RetryOperation(Func<Task<string>> operation, int maxRetries = 3) +{ + for (int i = 0; i < maxRetries; i++) + { + try + { + return await operation(); + } + catch when (i < maxRetries - 1) + { + await Task.Delay((1 << i) * 100); // Exponential backoff + } + } + return string.Empty; +}🧰 Tools
🪛 GitHub Check: build
[failure] 55-55:
This async method lacks 'await' operators and will run synchronously. Consider using the 'await' operator to await non-blocking API calls, or 'await Task.Run(...)' to do CPU-bound work on a background thread.
[failure] 55-55:
This async method lacks 'await' operators and will run synchronously. Consider using the 'await' operator to await non-blocking API calls, or 'await Task.Run(...)' to do CPU-bound work on a background thread.src/Consolonia.PlatformSupport/Clipboard/ClipboardProcessRunner.cs (2)
56-56: Use StringComparison for culture-invariant string operationsThe
Containsmethod should specify string comparison type for clarity and correctness.-public static bool FileExists(this string value) { return !string.IsNullOrEmpty(value) && !value.Contains("not found"); } +public static bool FileExists(this string value) { return !string.IsNullOrEmpty(value) && !value.Contains("not found", StringComparison.OrdinalIgnoreCase); }🧰 Tools
🪛 GitHub Check: build
[failure] 56-56:
'string.Contains(string)' has a method overload that takes a 'StringComparison' parameter. Replace this call in 'Consolonia.PlatformSupport.Clipboard.ClipboardProcessRunner.FileExists(string)' with a call to 'string.Contains(string, System.StringComparison)' for clarity of intent. (https://learn.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca1307)
[failure] 56-56:
'string.Contains(string)' has a method overload that takes a 'StringComparison' parameter. Replace this call in 'Consolonia.PlatformSupport.Clipboard.ClipboardProcessRunner.FileExists(string)' with a call to 'string.Contains(string, System.StringComparison)' for clarity of intent. (https://learn.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca1307)
90-96: Consider configurable timeout valuesHard-coded timeout value of 5000ms might not be suitable for all environments.
+private const int DefaultProcessTimeout = 5000; +public static (int exitCode, string result) Process( string cmd, string arguments, string input = null, - bool waitForOutput = true + bool waitForOutput = true, + int timeout = DefaultProcessTimeout ) { - if (!process.WaitForExit(5000)) + if (!process.WaitForExit(timeout))src/Consolonia.Core/Infrastructure/ConsoloniaPlatform.cs (1)
62-67: Consider cleaning up commented codeThe file contains several commented-out bindings that appear to be obsolete. Consider removing these comments to improve code maintainability.
- //.Bind<IClipboard>().ToConstant(null) - /*.Bind<IKeyboardNavigationHandler>().ToTransient<ArrowsAndKeyboardNavigationHandler>() todo: implement this navigation*/ - //.Bind<IClipboard>().ToConstant(new X11Clipboard(this)) - //.Bind<IPlatformSettings>().ToConstant(new PlatformSettingsStub()) - //.Bind<ISystemDialogImpl>().ToConstant(new GtkSystemDialog()) - /*.Bind<IMountedVolumeInfoProvider>().ToConstant(new LinuxMountedVolumeInfoProvider())*/; + ;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
src/Consolonia.Core/Infrastructure/ConsoloniaPlatform.cs(1 hunks)src/Consolonia.Core/Infrastructure/NaiveClipboard.cs(1 hunks)src/Consolonia.PlatformSupport/Clipboard/ClipboardProcessRunner.cs(1 hunks)src/Consolonia.PlatformSupport/Clipboard/MacOSXClipboard.cs(1 hunks)src/Consolonia.PlatformSupport/Clipboard/WSLClipboard.cs(1 hunks)src/Consolonia.PlatformSupport/Clipboard/X11Clipboard.cs(1 hunks)src/Consolonia.PlatformSupport/Clipboard/XClipClipboard.cs(1 hunks)src/Consolonia.PlatformSupport/Consolonia.PlatformSupport.csproj(1 hunks)src/Consolonia.PlatformSupport/PlatformSupportExtensions.cs(2 hunks)
🧰 Additional context used
🪛 GitHub Check: build
src/Consolonia.PlatformSupport/Clipboard/XClipClipboard.cs
[failure] 48-48:
This async method lacks 'await' operators and will run synchronously. Consider using the 'await' operator to await non-blocking API calls, or 'await Task.Run(...)' to do CPU-bound work on a background thread.
[failure] 48-48:
This async method lacks 'await' operators and will run synchronously. Consider using the 'await' operator to await non-blocking API calls, or 'await Task.Run(...)' to do CPU-bound work on a background thread.
src/Consolonia.PlatformSupport/Clipboard/ClipboardProcessRunner.cs
[failure] 56-56:
'string.Contains(string)' has a method overload that takes a 'StringComparison' parameter. Replace this call in 'Consolonia.PlatformSupport.Clipboard.ClipboardProcessRunner.FileExists(string)' with a call to 'string.Contains(string, System.StringComparison)' for clarity of intent. (https://learn.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca1307)
[failure] 56-56:
'string.Contains(string)' has a method overload that takes a 'StringComparison' parameter. Replace this call in 'Consolonia.PlatformSupport.Clipboard.ClipboardProcessRunner.FileExists(string)' with a call to 'string.Contains(string, System.StringComparison)' for clarity of intent. (https://learn.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca1307)
src/Consolonia.PlatformSupport/Clipboard/WSLClipboard.cs
[failure] 55-55:
This async method lacks 'await' operators and will run synchronously. Consider using the 'await' operator to await non-blocking API calls, or 'await Task.Run(...)' to do CPU-bound work on a background thread.
[failure] 55-55:
This async method lacks 'await' operators and will run synchronously. Consider using the 'await' operator to await non-blocking API calls, or 'await Task.Run(...)' to do CPU-bound work on a background thread.
src/Consolonia.PlatformSupport/Clipboard/MacOSXClipboard.cs
[failure] 43-43:
Exception type System.Exception is not sufficiently specific (https://learn.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca2201)
[failure] 43-43:
Exception type System.Exception is not sufficiently specific (https://learn.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca2201)
🪛 GitHub Actions: General Build
src/Consolonia.PlatformSupport/Clipboard/XClipClipboard.cs
[error] 48-48: This async method lacks 'await' operators and will run synchronously. Consider using the 'await' operator to await non-blocking API calls, or 'await Task.Run(...)' to do CPU-bound work on a background thread.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (4)
src/Consolonia.Core/Infrastructure/NaiveClipboard.cs (2)
13-13: Consider thread safety for clipboard operationsThe
_textfield is not thread-safe. In a multi-threaded environment, concurrent access to the clipboard could lead to race conditions.
16-20: Remove async/await from synchronous operationsThe async methods don't perform any asynchronous work, causing unnecessary overhead.
Also applies to: 32-36, 43-47
src/Consolonia.PlatformSupport/Consolonia.PlatformSupport.csproj (1)
4-4: Clarify the scope of clipboard changesWhile the PR's primary objective is to fix Windows paste performance, this change introduces X11 clipboard support. Could you explain if this is part of a broader clipboard refactoring effort?
Let's check if there are related clipboard implementation files:
✅ Verification successful
X11 clipboard is part of cross-platform clipboard architecture
The X11 clipboard implementation is part of a comprehensive cross-platform clipboard system that includes support for Windows, MacOS, and various Linux environments (X11, XClip, WSL). The change aligns with the existing architecture where each platform has its dedicated clipboard implementation with automatic detection and fallback mechanisms.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Look for clipboard-related files fd -e cs clipboard -d 3Length of output: 25
Script:
#!/bin/bash # Search for clipboard-related code and X11 clipboard usage rg -i "clipboard" --type cs rg "Medo\.X11" --type csLength of output: 11500
src/Consolonia.Core/Infrastructure/ConsoloniaPlatform.cs (1)
62-62: Verify clipboard initialization flowThe clipboard binding is commented out, suggesting clipboard initialization has been moved elsewhere. To ensure this change addresses the Windows paste performance issue:
- Confirm that the new clipboard initialization path properly handles Windows-specific optimizations for paste operations
- Verify that removing this binding doesn't leave any clipboard functionality gaps
Let's check the new clipboard initialization path:
✅ Verification successful
Clipboard initialization is properly handled with performance optimizations
The commented-out binding has been replaced with a robust platform-specific initialization system that:
- Uses optimized Windows clipboard implementation from Avalonia.Win32
- Includes specific paste performance optimizations for Windows
- Provides appropriate implementations for all supported platforms (Windows, macOS, Linux)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Look for the new clipboard initialization code rg -l "UseAutoDectectedClipboard|IClipboard" --type cs # Look for Windows-specific clipboard handling ast-grep --pattern 'class $_ { $$$ paste($$$) { $$$ } $$$ }'Length of output: 633
Script:
#!/bin/bash # Check platform support extensions for clipboard initialization cat src/Consolonia.PlatformSupport/PlatformSupportExtensions.cs # Check Windows implementation cat src/Consolonia.PlatformSupport/Win32Console.cs # Look for clipboard interface definition and usage ast-grep --pattern 'interface IClipboard { $$$ }'Length of output: 22026
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
src/Consolonia.PlatformSupport/PlatformSupportExtensions.cs (1)
50-55:⚠️ Potential issueImprove error handling for assembly loading
The current implementation could throw exceptions if assembly loading fails. Add proper exception handling to gracefully handle failures.
Apply this diff to enhance error handling:
+try +{ Assembly assembly = Assembly.Load("Avalonia.Win32"); - ArgumentNullException.ThrowIfNull(assembly, "Avalonia.Win32"); Type type = assembly.GetType(assembly.GetName().Name + ".ClipboardImpl"); - ArgumentNullException.ThrowIfNull(type, "ClipboardImpl"); var clipboard = Activator.CreateInstance(type) as IClipboard; return builder.With(clipboard ?? new NaiveClipboard()); +} +catch (Exception ex) when (ex is ReflectionTypeLoadException || ex is TypeLoadException) +{ + // Fallback to NaiveClipboard if assembly loading fails + return builder.With<IClipboard>(new NaiveClipboard()); +}
🧹 Nitpick comments (1)
.gitignore (1)
770-770: LGTM! Consider generalizing the VS Code ignore pattern.The addition of
src/.vscode/launch.jsonis appropriate as launch configurations are user-specific. However, for better maintainability, you might want to ignore all VS Code settings in thesrcdirectory.Consider using a more general pattern:
-src/.vscode/launch.json +src/**/.vscode/This would catch all VS Code settings directories under
src/, making it more maintainable as the project grows.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.gitignore(1 hunks)src/Consolonia.PlatformSupport/PlatformSupportExtensions.cs(3 hunks)
🧰 Additional context used
🪛 GitHub Check: build
src/Consolonia.PlatformSupport/PlatformSupportExtensions.cs
[failure] 77-77:
'string.Contains(string)' has a method overload that takes a 'StringComparison' parameter. Replace this call in 'Consolonia.PlatformSupportExtensions.IsWSLPlatform()' with a call to 'string.Contains(string, System.StringComparison)' for clarity of intent. (https://learn.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca1307)
[failure] 77-77:
'string.Contains(string)' has a method overload that takes a 'StringComparison' parameter. Replace this call in 'Consolonia.PlatformSupportExtensions.IsWSLPlatform()' with a call to 'string.Contains(string, System.StringComparison)' for clarity of intent. (https://learn.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca1307)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
src/Consolonia.PlatformSupport/Clipboard/ClipboardProcessRunner.cs (2)
27-37: Clarify the purpose of 'DoubleWaitForExit' methodThe
DoubleWaitForExitmethod contains a non-standard pattern of waiting for exit twice. Adding a comment to explain why this is necessary would improve code readability and maintainability.Consider adding an explanatory comment:
public static bool DoubleWaitForExit(this Process process) { bool result = process.WaitForExit(500); + // Ensure that all events are processed after the initial wait if (result) { process.WaitForExit(); } return result; }
73-79: Handle potential infinite wait in 'Process' methodIf the process does not exit within the specified timeout, the current implementation throws a
TimeoutException. To prevent potential infinite waits and improve robustness, consider adding logic to kill the process if it exceeds the timeout.Apply this diff to safely terminate the process on timeout:
if (!process.WaitForExit(5000)) { var timeoutError = $@"Process timed out. Command line: {process.StartInfo.FileName} {process.StartInfo.Arguments}."; + process.Kill(true); throw new TimeoutException(timeoutError); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/Consolonia.PlatformSupport/Clipboard/ClipboardProcessRunner.cs(1 hunks)src/Consolonia.PlatformSupport/Clipboard/MacOSXClipboard.cs(1 hunks)src/Consolonia.PlatformSupport/Clipboard/WSLClipboard.cs(1 hunks)src/Consolonia.PlatformSupport/Clipboard/X11Clipboard.cs(1 hunks)src/Consolonia.PlatformSupport/Clipboard/XClipClipboard.cs(1 hunks)src/Consolonia.PlatformSupport/PlatformSupportExtensions.cs(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/Consolonia.PlatformSupport/Clipboard/XClipClipboard.cs
- src/Consolonia.PlatformSupport/Clipboard/WSLClipboard.cs
- src/Consolonia.PlatformSupport/Clipboard/X11Clipboard.cs
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (3)
src/Consolonia.PlatformSupport/PlatformSupportExtensions.cs (3)
41-41: Fix typo in method name 'UseAutoDectectedClipboard'The method name
UseAutoDectectedClipboardis misspelled. It should beUseAutoDetectedClipboard.Apply this diff:
- .UseAutoDectectedClipboard() + .UseAutoDetectedClipboard()
62-62: Fix typo in method name 'UseAutoDectectedClipboard'The method name
UseAutoDectectedClipboardis misspelled. It should beUseAutoDetectedClipboard.Apply this diff:
-public static AppBuilder UseAutoDectectedClipboard(this AppBuilder builder) +public static AppBuilder UseAutoDetectedClipboard(this AppBuilder builder)
67-72: Improve exception handling for assembly loadingThe current implementation may throw exceptions if assembly loading fails. Consider adding proper exception handling to gracefully handle scenarios where the assembly or the type cannot be loaded.
Apply this diff to enhance error handling:
+try +{ Assembly assembly = Assembly.Load("Avalonia.Win32"); - ArgumentNullException.ThrowIfNull(assembly, "Avalonia.Win32"); Type type = assembly.GetType(assembly.GetName().Name + ".ClipboardImpl"); - ArgumentNullException.ThrowIfNull(type, "ClipboardImpl"); var clipboard = Activator.CreateInstance(type) as IClipboard; return builder.With(clipboard ?? new NaiveClipboard()); +} +catch (Exception ex) when (ex is FileNotFoundException || ex is TypeLoadException || ex is ReflectionTypeLoadException) +{ + // Fallback to NaiveClipboard if assembly or type loading fails + return builder.With<IClipboard>(new NaiveClipboard()); +}
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/Consolonia.PlatformSupport/PlatformSupportExtensions.cs (1)
67-72:⚠️ Potential issueImprove error handling for assembly loading
The current implementation could throw exceptions if assembly loading fails.
Apply this diff:
+ try { Assembly assembly = Assembly.Load("Avalonia.Win32"); - ArgumentNullException.ThrowIfNull(assembly, "Avalonia.Win32"); Type type = assembly.GetType(assembly.GetName().Name + ".ClipboardImpl"); - ArgumentNullException.ThrowIfNull(type, "ClipboardImpl"); var clipboard = Activator.CreateInstance(type) as IClipboard; return builder.With(clipboard ?? new NaiveClipboard()); + } catch (Exception ex) when (ex is ReflectionTypeLoadException || ex is TypeLoadException) { + // Fallback to NaiveClipboard if assembly loading fails + return builder.With<IClipboard>(new NaiveClipboard()); + }
🧹 Nitpick comments (14)
src/Consolonia.Gallery/Gallery/GalleryViews/GalleryAnimatedLines.axaml (3)
8-8: Consider explicitly setting HorizontalScrollBarVisibility.While vertical scrolling is properly configured, consider explicitly setting
HorizontalScrollBarVisibilityfor clearer behavior control.- <ScrollViewer VerticalScrollBarVisibility="Auto" > + <ScrollViewer VerticalScrollBarVisibility="Auto" + HorizontalScrollBarVisibility="Disabled">
10-11: Consider adding responsive sizing and animation controls.The fixed-size Panel (40x30) might limit responsiveness. Additionally, rapidly animating UI elements could affect users sensitive to motion.
Consider:
- Using relative sizing or Grid-based layout for better responsiveness
- Adding a way to disable animations (e.g., respecting system animation settings via
{x:Static SystemParameters.ClientAreaAnimation})
128-131: Enhance button accessibility and clarity.The button's purpose and impact should be more clearly communicated to users.
- <Button Content="Pause IO for 5 seconds" + <Button Content="Pause IO for 5 seconds" + AutomationProperties.HelpText="Temporarily pauses input/output operations for 5 seconds" + ToolTip.Tip="Temporarily stops all input/output operations" + KeyboardNavigation.TabIndex="0"src/Consolonia.Gallery/Gallery/GalleryViews/GalleryTextBlock.axaml (2)
16-16: Consider adding horizontal scroll behavior.While vertical scrolling is handled, consider explicitly setting
HorizontalScrollBarVisibilityfor better handling of long text content and international characters that might exceed the width.- <ScrollViewer VerticalScrollBarVisibility="Auto" > + <ScrollViewer VerticalScrollBarVisibility="Auto" + HorizontalScrollBarVisibility="Auto">
24-28: Remove hardcoded text selection.The hardcoded
SelectionStartandSelectionEndvalues might confuse users viewing the gallery. Consider removing these properties to show the natural state of a SelectableTextBlock.<SelectableTextBlock TextWrapping="Wrap" - SelectionStart="17" - SelectionEnd="26"> + >src/Consolonia.Gallery/Gallery/GalleryViews/GalleryProgressBar.axaml (4)
7-8: Great improvement with ScrollViewer!The addition of ScrollViewer with auto visibility improves content accessibility when it exceeds the viewport. This change aligns well with the gallery's usability improvements.
19-39: Consider adding easing function for visual consistencyThe first progress bar uses a linear animation while the second one uses ExponentialEaseInOut. Consider:
- Adding the same easing function for visual consistency
- Reducing the animation duration for a more engaging demo
<Animation Duration="0:0:15" IterationCount="INFINITE" PlaybackDirection="Normal"> + <Animation.Easing> + <ExponentialEaseInOut /> + </Animation.Easing> <KeyFrame Cue="0%">
41-67: Consider using relative height for better scalabilityThe vertical progress bar uses a fixed height of 7 units. Consider using relative sizing or binding for better adaptability across different display scales.
-Height="7" +Height="{Binding RelativeSource={RelativeSource FindAncestor, AncestorType=UserControl}, Path=FontSize}"
70-78: Add comments to explain different progress bar configurationsConsider adding comments to explain the purpose of each progress bar configuration. This would improve the educational value of the gallery examples.
<StackPanel Spacing="1"> + <!-- Demonstrates an indeterminate progress state --> <ProgressBar IsIndeterminate="True" /> + <!-- Shows a progress bar with custom maximum value --> <ProgressBar Value="5" Maximum="10" /> + <!-- Illustrates default range (0-100) at 50% --> <ProgressBar Value="50" /> + <!-- Demonstrates custom range from 25 to 75 --> <ProgressBar Value="50" Minimum="25" Maximum="75" /> </StackPanel>src/Consolonia.Gallery/Gallery/GalleryViews/GalleryRadioButton.axaml (1)
9-16: Consider adjusting the StackPanel spacingThe spacing value of "1" seems unusually small and might not provide enough visual separation between radio buttons. Consider increasing it for better readability and touch targets.
- Spacing="1"> + Spacing="4">src/Consolonia.Gallery/Gallery/GalleryViews/GalleryCalendarPicker.axaml (1)
20-57: Consider simplifying the nested layout structure.The current implementation uses multiple nested StackPanels which could impact performance and maintainability. Consider using a Grid or more efficient layout containers to achieve the same visual result with less nesting.
Example refactor using Grid:
- <StackPanel Orientation="Horizontal" - Margin="0,1,0,0" - HorizontalAlignment="Center" - Spacing="1"> - <StackPanel Orientation="Vertical"> + <Grid Margin="0,1,0,0" + HorizontalAlignment="Center" + RowDefinitions="Auto,Auto,Auto,Auto,Auto,Auto,Auto,Auto" + RowSpacing="1">src/Consolonia.PlatformSupport/PlatformSupportExtensions.cs (3)
40-42: Maintain naming consistency across auto-detection methodsThe method name
UseAutoDetectClipboardis inconsistent withUseAutoDetectedConsole. Consider renaming for consistency.Apply this diff:
- .UseAutoDetectClipboard() + .UseAutoDetectedClipboard()
58-59: Fix typo in documentationThere's a typo in the documentation: "vai" should be "via".
Apply this diff:
- /// the Mac clipboard APIs vai P/Invoke. + /// the Mac clipboard APIs via P/Invoke.
93-94: Improve command execution reliabilityThe bash command could be made more maintainable and the error handling could be improved.
Apply this diff:
+ private const string WSL_DETECTION_COMMAND = "uname -a"; + // xclip does not work on WSL, so we need to use the Windows clipboard vis Powershell - (int exitCode, string result) = ClipboardProcessRunner.Bash("uname -a", waitForOutput: true); + try { + (int exitCode, string result) = ClipboardProcessRunner.Bash(WSL_DETECTION_COMMAND, waitForOutput: true); + if (exitCode != 0) return false; + + return result.Contains("microsoft", StringComparison.OrdinalIgnoreCase) && + result.Contains("WSL", StringComparison.OrdinalIgnoreCase); + } + catch (Exception) { + return false; + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
src/Consolonia.Gallery/Gallery/GalleryViews/GalleryAnimatedLines.axaml(1 hunks)src/Consolonia.Gallery/Gallery/GalleryViews/GalleryBorders.axaml(1 hunks)src/Consolonia.Gallery/Gallery/GalleryViews/GalleryButton.axaml(1 hunks)src/Consolonia.Gallery/Gallery/GalleryViews/GalleryCalendar.axaml(1 hunks)src/Consolonia.Gallery/Gallery/GalleryViews/GalleryCalendarPicker.axaml(1 hunks)src/Consolonia.Gallery/Gallery/GalleryViews/GalleryProgressBar.axaml(1 hunks)src/Consolonia.Gallery/Gallery/GalleryViews/GalleryRadioButton.axaml(1 hunks)src/Consolonia.Gallery/Gallery/GalleryViews/GalleryTextBlock.axaml(1 hunks)src/Consolonia.Gallery/Gallery/GalleryViews/GalleryTextBox.axaml(1 hunks)src/Consolonia.PlatformSupport/PlatformSupportExtensions.cs(3 hunks)src/Tests/Consolonia.Gallery.Tests/Base/GalleryTestsBaseBase.cs(1 hunks)src/Tests/Consolonia.Gallery.Tests/TextBlockTests.cs(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- src/Consolonia.Gallery/Gallery/GalleryViews/GalleryBorders.axaml
- src/Consolonia.Gallery/Gallery/GalleryViews/GalleryButton.axaml
- src/Consolonia.Gallery/Gallery/GalleryViews/GalleryCalendar.axaml
🚧 Files skipped from review as they are similar to previous changes (2)
- src/Tests/Consolonia.Gallery.Tests/TextBlockTests.cs
- src/Consolonia.Gallery/Gallery/GalleryViews/GalleryTextBox.axaml
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (8)
src/Consolonia.Gallery/Gallery/GalleryViews/GalleryTextBlock.axaml (1)
51-57: Well-structured text examples!The organization of examples, particularly the international characters and text styles, provides a comprehensive demonstration of text rendering capabilities. The consistent spacing and grouping make it easy to understand different text features.
Also applies to: 78-105
src/Consolonia.Gallery/Gallery/GalleryViews/GalleryProgressBar.axaml (1)
9-15: Well-structured controls with clear labelsThe CheckBox controls are properly implemented with appropriate margins and descriptive labels.
src/Tests/Consolonia.Gallery.Tests/Base/GalleryTestsBaseBase.cs (1)
15-15: Please clarify the reason for changing the default width to 81 pixels.The default pixel buffer width has been changed from 80 to 81 pixels. This seems unrelated to the PR's main objective of fixing paste performance. Could you explain:
- Why this specific width (81) was chosen?
- Is this change intentional or accidentally included in this PR?
Let's verify if this change affects any existing tests:
✅ Verification successful
The change to 81 pixels appears intentional and safe.
The width change from 80 to 81 pixels is isolated to gallery tests and doesn't impact other parts of the codebase. Other components either use dynamic sizing or different specific dimensions for their tests. While this change seems unrelated to the PR's main objective of fixing paste performance, it might be addressing a subtle rendering or alignment issue specific to the gallery tests.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for tests that might be sensitive to buffer width rg -l "PixelBufferSize\(80," src/Tests # Check for any hardcoded width assumptions in test assertions rg -l "Assert.*80.*40" src/TestsLength of output: 75
Script:
#!/bin/bash # Find all PixelBufferSize instantiations ast-grep --pattern 'new PixelBufferSize($_,$_)' # Search for any width-related assertions or checks rg -i "width|size" src/Tests -A 2 -B 2 # Find the base class to understand the context better fd GalleryTestsBaseBase.cs -t f -x cat {}Length of output: 24646
src/Consolonia.Gallery/Gallery/GalleryViews/GalleryRadioButton.axaml (2)
7-7: LGTM! Good addition of ScrollViewerThe addition of ScrollViewer with auto visibility improves the component's usability by handling overflow content gracefully.
Also applies to: 66-66
17-36: LGTM! Well-structured three-state radio buttonsThe implementation correctly uses IsThreeState and {x:Null} binding for the indeterminate state, with clear labeling.
src/Consolonia.Gallery/Gallery/GalleryViews/GalleryCalendarPicker.axaml (2)
15-16: LGTM! Good improvement to content accessibility.The addition of ScrollViewer with auto visibility is a good enhancement that ensures content remains accessible when it overflows the viewport.
53-54: Verify validation support status.There's a discrepancy between the comment claiming validation is "Not supported" and the implementation using two-way binding to ValidatedDateExample. Please clarify if validation is supported and update either the comment or the implementation accordingly.
Let's check the code-behind and view model for validation implementation:
src/Consolonia.PlatformSupport/PlatformSupportExtensions.cs (1)
64-73: Verify Windows clipboard performance improvementThe implementation uses Avalonia's native Windows clipboard, which should address the performance issue. However, we should verify that this resolves the specific paste performance problem mentioned in the PR description.
Run the following script to check for any remaining KEY_EVENT-related code that might affect paste performance:
✅ Verification successful
Clipboard performance improvement confirmed
The implementation properly addresses the performance issue by:
- Replacing the keystroke simulation (CTRL+V) with native clipboard access
- Removing the overhead of processing multiple key events
- Using Avalonia's efficient Windows clipboard implementation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for KEY_EVENT handling in Windows-specific code rg -i "key_event|ctrl[_+]v|^v|paste" --type csLength of output: 4318
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
src/Consolonia.Core/Infrastructure/ConsoloniaPlatform.cs (2)
61-67: Consider documenting the clipboard initialization changes.The commented-out code provides historical context about different clipboard implementations, but it would be more helpful to add a comment explaining that clipboard initialization has been moved to the auto-detection system.
Add a comment like:
- //.Bind<IClipboard>().ToConstant(null) + // Clipboard initialization moved to UseAutoDetectClipboard in PlatformSupportExtensions + // for platform-specific (Windows/macOS/Linux) implementations
Line range hint
39-67: Consider addressing technical debt in platform initialization.There are several TODOs and commented-out code blocks that should be addressed:
- Navigation handler implementation
- Platform settings implementation
- Render timer and render loop configuration
These items might affect the overall platform stability and feature completeness.
Would you like me to help create GitHub issues to track these technical debt items? I can help prioritize them based on their potential impact on the platform's functionality.
src/Consolonia.PlatformSupport/Clipboard/ClipboardProcessRunner.cs (3)
6-9: Enhance XML documentation with platform-specific details.The current documentation could be more comprehensive. Consider adding:
- Platform-specific behavior and requirements
- Examples of usage
- Known limitations or considerations
/// <summary> - /// Helper class for console drivers to invoke shell commands to interact with the clipboard. + /// Helper class for console drivers to invoke shell commands to interact with the clipboard. + /// Provides platform-specific clipboard operations for Windows, Linux, macOS, and WSL environments. + /// Usage examples: + /// - Windows: Uses PowerShell commands + /// - Linux: Uses xclip/xsel + /// - macOS: Uses pbcopy/pbpaste + /// - WSL: Bridges to Windows clipboard through PowerShell /// </summary>
23-33: Document the double wait pattern and make timeout configurable.The double wait pattern is a known workaround for process handling, but its purpose and the magic timeout value need better documentation and configurability.
Consider these improvements:
+ /// <summary> + /// Implements a double wait pattern to ensure proper process exit. + /// This is necessary because some processes may signal they've exited + /// before their handles are fully released. + /// </summary> + /// <param name="process">The process to wait for</param> + /// <param name="timeoutMs">Timeout in milliseconds for the first wait</param> + /// <returns>True if the process exited within the timeout period</returns> - public static bool DoubleWaitForExit(this Process process) + public static bool DoubleWaitForExit(this Process process, int timeoutMs = 500) { - bool result = process.WaitForExit(500); + bool result = process.WaitForExit(timeoutMs); if (result) { process.WaitForExit(); } return result; }
9-10: Consider improving testability through abstraction.The direct creation of
Processinstances makes the class difficult to test. Consider introducing an abstraction layer for process creation and execution.This could be achieved by:
- Creating an interface for process operations
- Using dependency injection for the process factory
- Adding a test implementation for process simulation
Example interface:
public interface IProcessRunner { Task<(int exitCode, string result)> RunProcessAsync( string cmd, string arguments, string input = null, bool waitForOutput = true, int timeoutMs = 5000, CancellationToken cancellationToken = default); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/Consolonia.Core/Infrastructure/ConsoloniaPlatform.cs(1 hunks)src/Consolonia.PlatformSupport/Clipboard/ClipboardProcessRunner.cs(1 hunks)src/Consolonia.PlatformSupport/Clipboard/XClipClipboard.cs(1 hunks)src/Experimental/Consolonia.Editor/MainWindow.axaml.cs(0 hunks)src/Experimental/Consolonia.Editor/Resources/ResourceLoader.cs(0 hunks)
💤 Files with no reviewable changes (2)
- src/Experimental/Consolonia.Editor/MainWindow.axaml.cs
- src/Experimental/Consolonia.Editor/Resources/ResourceLoader.cs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Consolonia.PlatformSupport/Clipboard/XClipClipboard.cs
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (1)
src/Consolonia.Core/Infrastructure/ConsoloniaPlatform.cs (1)
61-61: Verify clipboard initialization after removing bindings.The removal of clipboard bindings from this file could lead to runtime issues if the new auto-detection system (
UseAutoDetectClipboard) is not properly initialized elsewhere in the codebase.Let's verify the clipboard initialization:
✅ Verification successful
Clipboard initialization is properly handled through auto-detection
The removal of explicit clipboard bindings is safe as the new auto-detection system in
PlatformSupportExtensions.UseAutoDetectClipboard()properly handles clipboard initialization for all supported platforms with appropriate fallback mechanisms.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if UseAutoDetectClipboard is called in the application startup # Search for UseAutoDetectClipboard usage echo "Searching for UseAutoDetectClipboard usage..." rg "UseAutoDetectClipboard" # Search for IClipboard bindings to ensure we haven't missed any echo "Searching for other IClipboard bindings..." rg "Bind<IClipboard>"Length of output: 732
Script:
#!/bin/bash # Check the implementation of UseAutoDetectClipboard and platform-specific clipboard implementations echo "Checking UseAutoDetectClipboard implementation..." ast-grep --pattern 'public static AppBuilder UseAutoDetectClipboard(this AppBuilder builder) { $$$ }' echo "\nSearching for clipboard implementations..." rg "class.*Clipboard.*:.*IClipboard" -A 2Length of output: 1746
Script:
#!/bin/bash # Check the registration logic in PlatformSupportExtensions echo "Checking clipboard registration logic..." rg -A 10 "UseAutoDetectClipboard.*{" src/Consolonia.PlatformSupport/PlatformSupportExtensions.csLength of output: 303
Script:
#!/bin/bash # Check the registration logic with fixed pattern echo "Checking clipboard registration implementation..." rg "UseAutoDetectClipboard" src/Consolonia.PlatformSupport/PlatformSupportExtensions.cs -B 2 -A 15Length of output: 2067
src/Consolonia.PlatformSupport/Clipboard/ClipboardProcessRunner.cs
Outdated
Show resolved
Hide resolved
src/Consolonia.PlatformSupport/Clipboard/ClipboardProcessRunner.cs
Outdated
Show resolved
Hide resolved
src/Consolonia.PlatformSupport/Clipboard/ClipboardProcessRunner.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
src/Consolonia.PlatformSupport/Clipboard/MacClipboard.cs (1)
47-59: Improve support check logic inCheckSupportThe
CheckSupportmethod currently useswhichto determine the existence ofpbcopyandpbpaste, but theresult.FileExists()check may not accurately confirm their availability.Consider relying on the exit code of the
whichcommand:private static bool CheckSupport() { - (int exitCode, string result) = ClipboardProcessRunner.Bash("which pbcopy", waitForOutput: true); - - if (exitCode != 0 || !result.FileExists()) + (int exitCode, _) = ClipboardProcessRunner.Bash("which pbcopy", waitForOutput: true); + + if (exitCode != 0) { return false; } - (exitCode, result) = ClipboardProcessRunner.Bash("which pbpaste", waitForOutput: true); - - return exitCode == 0 && result.FileExists(); + (exitCode, _) = ClipboardProcessRunner.Bash("which pbpaste", waitForOutput: true); + + return exitCode == 0; }src/Consolonia.PlatformSupport/Clipboard/WslClipboard.cs (2)
62-70: Handle errors inGetTextAsyncwhen PowerShell command failsIn the
GetTextAsyncmethod, if the PowerShell command fails, the method returns an empty string without indicating the error.Consider throwing an exception or logging the error to aid in debugging:
if (exitCode == 0) { return Task.FromResult(output); } +else +{ + throw new InvalidOperationException($"Failed to get clipboard text: {output} using powershell"); +}
16-38: Ensure thread-safe initialization of_powershellPathThe
_powershellPathstatic field is set without synchronization, which may lead to race conditions in a multithreaded environment.Consider using a thread-safe initialization pattern, such as
Lazy<T>:-private static string _powershellPath = string.Empty; +private static readonly Lazy<string> _powershellPath = new Lazy<string>(InitializePowerShellPath); private bool _isSupported; public WslClipboard() { - if (string.IsNullOrEmpty(_powershellPath)) + var powershellPath = _powershellPath.Value; + if (!string.IsNullOrEmpty(powershellPath)) { - // Initialization code - // ... - _powershellPath = result; + _isSupported = true; } } + +private static string InitializePowerShellPath() +{ + // Initialization code + // ... + return result; +}src/Consolonia.PlatformSupport/PlatformSupportExtensions.cs (1)
113-119: Expand terminal support inUseAutoDetectConsoleColorModeThe current implementation checks for specific terminal types to determine the console color mode. There may be other terminals that support RGB colors.
Consider using capabilities reported by
terminfoor environment variables to detect color support more accurately.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/Consolonia.PlatformSupport/Clipboard/ClipboardProcessRunner.cs(1 hunks)src/Consolonia.PlatformSupport/Clipboard/MacClipboard.cs(1 hunks)src/Consolonia.PlatformSupport/Clipboard/WslClipboard.cs(1 hunks)src/Consolonia.PlatformSupport/Clipboard/X11Clipboard.cs(1 hunks)src/Consolonia.PlatformSupport/PlatformSupportExtensions.cs(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/Consolonia.PlatformSupport/Clipboard/ClipboardProcessRunner.cs
- src/Consolonia.PlatformSupport/Clipboard/X11Clipboard.cs
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (2)
src/Consolonia.PlatformSupport/Clipboard/MacClipboard.cs (1)
61-77: VerifyDllImportpaths for compatibility across macOS versionsThe
DllImportattributes reference/System/Library/Frameworks/AppKit.framework/AppKit, which may not be consistent across different macOS versions.Please ensure that the specified paths are correct and consider using the standard Objective-C runtime library
libobjc.A.dyliborlibobjc.dylibif appropriate.src/Consolonia.PlatformSupport/PlatformSupportExtensions.cs (1)
66-71: Add exception handling for assembly loading inUseAutoDetectClipboardLoading assemblies at runtime can result in exceptions such as
FileNotFoundExceptionorTypeLoadException. Currently, the code throwsArgumentNullExceptionifassemblyortypeis null, but it would be better to catch these exceptions and provide a fallback.Consider modifying the code to handle exceptions gracefully:
try { Assembly assembly = Assembly.Load("Avalonia.Win32"); Type type = assembly.GetType(assembly.GetName().Name + ".ClipboardImpl"); var clipboard = Activator.CreateInstance(type) as IClipboard; return builder.With(clipboard ?? new NaiveClipboard()); } catch (Exception ex) when (ex is FileNotFoundException || ex is TypeLoadException) { // Fallback to NaiveClipboard if assembly loading fails return builder.With<IClipboard>(new NaiveClipboard()); }
Co-authored-by: <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
src/Consolonia.Gallery/Gallery/GalleryViews/GalleryRadioButton.axaml (1)
37-64: 🛠️ Refactor suggestionReorganize grouped radio buttons for better maintainability
The current implementation still has the same issues noted in the previous review:
- Options from groups A and B are interleaved across different StackPanels
- Group B options are numbered non-sequentially (1, 3, 2, 4)
This structure remains confusing and harder to maintain.
Consider applying the restructuring suggested in the previous review to keep related options together and maintain a logical sequence.
src/Consolonia.PlatformSupport/Clipboard/MacClipboard.cs (2)
47-53:⚠️ Potential issueAdd null checks in GetTextAsync to prevent exceptions.
The method needs null checks to handle empty clipboard scenarios safely.
55-71:⚠️ Potential issueEnsure proper release of unmanaged resources in SetTextAsync.
The current implementation may leak memory if an exception occurs between allocation and release.
🧹 Nitpick comments (3)
src/Consolonia.Gallery/Gallery/GalleryViews/GalleryRadioButton.axaml (1)
10-16: Consider adding comments to document access keysThe implementation looks good with proper coverage of states and access keys. Consider adding a comment to document the keyboard shortcuts created by the underscore prefixes (e.g., Alt+O for "Option").
src/Consolonia.PlatformSupport/Clipboard/MacClipboard.cs (2)
93-102: Improve error handling in CheckSupport method.The method should provide more descriptive error information when clipboard support is not available.
Apply this diff to enhance error reporting:
private static bool CheckSupport() { - (int exitCode, string result) = ClipboardProcessRunner.Bash("which pbcopy", waitForOutput: true); + try + { + (int exitCode, string result) = ClipboardProcessRunner.Bash("which pbcopy", waitForOutput: true); + if (exitCode != 0 || !result.FileExists()) + return false; - if (exitCode != 0 || !result.FileExists()) return false; + (exitCode, result) = ClipboardProcessRunner.Bash("which pbpaste", waitForOutput: true); + return exitCode == 0 && result.FileExists(); + } + catch (Exception ex) + { + System.Diagnostics.Debug.WriteLine($"Failed to check clipboard support: {ex.Message}"); + return false; + } - (exitCode, result) = ClipboardProcessRunner.Bash("which pbpaste", waitForOutput: true); - return exitCode == 0 && result.FileExists(); }
104-121: Consider adding XML documentation for P/Invoke methods.Adding documentation for the P/Invoke methods would improve maintainability and help other developers understand the native API usage.
Example for one method:
+/// <summary> +/// Gets a reference to an Objective-C class. +/// </summary> +/// <param name="className">The name of the class to get.</param> +/// <returns>A pointer to the class object.</returns> [DllImport("/System/Library/Frameworks/AppKit.framework/AppKit", CharSet = CharSet.Unicode)] private static extern nint objc_getClass(string className);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
src/Consolonia.Core/Infrastructure/ConsoloniaPlatform.cs(1 hunks)src/Consolonia.Core/Infrastructure/NaiveClipboard.cs(1 hunks)src/Consolonia.Gallery/Gallery/GalleryViews/GalleryAnimatedLines.axaml(1 hunks)src/Consolonia.Gallery/Gallery/GalleryViews/GalleryBorders.axaml(1 hunks)src/Consolonia.Gallery/Gallery/GalleryViews/GalleryButton.axaml(1 hunks)src/Consolonia.Gallery/Gallery/GalleryViews/GalleryCalendar.axaml(1 hunks)src/Consolonia.Gallery/Gallery/GalleryViews/GalleryCalendarPicker.axaml(1 hunks)src/Consolonia.Gallery/Gallery/GalleryViews/GalleryProgressBar.axaml(1 hunks)src/Consolonia.Gallery/Gallery/GalleryViews/GalleryRadioButton.axaml(1 hunks)src/Consolonia.Gallery/Gallery/GalleryViews/GalleryTextBlock.axaml(1 hunks)src/Consolonia.Gallery/Gallery/GalleryViews/GalleryTextBox.axaml(1 hunks)src/Consolonia.PlatformSupport/Clipboard/ClipboardProcessRunner.cs(1 hunks)src/Consolonia.PlatformSupport/Clipboard/MacClipboard.cs(1 hunks)src/Consolonia.PlatformSupport/Clipboard/WslClipboard.cs(1 hunks)src/Consolonia.PlatformSupport/Clipboard/X11Clipboard.cs(1 hunks)src/Consolonia.PlatformSupport/Clipboard/XClipClipboard.cs(1 hunks)src/Consolonia.PlatformSupport/PlatformSupportExtensions.cs(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (14)
- src/Consolonia.Gallery/Gallery/GalleryViews/GalleryProgressBar.axaml
- src/Consolonia.Gallery/Gallery/GalleryViews/GalleryAnimatedLines.axaml
- src/Consolonia.Gallery/Gallery/GalleryViews/GalleryCalendarPicker.axaml
- src/Consolonia.Gallery/Gallery/GalleryViews/GalleryBorders.axaml
- src/Consolonia.Gallery/Gallery/GalleryViews/GalleryCalendar.axaml
- src/Consolonia.Gallery/Gallery/GalleryViews/GalleryTextBlock.axaml
- src/Consolonia.Gallery/Gallery/GalleryViews/GalleryTextBox.axaml
- src/Consolonia.PlatformSupport/PlatformSupportExtensions.cs
- src/Consolonia.PlatformSupport/Clipboard/XClipClipboard.cs
- src/Consolonia.PlatformSupport/Clipboard/ClipboardProcessRunner.cs
- src/Consolonia.PlatformSupport/Clipboard/WslClipboard.cs
- src/Consolonia.Core/Infrastructure/NaiveClipboard.cs
- src/Consolonia.PlatformSupport/Clipboard/X11Clipboard.cs
- src/Consolonia.Gallery/Gallery/GalleryViews/GalleryButton.axaml
🔇 Additional comments (3)
src/Consolonia.Gallery/Gallery/GalleryViews/GalleryRadioButton.axaml (2)
7-7: LGTM: ScrollViewer implementation enhances usabilityThe addition of ScrollViewer with auto visibility is a good practice for handling overflow content, consistent with other gallery views.
Also applies to: 66-66
17-36: LGTM: Comprehensive three-state implementationThe three-state radio buttons implementation covers all possible states (checked, unchecked, indeterminate) with proper disabled state handling.
src/Consolonia.Core/Infrastructure/ConsoloniaPlatform.cs (1)
61-61: LGTM! Removing direct clipboard binding to support platform-specific implementations.The removal of the direct
IClipboardbinding aligns with the PR's objective to improve clipboard handling, allowing for platform-specific implementations to be used instead.
tomlm
changed the title
I was able to determine that the only time we get multiple KEY_EVENTs from win32 console API is when the terminal is simulating keystrokes by mapping CTRL+V => clipboard contents.
This delta changes it to emit CTRL+V sequence instead.
Fixes #239