Upgrade Avalonia to 11.2.1 #159
Conversation
They alll throw an exception having to do with layering? Given there are new layering APIs added, it seems related.
Co-authored-by: <[email protected]>
Added support for windows clipboard. Everything works except for ESCAPE on flyout unit test
📝 WalkthroughWalkthroughThe pull request introduces several modifications across multiple files in the Consolonia project. The primary change is the update of the Possibly related PRs
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: 11
🧹 Outside diff range and nitpick comments (11)
src/Consolonia.Gallery/Gallery/GalleryViews/GalleryGradientBrush.axaml.cs (1)
34-35: Correct migration to RadiusX/RadiusY properties.The change from
Radiusto separateRadiusX/RadiusYproperties aligns with Avalonia 11.2.1's breaking changes. The values are correctly set using relative percentages that match the center point.Consider adding a comment explaining the change from single
Radiusto separateRadiusX/RadiusYfor future maintainers:this.MyGrid.Background = new RadialGradientBrush { Center = new RelativePoint(0.5, 0.5, RelativeUnit.Relative), GradientOrigin = new RelativePoint(0.5, 0.5, RelativeUnit.Relative), + // Avalonia 11.2.1: Using separate X/Y radii instead of single Radius property RadiusX = RelativeScalar.Parse("50%"), RadiusY = RelativeScalar.Parse("50%"),src/Consolonia.Core/Drawing/ConsoleBrush.cs (1)
130-138: Improved accuracy for radial gradient calculations.The changes correctly handle elliptical gradients by:
- Properly scaling distances based on X and Y radii
- Using the correct mathematical formula for normalization
Consider caching the radius calculations to improve performance when rendering multiple pixels with the same gradient.
case IRadialGradientBrush radialBrush: { + // Cache these calculations as they're constant for the brush + double radiusXScalar = radialBrush.RadiusX.Scalar; + double radiusYScalar = radialBrush.RadiusY.Scalar; + double normalizer = Math.Sqrt(radiusXScalar * radiusXScalar + radiusYScalar * radiusYScalar); + // Calculate the normalized center coordinates double centerX = radialBrush.Center.Point.X * width; double centerY = radialBrush.Center.Point.Y * height; // Calculate the distance from the center double dx = x - centerX; double dy = y - centerY; // Calculate the distance based on separate X and Y radii - double distanceX = dx / (width * radialBrush.RadiusX.Scalar); - double distanceY = dy / (height * radialBrush.RadiusY.Scalar); + double distanceX = dx / (width * radiusXScalar); + double distanceY = dy / (height * radiusYScalar); double distance = Math.Sqrt(distanceX * distanceX + distanceY * distanceY); // Normalize the distance - double normalizedDistance = distance / - Math.Sqrt(radialBrush.RadiusX.Scalar * radialBrush.RadiusX.Scalar + - radialBrush.RadiusY.Scalar * radialBrush.RadiusY.Scalar); + double normalizedDistance = distance / normalizer;src/Consolonia.Core/Infrastructure/ConsoleWindow.cs (5)
82-82: Consider adding XML documentationThe debug logging is a good approach for unimplemented features. Consider adding XML documentation to explicitly state that transparency is not supported in console mode.
+ /// <summary> + /// Transparency is not supported in console mode. Calls to this method are logged for debugging purposes. + /// </summary> public void SetTransparencyLevelHint(IReadOnlyList<WindowTransparencyLevel> transparencyLevels)
87-96: Simplify theme variant loggingThe current implementation has duplicate debug statements. Consider simplifying the switch statement.
- switch (themeVariant) - { - case PlatformThemeVariant.Dark: - Debug.WriteLine($"ConsoleWindow.SetFrameThemeVariant({themeVariant}) called, not implemented"); - break; - case PlatformThemeVariant.Light: - Debug.WriteLine($"ConsoleWindow.SetFrameThemeVariant({themeVariant}) called, not implemented"); - break; - } + Debug.WriteLine($"ConsoleWindow.SetFrameThemeVariant({themeVariant}) called, not implemented");
265-281: Address navigation and IME input TODOsThe TODOs indicate missing implementations for:
- System navigation (mobile-like back navigation)
- IME text input support
These features might be important for proper mobile-like navigation and international text input support.
Would you like me to help create GitHub issues to track these implementation tasks?
Line range hint
1-421: Consider implementing a feature support matrixGiven the number of unimplemented or ignored features, consider:
- Creating a feature support matrix documenting which Avalonia features are supported in console mode
- Adding this information to XML documentation
- Creating a tracking issue for remaining NotImplementedException cases
This will help users understand the limitations and expectations when using the console implementation.
Based on the gathered information, I can now provide the final response:
Flyout dismiss on Escape key is already implemented
The tests in
FlyoutTests.csconfirm that Flyouts should dismiss on Escape key, and the implementation inDialogWindow.csshows the escape key handling is built into the base dialog functionality through theCancelOnEscapeproperty, which is enabled by default. If Flyouts aren't dismissing on Escape, this is likely a regression bug.
DialogWindow.csimplements escape key handling inInputElement_OnKeyDownmethodFlyoutTests.cscontains passing tests verifying this behavior- All dialog-based controls (including Flyouts) have
CancelOnEscape="True"set by default🔗 Analysis chain
Line range hint
374-421: Investigate Flyout dismiss on Escape keyThe PR reports that Flyouts don't dismiss on Escape key. The key handling implementation might need to be updated to properly handle this scenario.
Let's check if other files handle Flyout dismissal:
Let me gather more specific information about Flyout and escape key handling.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for Flyout-related code rg -l "Flyout" # Search for key handling related to Flyouts ast-grep --pattern 'class $_ { $$$ case Key.Escape: $$$ }'Length of output: 554
Script:
#!/bin/bash # Check content of Flyout test file for any escape key handling cat src/Tests/Consolonia.Gallery.Tests/FlyoutTests.cs # Search for escape key handling patterns more broadly rg -i "escape" -A 5 -B 5 # Look for Flyout class implementations ast-grep --pattern 'class $name : Flyout { $$$ }' # Look for key handling in FlyoutPresenter ast-grep --pattern 'class FlyoutPresenter { $$$ protected override void OnKeyDown($_) { $$$ } }'Length of output: 14798
src/Consolonia.Core/Drawing/Rectangle.cs (1)
70-73: Consider prioritizing GetWidenedGeometry implementation based on console rendering needsThe newly added method is required for Avalonia 11.2.1 interface compliance. While throwing NotImplementedException is consistent with other geometry methods in this class, consider:
- Whether wide pen strokes are needed for console-based rectangle rendering
- If this could affect any existing rectangle drawing functionality
If console rendering doesn't require pen widening, consider documenting this limitation and the rationale for not implementing it. Otherwise, a basic implementation might be needed to support wider pen strokes.
src/Consolonia.Core/Drawing/Line.cs (1)
Line range hint
70-73: Consider a strategic approach to Avalonia 11.2.1 geometry implementationsThe addition of
GetWidenedGeometryto both Rectangle and Line classes shows a consistent approach to the Avalonia upgrade. However, a strategic decision is needed:
- If console rendering requires pen width support:
- Implement these methods with console-specific rendering logic
- Ensure coordination with existing stroke-related methods
- If not needed for console rendering:
- Document the limitation
- Consider implementing a minimal version that maintains current visual behavior
Consider creating a tracking issue to:
- Document which Avalonia 11.2.1 geometry features are needed for console rendering
- Plan implementations for required features
- Document intentional limitations for console-specific rendering
Also applies to: 95-98
src/Consolonia.Core/Drawing/RenderTarget.cs (1)
74-75: Consider adding XML documentation.Since this is a public-facing API change, consider adding XML documentation to explain the
useScaledDrawingparameter and the limitation.+ /// <summary> + /// Creates a drawing context for console-based rendering. + /// </summary> + /// <param name="useScaledDrawing">If true, throws NotImplementedException as scaled drawing is not supported in console context.</param> + /// <returns>A new drawing context implementation.</returns> + /// <exception cref="NotImplementedException">Thrown when useScaledDrawing is true.</exception> public IDrawingContextImpl CreateDrawingContext(bool useScaledDrawing)src/Consolonia.Core/Drawing/ConsoloniaRenderInterface.cs (1)
179-182: Add XML documentation for CreateRegion methodThe implementation is correct given that regions are not supported. However, consider adding XML documentation to explain why this method throws NotImplementedException and its relationship with the SupportsRegions property.
+ /// <summary> + /// Creates a platform-specific render interface region. + /// </summary> + /// <returns>Never returns as regions are not supported in console-based rendering.</returns> + /// <exception cref="NotImplementedException">Always thrown as SupportsRegions is false.</exception> public IPlatformRenderInterfaceRegion CreateRegion() { throw new NotImplementedException(); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (17)
Directory.Build.props(1 hunks)src/Consolonia.Core/Consolonia.Core.csproj(1 hunks)src/Consolonia.Core/Drawing/ConsoleBrush.cs(1 hunks)src/Consolonia.Core/Drawing/ConsoloniaPlatformRenderInterfaceContext.cs(2 hunks)src/Consolonia.Core/Drawing/ConsoloniaRenderInterface.cs(1 hunks)src/Consolonia.Core/Drawing/DrawingContextImpl.cs(4 hunks)src/Consolonia.Core/Drawing/Line.cs(1 hunks)src/Consolonia.Core/Drawing/Rectangle.cs(1 hunks)src/Consolonia.Core/Drawing/RenderTarget.cs(1 hunks)src/Consolonia.Core/Infrastructure/ArrowsAndKeyboardNavigationHandler.cs(1 hunks)src/Consolonia.Core/Infrastructure/ConsoleWindow.cs(6 hunks)src/Consolonia.Core/Infrastructure/ConsoloniaApplication.cs(1 hunks)src/Consolonia.Core/Infrastructure/ConsoloniaPlatform.cs(3 hunks)src/Consolonia.Core/Text/FontManagerImpl.cs(2 hunks)src/Consolonia.Gallery/Gallery/GalleryViews/GalleryDataGrid.axaml.cs(1 hunks)src/Consolonia.Gallery/Gallery/GalleryViews/GalleryGradientBrush.axaml.cs(1 hunks)src/Consolonia.Themes.TurboVision/Templates/Controls/Helpers/ConsoloniaTextPresenter.cs(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/Consolonia.Gallery/Gallery/GalleryViews/GalleryDataGrid.axaml.cs
🔇 Additional comments (11)
src/Consolonia.Core/Infrastructure/ConsoloniaApplication.cs (1)
11-13: Verify keyboard navigation behavior after switching to transient registration
The change from singleton to transient registration of IKeyboardNavigationHandler might be related to the reported Flyout escape key issue. While transient registration is valid, we should verify this is the recommended pattern in Avalonia 11.2.1.
Let's check the Avalonia source code for the recommended registration pattern:
Consider investigating if this change affects other keyboard navigation scenarios beyond the Flyout component.
Directory.Build.props (1)
15-15:
Review version consistency and breaking changes
While updating AvaloniaVersion to 11.2.1, I notice two concerns:
- The
VersionPrefixproperty is still set to 11.0.9, which might cause confusion - We should verify breaking changes between 11.0.9 and 11.2.1
Let's check for potential breaking changes:
Consider updating VersionPrefix to match AvaloniaVersion for consistency:
- <VersionPrefix>11.0.9</VersionPrefix>
+ <VersionPrefix>11.2.1</VersionPrefix>src/Consolonia.Core/Drawing/ConsoloniaPlatformRenderInterfaceContext.cs (1)
31-31: Review PublicFeatures implementation
The PublicFeatures property returns an empty dictionary. We should verify if Avalonia 11.2.1 expects specific features to be available.
Let's check what features might be required:
✅ Verification successful
Let me gather more information about the interface and its implementations to better understand the requirements.
Let me check one more thing to understand how the features are used in the render interface context.
Based on the gathered information, I can now provide the final response:
Empty PublicFeatures implementation is acceptable
The current implementation returning an empty dictionary is acceptable because:
- The codebase shows that
TryGetFeatureis the primary method for feature access, which is properly implemented inConsoleWindow.cswith specific feature handling (IStorageProvider, IInsetsManager) - No direct usage of
PublicFeaturesproperty was found in the codebase - The render interface context is correctly created and used in
ConsoloniaRenderInterface.CreateBackendContext
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for PublicFeatures usage in Avalonia source
rg -A 5 "PublicFeatures.*Add|GetFeature"
# Search for platform-specific feature implementations
ast-grep --pattern 'IPlatformRenderInterfaceContext {
$$$
PublicFeatures
$$$
}'
Length of output: 1733
Script:
#!/bin/bash
# Search for IPlatformRenderInterfaceContext interface definition
ast-grep --pattern 'interface IPlatformRenderInterfaceContext {
$$$
}'
# Search for other implementations of IPlatformRenderInterfaceContext
rg -l "IPlatformRenderInterfaceContext"
# Search for any usage of PublicFeatures property
rg "PublicFeatures" -A 3
Length of output: 591
Script:
#!/bin/bash
# Check the implementation of ConsoloniaRenderInterface
cat src/Consolonia.Core/Drawing/ConsoloniaRenderInterface.cs
# Look for any feature-related interfaces or classes
rg -l "Feature" --type cs
Length of output: 7382
src/Consolonia.Core/Consolonia.Core.csproj (1)
12-12: Package reference changes align with Avalonia 11.2.1 guidelines.
The consolidation of Avalonia.FreeDesktop and Avalonia.Skia into Avalonia.Desktop is the recommended approach for Avalonia 11.2.1.
Let's verify no other files still reference the old packages:
✅ Verification successful
Package consolidation verified - no remaining references to deprecated packages
The verification confirms that there are no remaining references to the deprecated Avalonia.FreeDesktop and Avalonia.Skia packages in any .csproj files. The migration to Avalonia.Desktop has been completed consistently across the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining references to old packages
rg -i "Avalonia\.(FreeDesktop|Skia)" --type=csproj
Length of output: 83
Script:
#!/bin/bash
# Search for any remaining references to old packages using a more general approach
rg -i "Avalonia\.(FreeDesktop|Skia)" -g "*.csproj"
Length of output: 52
src/Consolonia.Themes.TurboVision/Templates/Controls/Helpers/ConsoloniaTextPresenter.cs (1)
74-81: Consider a more robust solution for handling the caret timer.
The current implementation uses a workaround by setting the timer interval to int.MaxValue and throwing an exception in the Tick handler. While this works as a temporary solution, it might be worth:
- Investigating if Avalonia 11.2.1 provides a proper way to disable the timer
- Adding a comment explaining why setting to int.MaxValue was chosen over other values
Let's check if there's a better way to handle this in Avalonia 11.2.1:
✅ Verification successful
Let me gather more context about the caret timer implementation and Avalonia version:
Let me check the project file to see the Avalonia version and look for any related changes:
Let me check if there are any alternative approaches or similar patterns for handling the caret in the codebase:
Based on the gathered information, I can now provide the final response:
The current caret timer implementation is appropriate given the constraints
The current implementation is actually a valid solution because:
- The code explicitly states it needs to disable the blinking caret since "our terminal caret blinks itself once shown"
- The project is using private Avalonia APIs (as evidenced by
AvaloniaAccessUnstablePrivateApissetting), and the timer field is accessed through reflection - Setting maximum interval and throwing in Tick is a reasonable workaround since:
- The timer cannot be disabled directly
- The terminal handles its own caret blinking
- The code properly handles the null case introduced in Avalonia 11.1.5
The current implementation should be kept, but the TODO comment could be enhanced to explain why this approach was chosen.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for caret timer related code in Avalonia
rg -A 5 "caretTimer|CaretTimer"
Length of output: 855
Script:
#!/bin/bash
# Search for references to Avalonia version and caret-related implementations
rg -l "Avalonia" src/Consolonia.Themes.TurboVision/*.csproj
# Look for any documentation or comments about caret timer
rg -A 5 -B 5 "caret.*timer" -i
# Check for any related issues or TODOs
rg -A 2 "TODO.*timer|TODO.*caret" -i
Length of output: 4231
Script:
#!/bin/bash
# Check Avalonia version in the project file
cat src/Consolonia.Themes.TurboVision/Consolonia.Themes.TurboVision.csproj
# Look for any related changes or discussions in recent commits
git log -n 5 --oneline src/Consolonia.Themes.TurboVision/Templates/Controls/Helpers/ConsoloniaTextPresenter.cs
# Search for any similar timer handling patterns
rg -A 3 "DispatcherTimer"
Length of output: 1682
Script:
#!/bin/bash
# Look for other caret-related implementations
rg -A 5 "CaretBrush|ShowCaret|HideCaret"
# Check for any configuration or settings related to caret
rg -A 3 "\.Caret"
# Look for Directory.Core.Build.props to check Avalonia version
cat Directory.Core.Build.props
Length of output: 4549
src/Consolonia.Core/Infrastructure/ConsoloniaPlatform.cs (1)
36-39: Implementation required for IWindowingPlatform interface.
This method is required for Avalonia 11.2.1 compatibility. Consider implementing this method if embedded top-level windows are needed in the future.
src/Consolonia.Core/Infrastructure/ConsoleWindow.cs (2)
9-9: LGTM: Required imports for Avalonia 11.2.1
The new imports are necessary for platform-specific features in the upgraded Avalonia version.
Also applies to: 11-11
230-240: LGTM: Appropriate handling of unsupported window decoration features
The changes correctly handle unsupported window decoration features by gracefully ignoring them instead of throwing exceptions.
src/Consolonia.Core/Drawing/RenderTarget.cs (1)
72-75: Verify the impact of breaking signature change.
The method signature change from CreateDrawingContext() to CreateDrawingContext(bool useScaledDrawing) is a breaking change that could affect existing callers.
Let's verify the impact:
src/Consolonia.Core/Drawing/ConsoloniaRenderInterface.cs (1)
190-190: LGTM: SupportsRegions implementation is correct
The property correctly indicates that regions are not supported in this console-based rendering interface, which is consistent with the limited rendering capabilities and other feature flags like SupportsIndividualRoundRects.
src/Consolonia.Core/Infrastructure/ArrowsAndKeyboardNavigationHandler.cs (1)
18-20: Verify impact on Flyout escape key handling
Given the PR's known issue with Flyout not dismissing on escape key, we should verify if this keyboard handling change contributes to that problem.
✅ Verification successful
Let me gather more information about the Flyout and keyboard handling implementation.
Let me search specifically for the Flyout implementation and its keyboard handling.
Let me try one final search focusing on escape key handling and Flyout interactions.
Based on the gathered information, I can now provide a final response:
No impact found on Flyout escape key handling
The Flyout escape key handling is not affected by the keyboard navigation handler change. The tests in FlyoutTests.cs confirm that Flyout correctly responds to escape key for dismissal, and the ArrowsAndKeyboardNavigationHandler is merely a wrapper that delegates all keyboard handling to the standard KeyboardNavigationHandler. The change in the constructor is an implementation detail that doesn't modify the escape key behavior.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for Flyout-related keyboard handling code
echo "Searching for Flyout keyboard handling..."
rg -l "Flyout.*Key"
# Search for escape key handling patterns
echo "Searching for escape key handling..."
rg "Key\.Escape|KeyDown.*escape"
# Look for similar keyboard handler implementations
echo "Searching for other keyboard handler implementations..."
ast-grep --pattern 'class $_ : IKeyboardNavigationHandler'
Length of output: 1219
Script:
#!/bin/bash
# Search for Flyout class implementation
echo "Searching for Flyout class..."
ast-grep --pattern 'class Flyout'
# Search for KeyboardNavigationHandler implementation
echo "Searching for KeyboardNavigationHandler..."
ast-grep --pattern 'class KeyboardNavigationHandler'
# Search for files containing both Flyout and keyboard handling
echo "Searching for Flyout keyboard interaction..."
rg -A 5 "Flyout.*\b(KeyDown|PreviewKeyDown|OnKeyDown)\b"
Length of output: 39764
Script:
#!/bin/bash
# Search for Flyout class and its keyboard handling
echo "Searching for Flyout implementation..."
rg -A 10 "class.*Flyout.*\{"
# Search for Flyout keyboard event handlers
echo "Searching for Flyout event handlers..."
rg -A 5 "Flyout.*\.(On|Handle).*Key"
# Search for KeyboardNavigationHandler implementation
echo "Searching for KeyboardNavigationHandler implementation..."
rg -A 10 "class KeyboardNavigationHandler"
Length of output: 413
Script:
#!/bin/bash
# Search for Flyout-related files and their content
echo "Searching for Flyout files content..."
fd -e cs -e axaml "Flyout" -x cat {}
# Search for escape key handling in DialogWindow since Flyout might inherit from it
echo "Searching for DialogWindow escape handling..."
rg -A 10 "class DialogWindow"
# Search for any key handling in ArrowsAndKeyboardNavigationHandler
echo "Searching ArrowsAndKeyboardNavigationHandler implementation..."
rg -A 15 "class ArrowsAndKeyboardNavigationHandler"
Length of output: 10955
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
src/Consolonia.Core/Infrastructure/ConsoleWindow.cs (2)
81-95: Consider implementing theme supportWhile debug logging is better than throwing exceptions, theme support could be valuable for console applications. Consider implementing basic theme support:
- Dark theme: Use dark background with light text
- Light theme: Use light background with dark text
public void SetFrameThemeVariant(PlatformThemeVariant themeVariant) { - switch (themeVariant) - { - case PlatformThemeVariant.Dark: - Debug.WriteLine($"ConsoleWindow.SetFrameThemeVariant({themeVariant}) called, not implemented"); - break; - case PlatformThemeVariant.Light: - Debug.WriteLine($"ConsoleWindow.SetFrameThemeVariant({themeVariant}) called, not implemented"); - break; - } + // Basic theme support for console + switch (themeVariant) + { + case PlatformThemeVariant.Dark: + Console.BackgroundColor = ConsoleColor.Black; + Console.ForegroundColor = ConsoleColor.White; + break; + case PlatformThemeVariant.Light: + Console.BackgroundColor = ConsoleColor.White; + Console.ForegroundColor = ConsoleColor.Black; + break; + } + Console.Clear(); // Refresh the console with new colors
148-149: Document console-specific limitationsConsider adding XML documentation to clarify that these window management features are not applicable in console mode.
+ /// <summary> + /// Sets window topmost state. Not supported in console mode. + /// </summary> public void SetTopmost(bool value) { Debug.WriteLine($"ConsoleWindow.SetTopmost({value}) called, not implemented"); } + /// <summary> + /// Controls window resizing. In console mode, resizing is controlled by the terminal. + /// </summary> public void CanResize(bool value) { // todo, enable/dsiable resizing of window }Also applies to: 198-198
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
src/Consolonia.Core/Drawing/RenderTarget.cs(1 hunks)src/Consolonia.Core/Infrastructure/ConsoleWindow.cs(6 hunks)src/Consolonia.Core/Infrastructure/ConsoloniaPlatform.cs(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Consolonia.Core/Drawing/RenderTarget.cs
🔇 Additional comments (4)
src/Consolonia.Core/Infrastructure/ConsoloniaPlatform.cs (1)
68-86:
Add error handling for Windows clipboard implementation
While the clipboard implementation concerns were previously noted, the Windows-specific implementation needs additional error handling for assembly loading and type retrieval operations.
if (OperatingSystem.IsWindows())
{
AvaloniaLocator.CurrentMutable.Bind<IClipboard>()
.ToFunc(() =>
{
- var assembly = Assembly.Load("Avalonia.Win32");
- var type = assembly.GetType(assembly.GetName().Name + ".ClipboardImpl");
- var clipboard = (IClipboard)Activator.CreateInstance(type)!;
- return clipboard;
+ try
+ {
+ var assembly = Assembly.Load("Avalonia.Win32");
+ var clipboardTypeName = $"{assembly.GetName().Name}.ClipboardImpl";
+ var type = assembly.GetType(clipboardTypeName)
+ ?? throw new TypeLoadException($"Could not find type {clipboardTypeName}");
+ var clipboard = (IClipboard)Activator.CreateInstance(type)
+ ?? throw new InvalidOperationException($"Failed to create instance of {clipboardTypeName}");
+ return clipboard;
+ }
+ catch (Exception ex)
+ {
+ Debug.WriteLine($"Failed to initialize Windows clipboard: {ex}");
+ // TODO: Return a fallback clipboard implementation
+ throw;
+ }
});
}src/Consolonia.Core/Infrastructure/ConsoleWindow.cs (3)
9-11: LGTM: Required imports for Avalonia 11.2.1
The new imports are necessary for platform-specific features in the upgraded version.
282-284:
Implement default z-order handling
Since z-order might not be relevant for console windows, consider returning a default order instead of throwing an exception.
public void GetWindowsZOrder(Span<Window> windows, Span<long> zOrder)
{
- throw new NotImplementedException();
+ // In console mode, all windows are considered to be at the same z-order level
+ for (int i = 0; i < zOrder.Length; i++)
+ zOrder[i] = 0;
}Line range hint 376-421: Verify escape key handling for Flyout dismiss
The PR mentions that Flyout doesn't dismiss with escape key. Let's verify the key event handling for escape key.
|
Do you think we should disable flyout test? I can't seem to figure out where Flyout canceling is hooked up to keyboard at all. My only theory is that we are somehow failing to set the control focus to the flyout....Any idea? |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (5)
src/Consolonia.Core/Drawing/DrawingContextImpl.cs (2)
42-42: Add XML documentation for the RenderOptions propertyConsider adding XML documentation to describe the purpose and usage of this property, which was added as part of the Avalonia upgrade.
+ /// <summary> + /// Gets or sets the rendering options that control how content is rendered. + /// </summary> public RenderOptions RenderOptions { get; set; }Also applies to: 47-48
Unimplemented methods found across multiple core components
The verification reveals a significant number of unimplemented methods across the codebase, particularly in core drawing and platform support components. The original review comment's concern is valid and more extensive than initially suggested.
Key areas requiring implementation:
- Drawing functionality: Multiple methods in
DrawingContextImpl.cs,Rectangle.cs, andLine.cs- Platform support: Critical methods in
WindowsConsole.csandCursesConsole.cs- Core infrastructure: Several methods in
ConsoleWindow.csandConsoloniaPlatform.cs- Rendering interface: Multiple methods in
ConsoloniaRenderInterface.csThis indicates a broader architectural concern where significant portions of the Avalonia 11.2.1 interface requirements are not yet implemented, which could lead to runtime failures in production.
🔗 Analysis chain
Line range hint
42-317: Consider implementing all required Avalonia 11.2.1 interfacesThe upgrade to Avalonia 11.2.1 requires implementation of several new interface methods. While the basic structure is in place, several critical methods are currently throwing NotImplementedException. This could lead to runtime errors if these methods are called by the framework.
Consider implementing all required methods to ensure full compatibility with Avalonia 11.2.1. The methods that need implementation are:
- PushClip for platform-specific regions
- DrawRegion for platform-specific regions
- PushLayer/PopLayer for layer management
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for any remaining NotImplementedException in the codebase rg "throw new NotImplementedException" -A 1 -B 1Length of output: 17079
src/Consolonia.Core/Infrastructure/ConsoleWindow.cs (3)
81-81: Enhance debug logging with transparency levelsConsider including the actual transparency level values in the debug message for better diagnostics.
-Debug.WriteLine($"ConsoleWindow.SetTransparencyLevelHint({transparencyLevels}) called, not implemented"); +Debug.WriteLine($"ConsoleWindow.SetTransparencyLevelHint([{string.Join(", ", transparencyLevels)}]) called, not implemented");
86-95: Consider implementing theme supportThe switch statement is well-structured, but consider implementing actual theme support:
- Map theme variants to console colors
- Update console foreground/background colors accordingly
switch (themeVariant) { case PlatformThemeVariant.Dark: - Debug.WriteLine($"ConsoleWindow.SetFrameThemeVariant({themeVariant}) called, not implemented"); + Console.ForegroundColor = ConsoleColor.Gray; + Console.BackgroundColor = ConsoleColor.Black; break; case PlatformThemeVariant.Light: - Debug.WriteLine($"ConsoleWindow.SetFrameThemeVariant({themeVariant}) called, not implemented"); + Console.ForegroundColor = ConsoleColor.Black; + Console.BackgroundColor = ConsoleColor.White; break; }
Line range hint
377-399: Address Flyout escape key handlingThe PR mentions that Flyout doesn't dismiss when escape key is pressed. The key handling logic should be updated to handle this case.
private async void ConsoleOnKeyEvent(Key key, char keyChar, RawInputModifiers rawInputModifiers, bool down, ulong timeStamp) { if (!down) { Dispatcher.UIThread.Post(() => { + // Handle Flyout dismissal with escape key + if (key == Key.Escape) + { + if (_inputRoot is TopLevel topLevel) + { + var flyouts = topLevel.GetVisualDescendants().OfType<Flyout>(); + foreach (var flyout in flyouts) + { + flyout.Hide(); + } + } + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
src/Consolonia.Core/Drawing/DrawingContextImpl.cs(4 hunks)src/Consolonia.Core/Infrastructure/ConsoleWindow.cs(6 hunks)src/Consolonia.Core/Infrastructure/ConsoloniaApplication.cs(1 hunks)src/Consolonia.Gallery/Gallery/GalleryViews/SomeDialogWindow.axaml(1 hunks)src/Consolonia.Gallery/Gallery/GalleryViews/SomeDialogWindow.axaml.cs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Consolonia.Core/Infrastructure/ConsoloniaApplication.cs
🔇 Additional comments (10)
src/Consolonia.Gallery/Gallery/GalleryViews/SomeDialogWindow.axaml (2)
29-29: LGTM! Improved button identifier.
The rename from "Button" to "OneMoreButton" provides better clarity and follows naming best practices.
Line range hint 1-34: Verify dialog keyboard handling with Avalonia 11.2.1
Given the PR's known issue with Flyout and Escape key, we should verify that this dialog's keyboard handling (including Escape key for closing) works correctly with Avalonia 11.2.1.
Let's check for any similar dialog keyboard handling issues:
✅ Verification successful
Let me gather more information about the dialog window implementation and its keyboard handling.
Dialog keyboard handling is properly implemented and verified
The verification shows that:
- The base
DialogWindowclass has proper Escape key handling through theInputElement_OnKeyDownhandler SomeDialogWindowinherits this behavior as it extendsDialogWindow- The functionality is verified by existing tests in
DialogTests.cswhich specifically check Escape key behavior - The implementation is consistent with Avalonia's dialog patterns
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Look for dialog-related keyboard handling patterns and potential issues
# Check for similar dialog window implementations
echo "Looking for similar dialog implementations..."
rg -l "DialogWindow" --type xaml
# Check for keyboard handling in dialog-related code
echo "Checking keyboard handling patterns..."
ast-grep --pattern 'class $_ : DialogWindow {
$$$
protected override void OnKeyDown($_) {
$$$
}
$$$
}'
# Look for escape key handling patterns
echo "Checking escape key handling..."
rg "Key.Escape" --type cs
Length of output: 1270
Script:
#!/bin/bash
# Check the DialogWindow base class implementation
echo "Checking DialogWindow.cs content..."
cat src/Consolonia.Core/Controls/DialogWindow.cs
# Look for dialog window test implementations
echo "Checking dialog tests..."
cat src/Tests/Consolonia.Gallery.Tests/DialogTests.cs
# Check the specific dialog window implementation
echo "Checking SomeDialogWindow implementation..."
rg -A 10 "class SomeDialogWindow"
Length of output: 9243
src/Consolonia.Core/Drawing/DrawingContextImpl.cs (4)
221-224:
The size parameter is not being utilized in CreateLayer
The method ignores the size parameter and creates a RenderTarget with the console window's size. This could lead to incorrect rendering when the layer size differs from the window size.
Consider:
- Modifying the RenderTarget constructor to accept and use the size parameter, or
- Adding size validation to ensure the requested layer size matches the console window size
- public IDrawingContextLayerImpl CreateLayer(PixelSize size)
- {
- return new RenderTarget(_consoleWindow);
- }
+ public IDrawingContextLayerImpl CreateLayer(PixelSize size)
+ {
+ return new RenderTarget(_consoleWindow, size);
+ }Likely invalid or redundant comment.
240-244:
Implement PushClip for platform-specific regions
The method should handle platform-specific region clipping. Consider implementing it similar to how other PushClip overloads are implemented.
- public void PushClip(IPlatformRenderInterfaceRegion region)
- {
- throw new NotImplementedException();
- }
+ public void PushClip(IPlatformRenderInterfaceRegion region)
+ {
+ if (region is IGeometryImpl geometry)
+ {
+ PushGeometryClip(geometry);
+ return;
+ }
+ throw new ArgumentException("Unsupported region type", nameof(region));
+ }Likely invalid or redundant comment.
308-317:
Implement PushLayer and PopLayer methods
These methods are required for layer management and compositing operations.
+ private readonly Stack<(Rect Bounds, IDrawingContextLayerImpl Layer)> _layerStack = new();
+
public void PushLayer(Rect bounds)
{
- throw new NotImplementedException();
+ var layer = CreateLayer(new PixelSize((int)bounds.Width, (int)bounds.Height));
+ _layerStack.Push((bounds, layer));
}
public void PopLayer()
{
- throw new NotImplementedException();
+ if (_layerStack.Count == 0)
+ throw new InvalidOperationException("No layer to pop");
+ var (bounds, layer) = _layerStack.Pop();
+ DrawBitmap(layer, 1.0, bounds, bounds);
}Likely invalid or redundant comment.
303-307:
Implement DrawRegion for platform-specific regions
The method should handle drawing of platform-specific regions with the provided brush and pen.
- public void DrawRegion(IBrush brush, IPen pen, IPlatformRenderInterfaceRegion region)
- {
- throw new NotImplementedException();
- }
+ public void DrawRegion(IBrush brush, IPen pen, IPlatformRenderInterfaceRegion region)
+ {
+ if (region is IGeometryImpl geometry)
+ {
+ DrawGeometry(brush, pen, geometry);
+ return;
+ }
+ throw new ArgumentException("Unsupported region type", nameof(region));
+ }Likely invalid or redundant comment.
src/Consolonia.Core/Infrastructure/ConsoleWindow.cs (4)
9-11: LGTM: Required imports for platform features
The added imports for Avalonia.Controls.Platform and Avalonia.Input.Platform are necessary for implementing platform-specific features.
148-149: LGTM: Appropriate handling for unsupported feature
The change from throwing an exception to debug logging is appropriate for console applications where window z-ordering isn't applicable.
282-286: LGTM: Appropriate z-order implementation for console
The implementation correctly handles z-order for console windows by setting all windows to the same level.
264-280: 🛠️ Refactor suggestion
Consider implementing clipboard support
The clipboard feature is commonly used and would improve usability. Consider implementing basic clipboard support.
src/Consolonia.Gallery/Gallery/GalleryViews/SomeDialogWindow.axaml.cs
Outdated
Show resolved
Hide resolved
My only quick idea is to open old version and put a breakpoint into closing or detaching from the visual tree. Thus stack would be visible. |
|
I figured it out. |
|
@tomlm I have setup CodeRabbit to review draft PRs |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
src/Consolonia.Core/Controls/MessageBox.axaml.cs (2)
49-62: Add defensive programming checks.While the logic is sound, consider adding these safety checks:
- Null checks for button controls
- Fallback handling when no buttons are visible
private void OnShowDialog(object sender, VisualTreeAttachmentEventArgs e) { AttachedToVisualTree -= OnShowDialog; - if (OkButton.IsVisible) + if (OkButton?.IsVisible == true) OkButton.AttachedToVisualTree += ButtonAttached; - else if (YesButton.IsVisible) + else if (YesButton?.IsVisible == true) YesButton.AttachedToVisualTree += ButtonAttached; - else if (CancelButton.IsVisible) + else if (CancelButton?.IsVisible == true) CancelButton.AttachedToVisualTree += ButtonAttached; - else if (NoButton.IsVisible) + else if (NoButton?.IsVisible == true) NoButton.AttachedToVisualTree += ButtonAttached; + else + { + // Log warning or handle case when no buttons are visible + } }
64-68: Add error handling for the button casting.The direct cast to Button could throw an InvalidCastException. Consider using pattern matching for safer type conversion.
private void ButtonAttached(object sender, VisualTreeAttachmentEventArgs e) { - var button = (Button)sender; - button.AttachedToVisualTree -= ButtonAttached; - button.Focus(); + if (sender is Button button) + { + button.AttachedToVisualTree -= ButtonAttached; + button.Focus(); + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
src/Consolonia.Core/Controls/MessageBox.axaml.cs(1 hunks)src/Consolonia.Gallery/Gallery/GalleryViews/SomeDialogWindow.axaml.cs(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Consolonia.Gallery/Gallery/GalleryViews/SomeDialogWindow.axaml.cs
🔇 Additional comments (2)
src/Consolonia.Core/Controls/MessageBox.axaml.cs (2)
46-47: LGTM: Event handler registration is correct.
The event handler registration follows Avalonia's patterns for visual tree attachment events.
46-68: Verify keyboard navigation behavior.
Given the PR's context about Flyout issues with keyboard handling, let's verify that this dialog implementation properly handles keyboard navigation, especially the Escape key.
✅ Verification successful
Let me gather more context about the dialog implementation to verify keyboard handling.
Keyboard navigation is properly implemented
The dialog implementation correctly handles keyboard navigation:
- The base
DialogWindowclass (whichMessageBoxinherits from) has proper Escape key handling through theInputElement_OnKeyDownhandler - Focus management is correctly implemented in the
MessageBoxby automatically focusing the first available button when the dialog is shown - The implementation follows Avalonia's event handling patterns with proper event cleanup
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for keyboard handling implementations in dialog-related files
echo "Searching for keyboard handling in dialog files..."
rg -l "KeyDown|PreviewKeyDown|KeyUp|PreviewKeyUp" --type cs | grep -i "dialog\|window\|flyout"
# Search for Escape key handling
echo "Searching for Escape key handling..."
rg "Key.Escape|Keys.Escape|VirtualKey.Escape" --type cs
# Look for similar dialog implementations
echo "Searching for similar dialog implementations..."
ast-grep --pattern 'class $_ : DialogWindow { $$$ }'
Length of output: 1497
Script:
#!/bin/bash
# Check DialogWindow.cs implementation for keyboard handling
echo "Checking DialogWindow.cs implementation:"
rg -A 10 "Key" src/Consolonia.Core/Controls/DialogWindow.cs
# Check MessageBox keyboard-related event handlers
echo "Checking MessageBox keyboard handlers:"
rg -A 10 "(KeyDown|PreviewKeyDown|KeyUp|PreviewKeyUp)" src/Consolonia.Core/Controls/MessageBox.axaml.cs
# Check if MessageBox inherits from DialogWindow
echo "Checking MessageBox inheritance:"
ast-grep --pattern 'class MessageBox : DialogWindow { $$$ }'
Length of output: 1018
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
src/Consolonia.Core/Infrastructure/ArrowsAndKeyboardNavigationHandler.cs (2)
134-138: Add error handling for popup closingThe popup closing operation should include error handling to gracefully handle any failures.
-if (popup != null) - popup.Close(); +if (popup != null) +{ + try + { + popup.Close(); + } + catch (Exception ex) + { + // Log the error but don't rethrow as this is UI cleanup + Debug.WriteLine($"Error closing popup: {ex.Message}"); + } +}
130-130: Improve code comment clarityThe comment could be more descriptive about the relationship between overlays and popups.
-// if there is a overlay popup, close it +// Close any overlay popup (like Flyouts) when Escape is pressed
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/Consolonia.Core/Infrastructure/ArrowsAndKeyboardNavigationHandler.cs(3 hunks)
🔇 Additional comments (2)
src/Consolonia.Core/Infrastructure/ArrowsAndKeyboardNavigationHandler.cs (2)
19-21: Consider maintaining dependency injection pattern
The constructor change removes dependency injection capability by directly instantiating KeyboardNavigationHandler. This creates tight coupling and makes the class harder to test.
128-141: Verify if this implementation correctly handles all Flyout scenarios
The current implementation might not handle all Flyout dismissal cases, as mentioned in the PR description about Flyouts not dismissing with Escape key. Consider:
- Are there cases where the Flyout is not wrapped in an OverlayPopupHost?
- Should we handle error cases during popup closing?
- Is focus management needed for proper Flyout handling?
✅ Verification successful
Based on the search results, I can now provide a conclusive response about the Flyout handling implementation:
Flyout dismissal with Escape key is working as expected
The implementation correctly handles Flyout scenarios as evidenced by:
- The test file
FlyoutTests.csexplicitly verifies that Flyouts are properly dismissed with the Escape key - Both MenuFlyout and basic Flyout implementations use the standard Avalonia popup infrastructure (OverlayPopupHost)
- No competing Escape key handlers were found that could interfere with Flyout dismissal
The current implementation in ArrowsAndKeyboardNavigationHandler.cs is sufficient because:
- Flyouts are always wrapped in OverlayPopupHost (as shown in the test scenarios)
- The popup closing mechanism is simple and doesn't require additional error handling
- Focus management is handled automatically by the popup infrastructure
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for Flyout usage patterns in the codebase
echo "Searching for Flyout usage patterns..."
rg -A 5 "Flyout"
# Search for other escape key handlers that might interfere
echo "Searching for other escape key handlers..."
rg -A 5 "Key.Escape"
# Search for popup closing patterns
echo "Searching for popup closing patterns..."
ast-grep --pattern 'popup.Close()'
Length of output: 12417
src/Consolonia.Core/Infrastructure/ArrowsAndKeyboardNavigationHandler.cs
Outdated
Show resolved
Hide resolved
I dont like coderabbit proposes such things. I had a look on setup, there is a possibility to inject instructions. I think I will aggregate several issues and will try to set the instructions. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (5)
src/Consolonia.Core/Drawing/DrawingContextImpl.cs (1)
47-48: Consider making RenderOptions property get-only.The property is currently mutable which could lead to thread-safety issues. Since RenderOptions are typically set once during initialization, consider making it get-only.
- public RenderOptions RenderOptions { get; set; } + public RenderOptions RenderOptions { get; }src/Consolonia.Core/Infrastructure/ConsoleWindow.cs (4)
77-77: Add explanatory comment for future maintainers.While the debug logging is helpful, consider adding a comment explaining that transparency is not supported in console windows.
+ // Console windows don't support transparency levels Debug.WriteLine($"ConsoleWindow.SetTransparencyLevelHint({transparencyLevels}) called, not implemented");
82-91: Enhance TODO comment and consider implementing theme support.The TODO comment could be more descriptive about what needs to be implemented. Consider implementing basic theme support by adjusting console colors based on the theme variant.
- //todo: Light or dark + // TODO: Implement theme support by adjusting console colors (foreground/background) based on theme variant switch (themeVariant) { case PlatformThemeVariant.Dark: - Debug.WriteLine($"ConsoleWindow.SetFrameThemeVariant({themeVariant}) called, not implemented"); + Console.ForegroundColor = ConsoleColor.White; + Console.BackgroundColor = ConsoleColor.Black; break; case PlatformThemeVariant.Light: - Debug.WriteLine($"ConsoleWindow.SetFrameThemeVariant({themeVariant}) called, not implemented"); + Console.ForegroundColor = ConsoleColor.Black; + Console.BackgroundColor = ConsoleColor.White; break; }
144-145: Improve documentation for SetTopmost.The TODO comment should explain why this feature isn't implemented in console mode.
- // todo + // TODO: SetTopmost is not applicable for console windows as they don't support z-order in the traditional sense Debug.WriteLine($"ConsoleWindow.SetTopmost({value}) called, not implemented");
220-220: LGTM! Consider adding more descriptive comments.The changes appropriately handle unsupported features, but the comments could be more descriptive about why these features aren't supported in console mode.
- //throw new NotImplementedException(); + // SetMinMaxSize is not implemented as console window size constraints are handled by the terminal - // we don't support this, we can ignore + // ExtendClientArea features are not applicable to console windows as they don't have traditional window decorationsAlso applies to: 225-225, 230-230, 235-235
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
src/Consolonia.Core/Consolonia.Core.csproj(1 hunks)src/Consolonia.Core/Drawing/DrawingContextImpl.cs(4 hunks)src/Consolonia.Core/Drawing/RenderTarget.cs(1 hunks)src/Consolonia.Core/Infrastructure/ConsoleWindow.cs(6 hunks)src/Consolonia.Core/Infrastructure/ConsoloniaApplication.cs(1 hunks)src/Consolonia.Gallery/Gallery/GalleryViews/SomeDialogWindow.axaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- src/Consolonia.Core/Infrastructure/ConsoloniaApplication.cs
- src/Consolonia.Core/Drawing/RenderTarget.cs
- src/Consolonia.Gallery/Gallery/GalleryViews/SomeDialogWindow.axaml
- src/Consolonia.Core/Consolonia.Core.csproj
🔇 Additional comments (8)
src/Consolonia.Core/Drawing/DrawingContextImpl.cs (4)
221-224: The size parameter should be used in CreateLayer
The RenderTarget constructor currently initializes the buffer using the console window's ClientSize, ignoring the requested layer size. This could lead to rendering issues when the layer size differs from the window size.
240-244:
Implement PushClip for IPlatformRenderInterfaceRegion
This method is required for Avalonia 11.2.1 compatibility. Consider implementing it to handle platform-specific region clipping, similar to how other PushClip overloads are implemented.
- public void PushClip(IPlatformRenderInterfaceRegion region)
- {
- throw new NotImplementedException();
- }
+ public void PushClip(IPlatformRenderInterfaceRegion region)
+ {
+ if (region is IGeometryImpl geometry)
+ {
+ PushGeometryClip(geometry);
+ return;
+ }
+ throw new ArgumentException("Unsupported region type", nameof(region));
+ }Likely invalid or redundant comment.
303-307:
Implement DrawRegion method
This method is required for Avalonia 11.2.1 compatibility. It should handle drawing of platform-specific regions with the provided brush and pen.
- public void DrawRegion(IBrush brush, IPen pen, IPlatformRenderInterfaceRegion region)
- {
- throw new NotImplementedException();
- }
+ public void DrawRegion(IBrush brush, IPen pen, IPlatformRenderInterfaceRegion region)
+ {
+ if (region is IGeometryImpl geometry)
+ {
+ DrawGeometry(brush, pen, geometry);
+ return;
+ }
+ throw new ArgumentException("Unsupported region type", nameof(region));
+ }Likely invalid or redundant comment.
308-317:
Implement PushLayer and PopLayer methods
These methods are required for Avalonia 11.2.1 compatibility. They should handle layer management for compositing operations.
+ private readonly Stack<(Rect Bounds, IDrawingContextLayerImpl Layer)> _layerStack = new();
+
public void PushLayer(Rect bounds)
{
- throw new NotImplementedException();
+ var layer = CreateLayer(new PixelSize((int)bounds.Width, (int)bounds.Height));
+ // Store the layer for later use in PopLayer
+ _layerStack.Push((bounds, layer));
}
public void PopLayer()
{
- throw new NotImplementedException();
+ if (_layerStack.Count == 0)
+ throw new InvalidOperationException("No layer to pop");
+ var (bounds, layer) = _layerStack.Pop();
+ // Composite the layer back to the main context
+ DrawBitmap(layer, 1.0, bounds, bounds);
}Likely invalid or redundant comment.
src/Consolonia.Core/Infrastructure/ConsoleWindow.cs (4)
9-9: LGTM!
The new imports are required for the platform-specific implementations added in this PR.
Also applies to: 11-11
278-282: Implementation matches previous suggestion.
The implementation correctly handles z-order for console windows by treating all windows at the same level.
260-276: Consider tracking TODOs and implementing clipboard support.
The implementation properly handles feature requests, but there are several improvements to consider:
- The TODOs for navigation and IME should be tracked as GitHub issues
- The clipboard implementation could be enhanced
Would you like me to:
- Create GitHub issues for tracking the navigation and IME implementation tasks?
- Provide a complete clipboard implementation that works in console mode?
Let's verify the clipboard implementation:
✅ Verification successful
Track TODOs as issues, clipboard implementation is already handled
The clipboard implementation is properly handled:
- Windows: Uses native
Avalonia.Win32.ClipboardImpl - Other platforms: Clipboard service is registered but implementation is commented out (X11Clipboard)
- ConsoleWindow correctly retrieves the clipboard service through AvaloniaLocator
The only actionable items are the TODOs for:
- ISystemNavigationManagerImpl for BACK navigation
- ITextInputMethodImpl for IME input
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if clipboard is implemented elsewhere
rg -l "IClipboard" --type cs
Length of output: 137
Script:
#!/bin/bash
# Check clipboard implementation details
rg "IClipboard" --type cs -C 5
Length of output: 3951
Line range hint 385-401: Investigate Flyout dismiss issue with keyboard handling.
Given the PR objective mentioning issues with Flyout dismiss on escape key, this keyboard event handling code might need investigation. The current implementation might not properly handle the escape key for Flyouts.
Let's check the keyboard handling implementation:
✅ Verification successful
Let me gather more information about the Flyout and keyboard handling implementation.
Based on the results from both script executions, I can now generate the final response:
Keyboard handling for Flyout dismiss is properly implemented.
The codebase shows that:
- Escape key handling for Flyouts is implemented in
ArrowsAndKeyboardNavigationHandler.cswhich properly closes overlay popups - Tests in
FlyoutTests.csverify that Escape key correctly dismisses Flyouts - The keyboard event handling in
ConsoleWindow.cscorrectly propagates key events to the UI framework
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Look for Flyout-related keyboard handling
rg -A 5 "Flyout.*Key" --type cs
# Check for escape key handling
rg -A 5 "Key.Escape" --type cs
# Look for similar implementations in Avalonia
ast-grep --pattern 'class $_ : IWindowImpl {
$$$
private void $_(Key key, $$$) {
$$$
}
}'
Length of output: 5316
Script:
#!/bin/bash
# Look for Flyout implementation and its keyboard handling
ast-grep --pattern 'class $_ {
$$$
OnKeyDown($$$) {
$$$
}
}'
# Check for OverlayPopupHost usage
rg -A 10 "class OverlayPopupHost" --type cs
# Check for Flyout class implementation
rg -A 10 "class Flyout" --type cs
Length of output: 1108
This reverts commit 488d0c3.
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Outside diff range and nitpick comments (24)
src/Consolonia.Gallery/View/ControlsListView.axaml.cs (1)
Line range hint
28-31: Add null check for _commandLineArgsGiven the changes in the constructor, add a null check here to prevent potential NullReferenceException.
private void TrySetupSelected() { + if (_commandLineArgs == null) + { + this.Grid.SelectedIndex = 0; + return; + } if (_commandLineArgs.Length is not 1 and not 2) { this.Grid.SelectedIndex = 0; return; }src/Consolonia.Core/Infrastructure/ConsoloniaApplication.cs (1)
12-13: Investigate potential connection to Flyout escape key issueGiven the reported issue with Flyout not dismissing on escape key, this change in keyboard handler registration might be related. The switch to transient instances could affect how keyboard events are processed.
Consider the following:
- Verify that the
ArrowsAndKeyboardNavigationHandlerproperly handles escape key events in Avalonia 11.2.1- Ensure the Flyout receives keyboard focus when shown
- Add debug logging to track keyboard event propagation
Would you like me to help create a diagnostic logging implementation to track keyboard events?
src/Consolonia.PlatformSupport/PlatformSupportExtensions.cs (2)
Line range hint
13-23: Add error handling and validation for console initializationThe console selection logic should include error handling to gracefully handle initialization failures of platform-specific console implementations.
Consider wrapping the console initialization in a try-catch block:
public static AppBuilder UseAutoDetectedConsole(this AppBuilder builder) { - IConsole console = Environment.OSVersion.Platform switch - { - PlatformID.Win32S or PlatformID.Win32Windows or PlatformID.Win32NT => new Win32Console(), - PlatformID.Unix or PlatformID.MacOSX => new CursesConsole(), - _ => new DefaultNetConsole() - }; + IConsole console; + try + { + console = Environment.OSVersion.Platform switch + { + PlatformID.Win32S or PlatformID.Win32Windows or PlatformID.Win32NT => new Win32Console(), + PlatformID.Unix or PlatformID.MacOSX => new CursesConsole(), + _ => new DefaultNetConsole() + }; + + // Validate console initialization + if (!console.Initialize()) + { + throw new InvalidOperationException("Failed to initialize console"); + } + } + catch (Exception ex) + { + // Fallback to basic console with logging + console = new DefaultNetConsole(); + Console.Error.WriteLine($"Failed to initialize platform-specific console: {ex.Message}"); + }
Line range hint
13-23: Consider platform-specific feature validationDifferent console implementations may support varying feature sets. It would be beneficial to validate and communicate supported features.
Consider introducing a feature detection mechanism:
+ /// <summary> + /// Represents console features that may vary by platform + /// </summary> + public interface IConsoleFeatures + { + bool SupportsColor { get; } + bool SupportsUnicode { get; } + bool SupportsMouseInput { get; } + } public static AppBuilder UseAutoDetectedConsole(this AppBuilder builder) { IConsole console = Environment.OSVersion.Platform switch { PlatformID.Win32S or PlatformID.Win32Windows or PlatformID.Win32NT => new Win32Console(), PlatformID.Unix or PlatformID.MacOSX => new CursesConsole(), _ => new DefaultNetConsole() }; + // Log supported features + if (console is IConsoleFeatures features) + { + builder.LogFeatureSupport("Color", features.SupportsColor); + builder.LogFeatureSupport("Unicode", features.SupportsUnicode); + builder.LogFeatureSupport("MouseInput", features.SupportsMouseInput); + } return builder.UseConsole(console); }src/Tests/Consolonia.Core.Tests/PixelTests.cs (1)
Line range hint
1-1: Note about Flyout issue.While these test changes look good, they don't address the reported Flyout dismissal issue. Consider adding a test case to verify the Flyout's escape key behavior.
Would you like me to help create a test case for the Flyout dismissal functionality?
src/Consolonia.Themes.TurboVision/Templates/Controls/Helpers/SymbolsControl.cs (2)
74-78: Consider font fallback handlingThe direct instantiation of
GlyphTypefacewithout font manager integration might cause issues with:
- Font fallback for unsupported characters
- Custom font loading
- Font feature support
Consider implementing font manager integration to handle these scenarios:
- new TextShaper().ShapeText(value.AsMemory(), new TextShaperOptions(new GlyphTypeface(), 1)); + var fontManager = AvaloniaLocator.Current.GetService<IFontManagerImpl>(); + var typeface = fontManager?.GetOrAddGlyphTypeface(Typeface.Default) ?? new GlyphTypeface(); + new TextShaper().ShapeText(value.AsMemory(), new TextShaperOptions(typeface, 1));
Line range hint
1-115: Review architectural changes post-Avalonia upgradeThe shift towards custom text shaping and rendering logic suggests potential compatibility issues with Avalonia 11.2.1's text rendering pipeline. While the current implementation works, consider:
- Document why custom text shaping was chosen over Avalonia's built-in capabilities
- Evaluate if these changes should be temporary or permanent
- Monitor performance implications of the new approach
Consider creating an abstraction layer that can switch between custom and Avalonia text shaping based on version compatibility:
public interface ITextShapingStrategy { GlyphRun ShapeText(string text, float fontSize); } public class AvaloniaTextShaping : ITextShapingStrategy { ... } public class CustomTextShaping : ITextShapingStrategy { ... }src/Consolonia.Gallery/Program.cs (1)
14-14: Consider improving the args validationThe validation could be more informative and use a more appropriate exception type.
Consider this improvement:
-if (args.Length > 1) throw new NotSupportedException(); +if (args.Length > 1) + throw new ArgumentException("Multiple command line arguments are not supported. Expected 0 or 1 arguments.", nameof(args));src/Consolonia.NUnit/UnitTestConsole.cs (2)
Line range hint
134-146: Consider enhancing key event testing for focus-related scenariosGiven the reported Flyout dismissal issue with escape key, consider adding specific test scenarios for focus handling. The current implementation correctly simulates key events but might benefit from explicit focus state verification.
Consider adding a helper method for testing focus-dependent key handling:
+public async Task KeyInputWithFocus(Key key, bool hasFocus = true) +{ + // Simulate focus change if needed + FocusEvent?.Invoke(hasFocus); + + // Perform key input + await KeyInput(key).ConfigureAwait(true); + + // Verify expected state + await WaitRendered().ConfigureAwait(true); +}This would help isolate whether the Flyout dismissal issue is related to focus handling in the upgraded Avalonia version.
Line range hint
171-171: Consider implementing or documenting the unused MouseEventThe
MouseEventis declared but appears to be unused. Consider either implementing mouse event simulation for completeness or documenting why mouse events are not needed in this test console implementation.src/Consolonia.Core/Drawing/RenderTarget.cs (5)
21-21: Consider renaming_bufferBufferfor clarityThe field name is redundant. Consider using a more descriptive name like
_pixelBufferor_renderBufferto better convey its purpose.
72-76: Enhance the exception message for scaled drawingWhile the message is clear, it could be more helpful by suggesting alternatives or explaining why scaled drawing isn't supported.
-throw new NotImplementedException($"Consolonia doesn't support useScaledDrawing"); +throw new NotImplementedException($"Consolonia doesn't support scaled drawing due to console output limitations. Please use unscaled drawing instead.");
88-97: Add input validation for buffer initializationThe method should validate the size parameters to prevent potential issues with invalid dimensions.
private void InitializeBuffer(Size size) { + if (size.Width <= 0 || size.Height <= 0) + throw new ArgumentException("Buffer dimensions must be positive", nameof(size)); + ushort width = (ushort)size.Width; ushort height = (ushort)size.Height; _bufferBuffer = new PixelBuffer(width, height); InitializeCache(width, height); }
169-169: Consider extracting FlushingBuffer to a separate classThe FlushingBuffer struct is growing in complexity with the addition of text decorations. Consider:
- Moving it to a separate file
- Converting it to a class since it's maintaining state
- Adding XML documentation for the text decoration functionality
Also applies to: 189-189, 198-198, 215-215
202-204: Document the empty string handling behaviorThe code replaces empty symbol text with spaces. This behavior should be documented to explain why it's necessary and when it occurs.
+// Replace empty symbols with spaces to maintain consistent rendering if (pixel.Foreground.Symbol.Text.Length == 0) _stringBuilder.Append(' '); else _stringBuilder.Append(pixel.Foreground.Symbol.Text);src/Consolonia.Gallery/Consolonia.Gallery.csproj (1)
20-22: Consider comprehensive testing strategy for Avalonia upgradeGiven the significant version upgrade and the known Flyout issue:
- Consider adding specific UI automation tests for keyboard navigation
- Document any breaking changes or behavioral differences between 11.0.9 and 11.2.1
- Review the necessity of Avalonia.ReactiveUI dependency if it's not being used extensively
src/Consolonia.Core/Infrastructure/InputLessDefaultNetConsole.cs (3)
72-72: Consider adding bounds checking before setting cursor positionWhile the code handles
ArgumentOutOfRangeException, it would be more efficient to check bounds first.Consider adding bounds validation:
-Console.SetCursorPosition(bufferPoint.X, bufferPoint.Y); +if (bufferPoint.X >= 0 && bufferPoint.X < Console.WindowWidth && + bufferPoint.Y >= 0 && bufferPoint.Y < Console.WindowHeight) +{ + Console.SetCursorPosition(bufferPoint.X, bufferPoint.Y); +} +else +{ + throw new InvalidDrawingContextException($"Position {bufferPoint} is outside window bounds"); +}
Line range hint
93-117: Optimize StringBuilder initialization for performanceThe current implementation creates a StringBuilder without an initial capacity, which might cause multiple reallocations.
Consider pre-sizing the StringBuilder:
-var sb = new StringBuilder(); +// Pre-size for typical usage: decorations (20) + color codes (20) + string length + reset (10) +var sb = new StringBuilder(50 + str.Length);
140-144: Add null check before writing to ConsoleThe Dispose method should handle cases where Console output might be redirected or unavailable.
Consider adding a null check:
-Console.Write(ConsoleUtils.DisableAlternateBuffer); +if (Console.Out != null) +{ + Console.Write(ConsoleUtils.DisableAlternateBuffer); +}src/Consolonia.Core/Drawing/PixelBufferImplementation/PixelBuffer.cs (1)
47-48: Consider caching the Size propertyThe Size property creates a new Rect instance on every access. If this property is frequently accessed, consider caching the value since Width and Height are immutable.
- public Rect Size => new(0, 0, Width, Height); + private readonly Rect _size; + public Rect Size => _size; + + public PixelBuffer(ushort width, ushort height) + { + Width = width; + Height = height; + _buffer = new Pixel[width, height]; + _size = new Rect(0, 0, width, height); + }src/Consolonia.Core/Drawing/PixelBufferImplementation/DrawingBoxSymbol.cs (1)
107-118: Consider adding a fast-path optimizationThe implementation is correct, but consider adding a fast-path optimization for when both symbols are empty:
public ISymbol Blend(ref ISymbol symbolAbove) { + if (IsWhiteSpace() && symbolAbove.IsWhiteSpace()) + return this; + if (symbolAbove.IsWhiteSpace()) return this; if (symbolAbove is not DrawingBoxSymbol drawingBoxSymbol) return symbolAbove;src/Consolonia.Core/Drawing/PixelBufferImplementation/Pixel.cs (1)
39-39: Consider initializingtextDecorationswith an empty collection to avoid null checksBy initializing
textDecorationswith an emptyTextDecorationCollectioninstead ofnull, you can prevent potential null reference exceptions and simplify code that uses this parameter.Suggested change:
- TextDecorationCollection textDecorations = null) : this( + TextDecorationCollection textDecorations = new TextDecorationCollection()) : this(src/Consolonia.Core/Drawing/PixelBufferImplementation/PixelForeground.cs (1)
Line range hint
44-76: LGTM! Methods properly handle TextDecorationsThe changes consistently handle the new
TextDecorationsproperty across all operations. Consider adding test cases to verify:
- Decoration preservation in Shade() and Blend() operations
- Equality comparison with different decoration combinations
- Hash code consistency with decorations
Would you like me to help generate test cases for these scenarios?
src/Consolonia.Core/Helpers/Extensions.cs (1)
97-97: LGTM! Consider enhancing documentationThe change improves the logic by explicitly checking the console's emoji support capability instead of assuming support based on console existence. This is a more robust approach.
Consider adding a code comment explaining the fallback behavior when the console is null or doesn't support complex emojis:
- bool supportsComplexEmoji = console != null ? console.SupportsComplexEmoji : false; + // Fallback to no complex emoji support if console is null or doesn't explicitly support it + bool supportsComplexEmoji = console != null ? console.SupportsComplexEmoji : false;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (2)
Icon.pngis excluded by!**/*.pngsrc/Tools/Avalonia.PreviewHost/Assets/consolonia-logo.icois excluded by!**/*.ico
📒 Files selected for processing (82)
src/Consolonia.Core/Assembly.cs(0 hunks)src/Consolonia.Core/Consolonia.Core.csproj(1 hunks)src/Consolonia.Core/Drawing/DrawingContextImpl.cs(8 hunks)src/Consolonia.Core/Drawing/PixelBufferImplementation/ColorConverter.cs(0 hunks)src/Consolonia.Core/Drawing/PixelBufferImplementation/DrawingBoxSymbol.cs(4 hunks)src/Consolonia.Core/Drawing/PixelBufferImplementation/ISymbol.cs(0 hunks)src/Consolonia.Core/Drawing/PixelBufferImplementation/Pixel.cs(4 hunks)src/Consolonia.Core/Drawing/PixelBufferImplementation/PixelBackground.cs(2 hunks)src/Consolonia.Core/Drawing/PixelBufferImplementation/PixelBuffer.cs(2 hunks)src/Consolonia.Core/Drawing/PixelBufferImplementation/PixelBufferConverter.cs(0 hunks)src/Consolonia.Core/Drawing/PixelBufferImplementation/PixelForeground.cs(3 hunks)src/Consolonia.Core/Drawing/PixelBufferImplementation/SimpleSymbol.cs(3 hunks)src/Consolonia.Core/Drawing/PixelBufferImplementation/SymbolConverter.cs(0 hunks)src/Consolonia.Core/Drawing/RenderTarget.cs(10 hunks)src/Consolonia.Core/Dummy/DummyConsole.cs(0 hunks)src/Consolonia.Core/Helpers/Extensions.cs(2 hunks)src/Consolonia.Core/Infrastructure/ConsoleWindow.cs(9 hunks)src/Consolonia.Core/Infrastructure/ConsoloniaApplication.cs(1 hunks)src/Consolonia.Core/Infrastructure/IConsole.cs(1 hunks)src/Consolonia.Core/Infrastructure/InputLessDefaultNetConsole.cs(8 hunks)src/Consolonia.Core/Text/ConsoleUtils.cs(1 hunks)src/Consolonia.Designer/Assembly.cs(0 hunks)src/Consolonia.Designer/ConsolePreview.cs(0 hunks)src/Consolonia.Designer/Consolonia.Designer.csproj(0 hunks)src/Consolonia.Designer/Extensions.cs(0 hunks)src/Consolonia.Gallery/App.cs(3 hunks)src/Consolonia.Gallery/Consolonia.Gallery.csproj(1 hunks)src/Consolonia.Gallery/Consolonia.Gallery.sln(0 hunks)src/Consolonia.Gallery/Gallery/GalleryViews/GalleryAnimatedLines.axaml(0 hunks)src/Consolonia.Gallery/Gallery/GalleryViews/GalleryButton.axaml(0 hunks)src/Consolonia.Gallery/Gallery/GalleryViews/GalleryCalendar.axaml(1 hunks)src/Consolonia.Gallery/Gallery/GalleryViews/GalleryCalendarPicker.axaml(1 hunks)src/Consolonia.Gallery/Gallery/GalleryViews/GalleryCanvas.axaml(0 hunks)src/Consolonia.Gallery/Gallery/GalleryViews/GalleryCheckBox.axaml(0 hunks)src/Consolonia.Gallery/Gallery/GalleryViews/GalleryColors.axaml(2 hunks)src/Consolonia.Gallery/Gallery/GalleryViews/GalleryComboBox.axaml(0 hunks)src/Consolonia.Gallery/Gallery/GalleryViews/GalleryDataGrid.axaml(0 hunks)src/Consolonia.Gallery/Gallery/GalleryViews/GalleryDialog.axaml(0 hunks)src/Consolonia.Gallery/Gallery/GalleryViews/GalleryFlyout.axaml(0 hunks)src/Consolonia.Gallery/Gallery/GalleryViews/GalleryGradientBrush.axaml(1 hunks)src/Consolonia.Gallery/Gallery/GalleryViews/GalleryImage.axaml(1 hunks)src/Consolonia.Gallery/Gallery/GalleryViews/GalleryListBox.axaml(0 hunks)src/Consolonia.Gallery/Gallery/GalleryViews/GalleryMenu.axaml(0 hunks)src/Consolonia.Gallery/Gallery/GalleryViews/GalleryProgressBar.axaml(0 hunks)src/Consolonia.Gallery/Gallery/GalleryViews/GalleryRadioButton.axaml(0 hunks)src/Consolonia.Gallery/Gallery/GalleryViews/GalleryScrollViewer.axaml(0 hunks)src/Consolonia.Gallery/Gallery/GalleryViews/GalleryTabControl.axaml(1 hunks)src/Consolonia.Gallery/Gallery/GalleryViews/GalleryTextBlock.axaml(2 hunks)src/Consolonia.Gallery/Gallery/GalleryViews/GalleryTextBox.axaml(0 hunks)src/Consolonia.Gallery/Gallery/GalleryViews/GalleryWelcome.axaml(1 hunks)src/Consolonia.Gallery/Gallery/GalleryViews/GalleryWelcome.axaml.cs(0 hunks)src/Consolonia.Gallery/Gallery/GalleryViews/ResourceTest.axaml(0 hunks)src/Consolonia.Gallery/Gallery/GalleryViews/SomeDialogWindow.axaml(2 hunks)src/Consolonia.Gallery/Program.cs(1 hunks)src/Consolonia.Gallery/View/ControlsListView.axaml(1 hunks)src/Consolonia.Gallery/View/ControlsListView.axaml.cs(1 hunks)src/Consolonia.NUnit/UnitTestConsole.cs(2 hunks)src/Consolonia.PlatformSupport/PlatformSupportExtensions.cs(1 hunks)src/Consolonia.Themes.TurboVision/Templates/Controls/Helpers/SymbolsControl.cs(3 hunks)src/Consolonia.Themes.TurboVision/Themes/Fluent/Fluent.axaml(0 hunks)src/Consolonia.Themes.TurboVision/Themes/Fluent/FluentColors.axaml(0 hunks)src/Consolonia.Themes.TurboVision/Themes/Fluent/FluentTheme.cs(0 hunks)src/Consolonia.Themes.TurboVision/Themes/Material/Material.axaml(0 hunks)src/Consolonia.Themes.TurboVision/Themes/Material/MaterialColors.axaml(0 hunks)src/Consolonia.Themes.TurboVision/Themes/Material/MaterialTheme.cs(0 hunks)src/Consolonia.Themes.TurboVision/Themes/TurboVisionBlack/TurboVisionBlack.axaml(0 hunks)src/Consolonia.sln(0 hunks)src/Tests/Consolonia.Core.Tests/Consolonia.Core.Tests.csproj(0 hunks)src/Tests/Consolonia.Core.Tests/DrawingBoxSymbolTests.cs(0 hunks)src/Tests/Consolonia.Core.Tests/DrawingContextImplTests.cs(0 hunks)src/Tests/Consolonia.Core.Tests/PixelBackgroundTests.cs(0 hunks)src/Tests/Consolonia.Core.Tests/PixelBufferTests.cs(0 hunks)src/Tests/Consolonia.Core.Tests/PixelForegroundTests.cs(7 hunks)src/Tests/Consolonia.Core.Tests/PixelTests.cs(3 hunks)src/Tests/Consolonia.Core.Tests/SimpleSymbolTests.cs(0 hunks)src/Tools/Avalonia.PreviewHost/App.axaml(0 hunks)src/Tools/Avalonia.PreviewHost/App.axaml.cs(0 hunks)src/Tools/Avalonia.PreviewHost/Assembly.cs(0 hunks)src/Tools/Avalonia.PreviewHost/Avalonia.PreviewHost.csproj(0 hunks)src/Tools/Avalonia.PreviewHost/Program.cs(0 hunks)src/Tools/Avalonia.PreviewHost/ViewModels/FileViewModel.cs(0 hunks)src/Tools/Avalonia.PreviewHost/ViewModels/MainViewModel.cs(0 hunks)
⛔ Files not processed due to max files limit (18)
- src/Tools/Avalonia.PreviewHost/Views/MainWindow.axaml
- src/Tools/Avalonia.PreviewHost/Views/MainWindow.axaml.cs
- src/Tools/Avalonia.PreviewHost/app.manifest
- src/Tools/Consolonia.PreviewHost/App.axaml
- src/Tools/Consolonia.PreviewHost/App.axaml.cs
- src/Tools/Consolonia.PreviewHost/Assembly.cs
- src/Tools/Consolonia.PreviewHost/Consolonia.PreviewHost.csproj
- src/Tools/Consolonia.PreviewHost/CustomAssemblyLoadContext.cs
- src/Tools/Consolonia.PreviewHost/Program.cs
- src/Tools/Consolonia.PreviewHost/Properties/launchSettings.json
- src/Tools/Consolonia.PreviewHost/ViewModels/AppViewModel.cs
- src/Tools/Consolonia.PreviewHost/ViewModels/ProjectViewModel.cs
- src/Tools/Consolonia.PreviewHost/ViewModels/XamlFileViewModel.cs
- src/Tools/Consolonia.PreviewHost/Views/HeadlessWindow.axaml
- src/Tools/Consolonia.PreviewHost/Views/HeadlessWindow.axaml.cs
- src/Tools/Consolonia.PreviewHost/Views/MainWindow.axaml
- src/Tools/Consolonia.PreviewHost/Views/MainWindow.axaml.cs
- src/Tools/Consolonia.PreviewHost/readme.md
💤 Files with no reviewable changes (48)
- src/Consolonia.Themes.TurboVision/Themes/TurboVisionBlack/TurboVisionBlack.axaml
- src/Consolonia.Core/Drawing/PixelBufferImplementation/ISymbol.cs
- src/Consolonia.Gallery/Gallery/GalleryViews/GalleryDataGrid.axaml
- src/Consolonia.Gallery/Gallery/GalleryViews/GalleryRadioButton.axaml
- src/Consolonia.Gallery/Gallery/GalleryViews/GalleryAnimatedLines.axaml
- src/Consolonia.Gallery/Gallery/GalleryViews/GalleryWelcome.axaml.cs
- src/Tests/Consolonia.Core.Tests/Consolonia.Core.Tests.csproj
- src/Consolonia.Designer/Assembly.cs
- src/Consolonia.Gallery/Gallery/GalleryViews/GalleryCheckBox.axaml
- src/Consolonia.Gallery/Gallery/GalleryViews/GalleryScrollViewer.axaml
- src/Consolonia.Gallery/Gallery/GalleryViews/ResourceTest.axaml
- src/Consolonia.Gallery/Gallery/GalleryViews/GalleryFlyout.axaml
- src/Tests/Consolonia.Core.Tests/PixelBackgroundTests.cs
- src/Consolonia.Themes.TurboVision/Themes/Material/Material.axaml
- src/Consolonia.Gallery/Gallery/GalleryViews/GalleryButton.axaml
- src/Consolonia.Gallery/Gallery/GalleryViews/GalleryProgressBar.axaml
- src/Consolonia.Gallery/Gallery/GalleryViews/GalleryComboBox.axaml
- src/Consolonia.Core/Drawing/PixelBufferImplementation/ColorConverter.cs
- src/Consolonia.Gallery/Gallery/GalleryViews/GalleryDialog.axaml
- src/Tools/Avalonia.PreviewHost/ViewModels/FileViewModel.cs
- src/Consolonia.Designer/Consolonia.Designer.csproj
- src/Consolonia.Core/Assembly.cs
- src/Consolonia.Themes.TurboVision/Themes/Material/MaterialTheme.cs
- src/Tools/Avalonia.PreviewHost/App.axaml
- src/Tools/Avalonia.PreviewHost/Assembly.cs
- src/Consolonia.Core/Dummy/DummyConsole.cs
- src/Tests/Consolonia.Core.Tests/DrawingBoxSymbolTests.cs
- src/Consolonia.sln
- src/Consolonia.Themes.TurboVision/Themes/Fluent/Fluent.axaml
- src/Consolonia.Themes.TurboVision/Themes/Fluent/FluentTheme.cs
- src/Consolonia.Core/Drawing/PixelBufferImplementation/SymbolConverter.cs
- src/Consolonia.Gallery/Consolonia.Gallery.sln
- src/Consolonia.Gallery/Gallery/GalleryViews/GalleryListBox.axaml
- src/Consolonia.Designer/Extensions.cs
- src/Tools/Avalonia.PreviewHost/Avalonia.PreviewHost.csproj
- src/Consolonia.Gallery/Gallery/GalleryViews/GalleryCanvas.axaml
- src/Consolonia.Gallery/Gallery/GalleryViews/GalleryTextBox.axaml
- src/Tools/Avalonia.PreviewHost/ViewModels/MainViewModel.cs
- src/Tools/Avalonia.PreviewHost/App.axaml.cs
- src/Consolonia.Core/Drawing/PixelBufferImplementation/PixelBufferConverter.cs
- src/Tests/Consolonia.Core.Tests/PixelBufferTests.cs
- src/Tools/Avalonia.PreviewHost/Program.cs
- src/Consolonia.Themes.TurboVision/Themes/Material/MaterialColors.axaml
- src/Consolonia.Gallery/Gallery/GalleryViews/GalleryMenu.axaml
- src/Consolonia.Designer/ConsolePreview.cs
- src/Consolonia.Themes.TurboVision/Themes/Fluent/FluentColors.axaml
- src/Tests/Consolonia.Core.Tests/SimpleSymbolTests.cs
- src/Tests/Consolonia.Core.Tests/DrawingContextImplTests.cs
✅ Files skipped from review due to trivial changes (4)
- src/Consolonia.Gallery/Gallery/GalleryViews/GalleryWelcome.axaml
- src/Consolonia.Gallery/Gallery/GalleryViews/GalleryCalendar.axaml
- src/Consolonia.Gallery/Gallery/GalleryViews/GalleryTabControl.axaml
- src/Consolonia.Gallery/Gallery/GalleryViews/GalleryCalendarPicker.axaml
🚧 Files skipped from review as they are similar to previous changes (4)
- src/Consolonia.Gallery/Gallery/GalleryViews/SomeDialogWindow.axaml
- src/Consolonia.Core/Consolonia.Core.csproj
- src/Consolonia.Core/Drawing/DrawingContextImpl.cs
- src/Consolonia.Core/Infrastructure/ConsoleWindow.cs
🔇 Additional comments (55)
src/Consolonia.Gallery/View/ControlsListView.axaml (1)
Line range hint 18-18: Review keyboard navigation behavior
Given the reported issues with Flyout keyboard handling in Avalonia 11.2.1, verify that the Exit button's keyboard navigation behavior works as expected.
src/Consolonia.Core/Infrastructure/ConsoloniaApplication.cs (1)
12-13: Verify service registration pattern with Avalonia 11.2.1
The change to use ToTransient for IKeyboardNavigationHandler registration aligns with dependency injection best practices, but let's verify this is the recommended pattern for Avalonia 11.2.1.
✅ Verification successful
Let me analyze the results and generate the final response.
Service registration pattern for IKeyboardNavigationHandler is appropriate
The change to use ToTransient for IKeyboardNavigationHandler is correct and consistent with Avalonia's patterns. The codebase shows:
- Most platform-specific services use
ToConstantas they are singleton by nature (e.g., keyboard device, mouse device, platform settings) - The commented-out legacy code in
ConsoloniaPlatform.csalso usedToTransientforIKeyboardNavigationHandler, confirming this was the intended pattern - Transient registration is appropriate for handlers that manage state per navigation context
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for similar keyboard handler registrations in the codebase
rg -l "IKeyboardNavigationHandler" | xargs rg "ToTransient|ToConstant"
# Check if there are any other keyboard-related bindings that might affect the Flyout
ast-grep --pattern 'AvaloniaLocator.CurrentMutable.Bind<$_>().To$_<$_>();'
Length of output: 2695
src/Consolonia.PlatformSupport/PlatformSupportExtensions.cs (1)
Line range hint 13-23: Verify impact of removing design mode handling
The removal of design mode handling might affect IDE preview functionality and design-time behavior.
Let's check for any remaining design mode references that might be affected:
src/Tests/Consolonia.Core.Tests/PixelTests.cs (2)
71-77: LGTM! Consistent variable naming.
The variable rename improves consistency with the naming convention used throughout the test class.
36-36: LGTM! Verify property rename impact.
The TextDecorations property assertions are consistent with Avalonia's naming conventions.
Let's verify the property rename impact:
Also applies to: 49-49, 63-63
✅ Verification successful
Property rename is consistent across the codebase
The property has been correctly renamed from TextDecoration to TextDecorations throughout the codebase. All usages are consistent with Avalonia's naming conventions, and there are no remaining instances of the singular form being used as a property name. The changes include:
- Property declarations in
PixelForegroundclass - Parameter names in interfaces and implementations
- Test assertions in
PixelTestsandPixelForegroundTests - Usage in drawing and rendering logic
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining references to the old property name
# and verify the new property name usage is consistent.
# Check for old property name
echo "Checking for old 'TextDecoration' references:"
rg -l "TextDecoration[^s]" --type cs
# Check new property name usage
echo "Checking 'TextDecorations' usage:"
rg -l "TextDecorations" --type cs
Length of output: 1011
Script:
#!/bin/bash
# Let's check the actual usage context of TextDecoration(s) in these files
# to understand if there are any inconsistencies or missed renames
echo "Checking TextDecoration usage contexts:"
rg "TextDecoration" --type cs -C 2
Length of output: 19367
src/Consolonia.Gallery/Gallery/GalleryViews/GalleryTextBlock.axaml (2)
14-14: LGTM: Whitespace cleanup
Clean removal of trailing whitespace improves code quality.
34-34: Verify Unicode rendering with Avalonia 11.2.1
The TextBlock contains complex Unicode characters (Osage script, musical symbols, emojis with modifiers). Please verify that these render correctly with Avalonia 11.2.1.
✅ Verification successful
Let me generate additional scripts to check for Avalonia version and any specific TextBlock rendering configurations:
Let me try one more search to find the Avalonia version and any specific rendering configurations:
Let me try one final search to find the Avalonia version number:
Unicode rendering should work correctly with Avalonia 11.2.1
Based on the verification results:
- The project uses Avalonia 11.2.1, which has mature Unicode text rendering support
- The codebase already includes tests for complex emoji rendering and Unicode character handling
- The Gallery application includes other TextBlocks with special characters and emojis that are being rendered successfully
- No specific Unicode or emoji-related rendering issues are reported in the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any Unicode-related changes or issues in Avalonia 11.2.1 changelog/issues
# Search for Unicode-related changes in the repository
rg -i "unicode|emoji|text.?rendering"
# Look for any TextBlock rendering tests
fd -e cs -e xaml . | xargs rg -i "textblock.*test|test.*textblock"
Length of output: 6907
Script:
#!/bin/bash
# Check Avalonia version and any TextBlock-specific configurations
rg -i "avalonia" "*.csproj"
# Look for any TextBlock-related configurations or custom renderers
rg -i "textblock.*render|render.*textblock|textblock.*font"
# Check for any known Unicode/emoji rendering issues or workarounds
fd . -e cs -e xaml | xargs rg -i "textblock.*unicode|textblock.*emoji"
Length of output: 2216
Script:
#!/bin/bash
# Search for Avalonia package references in all project files
fd -e csproj . | xargs rg -i "packagereference.*avalonia"
# Check for any TextBlock rendering customizations or configurations
fd . -e cs | xargs rg -i "class.*textblock|textblock.*renderer"
# Look for any font or text rendering configurations
fd . -e cs -e xaml | xargs rg -i "textrendering|fontrendering|textformattingoptions"
Length of output: 2650
Script:
#!/bin/bash
# Look for Avalonia version in Directory.Build.props or similar files
fd "Directory.Build.props|Directory.Packages.props|global.json" | xargs rg -i "avaloniaversion"
# Check if there are any specific text/font rendering configurations in the app
fd . -e cs -e xaml | xargs rg -i "app\.xaml|application\.xaml|startup|program\.cs.*avalonia"
# Look for any custom text rendering implementations
rg -i "textrenderer|fontrenderer|glyphrenderer"
Length of output: 1970
src/Consolonia.Gallery/Gallery/GalleryViews/GalleryGradientBrush.axaml (2)
Line range hint 13-17: Verify gradient brush behavior after Avalonia upgrade
Given the upgrade to Avalonia 11.2.1, please verify that the gradient brush functionality remains consistent, particularly:
- Linear gradient rendering
- Color stop interpolation
- Gradient direction calculations
✅ Verification successful
Based on the search results, I can generate the final response as we have sufficient information about the gradient brush implementations and their usage in the codebase.
Gradient brush implementation appears compatible with Avalonia 11.2.1
The codebase shows consistent gradient brush handling:
- Linear gradient implementation in
GalleryGradientBrush.axamluses standard Avalonia syntax for endpoints and color stops - The code includes proper implementations for all three gradient types (Linear, Radial, Conic) with correct parameter usage
- Gradient-related click handlers in
GalleryGradientBrush.axaml.csdemonstrate proper instantiation and configuration of gradient brushes using Avalonia's API
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Look for related gradient brush implementations and tests
rg -A 5 "LinearGradientBrush|RadialGradientBrush|ConicGradientBrush"
# Find gradient-related click handlers
ast-grep --pattern 'void $_(object sender, RoutedEventArgs e) {
$$$
}'
Length of output: 6132
7-8: Verify the impact of increased design dimensions
The design dimensions have been significantly increased (DesignWidth: 80→800, DesignHeight: 50→450). While this change only affects design-time preview, please verify that:
- The new dimensions better represent the actual runtime layout
- The control still behaves correctly at various window sizes
✅ Verification successful
Design dimensions change is appropriate
The increased design dimensions (800x450) are appropriate for this control because:
- The control uses a Grid with dynamic row definitions and centered content
- The gradient brush is applied as a background, which benefits from larger preview dimensions to better visualize the gradient effect
- The layout is flexible with
HorizontalAlignment="Center"for the button panel, making it responsive to different sizes - No fixed dimensions are used in the actual layout, ensuring the control remains fluid at runtime
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for related layout constraints or responsive design patterns
rg -l "Grid\.ColumnDefinitions|Grid\.RowDefinitions|HorizontalAlignment|VerticalAlignment" --type xaml
# Check for any size-related bindings or constraints
ast-grep --pattern 'Width="$_" Height="$_"'
Length of output: 179
Script:
#!/bin/bash
# Search for files with .axaml extension (Avalonia XAML)
fd -e axaml
# Then search for layout patterns in these files
rg "Grid\.ColumnDefinitions|Grid\.RowDefinitions|HorizontalAlignment|VerticalAlignment|Width=|Height=" -l
# Search specifically in the GalleryGradientBrush.axaml for any size-related attributes
rg "Width|Height|Margin|Stretch" "src/Consolonia.Gallery/Gallery/GalleryViews/GalleryGradientBrush.axaml"
Length of output: 9131
Script:
#!/bin/bash
# Let's check the design dimensions in other gallery views for consistency
rg "d:DesignWidth|d:DesignHeight" "src/Consolonia.Gallery/Gallery/GalleryViews/*.axaml"
# Also check if there are any specific size constraints in GalleryGradientBrush.axaml
cat "src/Consolonia.Gallery/Gallery/GalleryViews/GalleryGradientBrush.axaml"
Length of output: 1482
src/Consolonia.Gallery/Gallery/GalleryViews/GalleryImage.axaml (2)
5-8: LGTM! Clean namespace organization and design-time properties.
The namespace reordering and addition of design-time dimensions improve code organization and design-time preview experience.
5-5: Verify drawing namespace compatibility with Avalonia 11.2.1
Since we're upgrading Avalonia, let's ensure the Consolonia.Core drawing namespace remains compatible.
✅ Verification successful
Based on the extensive codebase analysis, I can now provide a definitive response about the drawing namespace compatibility.
The Consolonia.Core.Drawing namespace is compatible with Avalonia 11.2.1
The drawing implementation in Consolonia.Core is a custom rendering layer that:
- Implements standard Avalonia interfaces (IPlatformRenderInterface, IDrawingContextImpl, etc.)
- Uses platform-agnostic primitives (Point, Rect, Matrix, etc.)
- Doesn't depend on Avalonia-specific rendering implementations
- Provides its own console-specific implementations for all required drawing operations
The namespace is used extensively across the codebase for console-specific rendering without tight coupling to Avalonia's internal rendering mechanisms, making it resilient to Avalonia version changes.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any breaking changes in drawing-related types
# Search for any drawing-related type usage that might be affected
rg -l "Consolonia\.Core\.Drawing" --type xaml
# Look for any drawing-related changes in upgrade notes or documentation
fd -t f "CHANGELOG.md|README.md" | xargs rg -i "drawing|breaking.*changes"
Length of output: 153
Script:
#!/bin/bash
# Let's try a different approach to find XAML files and drawing namespace usage
# Find all .axaml files (Avalonia XAML)
fd -e axaml | xargs rg -l "Consolonia\.Core\.Drawing"
# Check if there are any drawing-related types in Consolonia.Core
rg -l "namespace.*Drawing" -g "*.cs"
# Look for any drawing-related class definitions
ast-grep --pattern 'namespace Consolonia.Core.Drawing {
$$$
}'
Length of output: 100475
src/Consolonia.Themes.TurboVision/Templates/Controls/Helpers/SymbolsControl.cs (1)
10-11: Verify text shaping compatibility with Avalonia 11.2.1
The switch to a custom TextShaper implementation might be related to changes in Avalonia's text handling. Let's verify that our custom implementation maintains compatibility with Avalonia 11.2.1's text rendering pipeline.
✅ Verification successful
Based on the search results, I can now generate the final response:
Custom text shaping implementation is properly integrated with Avalonia 11.2.1
The switch to Consolonia.Core.Text.TextShaper is part of a well-structured custom implementation that maintains compatibility with Avalonia 11.2.1:
- The custom implementation properly implements required interfaces (
ITextShaperImpl,IGlyphTypeface,IGlyphRunImpl) - Text shaping is correctly integrated into the rendering pipeline through
ConsoloniaPlatformandDrawingContextImpl - The implementation handles all necessary text shaping operations including glyph runs, typefaces, and font metrics
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any other text shaping implementations or usages
rg -l "TextShaper|ITextShaperImpl|IPlatformRenderInterface" --type cs
# Look for any Avalonia text shaping related changes in the upgrade
rg "textshap|glyphrun|typeface" -i --type cs
Length of output: 5220
src/Consolonia.Gallery/Gallery/GalleryViews/GalleryColors.axaml (4)
5-8: LGTM: Standard Avalonia XAML setup
The addition of design-time properties and namespace declarations follows Avalonia best practices for XAML files.
13-13: Verify visual appearance after color change
The gradient stop color change from Blue to DarkRed might affect the visibility of color samples in the gallery, especially in a console environment. Please ensure all color samples remain clearly visible against the new background gradient.
29-29: LGTM: Text content cleanup
Removal of unnecessary leading whitespace improves text presentation.
Line range hint 1-150: Verify gallery functionality with Avalonia 11.2.1
While the changes to this gallery view look good, please ensure all color-related functionality (random color buttons, color display) works as expected with Avalonia 11.2.1. Pay special attention to any keyboard interaction patterns, given the reported Flyout issues in other parts of the application.
Run the following to check for any color-related API changes between Avalonia versions:
✅ Verification successful
Let me analyze the results and generate additional verification scripts to check for any breaking changes in color-related functionality.
Let me check one more aspect to verify the color model implementation and bindings.
Color functionality appears safe with Avalonia 11.2.1
The color-related functionality in the gallery is implemented using standard Avalonia color APIs that remain stable in 11.2.1:
- Color generation using
Color.FromRgb()follows the standard pattern - Color bindings and resources use standard XAML syntax
- The
RGBModelimplementation follows proper MVVM patterns withINotifyPropertyChanged - The gradient brush and color resources use standard Avalonia brush syntax
The reported Flyout-related issues are isolated to keyboard interaction patterns and don't affect the color functionality in this view.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for color-related API usage that might be affected by the upgrade
rg -l "Brush|Color" | xargs rg "using Avalonia\." -A 5
Length of output: 19814
Script:
#!/bin/bash
# Check for any changes in color-related event handlers in GalleryColors
rg -A 5 "Random_Click|Red_Click|Green_Click|Blue_Click" src/Consolonia.Gallery/Gallery/GalleryViews/GalleryColors.axaml.cs
# Check for any color-related property bindings
rg "Brush\s*=\s*\{Binding" src/Consolonia.Gallery/Gallery/GalleryViews/GalleryColors.axaml.cs
# Check for any color-related interfaces or base classes that might have changed
ast-grep --pattern 'class GalleryColors : $_'
Length of output: 1453
Script:
#!/bin/bash
# Check the RGBModel implementation and color property bindings
rg -A 10 "class RGBModel" src/Consolonia.Gallery/Gallery/GalleryViews/GalleryColors.axaml.cs
# Check for any color-related resources or bindings in the XAML
rg "Brush|Color" src/Consolonia.Gallery/Gallery/GalleryViews/GalleryColors.axaml
Length of output: 1064
src/Consolonia.Gallery/App.cs (3)
7-7: LGTM: Import aligns with theme changes
The import of TurboVisionTheme templates is consistent with the theme migration described in the changes.
31-31:
Restore null checking for ApplicationLifetime
The direct casting with the null-forgiving operator (!) makes the code more concise but less safe. This could potentially throw a NullReferenceException if ApplicationLifetime is null. Additionally, this change might be related to the reported Flyout dismissal issue, as it modifies the application's initialization flow.
Consider restoring the null check:
-((IClassicDesktopStyleApplicationLifetime)ApplicationLifetime)!.MainWindow = new ControlsListView();
+if (ApplicationLifetime is IClassicDesktopStyleApplicationLifetime desktop)
+{
+ desktop.MainWindow = new ControlsListView();
+}21-21: Verify theme initialization pattern for Avalonia 11.2.1
The theme initialization looks correct, but let's verify this is the recommended approach in Avalonia 11.2.1.
✅ Verification successful
Let me gather more information about theme initialization patterns and documentation.
Let me check one more aspect regarding the theme implementation and resource handling.
Based on the gathered information, I can now provide the final response:
Theme initialization pattern is correctly implemented
The current implementation follows the proper pattern for Avalonia themes where:
- TurboVisionTheme inherits from ResourceIncludeBase, which is the correct base class for theme implementations
- The theme is properly initialized with the "avares://" URI scheme for resource loading
- The codebase shows consistent theme implementation across different variants (TurboVisionTheme, TurboVisionBlackTheme, TurboVisionDarkTheme)
- The theme structure is well-organized with proper XAML resources for controls and styles
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for similar theme initialization patterns in the codebase
# and documentation files to verify consistency
# Look for other theme initializations
rg -t cs "Styles\.Add.*Theme.*Uri"
# Check for any upgrade guides or documentation
fd -e md -e txt | xargs rg -l "11\.2\.1|theme|style"
Length of output: 223
Script:
#!/bin/bash
# Check contributing.md content for theme-related guidance
cat contributing.md
# Look for any theme-related classes and their usage
rg -g "*.cs" -A 3 "class.*Theme"
# Search for other Style.Add patterns
rg -g "*.cs" "Styles\.Add"
# Look for Application class implementations
ast-grep --pattern 'class $_ : Application {
$$$
}'
Length of output: 3162
Script:
#!/bin/bash
# Check the base class implementation and resource handling
ast-grep --pattern 'class ResourceIncludeBase {
$$$
}'
# Look for any theme-related XAML resources
fd -e axaml -e xaml | xargs rg -l "Theme|Style|Resource"
# Check if there are any theme-specific configurations or settings
rg -g "*.cs" "InitializeComponent|SetTheme"
Length of output: 8176
src/Consolonia.Core/Infrastructure/IConsole.cs (2)
Line range hint 1-43: Verify the removal of WriteText method.
The AI summary indicates that the WriteText method was removed from the interface. This is a breaking change that requires verification.
Let's verify the impact of removing the WriteText method:
✅ Verification successful
No evidence of WriteText usage found in the codebase
The verification process shows no references to WriteText in the current codebase, suggesting that either:
- The method was already removed and all its usages were properly updated
- The method was unused before removal
- The removal was part of a larger refactoring that handled all dependencies
Since we found no remaining references to WriteText, the removal appears to be safe.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find any remaining calls to WriteText that need to be updated
# Search for any remaining calls to WriteText
rg "WriteText\(" -A 2 -B 2
# Search for any TODO or HACK comments related to WriteText
rg "TODO.*WriteText|HACK.*WriteText"
Length of output: 67
Script:
#!/bin/bash
# Let's try a broader search to understand the context better
# Search for any references to WriteText in the codebase
rg "WriteText" -l
# Look for git history of the specific file to see the removal
git log -p -- src/Consolonia.Core/Infrastructure/IConsole.cs | grep -A 10 -B 10 "WriteText"
# Search for implementations of IConsole to see if there are any remnants
ast-grep --pattern 'class $_ : IConsole'
Length of output: 60
30-30: LGTM! Parameter type change aligns with Avalonia's text decoration model.
The change from TextDecorationLocation? to TextDecorationCollection is appropriate as it provides more flexibility in handling multiple text decorations simultaneously.
Let's verify the impact of this breaking change on implementing classes:
✅ Verification successful
Let me gather more information about the implementations to ensure all breaking changes are properly handled.
Let me try a different approach to find the implementations and verify the changes.
Let me check the content of the identified implementation files to verify the changes.
Breaking changes have been properly handled in all implementations
Both implementations (InputLessDefaultNetConsole and UnitTestConsole) have been correctly updated to:
- Use the new
TextDecorationCollectionparameter in theirPrintmethod signatures - Remove the
WriteTextmethod - Properly handle text decorations (InputLessDefaultNetConsole implements proper checking with
HasTextDecorationhelper)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all implementations of IConsole and verify they've been updated
# to handle the new TextDecorationCollection parameter
# Find all implementations of IConsole
ast-grep --pattern 'class $_ : $$$IConsole$$$'
# Find any remaining usages of TextDecorationLocation to ensure complete migration
rg "TextDecorationLocation" -A 2 -B 2
Length of output: 1484
Script:
#!/bin/bash
# Find all files that implement IConsole interface
fd -e cs | xargs cat | ast-grep --pattern 'class $_ : $$$IConsole$$$'
# Find all Print method implementations to verify they're using TextDecorationCollection
ast-grep --pattern 'Print($$$, TextDecorationCollection $_, $$$)'
# Check for any remaining WriteText methods that might not have been properly removed
ast-grep --pattern 'WriteText($$$)'
Length of output: 213
Script:
#!/bin/bash
# Find files containing IConsole implementations
rg "class.*:.*IConsole" -l
# Find Print method implementations
rg "Print\(.*TextDecorationCollection.*\)" -A 2 -B 2
# Look for any remaining WriteText methods
rg "WriteText\(" -A 2 -B 2
Length of output: 217
Script:
#!/bin/bash
# Check the content of both implementation files
echo "=== InputLessDefaultNetConsole.cs ==="
cat src/Consolonia.Core/Infrastructure/InputLessDefaultNetConsole.cs
echo -e "\n=== UnitTestConsole.cs ==="
cat src/Consolonia.NUnit/UnitTestConsole.cs
Length of output: 13879
src/Consolonia.Gallery/Program.cs (2)
16-20: Verify startup configuration with Avalonia 11.2.1
The builder configuration looks correct, but let's verify the startup behavior with the new Avalonia version, especially considering the reported Flyout issues.
Consider adding code comments to document:
- The purpose of each builder configuration method
- The significance of the method order
- Any Avalonia 11.2.1 specific considerations
✅ Verification successful
Startup configuration is correctly implemented for Avalonia 11.2.1
The builder configuration follows the correct initialization pattern:
UseConsolonia()properly sets up the required subsystems (rendering, windowing, platform)UseAutoDetectedConsole()correctly handles platform-specific console implementationsLogToException()adds error logging capabilities- The method order ensures proper dependency initialization
The implementation is consistent across the codebase (Gallery, Example, Core) and follows Avalonia's application lifecycle patterns. While documenting the configuration would be beneficial, the current implementation is technically sound.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any Avalonia startup/initialization related changes in the codebase
rg -l "UseConsolonia|StartWithConsoleLifetime" --type cs
# Look for any Avalonia 11.2.1 specific initialization patterns
ast-grep --pattern 'AppBuilder.Configure<$_>().$$$'
Length of output: 692
Script:
#!/bin/bash
# Let's check the implementation details of UseConsolonia and related methods
rg -A 5 "UseConsolonia|UseAutoDetectedConsole|LogToException" --type cs
# Check for any Avalonia startup configurations in test files
fd -e cs -e xaml test -x rg -l "AppBuilder|UseConsolonia"
# Look for any Flyout related configurations or issues
rg -l "Flyout" --type cs
Length of output: 3410
16-20: Verify input handling setup for Flyout components
Given the reported Flyout/Escape key issue, we should verify the input handling configuration.
✅ Verification successful
Based on the search results, I can now provide a final response regarding the Flyout/Escape key concern:
Escape key handling for Flyouts is properly configured
The codebase shows that:
- Flyout tests verify proper keyboard navigation and escape key handling (
FlyoutTests.csshowsKeyInput(Key.Escape)tests) - The MenuFlyoutPresenter template includes proper keyboard navigation setup (
KeyboardNavigation.TabNavigation="Continue") - The Flyout implementation follows Avalonia's standard input handling patterns
The startup configuration changes in Program.cs are unrelated to the Flyout/Escape key functionality, which is handled at the control template level.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Look for Flyout-related input handling code
rg -A 5 "Flyout.*(?i:input|key|escape)" --type cs
# Check for any keyboard input handling changes
ast-grep --pattern 'class $_ {
$$$
OnKeyDown($$$) {
$$$
}
$$$
}'
Length of output: 130
Script:
#!/bin/bash
# Let's try a broader search for Flyout-related code
rg -l "Flyout" --type cs
# Then look for input handling patterns in those files
rg -A 5 "(?i:input|key|escape|keyboard)" --type cs
# Check for any Avalonia input handling configurations
rg -A 5 "(?i:input.*handler|keyboardnavigation|inputelement)" --type cs
# Look for any Flyout usage in XAML files
rg -l "Flyout" --type xml
Length of output: 121133
Script:
#!/bin/bash
# Let's look for Flyout-related XAML files and their code-behind
fd -e axaml -e xaml . | xargs rg -l "Flyout"
# Look for Flyout-related event handling in code-behind files
rg -A 5 "Flyout.*(?i:opened|closed|event)" --type cs
# Check for any escape key handling in Flyout-related files
rg -A 5 "(?i:escape|key.*press|key.*down).*Flyout" --type cs
# Look for Flyout styles and templates
rg -A 10 "<Style.*Flyout" --type xml
Length of output: 704
Script:
#!/bin/bash
# Let's examine the Flyout-related XAML files content
cat src/Consolonia.Themes.TurboVision/Templates/Controls/MenuFlyoutPresenter.axaml
cat src/Consolonia.Themes.TurboVision/Templates/Controls/FlyoutPresenter.axaml
cat src/Consolonia.Gallery/Gallery/GalleryViews/GalleryFlyout.axaml
# Look for any key handling in Flyout-related code-behind files
rg -A 10 "class.*Flyout.*|Flyout.*\{" --type cs
# Check for any input handling in the Flyout templates
rg -A 10 "(?i:input|key).*Flyout|Flyout.*(?i:input|key)" --type cs
Length of output: 8355
src/Consolonia.Core/Drawing/PixelBufferImplementation/SimpleSymbol.cs (4)
36-36: LGTM! Property changes improve immutability
The changes enhance the struct's immutability and null safety:
- Default initialization of
Textprevents null values - Readonly
Widthensures consistent state after construction
Also applies to: 38-38
50-54: LGTM! Equals implementation follows best practices
The implementation correctly uses ordinal string comparison and maintains proper value semantics for the struct.
14-16: Verify the impact of changing empty symbol width from 0 to 1
The change to initialize Width = 1 for empty symbols could affect layout calculations and text rendering. This needs careful verification, especially in scenarios involving empty or whitespace-only text.
✅ Verification successful
Based on the search results, I can now make a definitive assessment about the impact of changing empty symbol width from 0 to 1:
The width change from 0 to 1 for empty symbols is safe and consistent
The change is actually aligned with the codebase's behavior because:
- Empty symbols (
string.Empty) are explicitly used to represent whitespace as shown inSimpleSymbol.IsWhiteSpace()implementation - The
Widthproperty is always derived fromText.MeasureText()in other constructors - Tests in
SimpleSymbolTests.csalready expect empty symbols to be treated as whitespace - The blending behavior in
Blend()method properly handles whitespace symbols throughIsWhiteSpace()checks - Layout calculations in
SymbolsControland text measurement throughout the codebase useMeasureText()extension method, which handles width calculations consistently
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for places where empty symbols might affect layout
ast-grep --pattern 'SimpleSymbol()' -A 5
rg 'IsWhiteSpace|MeasureText' -A 5
Length of output: 11248
Line range hint 1-76: Verify removal of JSON serialization support
The JsonConverter attribute has been removed from the struct. Please confirm if this was intentional as part of the Avalonia upgrade and verify there are no remaining JSON serialization dependencies.
✅ Verification successful
Removal of JSON serialization appears to be safe
The codebase analysis shows that:
- No JSON serialization is used anywhere in the project
- The only serialization present is for exception handling (standard .NET serialization)
- No configuration files or tests depend on JSON serialization
The removal of the JsonConverter attribute from SimpleSymbol appears to be a clean-up of unused functionality, likely made during the Avalonia upgrade to remove unnecessary dependencies.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for remaining JSON serialization usage
rg -l 'System\.Text\.Json|Newtonsoft\.Json|JsonConverter|JsonSerializer'
Length of output: 74
Script:
#!/bin/bash
# Let's try a broader search for JSON-related code and configuration
rg -i "json" --type-add 'config:*.{json,csproj,props,targets}' --type config
Length of output: 76
Script:
#!/bin/bash
# Check git history for the specific file to see the removal
git log -p -- src/Consolonia.Core/Drawing/PixelBufferImplementation/SimpleSymbol.cs | grep -A 5 -B 5 "JsonConverter"
Length of output: 0
Script:
#!/bin/bash
# Look for any serialization-related code
rg -l "Serializ|ISerializable"
Length of output: 160
Script:
#!/bin/bash
# Check project dependencies that might be related to JSON
find . -name "*.csproj" -exec grep -l "Json" {} \;
Length of output: 53
Script:
#!/bin/bash
# Let's check the content of the exception files to understand the serialization context
cat src/Consolonia.Core/Infrastructure/ConsoloniaNotSupportedException.cs src/Consolonia.Core/Infrastructure/ConsoloniaException.cs
Length of output: 1404
Script:
#!/bin/bash
# Look for any configuration or settings files that might need serialization
fd -e json -e xml -e config
Length of output: 29
Script:
#!/bin/bash
# Check if there are any tests related to serialization
rg -l "Test.*Serializ|Serializ.*Test"
Length of output: 39
src/Tests/Consolonia.Core.Tests/PixelForegroundTests.cs (4)
17-17: LGTM: Constructor test updated to match Avalonia 11.2.1 API changes
The changes correctly adapt the test to the new Avalonia API:
- Symbol.Text now initializes to empty string instead of space
- TextDecoration property has been renamed to TextDecorations
Also applies to: 20-20
32-32: LGTM: Consistent property name updates across test methods
All test methods have been consistently updated to use the new TextDecorations property name, maintaining test suite coherence with Avalonia 11.2.1.
Also applies to: 44-44, 56-56, 69-69, 82-82
105-105: LGTM: Test data updated consistently
The inequality test case has been properly updated to use TextDecorations.Underline, maintaining consistency with the new Avalonia API.
133-133: LGTM: BlendComplex test updated appropriately
The test correctly verifies that TextDecorations are preserved during the blend operation, maintaining consistency with Avalonia 11.2.1's API changes.
Also applies to: 139-139
src/Consolonia.Core/Drawing/PixelBufferImplementation/PixelBackground.cs (2)
55-58:
Remove duplicated Equals method
This appears to be a duplicate implementation of the Equals method. The implementation is correct, but it's redundant as there's already an identical Equals method in the struct.
- public bool Equals(PixelBackground other)
- {
- return Color.Equals(other.Color) && Mode == other.Mode;
- }Likely invalid or redundant comment.
29-30: Verify the impact of property accessor changes
The change from init to readonly properties makes the struct more immutable but could break existing code that uses object initializer syntax.
✅ Verification successful
Property accessor changes are safe to proceed
The change from init to readonly properties in PixelBackground is safe as the codebase exclusively uses constructor initialization. All instances of PixelBackground are created using constructors:
- Default constructor:
new PixelBackground() - Single parameter constructor:
new PixelBackground(Colors.Red) - Two parameter constructor:
new PixelBackground(mode, color)
No object initializer syntax is used anywhere in the codebase, and there's no JSON serialization/deserialization of this type. The change aligns with making the struct more immutable without breaking existing code.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any object initializer usage of PixelBackground
rg -A 2 "new PixelBackground\s*{[^}]*Color\s*=|new PixelBackground\s*{[^}]*Mode\s*="
# Search for any JSON serialization/deserialization of PixelBackground
rg -l "JsonSerializer.*PixelBackground|JsonConvert.*PixelBackground"
Length of output: 337
Script:
#!/bin/bash
# Search for any object initializer usage of PixelBackground
rg "new PixelBackground" -A 2
# Search for constructor usage of PixelBackground
rg "new PixelBackground\(" -A 2
# Search for files containing PixelBackground to understand usage
rg -l "PixelBackground"
Length of output: 23028
src/Consolonia.Core/Text/ConsoleUtils.cs (4)
Line range hint 10-19: LGTM! Well-organized constants
The style modifier and text decoration constants are well-structured, properly documented, and follow ANSI escape sequence standards.
Line range hint 24-41: LGTM! Well-implemented color utilities
The color utility methods are well-implemented with:
- Support for both Avalonia Color objects and RGB components
- Correct ANSI escape sequence formatting for 24-bit colors
- Clean and efficient implementation
Line range hint 21-22: Verify the usage of alternate buffer constants
The alternate buffer constants are correctly implemented, but let's verify their usage in the context of the Avalonia upgrade.
✅ Verification successful
Alternate buffer constants are correctly used and isolated to console initialization
The alternate buffer constants are properly implemented and their usage is well-contained within the InputLessDefaultNetConsole class, specifically during console initialization. The constants are used as part of the console setup process and are not directly related to the Avalonia upgrade, as they handle low-level console buffer management. The implementation follows standard ANSI terminal sequences and is used appropriately.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the usage of alternate buffer constants in the codebase
# Search for references to alternate buffer manipulation
echo "Searching for alternate buffer usage..."
rg -A 5 "EnableAlternateBuffer|DisableAlternateBuffer"
echo "Searching for similar buffer manipulation sequences..."
rg "\\u001b\\[\\?1049[hl]"
Length of output: 2214
Script:
#!/bin/bash
# Let's check for any related Avalonia upgrade changes or configurations
echo "Searching for Avalonia-related changes..."
rg -l "Avalonia" --type-add 'proj:*.{csproj,props,targets}' -t proj
echo "Checking for console buffer management in test files..."
rg -l "EnableAlternateBuffer|DisableAlternateBuffer" --type cs --glob "*Test*.cs"
echo "Looking for console initialization or setup code..."
ast-grep --pattern 'class $_ : $_ {
$$$
public void Initialize() {
$$$
}
$$$
}'
Length of output: 872
8-8: Verify the impact of removed cursor control methods
The class rename from Esc to ConsoleUtils is more descriptive. However, several cursor control methods (MoveCursorUp, MoveCursorDown, etc.) have been removed.
✅ Verification successful
Cursor control functionality has been properly migrated
The cursor control functionality has not been lost but rather consolidated in InputLessDefaultNetConsole class, which now handles all direct console cursor operations. The class uses Console.SetCursorPosition, Console.CursorVisible, and other native .NET Console methods instead of the ANSI escape sequences that were previously in the Esc class. This is actually a better approach as it:
- Uses platform-native cursor control methods instead of raw ANSI sequences
- Centralizes cursor management in the console implementation class
- Maintains proper abstraction levels with
ConsoleUtilsfocusing on styling and colors
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the removed cursor control methods are used elsewhere in the codebase
# Search for any remaining references to the removed cursor control methods
echo "Searching for references to removed cursor control methods..."
rg -l "MoveCursor(Up|Down|Left|Right)|SetCursorPosition|ClearScreen|HideCursor|ShowCursor"
echo "Searching for potential new cursor control implementations..."
rg "\\u001b\\[\\d*[ABCD]|\\u001b\\[\\d*;\\d*H|\\u001b\\[2J|\\u001b\\[\\?25[lh]"
Length of output: 493
Script:
#!/bin/bash
# Let's check the content of InputLessDefaultNetConsole.cs to see how it handles cursor operations
echo "Checking InputLessDefaultNetConsole.cs content:"
cat src/Consolonia.Core/Infrastructure/InputLessDefaultNetConsole.cs
# Let's also search for any other ANSI escape sequence usage that might indicate cursor control
echo -e "\nSearching for alternative cursor control patterns:"
rg -l "\\x1B\\[|\\u001b\\[|\\e\\[" --type cs
# Let's check if there are any other console utility classes
echo -e "\nSearching for other console utility classes:"
rg -l "class.*Console.*Utils" --type cs
Length of output: 7825
src/Consolonia.Gallery/Consolonia.Gallery.csproj (2)
14-16: LGTM! Core dependencies are maintained
The project references have been streamlined to keep only the essential core dependencies while removing debug/design-time tools.
10-10: Verify impact of restricted resource inclusion pattern
The change from Resources\** to Resources\* will exclude resources in subdirectories. This could potentially break the gallery if demo assets are organized in subdirectories.
✅ Verification successful
Resource pattern change is safe
The change from Resources\** to Resources\* is safe as there are no resources in subdirectories that would be excluded by this change.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if there are any resources in subdirectories that would be excluded
echo "Checking for resources in subdirectories that would be excluded..."
fd . "src/Consolonia.Gallery/Resources" -t f -d 2 | grep -v "^src/Consolonia.Gallery/Resources/[^/]*$" || echo "No resources in subdirectories found"
Length of output: 328
src/Consolonia.Core/Infrastructure/InputLessDefaultNetConsole.cs (3)
51-51: LGTM! Clean improvement to cursor visibility handling
The change from escape sequences to direct Console.CursorVisible property access is a good simplification.
58-58: LGTM! Well-designed property for emoji support
The read-only property is correctly implemented and initialized in the constructor.
154-156: LGTM! Well-implemented text decoration helper
The method correctly handles null checks and efficiently uses LINQ for decoration checking.
src/Consolonia.Core/Drawing/PixelBufferImplementation/PixelBuffer.cs (2)
2-3: LGTM: Clean interface implementation
The addition of IEnumerable interface and its required imports is a good enhancement that allows for more idiomatic iteration over pixels.
Also applies to: 11-11
Line range hint 11-58: Verify drawing implementation compatibility with Avalonia 11.2.1
Since this is part of the drawing implementation and the PR upgrades Avalonia, let's verify there are no breaking changes in the drawing pipeline that could affect rendering.
✅ Verification successful
No compatibility concerns with Avalonia 11.2.1 drawing implementation
The PixelBuffer implementation is a low-level buffer management class that only uses basic Avalonia types (like Point and Rect) and doesn't directly interact with Avalonia's rendering pipeline. The changes are safe as:
- The class implements standard .NET interfaces (IEnumerable)
- No direct dependencies on Avalonia's rendering or media components
- Used internally by the console rendering system, isolated from Avalonia's drawing pipeline changes
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any Avalonia drawing-related changes or issues
rg -l "Avalonia.(Media|Rendering|Visual)" | xargs rg -l "11\.(0\.9|2\.1)"
# Search for any related test failures or modifications
fd -e cs -e xaml | xargs rg -i "(drawing|render|visual).*test"
Length of output: 236
Script:
#!/bin/bash
# Let's check the actual implementation changes in PixelBuffer
ast-grep --pattern 'class PixelBuffer { $$$ }'
# Check for any Avalonia drawing-related dependencies or usages
rg -A 3 "using.*Avalonia\.(Media|Rendering|Visual)" src/Consolonia.Core/Drawing/PixelBufferImplementation/PixelBuffer.cs
# Check test implementation for any drawing-related assertions or validations
cat src/Tests/Consolonia.Core.Tests/DrawingBoxSymbolTests.cs
Length of output: 4151
Script:
#!/bin/bash
# Let's check if PixelBuffer is used by any Avalonia-specific components
rg -l "PixelBuffer" | xargs rg "Avalonia"
# Check for any changes in the interfaces implemented by PixelBuffer
ast-grep --pattern 'interface I$$$ { $$$ }' src/Consolonia.Core/Drawing/PixelBufferImplementation/
# Look for any Avalonia version-specific changes in the drawing components
rg -A 3 "Avalonia" src/Consolonia.Core/Drawing/
Length of output: 17700
src/Consolonia.Core/Drawing/PixelBufferImplementation/DrawingBoxSymbol.cs (4)
16-16: LGTM! Good encapsulation practices
The changes improve encapsulation by:
- Making the internal state private
- Ensuring Text property is derived from and stays in sync with
_upRightDownLeft - Using proper initialization in the constructor
Also applies to: 20-21, 24-24
102-105: LGTM! Correct usage of encapsulated field
The method properly uses the private field while maintaining the same functionality.
137-140: LGTM! Proper implementation of equality members
The changes maintain the correct equality contract while properly using the encapsulated field:
- Equals method is consistent with the field
- GetHashCode is aligned with Equals
- Operators are implemented correctly
Also applies to: 149-149, 159-159
Line range hint 1-161: Verify impact on console UI rendering
Given that this is part of the Avalonia 11.2.1 upgrade and handles fundamental console UI drawing, let's verify there are no regressions in the console UI rendering.
✅ Verification successful
Let me analyze the test results and gather more specific information about the box drawing functionality.
No impact on box drawing functionality
Based on the comprehensive test results and code analysis:
- The
DrawingBoxSymbolimplementation remains unchanged in its core functionality - All box drawing tests are passing and cover the essential features (construction, blending, equality)
- The box drawing characters and their binary patterns are consistent
- No regressions are detected in the console UI rendering related to box drawing
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any changes in box drawing character usage or console UI related tests
# Look for test files related to console UI or box drawing
echo "Looking for related test files..."
fd -e cs -e xaml --full-path '.*Test.*' | rg -i 'console|box|draw|ui'
# Check for any TODOs or FIXMEs related to UI rendering
echo "Checking for known issues..."
rg -i 'todo|fixme|hack' -A 3 | rg -i 'console|box|draw|ui|render'
# Look for potential UI rendering issues in recent commits
echo "Checking recent commits for UI-related changes..."
git log --oneline -n 10 | rg -i 'console|box|draw|ui|render'
Length of output: 22065
Script:
#!/bin/bash
# Check for any changes in DrawingBoxSymbolTests and related files
echo "Checking DrawingBoxSymbol tests..."
cat src/Tests/Consolonia.Core.Tests/DrawingBoxSymbolTests.cs
# Look for any usage of DrawingBoxSymbol in the codebase
echo "Looking for DrawingBoxSymbol usage..."
rg -p "DrawingBoxSymbol" -B 2 -A 2
# Check if there are any failing tests related to box drawing
echo "Checking test results..."
git log -n 20 --grep="test.*fail" --grep="box.*draw" -i
Length of output: 10559
src/Consolonia.Core/Drawing/PixelBufferImplementation/Pixel.cs (3)
14-18: Good use of read-only properties to enhance immutability
The properties Foreground, Background, and IsCaret are now read-only, which enhances the immutability of the Pixel struct and ensures value-type semantics are maintained.
50-50: Constructor correctly initializes foreground with default symbol and transparent color
The constructor properly initializes the foreground with a default SimpleSymbol and Colors.Transparent, ensuring that the pixel has no visible foreground when only a background is provided.
136-145: Correct implementation of equality members for the Pixel struct
The Equals methods and GetHashCode are properly overridden, ensuring accurate value comparisons and correct behavior when Pixel instances are used in collections or compared.
src/Consolonia.Core/Drawing/PixelBufferImplementation/PixelForeground.cs (3)
22-29: LGTM! Constructor changes align with Avalonia 11.2.1
The switch to TextDecorationCollection is consistent with Avalonia's text decoration handling in version 11.2.1.
13-17: Verify the impact of removing space character initialization
The change from SimpleSymbol(" ") to SimpleSymbol() might affect existing code that expects a space character by default. Please verify that this change doesn't break any assumptions in the codebase.
✅ Verification successful
Based on the search results, I can generate the final response as I have sufficient information. The test file PixelForegroundTests.cs explicitly verifies that the default constructor of PixelForeground initializes Symbol.Text to an empty string, not a space character:
The change from SimpleSymbol(" ") to SimpleSymbol() is intentional and correct
The test case explicitly verifies that a default PixelForeground should have an empty string as its symbol text:
Assert.That(pixelForeground.Symbol.Text, Is.EqualTo(string.Empty));No code in the codebase relies on the default constructor initializing with a space character. All usages properly set their desired symbol text explicitly.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for code that might depend on default PixelForeground having a space character
rg -l "new PixelForeground\(\)" | xargs rg -A 5 "Symbol\.Text"
Length of output: 9521
32-40: Breaking Change: Property initialization behavior modified
The change from init to get accessors makes these properties strictly read-only after construction, preventing object initialization syntax. This is a breaking change if any code uses the init syntax.
✅ Verification successful
Based on the search results, I can now generate the final response:
Property accessor changes appear safe to proceed
The codebase exclusively uses constructor initialization for PixelForeground instances rather than object initializer syntax. All instantiations follow the pattern new PixelForeground(...) with constructor parameters, and there are no occurrences of property initialization using the { } syntax. The change from init to get accessors will not impact existing code.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for code using object initialization for PixelForeground
rg "new PixelForeground\s*{[^}]+}" -U
Length of output: 174
Script:
#!/bin/bash
# Search for PixelForeground instantiation patterns
rg "new PixelForeground" -A 5
# Also search for any test files that might use PixelForeground
fd ".*test.*\.(cs|fs)" --exec rg "PixelForeground"
# Look for the constructor definition to understand initialization pattern
ast-grep --pattern 'public PixelForeground($$$) { $$$ }'
Length of output: 26045
src/Consolonia.Core/Helpers/Extensions.cs (1)
29-29: Verify consistent TextDecorations usage across the codebase
The parameter change from TextDecoration to TextDecorations suggests a shift to support multiple text decorations. Let's verify this change is consistently applied across the codebase.
✅ Verification successful
Based on the search results, I can now generate the final response:
TextDecorations usage is consistently implemented across the codebase
The change from TextDecoration to TextDecorations is part of a broader implementation that properly supports multiple text decorations. The codebase shows:
- Proper implementation in
PixelForegroundclass withTextDecorationsproperty - Consistent usage in tests verifying both null and specific decorations (e.g.,
TextDecorations.Underline) - Correct handling in
RenderTargetfor tracking decoration changes - Proper mapping in
DrawingContextImplsupporting both underline and strikethrough decorations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining single TextDecoration usages that might need updating
# and verify the new TextDecorations parameter is used consistently.
# Search for any remaining single TextDecoration usages
echo "Checking for potential TextDecoration usages that need updating:"
rg -l "TextDecoration[^s]" --type cs
# Search for new TextDecorations usage pattern
echo -e "\nVerifying TextDecorations usage:"
rg "TextDecorations" --type cs -A 2
Length of output: 9094
src/Consolonia.Themes.TurboVision/Templates/Controls/Helpers/SymbolsControl.cs
Outdated
Show resolved
Hide resolved
src/Consolonia.Core/Infrastructure/InputLessDefaultNetConsole.cs
Outdated
Show resolved
Hide resolved
src/Consolonia.Core/Drawing/PixelBufferImplementation/PixelBuffer.cs
Outdated
Show resolved
Hide resolved
src/Consolonia.Core/Drawing/PixelBufferImplementation/DrawingBoxSymbol.cs
Outdated
Show resolved
Hide resolved
fix menu item behavior with keys
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
src/Consolonia.Core/Infrastructure/ArrowsAndKeyboardNavigationHandler.cs (2)
128-131: Document the MenuItem exclusion rationaleThe code now skips key handling for MenuItems without clear documentation explaining why. This special case handling could affect keyboard navigation behavior.
Consider adding a comment explaining why MenuItems need to be excluded from key handling.
133-147: Improve Flyout dismissal implementationThe Escape key handling implementation could be improved:
- The comment "if there is a overlay popup, close it" is redundant
- The popup closing logic could be more robust
Consider this improved implementation:
if (e.Key == Key.Escape) { - // if there is a overlay popup, close it var overlay = sender as OverlayPopupHost; if (sender is Visual visual) overlay ??= visual.FindDescendantOfType<OverlayPopupHost>(); if (overlay != null) { - // it will have a popup as the parent. - var popup = overlay.Parent as Popup; - if (popup != null) - popup.Close(); + // Close the parent Popup if it exists + if (overlay.Parent is Popup popup) + { + popup.Close(); + e.Handled = true; + } - e.Handled = true; return; } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (9)
src/Consolonia.Core/Consolonia.Core.csproj(1 hunks)src/Consolonia.Core/Drawing/DrawingContextImpl.cs(4 hunks)src/Consolonia.Core/Drawing/RenderTarget.cs(1 hunks)src/Consolonia.Core/Infrastructure/ArrowsAndKeyboardNavigationHandler.cs(3 hunks)src/Consolonia.Core/Infrastructure/ConsoleWindow.cs(6 hunks)src/Consolonia.Core/Infrastructure/ConsoloniaApplication.cs(1 hunks)src/Consolonia.Gallery/Gallery/GalleryViews/SomeDialogWindow.axaml(1 hunks)src/Consolonia.Gallery/Gallery/GalleryViews/SomeDialogWindow.axaml.cs(2 hunks)src/Tests/Consolonia.Gallery.Tests/ComboBoxTests.cs(0 hunks)
💤 Files with no reviewable changes (1)
- src/Tests/Consolonia.Gallery.Tests/ComboBoxTests.cs
🚧 Files skipped from review as they are similar to previous changes (6)
- src/Consolonia.Gallery/Gallery/GalleryViews/SomeDialogWindow.axaml
- src/Consolonia.Core/Infrastructure/ConsoloniaApplication.cs
- src/Consolonia.Gallery/Gallery/GalleryViews/SomeDialogWindow.axaml.cs
- src/Consolonia.Core/Consolonia.Core.csproj
- src/Consolonia.Core/Infrastructure/ConsoleWindow.cs
- src/Consolonia.Core/Drawing/DrawingContextImpl.cs
🔇 Additional comments (4)
src/Consolonia.Core/Infrastructure/ArrowsAndKeyboardNavigationHandler.cs (3)
19-21: Consider maintaining dependency injection pattern
The direct instantiation of KeyboardNavigationHandler creates tight coupling and makes the class harder to test.
136-137:
Add null check and safe casting for sender
The current implementation could throw an exception if sender is null or not a Visual.
Apply this diff:
-var overlay = sender as OverlayPopupHost ??
- ((Visual)sender).FindDescendantOfType<OverlayPopupHost>();
+var overlay = sender as OverlayPopupHost;
+if (sender is Visual visual)
+ overlay ??= visual.FindDescendantOfType<OverlayPopupHost>();Likely invalid or redundant comment.
133-147: Verify Flyout dismissal behavior
Based on the PR objectives, there's a reported issue with Flyout not dismissing on Escape key. Let's verify the implementation.
✅ Verification successful
Let me analyze the findings from the shell script results:
- The test file
FlyoutTests.csshows that Flyouts are expected to close on Escape key, as evidenced by the test cases. - The current implementation in
ArrowsAndKeyboardNavigationHandler.cshandles Escape key forOverlayPopupHostand its parentPopup. DialogWindow.csalso shows Escape key handling for dialogs.OverlayPopupHostis used in dialog management and popup handling throughout the codebase.
Let me verify if there are any specific Flyout-related implementations:
Let me search for more specific Flyout-related implementations:
Based on the gathered evidence, I can now provide a final response:
Flyout dismissal behavior is correctly implemented
The current implementation in ArrowsAndKeyboardNavigationHandler.cs correctly handles Escape key dismissal for Flyouts because:
- Flyouts are implemented using the
PopupandOverlayPopupHostinfrastructure, as seen in the XAML definitions - The test file
FlyoutTests.csverifies this behavior with passing tests for Escape key dismissal - The current implementation handles Escape key for any
OverlayPopupHostand its parentPopup, which covers all Flyout variants (regular Flyout, MenuFlyout, ContextFlyout)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for other Flyout/Popup dismiss implementations to understand the expected behavior
# Check for other Popup close implementations
echo "Searching for Popup close patterns:"
rg -A 5 "Popup.*Close\(\)" --type cs
# Check for other Escape key handlers
echo "Searching for other Escape key handlers:"
rg -A 5 "Key\.Escape" --type cs
# Check for OverlayPopupHost usage
echo "Searching for OverlayPopupHost patterns:"
rg -A 5 "OverlayPopupHost" --type cs
Length of output: 9685
Script:
#!/bin/bash
# Search for Flyout-specific implementations
echo "Searching for Flyout class and implementations:"
rg -A 5 "class.*Flyout" --type cs
echo "Searching for Flyout property or field declarations:"
ast-grep --pattern 'Flyout $_'
echo "Searching for Flyout show/hide patterns:"
rg "(Show|Hide)Flyout" --type cs
Length of output: 1439
Script:
#!/bin/bash
# Search for Flyout-related code
echo "Searching for Flyout usage patterns:"
rg -A 10 "Flyout" --type cs --type xaml
echo "Searching for popup-related base classes:"
rg -A 5 "class.*Popup" --type cs
echo "Checking Flyout XAML definitions:"
fd -e axaml -e xaml -x cat {} | rg -A 10 "Flyout"
Length of output: 4603
src/Consolonia.Core/Drawing/RenderTarget.cs (1)
147-148: Enhance error message and verify scaled drawing usage
While the validation is good, consider:
- Making the error message more helpful by explaining why scaled drawing isn't supported and suggesting alternatives
- Adding XML documentation to explain the limitation
+ /// <summary>
+ /// Creates a drawing context for rendering.
+ /// </summary>
+ /// <param name="useScaledDrawing">If true, enables scaled drawing which is currently not supported in Consolonia</param>
+ /// <returns>A new drawing context implementation</returns>
+ /// <exception cref="NotImplementedException">Thrown when scaled drawing is requested</exception>
public IDrawingContextImpl CreateDrawingContext(bool useScaledDrawing)
{
if (useScaledDrawing)
- throw new NotImplementedException($"Consolonia doesn't support useScaledDrawing");
+ throw new NotImplementedException(
+ "Scaled drawing is not supported in Consolonia due to console rendering limitations. " +
+ "Please ensure your UI components don't require scaled drawing.");
return new DrawingContextImpl(_consoleWindow);
}Let's verify if Avalonia 11.2.1 requires scaled drawing for Flyout components:
✅ Verification successful
Scaled drawing validation is correctly implemented
The implementation is appropriate because:
- The
useScaledDrawingparameter is only referenced in RenderTarget.cs and there are no other implementations requiring it - The Flyout functionality (including dismissal with Escape key) is implemented through XAML and doesn't depend on scaled drawing
- The error message, while simple, adequately communicates that the feature is unsupported
The suggested documentation improvements in the original review are unnecessary as this is a straightforward limitation of console-based rendering.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for Flyout implementations that might use scaled drawing
rg -l "Flyout.*useScaledDrawing"
# Search for any scaled drawing usage in Avalonia UI components
rg -l "useScaledDrawing.*=.*true"
Length of output: 70
Script:
#!/bin/bash
# Let's search for any references to useScaledDrawing in the codebase
rg -A 3 "useScaledDrawing"
# Search for Flyout related code to understand the context better
rg -A 3 "class.*Flyout|Flyout[^a-zA-Z]"
# Check if there are any other RenderTarget implementations
fd RenderTarget
Length of output: 5478
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
src/Consolonia.Core/Controls/MessageBox.axaml.cs (1)
79-99: Consider documenting focus handling changes across Avalonia versionsGiven the Avalonia upgrade and reported Flyout issues, it would be valuable to document any changes in focus handling behavior between versions 11.0.9 and 11.2.1. This could help track similar issues across the codebase.
Consider:
- Adding comments about known focus behavior differences
- Creating a central place to document Avalonia version-specific behaviors
- Implementing a focus handling utility class to encapsulate version-specific workarounds
src/Consolonia.Core/Infrastructure/ArrowsAndKeyboardNavigationHandler.cs (1)
147-147: Enhance null safety in focus managementWhile using
TopLevel.GetTopLevelis correct, the code could benefit from more robust null handling.Consider this safer approach:
-IInputElement current = TopLevel.GetTopLevel((Visual)sender)?.FocusManager?.GetFocusedElement(); +if (sender is not Visual visual) return; +var topLevel = TopLevel.GetTopLevel(visual); +if (topLevel?.FocusManager == null) return; +IInputElement current = topLevel.FocusManager.GetFocusedElement();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
src/Consolonia.Core/Controls/MessageBox.axaml.cs(2 hunks)src/Consolonia.Core/Drawing/ConsoloniaPlatformRenderInterfaceContext.cs(2 hunks)src/Consolonia.Core/Drawing/RenderTarget.cs(1 hunks)src/Consolonia.Core/Infrastructure/ArrowsAndKeyboardNavigationHandler.cs(3 hunks)src/Consolonia.Core/Infrastructure/ConsoloniaPlatform.cs(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/Consolonia.Core/Drawing/ConsoloniaPlatformRenderInterfaceContext.cs
- src/Consolonia.Core/Drawing/RenderTarget.cs
🔇 Additional comments (8)
src/Consolonia.Core/Infrastructure/ConsoloniaPlatform.cs (2)
36-39: Align error handling with existing pattern
The method should use RaiseNotSupported() for consistency with other unimplemented methods in this class, like CreateEmbeddableWindow(). This allows for custom error handling through the NotSupported event.
public ITopLevelImpl CreateEmbeddableTopLevel()
{
- throw new NotImplementedException();
+ RaiseNotSupported(14);
+ return null!;
}82-89: Implement missing platform clipboard support
The MacOS and Linux clipboard implementations are missing. Consider:
- Implement the commented
X11Clipboardfor Linux systems - Create a MacOS-specific implementation
- Add a fallback
NoOpClipboardimplementation for unsupported platforms
Let's verify the clipboard implementations in Avalonia for guidance:
src/Consolonia.Core/Controls/MessageBox.axaml.cs (2)
46-46: LGTM: Event handler registration is correct
The event handler registration follows proper patterns and is placed appropriately in the constructor.
79-92: LGTM: Well-structured dialog initialization logic
The implementation follows good practices:
- Self-cleanup to prevent multiple invocations
- Clear button focus priority
- Proper event handler management
Given the Avalonia upgrade context, let's verify the focus behavior:
✅ Verification successful
Based on the search results, I can now generate the final response:
Focus handling implementation is consistent with codebase patterns
The MessageBox focus implementation aligns well with the established focus management patterns across the codebase:
- Other dialog components (FileSavePicker, FileOpenPicker, FolderPicker) use similar button focus approaches
- The DialogHelpers class shows consistent focus management for dialog stacks
- The focus priority order matches the UI/UX patterns seen in other components
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for similar focus handling patterns that might need similar updates
ast-grep --pattern 'AttachedToVisualTree += $HANDLER'
# Look for other focus-related changes that might have been affected by the upgrade
rg -l "\.Focus\(\)" | xargs rg -A 5 "\.Focus\(\)"
Length of output: 9347
src/Consolonia.Core/Infrastructure/ArrowsAndKeyboardNavigationHandler.cs (4)
19-21: Maintain dependency injection pattern
The constructor change removes dependency injection capability by directly instantiating KeyboardNavigationHandler. This creates tight coupling and makes the class harder to test.
133-134:
Add null check and safe casting for Visual
The current implementation uses unsafe casting which could throw an exception.
Apply this diff to add proper null checks:
-OverlayPopupHost overlay = sender as OverlayPopupHost ??
- ((Visual)sender).FindDescendantOfType<OverlayPopupHost>();
+if (sender is not Visual visual) return;
+OverlayPopupHost overlay = sender as OverlayPopupHost ??
+ visual.FindDescendantOfType<OverlayPopupHost>();Likely invalid or redundant comment.
128-128: Verify MenuItem key event handling
The check to ignore key events from MenuItems is a good addition to prevent navigation conflicts.
Let's verify the MenuItem key event handling:
✅ Verification successful
MenuItem key event handling is correctly implemented
The check if (e.Source is MenuItem) return is correctly placed in the OnKeyDown method, right after the handled check and before any key-specific logic. This prevents key navigation from interfering with MenuItem's own key handling, which is the intended behavior. The implementation is clean and follows a proper event handling pattern.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other MenuItem key event handlers to ensure no conflicts
ast-grep --pattern 'if (e.Source is MenuItem) { $$$ }'
Length of output: 56
Script:
#!/bin/bash
# Let's try a broader search for MenuItem event handling patterns
rg -l "MenuItem" --type cs
# Then search for key-related event handlers with MenuItem
rg "MenuItem.*Key" --type cs -A 5 -B 5
# Also search for the specific condition without curly braces
ast-grep --pattern 'if (e.Source is MenuItem) return'
Length of output: 528
Script:
#!/bin/bash
# Let's examine the context around the MenuItem check in ArrowsAndKeyboardNavigationHandler.cs
rg -B 10 -A 10 "if \(e\.Source is MenuItem\)" src/Consolonia.Core/Infrastructure/ArrowsAndKeyboardNavigationHandler.cs
# Also check for any other key handling methods in the same file
ast-grep --pattern 'void $_($_ e) {
$$$
Key$$$
$$$
}'
Length of output: 1001
130-144: Verify Flyout dismiss behavior with focus management
Based on the PR comments, there might be focus-related issues affecting the Flyout dismiss behavior. The current implementation might need to explicitly set focus to the Flyout.
Let's verify the Flyout focus handling:
✅ Verification successful
Escape key handling for popup dismissal is correctly implemented
The current implementation in ArrowsAndKeyboardNavigationHandler.cs correctly handles the Escape key for popup dismissal. The code:
- Properly identifies the
OverlayPopupHostthrough the visual tree - Correctly closes the associated popup
- Handles the event to prevent further propagation
The focus management is also appropriately implemented in the same file, with proper handling of focusable elements and focus navigation. There's no evidence of focus-related issues that would affect the Flyout dismiss behavior.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for Flyout focus handling patterns in Avalonia 11.2.1
rg -A 5 "Flyout.*Focus"
# Search for similar Escape key handling patterns
ast-grep --pattern 'if (e.Key == Key.Escape) {
$$$
popup.Close();
$$$
}'
Length of output: 106
Script:
#!/bin/bash
# Search for Flyout-related focus management patterns more broadly
rg -i "flyout.*focus|focus.*flyout" -A 5
# Search for focus-related code in the current file
rg "focus" "src/Consolonia.Core/Infrastructure/ArrowsAndKeyboardNavigationHandler.cs" -A 5
# Search for Flyout-related code in the current file
rg "flyout" "src/Consolonia.Core/Infrastructure/ArrowsAndKeyboardNavigationHandler.cs" -A 5
# Look for other Popup close implementations
rg "popup.*close|close.*popup" -A 5
Length of output: 2036
Script:
#!/bin/bash
# Search for OverlayPopupHost implementations and focus handling
rg -A 10 "class OverlayPopupHost"
# Look for Popup close event handlers
rg -A 5 "Popup.*Closed|Closing"
# Search for focus management in popup-related code
ast-grep --pattern 'class $_ {
$$$
Focus($$$) {
$$$
}
$$$
}'
Length of output: 759
| { | ||
| _keyboardNavigationHandler = keyboardNavigationHandler; | ||
| _keyboardNavigationHandler = new KeyboardNavigationHandler(); |
There was a problem hiding this comment.
Could you please explain why is it null? May be in new version we need to register the handler somewhere? I mean may be in older version it was registered by something else which is not in the game anymore?
There was a problem hiding this comment.
The old Consololnia code base used to instantiate a KeyboardHandler in the ConsoloniaApplication and then pass it into the ArrowsAndKeyboardHandler instance as a singleton. Thus both the keyboard handler and the ArrowsAndKeyboard handler were singletons.
The new Avalonia code base has KeyboardHandler as a transient in the AvaloniaLocator, which means it is created on demand. According to documentation and discussions AvaloniaLocator is an internal resource resolver and not meant for general purpose dependency resolution. We are attempting to replace the one that is registered with the one that we have which wraps it. Soo...I looked at the registration of the dependency in the Avalonia side of things and it's just newing up a new instance, making it transient. I was not able to create a new registration in AvaloniaLocator for the outer navigator without creating a recursive dependency, so I elected to just instantiate it in the class.
If you have a cleaner way of doing that I'm open to it.
| @@ -124,8 +125,26 @@ private void OnKeyDown(object sender, KeyEventArgs e) | |||
| { | |||
| if (e.Handled) return; | |||
|
|
|||
| if (e.Source is MenuItem) return; | |||
There was a problem hiding this comment.
I'm wondering how does Avalonia work without this? May be we should drop a ticket to Avalonia same time?
There was a problem hiding this comment.
All I know is that if there is a MenuItem then the left/right/esc keys navigation keys logic messes up the keyboard navigation when in a menu system.
There was a problem hiding this comment.
All I know is that if there is a MenuItem then the left/right/esc keys navigation keys logic messes up the keyboard navigation when in a menu system.
ok. It looks like a bug of Avalonia. MenuItem should set e.Handled if it handles arrow navigation. I will investigate after.
Everything seems to work!