Add support for full RGB colors and TextDecorations #126
Conversation
Overhauled various files through code refactoring, removing unnecessary attributes, updating type definitions, cleaning up method signatures. Significant changes were made to the Core/Infrastructure platform and several TurboVision themes and templates.
This commit transitions helper methods to use a new syntax, and adjusts various bindings in grid items and data templates. In several instances, template-related properties such as design width and height were removed. Changes were also made to focus management, specifically transitioning from using the deprecated FocusManager.Instance to using AvaloniaLocator for service retrieval. The GlyphRun creation was refactored in SymbolsControl.cs.
Introduced Avalonia threading in ConsoloniaPlatform, and bound to a new ManagedDispatcherImpl. Moreover, text shaping has been improved in SymbolsControl by replacing the default measure with an enumerable list of GlyphInfo. Some code has been commented out in ApplicationStartup for later consideration.
Removed dependency on IPlatformRenderInterface in several methods of ConsoloniaRenderInterface and adjusted corresponding logic. Improved DrawGlyphRun method's implementation in DrawingContextImpl, modifying glyphRun checks and string drawing. Made small adjustments to classes MoveConsoleCaretToPositionBrush and FourBitColorBrush for better alignment with IImmutableBrush.
The transform in the DrawingContextImpl.cs was updated to be initialized with Matrix identity. Removed the unused methods related to geometry and rendering in ConsoloniaRenderInterface.cs. Made a small adjustment to exception message in RenderTarget.cs for clarity.
…to tomlm/crayon
WalkthroughThe changes in this pull request encompass multiple modifications across various files within the Consolonia project. Key updates include the introduction of a new Changes
Poem
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: 35
🧹 Outside diff range and nitpick comments (42)
src/Consolonia.Core/Drawing/PixelBufferImplementation/SimpleSymbol.cs (3)
Line range hint
5-10: Add XML documentation to improve code clarity.Consider adding XML documentation to describe the purpose of this struct and its constructor parameters. This will help other developers understand how to use this type correctly.
+/// <summary> +/// Represents a simple character symbol in the console buffer. +/// </summary> public readonly struct SimpleSymbol : ISymbol { + /// <summary> + /// Initializes a new instance of the <see cref="SimpleSymbol"/> struct. + /// </summary> + /// <param name="character">The character to represent.</param> public SimpleSymbol(char character) { _character = character; }
Line range hint
19-22: Fix incorrect whitespace detection logic.The current implementation checks for
char.MinValuewhich is not the correct way to detect whitespace. This could lead to bugs in whitespace handling. Usechar.IsWhiteSpace()instead for proper Unicode whitespace detection.public bool IsWhiteSpace() { - return _character == char.MinValue; + return char.IsWhiteSpace(_character); }
Line range hint
5-28: Consider implementing equality comparison.Since this is a value type used in a buffer implementation, it would benefit from proper equality comparison support. This would improve performance when comparing symbols and make the type more robust.
-public readonly struct SimpleSymbol : ISymbol +public readonly struct SimpleSymbol : ISymbol, IEquatable<SimpleSymbol> { // ... existing code ... + + public bool Equals(SimpleSymbol other) + { + return _character == other._character; + } + + public override bool Equals(object? obj) + { + return obj is SimpleSymbol other && Equals(other); + } + + public override int GetHashCode() + { + return _character.GetHashCode(); + } + + public static bool operator ==(SimpleSymbol left, SimpleSymbol right) + { + return left.Equals(right); + } + + public static bool operator !=(SimpleSymbol left, SimpleSymbol right) + { + return !left.Equals(right); + } }src/Tests/Consolonia.GalleryTests/TextBlockTests.cs (1)
Line range hint
13-21: Add test coverage for new features.The test suite should be expanded to cover the newly added features from this PR:
- RGB colors support
- Text decorations (Underline)
- FontWeight and FontStyle properties
Would you like me to help generate additional test cases for these features?
src/Consolonia.Core/Infrastructure/IConsole.cs (1)
23-23: Consider performance optimization for styled text rendering.The new styling capabilities (RGB colors, font styles, weights, and decorations) might impact rendering performance, especially with frequent updates or large text blocks. Consider:
- Implementing a caching mechanism for commonly used style combinations
- Batch rendering operations where possible
- Adding a simpler overload for basic text printing without styling
src/Consolonia.Gallery/Gallery/GalleryViews/SomeDialogWindow.axaml (1)
Line range hint
13-28: Consider adding text decorations for better visual hierarchy.Since this PR adds support for text decorations, consider enhancing the ASCII art presentation with appropriate text decorations or font weights to improve visual appeal.
- Foreground="DarkRed">. + Foreground="DarkRed" + FontWeight="Bold" + TextDecorations="Underline">.src/Consolonia.Themes.TurboVision/Templates/Controls/Popup.axaml (1)
13-16: Consider removing the commented-out code block.The commented Border element appears to be obsolete:
- It uses an undefined namespace prefix 'notImportant'
- Contains a hardcoded color instead of following the theme-based approach
- If this code is no longer needed, it should be removed to maintain code cleanliness
src/Consolonia.Core/Text/TextShaper.cs (1)
Wide character handling needs improvement and referenced issue is still open
The referenced GitHub issue (#2) "Character ↓ is determined as wide while being not wide" is still open and active. The current implementation replaces wide characters with '?' which affects text rendering quality. The codebase lacks proper wide character width calculation utilities, making this a valid concern.
src/Consolonia.Core/Text/TextShaper.cs: Current implementation uses a basic IsWideChar check and replaces wide characters with '?'src/Consolonia.Core/Infrastructure/InputLessDefaultNetConsole.cs: Same wide character handling limitation is present🔗 Analysis chain
Line range hint
31-35: Improve wide character handling implementation.Currently, wide characters that are letters or digits are replaced with '?', which could lead to poor user experience for non-ASCII text. The TODO comment references an issue for proper wide character support.
Let's check the status of the referenced issue:
Consider implementing proper wide character support using one of these approaches:
- Use a dedicated Unicode width calculation library
- Implement width calculation based on Unicode character properties
- Use platform-specific console information to determine character width
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if the issue is still open and relevant gh issue view 2 --repo SlimeNull/NullLib.ConsoleEx 2>/dev/null || echo "Issue not accessible or doesn't exist"Length of output: 399
Script:
#!/bin/bash # Let's check the current implementation of wide character handling ast-grep --pattern 'IsWideChar($_) { $$$ }' # Also check for any related configuration or constants rg -A 5 "IsWideChar|WideChar"Length of output: 1816
Script:
#!/bin/bash # Let's check the ConsoleText class implementation to understand the IsWideChar method ast-grep --pattern 'class ConsoleText { $$$ }' # Also check for any existing wide character handling utilities rg -l "WideCharTable|EastAsianWidth|CharWidth"Length of output: 99
🧰 Tools
🪛 GitHub Check: build
[warning] 17-17:
"[ConstantNullCoalescingCondition] '??' left operand is never null" on /home/runner/work/Consolonia/Consolonia/src/Consolonia.Core/Text/TextShaper.cs(17,37)src/Consolonia.Themes.TurboVision/Themes/TurboVisionDark/TurboVisionDarkColors.axaml (2)
9-18: LGTM: UI element colors updated with good contrast.The UI element colors maintain good contrast while enabling future RGB color flexibility. Consider adding a comment documenting the color choices and their intended use cases.
Add a comment block explaining the color scheme:
+ <!-- UI Element Colors + ThemeChooserBackgroundBrush: Used for selection menus + ThemeActionBackgroundBrush: Used for interactive elements + ThemeSelectionBackgroundBrush/ForegroundBrush: Used for selected items + -->
5-28: Consider grouping related colors for better maintainability.While the color definitions are correct, consider organizing them into logical groups using comments or spacing to improve maintainability:
- Basic theme colors (foreground/background)
- Interactive UI elements (chooser/action/selection)
- Decorative elements (border/shade)
- Status indicators (error)
This organization would make it easier to maintain and extend the theme in the future while keeping related colors together.
src/Consolonia.Themes.TurboVision/Themes/TurboVisionBlack/TurboVisionBlackColors.axaml (1)
5-28: Excellent color harmony across the theme.The transition to
ConsoleBrushsuccessfully maintains the classic Turbo Vision aesthetic while enabling full RGB color support. The color choices create a cohesive theme with clear visual hierarchy and good contrast ratios.Consider documenting the color palette and its usage patterns in a style guide to ensure consistency as the application grows.
src/Consolonia.Themes.TurboVision/Templates/TurboVisionDefaultColors.axaml (1)
7-14: LGTM! Transition to ConsoleBrush enhances color support.The switch from FourBitColorBrush to ConsoleBrush aligns with the PR objective of supporting full RGB colors. The ForestGreen color choice for ThemeActionBackgroundBrush better matches the classic TurboVision theme.
Consider fixing the inconsistent indentation in the
Colorattributes:<drawing:ConsoleBrush x:Key="ThemeForegroundBrush" - Color="Black" /> + Color="Black" />src/Consolonia.Gallery/Gallery/GalleryViews/GalleryListBox.axaml (2)
Line range hint
3-3: Consider removing unused namespace.The
drawingnamespace import appears to be unused after removingFourBitColorBrushreferences.- xmlns:drawing="clr-namespace:Consolonia.Core.Drawing;assembly=Consolonia.Core"
Line range hint
21-21: Update comment to reflect actual styling.The comment mentions "highlighted" items, but we're now using specific colors (Red and Blue). Consider updating the description to be more precise.
- <TextBlock>Each 5th item is highlighted with nth-child(5n+3) and nth-last-child(5n+4) rules.</TextBlock> + <TextBlock>Items are colored using nth-child(3) (Red) and nth-last-child(4) (Blue) rules.</TextBlock>src/Consolonia.Themes.TurboVision/Templates/Controls/CaretControl.axaml (1)
22-22: Consider implementing a proper solution for CheckBox redraw.The comment indicates this TextBlock is a hack to force Line redrawing in CheckBox. Consider investigating and implementing a proper solution to handle the redraw mechanism correctly.
Would you like me to help investigate alternative approaches to handle the CheckBox redraw mechanism without requiring this hack?
src/Consolonia.Gallery/Gallery/GalleryViews/GalleryGradientBrush.xaml.cs (3)
12-13: Add XML documentation for the public class.Since this is a public class, consider adding XML documentation to describe its purpose and usage.
[GalleryOrder(1000)] +/// <summary> +/// A user control that demonstrates different types of gradient brushes in the Consolonia Gallery. +/// </summary> public partial class GalleryGradientBrush : UserControl
20-23: Consider adding null check for XAML loading.The XAML loader could potentially throw if the XAML file is missing or corrupted.
private void InitializeComponent() { - AvaloniaXamlLoader.Load(this); + try + { + AvaloniaXamlLoader.Load(this); + } + catch (Exception ex) + { + throw new InvalidOperationException("Failed to load XAML component.", ex); + } }
25-70: Consider consolidating click handlers using an enum-based approach.The three click handlers follow a similar pattern and could be consolidated into a single method to reduce code duplication and improve maintainability.
Here's a suggested approach:
private enum GradientType { Linear, Radial, Conic } private void ApplyGradient(GradientType type) { var grid = this.FindControl<Grid>("MyGrid") ?? throw new InvalidOperationException("MyGrid not found in the visual tree"); grid.Background = type switch { GradientType.Linear => CreateLinearGradient(), GradientType.Radial => CreateRadialGradient(), GradientType.Conic => CreateConicGradient(), _ => throw new ArgumentOutOfRangeException(nameof(type)) }; } private void Gradient_Click(object sender, Avalonia.Interactivity.RoutedEventArgs e) { if (Enum.TryParse<GradientType>(((Button)sender).Name.Replace("_Click", ""), out var type)) ApplyGradient(type); }This approach would:
- Reduce code duplication
- Make it easier to add new gradient types
- Centralize the null checking logic
- Make the code more maintainable
src/Consolonia.Gallery/Gallery/GalleryViews/GalleryTextBlock.axaml (1)
14-14: Consider readability in color combinations.While this effectively demonstrates the new RGB color capabilities, the combination of Chartreuse on DarkMagenta might be challenging to read. Consider adding an example with more readable color combinations alongside this demonstration.
src/Consolonia.Gallery/Gallery/GalleryViews/GalleryComboBox.axaml (1)
50-50: LGTM! Consider RGB values for precise control.The transition from FourBitColorBrush to direct color value aligns with the PR objectives. While the named color "Red" works well for this gallery example, remember that you now have the flexibility to use precise RGB values if needed (e.g., "#FF0000" or "rgb(255,0,0)").
src/Consolonia.Themes.TurboVision/Templates/Controls/Helpers/SymbolsControl.cs (3)
40-40: Remove extra blank line for consistent spacing.The extra blank line creates inconsistent spacing with surrounding code.
- private string _text;
Line range hint
71-83: Add proper resource cleanup and null safety.The Text property setter has several potential issues:
- Memory leak: Previous
_shapedTextis not disposed- No bounds check before accessing Text[0] in Fill mode
- Hardcoded font size limits flexibility
Consider this safer implementation:
set { + var oldShapedText = _shapedText; _text = value; _shapedText = new GlyphRun(new GlyphTypeface(), - 1, + FontSize, // Add FontSize property (_text ?? string.Empty).AsMemory(), TextShaper.Convert(_text ?? string.Empty).ToImmutableArray(), default(Point)); + oldShapedText?.Dispose(); }
Line range hint
14-24: Add support for text decorations and font properties.To align with the PR objectives of supporting text decorations and font properties, consider adding:
- Text decoration support (Underline)
- Font weight support
- Font style support
Add these properties similar to how TextBlock implements them:
public static readonly StyledProperty<TextDecorationCollection> TextDecorationsProperty = TextBlock.TextDecorationsProperty.AddOwner<SymbolsControl>(); public static readonly StyledProperty<FontWeight> FontWeightProperty = TextBlock.FontWeightProperty.AddOwner<SymbolsControl>(); public static readonly StyledProperty<FontStyle> FontStyleProperty = TextBlock.FontStyleProperty.AddOwner<SymbolsControl>(); public TextDecorationCollection TextDecorations { get => GetValue(TextDecorationsProperty); set => SetValue(TextDecorationsProperty, value); } public FontWeight FontWeight { get => GetValue(FontWeightProperty); set => SetValue(FontWeightProperty, value); } public FontStyle FontStyle { get => GetValue(FontStyleProperty); set => SetValue(FontStyleProperty, value); }Then update the GlyphRun creation to use these properties.
src/Consolonia.Gallery/Gallery/GalleryViews/GalleryTextBox.axaml (1)
60-61: LGTM! Consider showcasing RGB color capability.The transition from FourBitColorBrush to direct color values looks good and aligns with the project's direction. Since this PR adds support for full RGB colors, you might consider adding another example that demonstrates an RGB color value (e.g., "#FF1493") to showcase this new capability.
Example enhancement:
SelectionBrush="Magenta" SelectionForegroundBrush="Yellow" /> + <TextBox Text="RGB selection brush" + SelectionStart="0" + SelectionEnd="8" + SelectionBrush="#FF1493" + SelectionForegroundBrush="#FFD700" />src/Consolonia.Themes.TurboVision/Templates/Controls/Helpers/ConsoloniaTextPresenter.cs (3)
61-64: LGTM! Consider adding XML documentation.The brush conversion and validation logic is well-implemented. The error message clearly explains the transparency requirement.
Consider adding XML documentation to explain why the caret brush must have a transparent background:
+ /// <summary> + /// Validates that the CaretBrush has a transparent background, which is required for proper + /// rendering of the caret over text content without obscuring it. + /// </summary> + /// <exception cref="NotSupportedException">Thrown when the brush's background mode is not transparent.</exception> ConsoleBrush brush = ConsoleBrush.FromBrush(args.NewValue.Value, PixelBackgroundMode.Transparent);
77-77: Enhance the inline comment to explain the custom caret requirement.The initialization is correct, but the comment could better explain why custom caret drawing is needed.
- CaretBrush = new ConsoleBrush(Colors.Black, PixelBackgroundMode.Transparent); // we want to draw own caret + CaretBrush = new ConsoleBrush(Colors.Black, PixelBackgroundMode.Transparent); // Custom caret drawing is required to support console-specific rendering and avoid conflicts with the default WPF caret
Line range hint
71-74: Address the timer disabling hack and exception throwing.The current implementation has several issues:
- Setting maximum interval is a workaround that doesn't properly disable the timer
- Throwing an exception in the tick handler could crash the application
- The TODO comment doesn't provide enough context
Consider these alternatives:
- Extend TextPresenter to expose a way to disable the timer
- Use reflection to stop the timer completely:
- var caretTickTimer = (DispatcherTimer)TickTimerField.GetValue(this); - caretTickTimer!.Interval = - TimeSpan.FromMilliseconds(int - .MaxValue); //see DispatcherTimer.Interval, since we can not disable it, setting it to the longest interval possible - caretTickTimer!.Tick += (_, _) => throw new NotImplementedException("How to disable timer completely?"); + // Disable the caret timer since we rely on terminal's native caret blinking + var caretTickTimer = (DispatcherTimer)TickTimerField.GetValue(this); + if (caretTickTimer != null) + { + caretTickTimer.Stop(); + // Clear any existing handlers to prevent memory leaks + caretTickTimer.Tick -= caretTickTimer.Tick; + }
- Create an issue to track proper timer handling in the base TextPresenter
Would you like me to create a GitHub issue to track the proper implementation of timer handling?
src/Consolonia.Core/Drawing/ConsoloniaRenderInterface.cs (1)
Line range hint
128-131: Consider improving the exception message.The current message "Investigate this cases" is vague. Consider providing more context about why non-null
graphicsApiContextcases need investigation and what developers should do when encountering this scenario.- throw new NotImplementedException("Investigate this cases"); + throw new NotImplementedException("Graphics API contexts are not yet supported. Please use null context for console rendering.");src/Tests/Consolonia.TestsCore/PixelTests.cs (2)
31-65: Improve test readability and maintainability.The relative color tests could benefit from:
- Documentation explaining the expected behavior
- Constants or helper methods to reduce duplication
- Clearer test names describing the scenarios
Example improvement:
+ /// <summary> + /// Tests that colors shade towards the background color: + /// - When foreground is lighter than background, it should get darker + /// - When foreground is darker than background, it should get lighter + /// </summary> [Test] - public void TestRelativeShade() + public void Shade_ShouldMoveColorTowardsBackground() { - Color foreground = Colors.LightGray; - Color background = Colors.DarkGray; + // Helper method to verify color movement + static void AssertColorMovement(Color original, Color result, bool expectLighter) + { + var comparison = expectLighter ? Is.GreaterThan : Is.LessThan; + Assert.That(result.R, comparison(original.R), "Red component"); + Assert.That(result.G, comparison(original.G), "Green component"); + Assert.That(result.B, comparison(original.B), "Blue component"); + } + // Test 1: Lighter foreground should get darker + var (foreground, background) = (Colors.LightGray, Colors.DarkGray); var newColor = foreground.Shade(background); - Assert.That(newColor.R < foreground.R); - Assert.That(newColor.G < foreground.G); - Assert.That(newColor.B < foreground.B); + AssertColorMovement(foreground, newColor, expectLighter: false);
1-133: Consider extracting common test utilities to a shared test helper.While the test coverage is good, consider creating a
ColorTestHelperclass to:
- Share common color assertions and utilities across test files
- Provide standard test data sets for color manipulation
- Implement common color distance and comparison functions
This will be particularly valuable as more color-related features are added to the framework.
src/Consolonia.Gallery/Gallery/GalleryViews/GalleryAnimatedLines.axaml (1)
Line range hint
7-120: Consider documenting animation performance impactWhile the animations are well-structured, it would be beneficial to document any performance considerations when running multiple concurrent animations in a console environment, especially with the new color system.
src/Tests/Consolonia.TestsCore/UnitTestConsole.cs (1)
56-65: Consider adding parameter validationWhile the implementation correctly supports the new color and text decoration features, consider adding validation for the input parameters to ensure robustness:
- Validate that
stris not null- Ensure
bufferPointcoordinates are within boundsvoid IConsole.Print(PixelBufferCoordinate bufferPoint, Color background, Color foreground, FontStyle style, FontWeight weight, TextDecorationCollection textDecorations, string str) { + if (str == null) + throw new ArgumentNullException(nameof(str)); + + if (bufferPoint.X >= _size.Width || bufferPoint.Y >= _size.Height) + throw new ArgumentOutOfRangeException(nameof(bufferPoint)); + (ushort x, ushort y) = bufferPoint;src/Consolonia.PlatformSupport/WindowsConsole.cs (1)
175-178: Document the rationale for handling combined MouseMoved & DoubleClick events.While the implementation is correct, it would be helpful to add a comment explaining why this specific combination of events requires special handling. This will help future maintainers understand the edge case being addressed.
+ // Handle edge case where mouse movement occurs during double-click + // We need to raise both events to ensure proper event sequencing case WindowsConsole.EventFlags.MouseMoved | WindowsConsole.EventFlags.DoubleClick: RaiseMouseEvent(RawPointerEventType.LeftButtonDown, point, null, inputModifiers); RaiseMouseEvent(RawPointerEventType.Move, point, null, inputModifiers); return false;src/Consolonia.Gallery/Gallery/GalleryViews/GalleryColors.axaml (1)
1-16: LGTM! Consider adding x:DataType for better type checking.The namespace declarations and resource definitions are well-structured. The LinearGradientBrush demonstrates the new color capabilities nicely.
Consider adding
x:DataTypeto enable compile-time checking of data bindings:<UserControl xmlns="https://github.com/avaloniaui" xmlns:x="http://schemas.microsoft.com/winfx/2006/xaml" xmlns:d="http://schemas.microsoft.com/expression/blend/2008" xmlns:mc="http://schemas.openxmlformats.org/markup-compatibility/2006" xmlns:drawing="clr-namespace:Consolonia.Core.Drawing;assembly=Consolonia.Core" + xmlns:galleryViews="clr-namespace:Consolonia.Gallery.Gallery.GalleryViews" mc:Ignorable="d" d:DesignWidth="800" d:DesignHeight="450" + x:DataType="galleryViews:GalleryColors" x:Class="Consolonia.Gallery.Gallery.GalleryViews.GalleryColors">src/Consolonia.Gallery/Gallery/GalleryViews/GalleryDataGrid.axaml.cs (1)
Line range hint
666-673: Fix property change notification in IsBanned property.The property change notification is using the backing field name (
_isBanned) instead of the property name (IsBanned). This could break UI updates and data binding.Apply this fix:
public bool IsBanned { get => _isBanned; init { _isBanned = value; - OnPropertyChanged(nameof(_isBanned)); + OnPropertyChanged(nameof(IsBanned)); } }src/Consolonia.Core/Drawing/PixelBufferImplementation/PixelForeground.cs (1)
20-20: Reminder: Address the TODO comment in theSymbolpropertyThere is a TODO comment indicating that a reference is needed for the
Symbolproperty:// now working with 16 bit unicode only for simplicity //todo: reference herePlease provide the necessary reference or documentation to complete this section.
Would you like assistance in adding the appropriate reference or opening a GitHub issue to track this task?
src/Consolonia.Core/Drawing/PixelBufferImplementation/Pixel.cs (2)
75-75: Remove redundant 'this.' qualifiersIn lines 75 and 79, the use of the
this.qualifier is unnecessary because there is no ambiguity with local variables or parameters. Removing these qualifiers can make the code cleaner and more concise.Apply this diff to remove the redundant qualifiers:
if (pixelAbove.Foreground.Symbol is DrawingBoxSymbol box && - this.Foreground.Symbol is SimpleSymbol simpleSymbol && + Foreground.Symbol is SimpleSymbol simpleSymbol && simpleSymbol.GetCharacter() != (Char)0) { // this is a line being drawn through text. add TextDecoration for underline. - newForeground = new PixelForeground(this.Foreground.Symbol, this.Foreground.Weight, this.Foreground.Style, TextDecorations.Underline, this.Foreground.Color); + newForeground = new PixelForeground(Foreground.Symbol, Foreground.Weight, Foreground.Style, TextDecorations.Underline, Foreground.Color); }Also applies to: 79-79
🧰 Tools
🪛 GitHub Check: build
[notice] 75-75:
"[ArrangeThisQualifier] Qualifier 'this.' is redundant" on /home/runner/work/Consolonia/Consolonia/src/Consolonia.Core/Drawing/PixelBufferImplementation/Pixel.cs(75,25)
76-76: Remove unnecessary cast toISymbolIn line 76, the cast to
(ISymbol)is redundant becausesimpleSymbolis already of typeSimpleSymbol, which implementsISymbol. Removing the unnecessary cast simplifies the code.Apply this diff to remove the unnecessary cast:
- ((ISymbol)simpleSymbol).GetCharacter() != (Char)0) + simpleSymbol.GetCharacter() != (Char)0)src/Consolonia.Core/Drawing/PixelBufferImplementation/PixelOperations.cs (2)
88-88: Remove redundant parenthesesThe return statement contains unnecessary parentheses that can be removed to improve readability.
Apply this change:
-return (0.299 * color.R + 0.587 * color.G + 0.114 * color.B); +return 0.299 * color.R + 0.587 * color.G + 0.114 * color.B;🧰 Tools
🪛 GitHub Check: build
[notice] 88-88:
"[ArrangeRedundantParentheses] Redundant parentheses" on /home/runner/work/Consolonia/Consolonia/src/Consolonia.Core/Drawing/PixelBufferImplementation/PixelOperations.cs(88,20)
42-42: Remove redundant parentheses in conditionsThere are redundant parentheses in your conditional statements that can be removed to clean up the code.
Apply these changes:
For line 42:
-if (foregroundBrightness > backgroundBrightness || (foregroundBrightness == backgroundBrightness && foregroundBrightness > 0)) +if (foregroundBrightness > backgroundBrightness || foregroundBrightness == backgroundBrightness && foregroundBrightness > 0)For line 70:
-if (foregroundBrightness > backgroundBrightness || (foregroundBrightness == backgroundBrightness && foregroundBrightness < Byte.MaxValue)) +if (foregroundBrightness > backgroundBrightness || foregroundBrightness == backgroundBrightness && foregroundBrightness < Byte.MaxValue)Also applies to: 70-70
🧰 Tools
🪛 GitHub Check: build
[notice] 42-42:
"[ArrangeRedundantParentheses] Redundant parentheses" on /home/runner/work/Consolonia/Consolonia/src/Consolonia.Core/Drawing/PixelBufferImplementation/PixelOperations.cs(42,64)
[warning] 42-42:
"[CompareOfFloatsByEqualityOperator] Equality comparison of floating point numbers. Possible loss of precision while rounding values" on /home/runner/work/Consolonia/Consolonia/src/Consolonia.Core/Drawing/PixelBufferImplementation/PixelOperations.cs(42,65)src/Consolonia.Core/Drawing/RenderTarget.cs (1)
197-198: Remove commented code for clarityAt line 197, the inline comment
/*|| character is '保' or '哥'*/appears unnecessary and may cause confusion. Removing unused or commented-out code improves readability and maintainability.Apply this diff to remove the commented code:
- if (char.IsControl(character) /*|| character is '保' or '哥'*/) + if (char.IsControl(character))src/Consolonia.Core/Drawing/DrawingContextImpl.cs (1)
398-404: Clean up commented code or implement newline handling.The case for the newline character
'\n'contains commented-out code and currently does not perform any action. If newline handling is not required here, consider removing the commented code to maintain code clarity. Otherwise, if handling newlines is necessary, please implement the appropriate logic.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (52)
src/Consolonia.Core/Consolonia.Core.csproj(1 hunks)src/Consolonia.Core/Drawing/ConsoleBrush.cs(1 hunks)src/Consolonia.Core/Drawing/ConsoloniaRenderInterface.cs(1 hunks)src/Consolonia.Core/Drawing/DrawingContextImpl.cs(8 hunks)src/Consolonia.Core/Drawing/FourBitColorBrush.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/PixelForeground.cs(1 hunks)src/Consolonia.Core/Drawing/PixelBufferImplementation/PixelOperations.cs(1 hunks)src/Consolonia.Core/Drawing/PixelBufferImplementation/SimpleSymbol.cs(1 hunks)src/Consolonia.Core/Drawing/RenderTarget.cs(8 hunks)src/Consolonia.Core/Infrastructure/IConsole.cs(2 hunks)src/Consolonia.Core/Infrastructure/InputLessDefaultNetConsole.cs(3 hunks)src/Consolonia.Core/Text/FontManagerImpl.cs(1 hunks)src/Consolonia.Core/Text/GlyphRunImpl.cs(1 hunks)src/Consolonia.Core/Text/GlyphTypeface.cs(2 hunks)src/Consolonia.Core/Text/TextShaper.cs(1 hunks)src/Consolonia.Gallery/Consolonia.Gallery.csproj(1 hunks)src/Consolonia.Gallery/Gallery/GalleryViews/GalleryAnimatedLines.axaml(3 hunks)src/Consolonia.Gallery/Gallery/GalleryViews/GalleryButton.axaml(1 hunks)src/Consolonia.Gallery/Gallery/GalleryViews/GalleryColors.axaml(1 hunks)src/Consolonia.Gallery/Gallery/GalleryViews/GalleryColors.axaml.cs(1 hunks)src/Consolonia.Gallery/Gallery/GalleryViews/GalleryComboBox.axaml(1 hunks)src/Consolonia.Gallery/Gallery/GalleryViews/GalleryDataGrid.axaml.cs(2 hunks)src/Consolonia.Gallery/Gallery/GalleryViews/GalleryDialog.axaml(2 hunks)src/Consolonia.Gallery/Gallery/GalleryViews/GalleryGradientBrush.axaml(1 hunks)src/Consolonia.Gallery/Gallery/GalleryViews/GalleryGradientBrush.xaml.cs(1 hunks)src/Consolonia.Gallery/Gallery/GalleryViews/GalleryListBox.axaml(1 hunks)src/Consolonia.Gallery/Gallery/GalleryViews/GalleryTextBlock.axaml(1 hunks)src/Consolonia.Gallery/Gallery/GalleryViews/GalleryTextBox.axaml(1 hunks)src/Consolonia.Gallery/Gallery/GalleryViews/GalleryWelcome.axaml(1 hunks)src/Consolonia.Gallery/Gallery/GalleryViews/SomeDialogWindow.axaml(1 hunks)src/Consolonia.Gallery/View/ControlsListView.axaml(1 hunks)src/Consolonia.PlatformSupport/WindowsConsole.cs(4 hunks)src/Consolonia.Themes.TurboVision/Templates/Controls/CalendarItem.axaml(3 hunks)src/Consolonia.Themes.TurboVision/Templates/Controls/CaretControl.axaml(1 hunks)src/Consolonia.Themes.TurboVision/Templates/Controls/DataGrid.axaml(1 hunks)src/Consolonia.Themes.TurboVision/Templates/Controls/DialogWindow.axaml(1 hunks)src/Consolonia.Themes.TurboVision/Templates/Controls/DialogWrap.axaml(1 hunks)src/Consolonia.Themes.TurboVision/Templates/Controls/Helpers/AreIntegersEqualConverter.cs(1 hunks)src/Consolonia.Themes.TurboVision/Templates/Controls/Helpers/ConsoloniaTextPresenter.cs(2 hunks)src/Consolonia.Themes.TurboVision/Templates/Controls/Helpers/SymbolsControl.cs(1 hunks)src/Consolonia.Themes.TurboVision/Templates/Controls/Popup.axaml(1 hunks)src/Consolonia.Themes.TurboVision/Templates/Controls/TextBlock.axaml(1 hunks)src/Consolonia.Themes.TurboVision/Templates/TurboVisionDefaultColors.axaml(1 hunks)src/Consolonia.Themes.TurboVision/Themes/TurboVisionBlack/TurboVisionBlackColors.axaml(1 hunks)src/Consolonia.Themes.TurboVision/Themes/TurboVisionDark/TurboVisionDarkColors.axaml(1 hunks)src/Example/Views/DataGridTestWindow.axaml(2 hunks)src/Tests/Consolonia.GalleryTests/TextBlockTests.cs(1 hunks)src/Tests/Consolonia.TestsCore/Consolonia.TestsCore.csproj(1 hunks)src/Tests/Consolonia.TestsCore/PixelTests.cs(1 hunks)src/Tests/Consolonia.TestsCore/UnitTestConsole.cs(2 hunks)
💤 Files with no reviewable changes (1)
- src/Consolonia.Core/Drawing/FourBitColorBrush.cs
✅ Files skipped from review due to trivial changes (3)
- src/Consolonia.Gallery/Gallery/GalleryViews/GalleryDialog.axaml
- src/Consolonia.Gallery/View/ControlsListView.axaml
- src/Consolonia.Themes.TurboVision/Templates/Controls/TextBlock.axaml
🧰 Additional context used
📓 Learnings (3)
src/Consolonia.Themes.TurboVision/Templates/TurboVisionDefaultColors.axaml (1)
Learnt from: tomlm
PR: jinek/Consolonia#122
File: src/Consolonia.Themes.TurboVision/Themes/TurboVisionDark/TurboVisionDarkColors.axaml:5-28
Timestamp: 2024-10-30T18:32:14.211Z
Learning: In the project, using `Mode="Shaded"` in `ConsoleBrush` within theme files is intentional and does not cause runtime issues.
src/Consolonia.Themes.TurboVision/Themes/TurboVisionBlack/TurboVisionBlackColors.axaml (1)
Learnt from: tomlm
PR: jinek/Consolonia#122
File: src/Consolonia.Themes.TurboVision/Themes/TurboVisionDark/TurboVisionDarkColors.axaml:5-28
Timestamp: 2024-10-30T18:32:14.211Z
Learning: In the project, using `Mode="Shaded"` in `ConsoleBrush` within theme files is intentional and does not cause runtime issues.
src/Consolonia.Themes.TurboVision/Themes/TurboVisionDark/TurboVisionDarkColors.axaml (1)
Learnt from: tomlm
PR: jinek/Consolonia#122
File: src/Consolonia.Themes.TurboVision/Themes/TurboVisionDark/TurboVisionDarkColors.axaml:5-28
Timestamp: 2024-10-30T18:32:14.211Z
Learning: In the project, using `Mode="Shaded"` in `ConsoleBrush` within theme files is intentional and does not cause runtime issues.
🪛 GitHub Check: build
src/Consolonia.Core/Drawing/ConsoleBrush.cs
[warning] 189-189:
"[PossibleNullReferenceException] Possible 'System.NullReferenceException'" on /home/runner/work/Consolonia/Consolonia/src/Consolonia.Core/Drawing/ConsoleBrush.cs(189,24)
src/Consolonia.Core/Drawing/DrawingContextImpl.cs
[warning] 145-145:
"[AccessToModifiedClosure] Captured variable is modified in the outer scope" on /home/runner/work/Consolonia/Consolonia/src/Consolonia.Core/Drawing/DrawingContextImpl.cs(145,93)
[warning] 145-145:
"[AccessToModifiedClosure] Captured variable is modified in the outer scope" on /home/runner/work/Consolonia/Consolonia/src/Consolonia.Core/Drawing/DrawingContextImpl.cs(145,96)
src/Consolonia.Core/Drawing/PixelBufferImplementation/Pixel.cs
[warning] 31-31:
"[RedundantArgumentDefaultValue] The parameter 'style' has the same default value" on /home/runner/work/Consolonia/Consolonia/src/Consolonia.Core/Drawing/PixelBufferImplementation/Pixel.cs(31,70)
[warning] 31-31:
"[RedundantArgumentDefaultValue] The parameter 'weight' has the same default value" on /home/runner/work/Consolonia/Consolonia/src/Consolonia.Core/Drawing/PixelBufferImplementation/Pixel.cs(31,88)
[notice] 75-75:
"[ArrangeThisQualifier] Qualifier 'this.' is redundant" on /home/runner/work/Consolonia/Consolonia/src/Consolonia.Core/Drawing/PixelBufferImplementation/Pixel.cs(75,25)
[notice] 79-79:
"[ArrangeThisQualifier] Qualifier 'this.' is redundant" on /home/runner/work/Consolonia/Consolonia/src/Consolonia.Core/Drawing/PixelBufferImplementation/Pixel.cs(79,61)
[notice] 79-79:
"[ArrangeThisQualifier] Qualifier 'this.' is redundant" on /home/runner/work/Consolonia/Consolonia/src/Consolonia.Core/Drawing/PixelBufferImplementation/Pixel.cs(79,85)
[notice] 79-79:
"[ArrangeThisQualifier] Qualifier 'this.' is redundant" on /home/runner/work/Consolonia/Consolonia/src/Consolonia.Core/Drawing/PixelBufferImplementation/Pixel.cs(79,109)
[notice] 79-79:
"[ArrangeThisQualifier] Qualifier 'this.' is redundant" on /home/runner/work/Consolonia/Consolonia/src/Consolonia.Core/Drawing/PixelBufferImplementation/Pixel.cs(79,160)
src/Consolonia.Core/Drawing/PixelBufferImplementation/PixelForeground.cs
[warning] 12-12:
"[ConstantNullCoalescingCondition] '??' left operand is never null" on /home/runner/work/Consolonia/Consolonia/src/Consolonia.Core/Drawing/PixelBufferImplementation/PixelForeground.cs(12,32)
src/Consolonia.Core/Drawing/PixelBufferImplementation/PixelOperations.cs
[notice] 42-42:
"[ArrangeRedundantParentheses] Redundant parentheses" on /home/runner/work/Consolonia/Consolonia/src/Consolonia.Core/Drawing/PixelBufferImplementation/PixelOperations.cs(42,64)
[notice] 70-70:
"[ArrangeRedundantParentheses] Redundant parentheses" on /home/runner/work/Consolonia/Consolonia/src/Consolonia.Core/Drawing/PixelBufferImplementation/PixelOperations.cs(70,64)
[notice] 88-88:
"[ArrangeRedundantParentheses] Redundant parentheses" on /home/runner/work/Consolonia/Consolonia/src/Consolonia.Core/Drawing/PixelBufferImplementation/PixelOperations.cs(88,20)
[warning] 42-42:
"[CompareOfFloatsByEqualityOperator] Equality comparison of floating point numbers. Possible loss of precision while rounding values" on /home/runner/work/Consolonia/Consolonia/src/Consolonia.Core/Drawing/PixelBufferImplementation/PixelOperations.cs(42,65)
[warning] 70-70:
"[CompareOfFloatsByEqualityOperator] Equality comparison of floating point numbers. Possible loss of precision while rounding values" on /home/runner/work/Consolonia/Consolonia/src/Consolonia.Core/Drawing/PixelBufferImplementation/PixelOperations.cs(70,65)
src/Consolonia.Core/Drawing/RenderTarget.cs
[warning] 123-123:
"[BadChildStatementIndent] Line indent is not restored to the previous level around child statement" on /home/runner/work/Consolonia/Consolonia/src/Consolonia.Core/Drawing/RenderTarget.cs(123,1)
src/Consolonia.Core/Text/FontManagerImpl.cs
[notice] 38-38:
"[ArrangeTrailingCommaInMultilineLists] Remove trailing comma to conform to code style" on /home/runner/work/Consolonia/Consolonia/src/Consolonia.Core/Text/FontManagerImpl.cs(38,30)
src/Consolonia.Core/Text/TextShaper.cs
[warning] 17-17:
"[ConstantNullCoalescingCondition] '??' left operand is never null" on /home/runner/work/Consolonia/Consolonia/src/Consolonia.Core/Text/TextShaper.cs(17,37)
src/Consolonia.PlatformSupport/WindowsConsole.cs
[notice] 166-166:
"[ArrangeRedundantParentheses] Redundant parentheses" on /home/runner/work/Consolonia/Consolonia/src/Consolonia.PlatformSupport/WindowsConsole.cs(166,39)
🔇 Additional comments (66)
src/Consolonia.Gallery/Gallery/GalleryViews/GalleryWelcome.axaml (1)
4-6: LGTM! Welcome message styling implemented correctly.
The TextBlock implementation properly demonstrates the new text decoration and font weight features introduced in this PR.
src/Consolonia.Core/Drawing/PixelBufferImplementation/SimpleSymbol.cs (1)
Line range hint 24-27: Verify blending behavior and optimize parameter passing.
The current implementation always returns the symbol above without any blending logic. This seems overly simplistic, especially considering the PR's objectives for enhanced text decorations. Also, the ref parameter is unnecessary since the value isn't modified.
- Consider whether this implementation aligns with the requirements for text decorations and overlapping characters.
- Remove the unnecessary
refmodifier.
-public ISymbol Blend(ref ISymbol symbolAbove)
+public ISymbol Blend(ISymbol symbolAbove)
{
+ if (symbolAbove == null)
+ throw new ArgumentNullException(nameof(symbolAbove));
return symbolAbove;
}src/Consolonia.Core/Consolonia.Core.csproj (1)
8-8: Consider using version variable for consistency.
While the addition of the Crayon package aligns with the PR objectives for RGB color support, consider using a version variable like other Avalonia packages for consistent version management.
Let's verify the package compatibility:
src/Tests/Consolonia.TestsCore/Consolonia.TestsCore.csproj (1)
5-7: LGTM! Good practice setting IsPackable to false.
Setting IsPackable to false is the correct approach for test projects to prevent them from being packaged as NuGet packages.
src/Tests/Consolonia.GalleryTests/TextBlockTests.cs (1)
18-18:
Verify text alignment behavior.
The right-aligned text assertion appears inconsistent with other alignment tests:
- Left aligned: Has "│" prefix
- Right aligned: Only has "│" suffix
- Center aligned: No border characters
This asymmetry might indicate an underlying rendering issue.
Consider standardizing the border characters across all alignment tests:
- "Right aligned text│",
+ "│Right aligned text│",src/Consolonia.Themes.TurboVision/Templates/Controls/DialogWrap.axaml (1)
7-7: LGTM! Consistent with the transition to ConsoleBrush.
The Background property update aligns with the PR's objective of transitioning from FourBitColorBrush to ConsoleBrush while maintaining the shaded mode functionality.
Let's verify the consistency of ConsoleBrush usage across other dialog-related templates:
✅ Verification successful
ConsoleBrush transition is consistent across dialog-related templates
The verification confirms that:
- No remaining
FourBitColorBrushusages were found in any dialog templates - The
ConsoleBrush Mode=Shadedpattern is consistently used in dialog-related components where shaded backgrounds are needed - The change in
DialogWrap.axamlaligns with the broader theme system, which uses a mix ofConsoleBrushand theme-specific brushes for different UI elements
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify ConsoleBrush usage in dialog-related XAML files
# Expected: All dialog-related templates should use ConsoleBrush consistently
# Search for any remaining FourBitColorBrush usage in dialog templates
echo "Checking for any remaining FourBitColorBrush usage in dialog templates..."
fd --extension axaml --exec rg "FourBitColorBrush" {}
# Verify ConsoleBrush usage pattern in dialog templates
echo "Verifying ConsoleBrush usage pattern..."
fd --extension axaml --exec rg "ConsoleBrush Mode=Shaded" {}
Length of output: 511
Script:
#!/bin/bash
# Let's get more context about the ConsoleBrush usage patterns
# First, let's find all XAML files in dialog-related directories
echo "=== Dialog-related XAML files ==="
fd ".*\.axaml$" src/Consolonia.Themes.TurboVision/Templates/Controls/
# Now let's check all Background properties in these files
echo -e "\n=== All Background property usages in dialog templates ==="
fd ".*\.axaml$" src/Consolonia.Themes.TurboVision/Templates/Controls/ --exec rg "Background=\"|Background=\{" -A 1 {}
# Let's also check if there are any other brush types being used
echo -e "\n=== Other brush usages in dialog templates ==="
fd ".*\.axaml$" src/Consolonia.Themes.TurboVision/Templates/Controls/ --exec rg "Brush" {}
Length of output: 30150
src/Consolonia.Core/Infrastructure/IConsole.cs (2)
6-6: LGTM: Required import for enhanced text styling support.
The addition of Avalonia.Media import is necessary to support the new color and text decoration types.
23-23: Verify implementations of the updated Print method.
The signature change from ConsoleColor to Color with additional styling parameters is a breaking change that requires updates to all implementing classes.
Let's verify all implementations have been updated:
✅ Verification successful
Let me check the actual implementations of the Print method in these files to verify if they match the updated signature.
Let me check the UnitTestConsole.cs file content directly since the grep didn't show its Print method implementation.
All implementations have been properly updated to match the new Print method signature
Both implementations of IConsole have been verified:
- InputLessDefaultNetConsole implements the Print method with the new signature including Color, FontStyle, FontWeight, and TextDecorationCollection parameters
- UnitTestConsole also implements the Print method with the exact same updated signature
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all implementations of IConsole interface and verify Print method signature
# Find all implementations of IConsole
echo "Searching for IConsole implementations..."
rg -l "class.*:.*IConsole"
# Find all Print method implementations to verify signature
echo -e "\nVerifying Print method signatures..."
ast-grep --pattern 'void Print(PixelBufferCoordinate $_, Color $_, Color $_, FontStyle $_, FontWeight $_, TextDecorationCollection $_, string $_)'
Length of output: 474
Script:
#!/bin/bash
# Check the Print method implementation in both files
echo "=== InputLessDefaultNetConsole.cs ==="
rg "void Print\(" -A 1 src/Consolonia.Core/Infrastructure/InputLessDefaultNetConsole.cs
echo -e "\n=== UnitTestConsole.cs ==="
rg "void Print\(" -A 1 src/Tests/Consolonia.TestsCore/UnitTestConsole.cs
Length of output: 515
Script:
#!/bin/bash
# Get the full content of UnitTestConsole.cs to verify the Print method implementation
echo "=== UnitTestConsole.cs content ==="
cat src/Tests/Consolonia.TestsCore/UnitTestConsole.cs
Length of output: 6403
src/Consolonia.Gallery/Gallery/GalleryViews/GalleryButton.axaml (1)
14-15: LGTM! Color property updates align with RGB support initiative.
The transition from FourBitColorBrush to direct color values is consistent with the PR's objective of supporting full RGB colors. The color choices maintain visual similarity while leveraging the enhanced color system.
src/Consolonia.Core/Text/GlyphRunImpl.cs (1)
31-31:
Implement proper intersection testing for text decorations and selection.
Returning an empty list instead of throwing NotImplementedException makes the method technically compliant, but it might affect functionality that depends on proper intersection testing, such as:
- Text selection behavior
- Accurate underline placement (which is now supported per PR objectives)
- Mouse interaction features
Let's verify the impact:
Consider implementing proper intersection testing to support:
- Text selection functionality
- Accurate underline rendering
- Mouse interaction features
Would you like me to:
- Generate a proper implementation of
GetIntersections? - Add unit tests to verify the behavior?
src/Consolonia.Gallery/Consolonia.Gallery.csproj (3)
20-24: LGTM: XAML file configuration is correct.
The AvaloniaXaml configuration follows the standard Avalonia project structure with proper MSBuild compilation settings.
26-31: LGTM: Code-behind file configuration is correct.
The code-behind file is properly configured with the correct SubType and DependentUpon properties, establishing the right relationship with its XAML file.
20-31: Verify gallery integration of the new gradient brush component.
The project configuration looks correct, but let's ensure the new gradient brush component is properly integrated into the gallery navigation.
src/Consolonia.Core/Drawing/PixelBufferImplementation/PixelBackground.cs (3)
15-15: LGTM! Property change aligns with new color system
The property type change from ConsoleColor to Color is correctly implemented and maintains immutability.
9-13: Verify all constructor calls are updated
The constructor signature change from ConsoleColor to nullable Color is a breaking change. While the implementation looks good, we should ensure all existing calls are updated.
✅ Verification successful
The previous ast-grep pattern didn't work well, so I'm trying with ripgrep to find constructor calls. I'm also specifically looking in test files since they often contain usage examples. This will help us verify if all constructor calls have been properly updated.
All constructor calls are properly updated
I've reviewed all instances of new PixelBackground() constructor calls in the codebase, and they all follow the new signature pattern correctly:
- They either use
PixelBackgroundModealone (defaulting to Black color) - Or properly provide both
PixelBackgroundModeandColorparameters
No legacy calls using the old ConsoleColor parameter were found, indicating the migration is complete.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for constructor calls to verify they've been updated
ast-grep --pattern 'new PixelBackground($$$)'
Length of output: 364
Script:
#!/bin/bash
# Search for PixelBackground constructor calls using ripgrep
rg "new PixelBackground\(" -A 2
# Also search for any test files that might use PixelBackground
rg "PixelBackground" --type-add 'test:*{Test,Tests,Spec,Specs}.cs' --type test
Length of output: 2368
Line range hint 20-38: Verify Color.Shade() extension method implementation
The code looks good, but we should verify the implementation of the Color.Shade() extension method to ensure it provides the expected behavior for the full RGB color space.
✅ Verification successful
Color.Shade() implementation is well-designed for RGB color space
The Shade() extension method has a robust implementation that:
- Uses perceived brightness calculation based on standard RGB coefficients (0.299R + 0.587G + 0.114B)
- Adjusts colors by a fixed step of 32 for consistent shading
- Handles both darkening and brightening based on background comparison
- Includes proper bounds checking to prevent overflow
- Applies the same adjustment to all RGB components to maintain color relationships
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for the Shade extension method implementation
ast-grep --pattern 'static $_Shade(this Color $_) {
$$$
}'
Length of output: 364
Script:
#!/bin/bash
# Search for the Shade extension method implementation with a more flexible pattern
rg -U "public static.*Shade.*Color" -A 10
Length of output: 2737
Script:
#!/bin/bash
# Let's check the GetPerceivedBrightness method and _colorAdjustment constant
rg -U "GetPerceivedBrightness|_colorAdjustment" -B 2 -A 5 "src/Consolonia.Core/Drawing/PixelBufferImplementation/PixelOperations.cs"
Length of output: 3040
src/Consolonia.Gallery/Gallery/GalleryViews/GalleryGradientBrush.axaml (2)
1-9: Verify the necessity of the drawing namespace import.
The drawing namespace is imported but not used in the XAML. If it's not used in the code-behind file either, consider removing it.
✅ Verification successful
The drawing namespace import is correctly used
The drawing namespace import is necessary as the code-behind file uses Consolonia.Core.Drawing namespace, which is required for the gradient brush implementations in the click handlers.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the drawing namespace is used in the code-behind file
rg -l "drawing\." "src/Consolonia.Gallery/Gallery/GalleryViews/GalleryGradientBrush.xaml.cs"
Length of output: 595
Script:
#!/bin/bash
# Check the code-behind file content to see if drawing namespace is used
rg -A 5 'using.*Drawing|Linear_Click|Radial_Click|Conic_Click' src/Consolonia.Gallery/Gallery/GalleryViews/GalleryGradientBrush.xaml.cs
Length of output: 1296
18-22: 🛠️ Refactor suggestion
Enhance button styling and verify click handlers.
The buttons could benefit from improved styling and we should verify the click handler implementations.
Consider applying these styling improvements:
- <Button Click="Linear_Click" Background="Gray">Linear</Button>
- <Button Click="Radial_Click" Background="Gray">Radial</Button>
- <Button Click="Conic_Click" Background="Gray">Conic</Button>
+ <Button Click="Linear_Click" Classes="accent" Margin="4,0">Linear</Button>
+ <Button Click="Radial_Click" Classes="accent" Margin="4,0">Radial</Button>
+ <Button Click="Conic_Click" Classes="accent" Margin="4,0">Conic</Button>✅ Verification successful
The previous output shows that Linear_Click and Radial_Click are properly implemented, but we need to verify the Conic_Click implementation and check if there are existing button styles before making style recommendations.
The previous output shows that the Conic_Click handler exists and starts setting up a ConicGradientBrush, but we need to see its complete implementation and verify if the "accent" class is used elsewhere in the project before making style recommendations.
Consider using theme-consistent button styling
The click handlers are properly implemented, but the button styling can be improved. Since there's no existing usage of the "accent" class for buttons in the project, let's revise the styling suggestion:
- <Button Click="Linear_Click" Background="Gray">Linear</Button>
- <Button Click="Radial_Click" Background="Gray">Radial</Button>
- <Button Click="Conic_Click" Background="Gray">Conic</Button>
+ <Button Click="Linear_Click" Margin="4,0">Linear</Button>
+ <Button Click="Radial_Click" Margin="4,0">Radial</Button>
+ <Button Click="Conic_Click" Margin="4,0">Conic</Button>- Remove the hard-coded gray background to use the default theme-consistent button styling
- Add margins for better spacing between buttons
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if all click handlers are implemented in the code-behind
rg -A 2 "Linear_Click|Radial_Click|Conic_Click" "src/Consolonia.Gallery/Gallery/GalleryViews/GalleryGradientBrush.xaml.cs"
Length of output: 595
Script:
#!/bin/bash
# Let's check the complete implementation of the Conic_Click handler
rg -A 5 "Conic_Click" "src/Consolonia.Gallery/Gallery/GalleryViews/GalleryGradientBrush.xaml.cs"
# Let's also check if there are any existing button styles or classes defined
rg -l "Button" "src/Consolonia.Gallery/Styles"
Length of output: 482
Script:
#!/bin/bash
# Let's check if there are any existing button styles in the entire project
fd -e axaml -e xaml . | xargs rg -l "Button.*Classes=\"accent\""
# Let's also verify if the Conic_Click implementation is complete
rg -A 10 "Conic_Click" "src/Consolonia.Gallery/Gallery/GalleryViews/GalleryGradientBrush.xaml.cs"
Length of output: 680
src/Consolonia.Themes.TurboVision/Templates/Controls/Popup.axaml (1)
10-10: LGTM! The ConsoleBrush implementation looks correct.
The transparent mode is appropriate for a popup background, and the change aligns with the PR's objective of transitioning from FourBitColorBrush to ConsoleBrush.
src/Consolonia.Core/Text/TextShaper.cs (1)
17-17: TODO comment indicates incomplete RTL support.
The comment /*todo: must be 1 for right to left?*/ suggests that right-to-left text direction support is not fully implemented, which could affect internationalization.
Let's check for other RTL-related TODOs or implementations:
Would you like me to help implement proper RTL support or create an issue to track this requirement?
🧰 Tools
🪛 GitHub Check: build
[warning] 17-17:
"[ConstantNullCoalescingCondition] '??' left operand is never null" on /home/runner/work/Consolonia/Consolonia/src/Consolonia.Core/Text/TextShaper.cs(17,37)
src/Consolonia.Core/Text/FontManagerImpl.cs (2)
35-39: LGTM: Property initialization aligns with text styling support.
The changes to initialize Weight and Style properties align well with the PR's objective to support text decorations and styling. This modification works in conjunction with the changes in GlyphTypeface.cs where these properties were made settable.
🧰 Tools
🪛 GitHub Check: build
[notice] 38-38:
"[ArrangeTrailingCommaInMultilineLists] Remove trailing comma to conform to code style" on /home/runner/work/Consolonia/Consolonia/src/Consolonia.Core/Text/FontManagerImpl.cs(38,30)
35-39: Consider adding validation and error handling.
While the property initialization looks good, there are a few concerns:
- The TODO comment about font validation remains unaddressed.
- The method always returns true without validating the input parameters.
- There's no error handling for invalid weight/style combinations.
Let's check if there are any validation patterns in other typeface-related code:
Remove the trailing comma after Style = style to conform to the code style.
🧰 Tools
🪛 GitHub Check: build
[notice] 38-38:
"[ArrangeTrailingCommaInMultilineLists] Remove trailing comma to conform to code style" on /home/runner/work/Consolonia/Consolonia/src/Consolonia.Core/Text/FontManagerImpl.cs(38,30)
src/Consolonia.Themes.TurboVision/Themes/TurboVisionDark/TurboVisionDarkColors.axaml (2)
5-8: LGTM: Basic theme colors updated to support RGB.
The transition from FourBitColorBrush to ConsoleBrush for the basic theme colors is consistent with the PR's objective to support full RGB colors while maintaining the original TurboVision aesthetic.
19-28: LGTM: Special purpose colors properly configured.
The special purpose colors are correctly configured, including the intentional use of Mode="Shaded" for ThemeShadeBrush. The error and border colors provide good visibility while maintaining theme consistency.
Let's verify the ConsoleBrush implementation supports the shaded mode:
✅ Verification successful
ConsoleBrush implementation properly supports shaded mode
The verification confirms that:
ConsoleBrushhas aModeproperty of typePixelBackgroundModePixelBackgroundModeenum explicitly includes theShadedvalue- The implementation properly handles the mode through the
ModePropertystyled property
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify ConsoleBrush supports shaded mode
ast-grep --pattern 'class ConsoleBrush {
$$$
Mode $$$
$$$
}'
Length of output: 68
Script:
#!/bin/bash
# Let's try a different approach to find ConsoleBrush implementation
fd ConsoleBrush.cs
# Search for Mode property or enum in the codebase
rg -A 5 "Mode\s*{|enum\s+Mode"
Length of output: 208
Script:
#!/bin/bash
# Let's examine the ConsoleBrush implementation
cat src/Consolonia.Core/Drawing/ConsoleBrush.cs
# Fix the regex syntax for Mode search
rg -A 5 "(Mode\s*\{|enum\s+Mode)"
Length of output: 10192
Script:
#!/bin/bash
# Let's find the PixelBackgroundMode enum definition
cat src/Consolonia.Core/Drawing/PixelBufferImplementation/PixelBackgroundMode.cs 2>/dev/null || fd PixelBackgroundMode.cs
Length of output: 254
src/Consolonia.Themes.TurboVision/Themes/TurboVisionBlack/TurboVisionBlackColors.axaml (6)
5-8: LGTM! Basic theme colors provide good contrast.
The gray foreground on black background maintains readability while staying true to the classic Turbo Vision theme.
9-12: LGTM! Special purpose backgrounds use distinct colors.
The DarkBlue and DarkMagenta backgrounds provide clear visual distinction for different UI elements, matching the Turbo Vision style.
13-16: LGTM! Selection colors provide clear visual feedback.
The white text on dark gray background ensures selected items stand out clearly.
17-20: LGTM! Alternative and border colors maintain visual hierarchy.
The subtle use of grays helps organize the UI without being distracting.
21-26: LGTM! Special effect colors enhance UI depth.
The shading effects and notification colors are well chosen. Note that the Mode="Shaded" is intentionally used here for the shadow effect.
27-28: LGTM! Error color follows convention.
The red color provides clear error indication, following standard UI practices.
src/Consolonia.Themes.TurboVision/Templates/TurboVisionDefaultColors.axaml (4)
15-18: LGTM! Selection colors match TurboVision theme.
The selection colors (DarkBlue background with White foreground) maintain the classic TurboVision look while benefiting from the new ConsoleBrush capabilities.
19-24: LGTM! Alternative and border colors provide good contrast.
The color choices for alternative background (White) and borders (DarkGray) maintain visual hierarchy while providing sufficient contrast for readability.
25-32: LGTM! Special effect brushes correctly configured.
The special effect brushes maintain their modes ("Shaded" and "Transparent") in the transition to ConsoleBrush, which is intentional and works as expected at runtime.
7-32: Verify complete transition from FourBitColorBrush.
Let's ensure all theme files have been updated to use ConsoleBrush consistently.
✅ Verification successful
Transition to ConsoleBrush is complete and consistent
The verification shows that:
- No instances of
FourBitColorBrushwere found in any.axamlor.xamlfiles ConsoleBrushis consistently used across all theme files including:- Default theme (TurboVisionDefaultColors.axaml)
- Dark theme (TurboVisionDarkColors.axaml)
- Black theme (TurboVisionBlackColors.axaml)
- Various control templates
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining FourBitColorBrush usage in theme files
# Test 1: Search for any remaining FourBitColorBrush references
echo "Checking for remaining FourBitColorBrush usage:"
rg "FourBitColorBrush" -g "*.axaml" -g "*.xaml"
# Test 2: Verify ConsoleBrush usage in other theme files
echo -e "\nVerifying ConsoleBrush usage in theme files:"
rg "ConsoleBrush" -g "*.axaml" -g "*.xaml"
Length of output: 7049
src/Consolonia.Gallery/Gallery/GalleryViews/GalleryListBox.axaml (1)
9-9: LGTM! Color value changes align with RGB support.
The transition from FourBitColorBrush to direct color values successfully implements the PR objective of supporting full RGB colors.
Let's verify there are no remaining FourBitColorBrush references in XAML files:
Also applies to: 13-13
✅ Verification successful
No remaining FourBitColorBrush references found - RGB color transition is complete
The verification confirms that:
- The specific file correctly uses direct color values ("Red" and "Blue")
- No
FourBitColorBrushreferences were found in any XAML/AXAML files - No
FourBitColorBrushreferences were found in C# code
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining FourBitColorBrush references in XAML files
rg -l "FourBitColorBrush" --glob "*.axaml" --glob "*.xaml"
Length of output: 58
Script:
#!/bin/bash
# Let's try a broader search to find any remaining references to FourBitColorBrush
# First, let's find all XAML/AXAML files
fd "\.(xaml|axaml)$" -t f -x cat {} | grep -i "FourBitColorBrush"
Length of output: 67
Script:
#!/bin/bash
# Let's also check for any C# code that might still be using FourBitColorBrush
rg "FourBitColorBrush" --type cs -C 2
Length of output: 37
Script:
#!/bin/bash
# Let's also verify the specific file mentioned in the review for context
cat "src/Consolonia.Gallery/Gallery/GalleryViews/GalleryListBox.axaml"
Length of output: 2172
src/Consolonia.Core/Text/GlyphTypeface.cs (2)
64-65: Consider adding validation and thread safety measures.
While making these properties settable aligns with the PR's text styling objectives, consider:
- Adding validation in the setters to ensure only valid values are assigned
- Implementing thread-safety measures if these properties can be modified during rendering
Let's verify if these properties are accessed from multiple threads:
76-77: LGTM! Consider additional test cases.
The underline metrics changes are well-documented and align with the text decoration support objectives. The position and thickness values are appropriate for console-based rendering.
Let's verify if there are test cases covering these scenarios:
✅ Verification successful
Text decoration implementation is well-tested and properly integrated
The underline metrics changes in GlyphTypeface.cs are verified to be working correctly based on the following evidence:
- The implementation is used in the Gallery demo (
GalleryTextBlock.axamlandGalleryWelcome.axaml) showcasing underline decorations - The
InputLessDefaultNetConsoleproperly handles underline rendering using Crayon.Output.Underline - The
RenderTargetclass correctly manages text decorations through its caching and rendering pipeline - There's a smart fallback mechanism in
Pixel.csthat converts drawn lines over text into proper underline decorations
The implementation is robust and handles various scenarios including:
- Text decoration state management
- Proper rendering in the console
- Integration with the pixel buffer system
- Demo cases in the gallery application
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for tests related to text decorations and underline rendering
rg -l "Test" | xargs rg "TextDecorations|Underline|GlyphTypeface"
Length of output: 65
Script:
#!/bin/bash
# Let's try a more targeted search for test files and text decoration related code
fd ".*Test.*\.cs$" | xargs rg -l "TextDecorations|Underline|GlyphTypeface"
# Also search for any usage of text decorations in the codebase
rg -i "textdecorations?" -A 3 -B 3
Length of output: 18095
src/Consolonia.Themes.TurboVision/Templates/Controls/CaretControl.axaml (1)
22-22: Verify if this commented-out brush configuration is still needed.
The code shows a commented-out ConsoleBrush configuration. If this is no longer needed, consider removing it entirely. If it's still required, it should be uncommented.
✅ Verification successful
Let me gather more context about this specific hack and its usage.
Let me check the CheckBox implementation and its relationship with CaretControl to understand if this hack is still needed.
Based on the gathered information, I can now provide a final response:
The commented-out brush configuration should remain commented out
The TextBlock with the transparent brush was originally implemented as a hack to force Line redrawing in CheckBox, as indicated by the comment. Looking at the git history and current implementation:
- The hack is still needed as indicated by the preserved comment "This TextBlock is a hack. Otherwise Line below does not get redrawn in CheckBox"
- The background is now properly handled by
Background="{DynamicResource ThemeActionBackgroundBrush}"on the TextBlock - The commented-out section was part of the transition from
FourBitColorBrushtoConsoleBrush, but it's not needed since the background is handled by the dynamic resource
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if there are any references to this hack or similar patterns
# in other files that might indicate if this is still needed
# Search for similar TextBlock hacks in other XAML files
rg -l "TextBlock.*hack.*Line.*redrawn.*CheckBox"
# Search for related CheckBox redraw implementations
ast-grep --pattern 'class $_ : CheckBox {
$$$
}'
Length of output: 175
Script:
#!/bin/bash
# Let's look at the full content of CaretControl.axaml to understand the context
cat src/Consolonia.Themes.TurboVision/Templates/Controls/CaretControl.axaml
# Search for any other files that might be using or referencing CaretControl
rg -l "CaretControl"
# Look for any other similar transparent brush configurations
rg "Mode=\"Transparent\""
Length of output: 4321
Script:
#!/bin/bash
# Check the CheckBox implementation
cat src/Consolonia.Themes.TurboVision/Templates/Controls/CheckBox.axaml
# Check the CaretControl implementation
cat src/Consolonia.Themes.TurboVision/Templates/Controls/Helpers/CaretControl.cs
# Look for any recent changes or commits related to this hack
git log -p src/Consolonia.Themes.TurboVision/Templates/Controls/CaretControl.axaml
Length of output: 16145
src/Consolonia.Gallery/Gallery/GalleryViews/GalleryTextBlock.axaml (1)
16-27: LGTM! Well-structured text manipulation examples.
The examples effectively demonstrate text trimming and alignment capabilities with good visual separation using the dark gray background.
src/Consolonia.Gallery/Gallery/GalleryViews/GalleryComboBox.axaml (1)
57-57: LGTM! Color transition successfully implemented.
The change from FourBitColorBrush to direct color value maintains the same visual appearance while aligning with the new color system implementation.
src/Consolonia.Core/Drawing/ConsoloniaRenderInterface.cs (1)
Line range hint 110-120: LGTM: LoadBitmap implementation follows the class pattern.
The new method maintains consistency with other bitmap-related methods in the class. Given the PR's focus on RGB color support, this interface method will be valuable for future color-related functionality.
src/Example/Views/DataGridTestWindow.axaml (1)
48-50: LGTM! Proper implementation of ConsoleBrush.
The transition from FourBitColorBrush to ConsoleBrush is correctly implemented here, aligning with the PR's objective of enhancing color support.
src/Tests/Consolonia.TestsCore/PixelTests.cs (1)
1-10: LGTM! Well-structured test class setup.
The file is properly organized with appropriate imports and follows testing best practices.
src/Consolonia.Gallery/Gallery/GalleryViews/GalleryAnimatedLines.axaml (3)
7-8: LGTM: Panel dimensions are appropriate
The panel height of 30 units provides adequate space for the animated borders.
88-88: LGTM: Consistent brush styling
The LineBrush configuration maintains consistency with the other border elements while using the simplified color assignment approach.
48-50: Verify color rendering in console environment
The transition to direct color assignment looks good, but we should verify that "Blue" renders correctly in different console environments.
✅ Verification successful
Color usage is consistent and properly supported
The verification shows that:
- No remaining
FourBitColorBrushusages were found - "Blue" color is consistently used across the codebase:
- In the gallery examples (GalleryColors.axaml, GalleryListBox.axaml)
- In theme definitions (TurboVisionDarkColors.axaml)
- In the animated lines implementation (GalleryAnimatedLines.axaml)
The direct color assignment is properly supported by the theming system and follows the same pattern used in other components.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any remaining FourBitColorBrush usage in XAML files
rg -l "FourBitColorBrush" --type xaml
# Check for consistent color naming across XAML files
rg '"Blue"' --type xaml -C 2
Length of output: 126
Script:
#!/bin/bash
# Search for FourBitColorBrush usage without file type restriction
rg -l "FourBitColorBrush"
# Search for Blue color usage in relevant files
rg '"Blue"' -C 2
# Find all XAML files to understand the extension
fd "\\.axaml$|\\.xaml$"
Length of output: 8810
src/Tests/Consolonia.TestsCore/UnitTestConsole.cs (1)
11-11: LGTM: Required import for new text styling features
The addition of Avalonia.Media import is necessary to support the new RGB colors and text decoration capabilities.
src/Consolonia.Themes.TurboVision/Templates/Controls/DialogWindow.axaml (1)
29-34: LGTM! Appropriate use of ConsoleBrush modes for dialog shadow effect.
The changes effectively implement the shadow effect for the dialog window:
- The outer Border uses
Mode=Shadedwhich is suitable for creating a backdrop effect - The inner Border uses
Mode=Transparentwhich is appropriate for defining the shadow's edge
src/Consolonia.Themes.TurboVision/Templates/Controls/CalendarItem.axaml (2)
24-24: LGTM: Appropriate use of ConsoleBrush
The transition to ConsoleBrush with Transparent mode is consistent with the PR objectives and provides a clean implementation for the button background.
35-35: 🛠️ Refactor suggestion
Consider adding visual feedback for pressed state
The current transparent background might not provide sufficient visual feedback when the button is pressed. Consider using a non-transparent color or different visual indicator to improve user experience.
You could modify the background to provide better visual feedback:
- Value="{drawing:ConsoleBrush Mode=Transparent}"
+ Value="{drawing:ConsoleBrush Mode=Normal, Color={StaticResource ThemeHighlightBrush}}"src/Consolonia.Themes.TurboVision/Templates/Controls/DataGrid.axaml (1)
279-279: LGTM! Transition to ConsoleBrush aligns with RGB color support.
The change from FourBitColorBrush to ConsoleBrush with transparent mode maintains existing functionality while enabling RGB color support.
Let's verify the ConsoleBrush usage across other DataGrid-related files:
✅ Verification successful
Transition to ConsoleBrush is consistent across DataGrid implementations
The verification shows:
- No remaining FourBitColorBrush usage in DataGrid-related files
- ConsoleBrush is consistently used across all DataGrid implementations:
- Transparent mode in the theme template
- Specific colors in the Gallery examples (Yellow, DarkGoldenrod, Green)
- Gray color in the test window
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining FourBitColorBrush usage in DataGrid-related files
# and verify consistent ConsoleBrush usage
# Search for any remaining FourBitColorBrush in DataGrid files
echo "Checking for remaining FourBitColorBrush usage in DataGrid files..."
rg "FourBitColorBrush" -l | rg -i "datagrid"
# Verify ConsoleBrush usage pattern in DataGrid files
echo "Verifying ConsoleBrush usage pattern..."
rg "ConsoleBrush" -A 2 | rg -i "datagrid"
Length of output: 1572
src/Consolonia.Gallery/Gallery/GalleryViews/GalleryDataGrid.axaml.cs (2)
12-12: LGTM: Required namespace for RGB color support.
The addition of Avalonia.Media namespace is necessary for accessing the Colors type, aligning with the PR's objective to support full RGB colors.
613-616: LGTM: Proper migration to ConsoleBrush.
The conversion from FourBitColorBrush to ConsoleBrush is implemented correctly, maintaining the same color logic while leveraging the new RGB color support.
src/Consolonia.Core/Drawing/PixelBufferImplementation/PixelForeground.cs (2)
9-16: LGTM: Enhanced constructor with new styling options
The updated constructor now accepts additional styling parameters such as FontWeight, FontStyle, and TextDecorationCollection. This enhancement increases the flexibility and customization of the PixelForeground struct.
🧰 Tools
🪛 GitHub Check: build
[warning] 12-12:
"[ConstantNullCoalescingCondition] '??' left operand is never null" on /home/runner/work/Consolonia/Consolonia/src/Consolonia.Core/Drawing/PixelBufferImplementation/PixelForeground.cs(12,32)
31-32: LGTM: Correct implementation of the Shade method
The Shade method correctly creates a new shaded color and returns a new PixelForeground instance with the updated color:
Color newColor = Color.Shade();
return new PixelForeground(Symbol, Weight, Style, TextDecorations, newColor);src/Consolonia.Core/Drawing/PixelBufferImplementation/Pixel.cs (3)
2-5: LGTM on added using directives
The added using directives for System.Diagnostics.CodeAnalysis, Avalonia.Controls.Documents, and Avalonia.Media are appropriate and necessary for the new implementations.
15-15: Properties are properly implemented
The added properties Foreground and Background are read-only and correctly implemented, enhancing the immutability of the Pixel struct.
Also applies to: 17-17
25-26: Constructor overloads enhance flexibility
The new constructor overloads provide greater flexibility in creating Pixel instances with various combinations of parameters. They are well-defined and enhance the usability of the struct.
Also applies to: 35-36, 41-41
src/Consolonia.Core/Infrastructure/InputLessDefaultNetConsole.cs (5)
1-2: LGTM!
The added using directives for System and System.Diagnostics are appropriate for the functionality provided.
9-9: LGTM!
Including Avalonia.Media is necessary for handling Color, FontStyle, and related media types.
82-85: Ensure proper handling of text decorations
The code correctly checks for underlining in textDecorations and applies it using Crayon.Output.Underline(str). Ensure that this implementation correctly renders underlined text across all intended environments and that there are no issues with overlapping styles.
93-95: LGTM!
The usage of Crayon.Output.Rgb for rendering text with specified foreground and background colors enhances the console's visual output effectively.
87-92: 🛠️ Refactor suggestion
Verify handling of FontWeight values and existence of Shade and Brighten methods
The conditional statements adjust the foreground color based on the FontWeight by applying Shade and Brighten methods. Please ensure:
- All possible
FontWeightvalues are covered: Consider adding a default case or using aswitchstatement to handle any unforeseen or futureFontWeightvalues, ensuring consistent color adjustments. - Methods
ShadeandBrightenare implemented: Verify that theColorclass has these extension methods properly defined and included in the project. If they are custom methods, ensure they are accessible where needed.
Run the following script to check for the definitions of these methods:
✅ Verification successful
Let me verify the FontWeight enum values to ensure all cases are covered.
Let me try a different approach to find the FontWeight enum.
Extension methods are properly implemented, enum handling is comprehensive
The verification shows that both Shade and Brighten extension methods for Color are properly implemented in src/Consolonia.Core/Drawing/PixelBufferImplementation/PixelOperations.cs, with both single-parameter and two-parameter overloads available. Regarding the FontWeight handling, the code covers all three logical groupings of font weights:
- Normal weight
- Lighter weights (Thin, ExtraLight, Light)
- Heavier weights (Medium through ExtraBlack)
This implementation ensures that all possible font weight values are handled with appropriate color adjustments, making a default case unnecessary.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for the definitions of `Shade` and `Brighten` extension methods for `Color`.
# Test: Find where `Shade` and `Brighten` methods are defined.
# Expect: Extension methods for `Color` should be found.
rg -t cs 'static Color Shade\(this Color'
rg -t cs 'static Color Brighten\(this Color'
Length of output: 642
Script:
#!/bin/bash
# Search for FontWeight enum definition
ast-grep --pattern 'enum FontWeight {
$$$
}'
Length of output: 48
Script:
#!/bin/bash
# Search for FontWeight enum or class definition using ripgrep
rg -t cs "enum\s+FontWeight|class\s+FontWeight" -B 1 -A 15
Length of output: 60
src/Consolonia.Core/Drawing/RenderTarget.cs (1)
8-9: LGTM
The changes in these sections enhance the rendering capabilities by accommodating more detailed attributes for each pixel. The implementations are correct and align with the PR objectives.
Also applies to: 24-24, 92-92, 160-164, 208-208, 213-218
src/Consolonia.Core/Drawing/ConsoleBrush.cs (1)
126-139:
Verify the implementation of IConicGradientBrush handling
The current implementation for IConicGradientBrush computes horizontal and vertical relative positions and then blends the colors, similar to a linear gradient. However, conic gradients are defined based on the angle from the center point to the given position. This approach may result in incorrect color interpolation for conic gradients.
Would you like assistance in revising this implementation to accurately handle conic gradients using angle calculations from the center point?
src/Consolonia.Core/Drawing/DrawingContextImpl.cs (1)
Line range hint 358-418: Ensure all calls to DrawStringInternal are updated with the new typeface parameter.
The method signature of DrawStringInternal has been updated to include the IGlyphTypeface typeface parameter. Please verify that all calls to this method within the codebase have been updated to pass the appropriate typeface argument to prevent any runtime errors.
You can run the following script to find and review all usages of DrawStringInternal:
This script searches the repository for calls to DrawStringInternal outside of DrawingContextImpl.cs and displays the lines immediately after the match for context.
✅ Verification successful
No action needed - method is correctly used throughout the codebase
The verification shows that DrawStringInternal is a private method within DrawingContextImpl.cs and is only called once within the same file. The call correctly provides all required parameters including the typeface parameter (glyphRun.GlyphTypeface). Since the method is private, it cannot be called from outside this class, so no other calls need to be updated.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify all calls to 'DrawStringInternal' include the new 'typeface' parameter.
# Search for 'DrawStringInternal' method calls in the codebase.
rg --glob '!DrawingContextImpl.cs' 'DrawStringInternal\s*\(' -A 1
Length of output: 67
Script:
#!/bin/bash
# Let's try a different approach to find all calls to DrawStringInternal
# First, let's check if this is a private method within DrawingContextImpl.cs
ast-grep --pattern 'private void DrawStringInternal($$$)'
# Also search for any direct calls to DrawStringInternal within DrawingContextImpl.cs
rg 'DrawStringInternal\s*\(' src/Consolonia.Core/Drawing/DrawingContextImpl.cs -A 1
Length of output: 372
src/Consolonia.Themes.TurboVision/Templates/Controls/Helpers/AreIntegersEqualConverter.cs
Outdated
Show resolved
Hide resolved
src/Consolonia.Core/Drawing/PixelBufferImplementation/PixelBackground.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Outside diff range and nitpick comments (14)
src/Consolonia.Core/Drawing/PixelBufferImplementation/PixelForeground.cs (1)
8-16: LGTM: Well-structured constructor with proper null checksThe constructor is well-implemented with:
- Appropriate default values for styling parameters
- Proper null checking for required parameters
- Backward compatibility through optional parameters
Consider adding XML documentation to describe the parameters and their default values:
+ /// <summary> + /// Initializes a new instance of the PixelForeground struct. + /// </summary> + /// <param name="symbol">The symbol to display. Cannot be null.</param> + /// <param name="weight">The font weight. Defaults to Normal.</param> + /// <param name="style">The font style. Defaults to Normal.</param> + /// <param name="textDecorations">Optional text decorations.</param> + /// <param name="color">The text color. Defaults to White if null.</param> public PixelForeground(ISymbol symbol, FontWeight weight = FontWeight.Normal,src/Consolonia.Core/Drawing/PixelBufferImplementation/PixelOperations.cs (3)
10-10: Consider making_colorAdjustmentconst and document the magic number.The field can be declared as
constsince it's initialized with a constant value. Additionally, consider documenting why the value 32 was chosen for color adjustment.- private static readonly int _colorAdjustment = 32; + /// <summary> + /// The amount by which to adjust RGB values when shading/brightening colors. + /// This value provides a balanced visual difference without being too dramatic. + /// </summary> + private const int ColorAdjustment = 32;🧰 Tools
🪛 GitHub Check: build
[failure] 10-10:
Field '_colorAdjustment' is declared as 'readonly' but is initialized with a constant value. Mark this field as 'const' instead.
[failure] 10-10:
Field '_colorAdjustment' is declared as 'readonly' but is initialized with a constant value. Mark this field as 'const' instead.
12-30: Enhance method documentation with examples and return value details.The documentation could be more helpful by including example usage and describing the expected output.
/// <summary> - /// Make color darker + /// Makes the color darker by reducing its RGB values. /// </summary> - /// <param name="color"></param> - /// <returns></returns> + /// <param name="color">The color to shade</param> + /// <returns>A new Color instance with reduced RGB values</returns> + /// <example> + /// var darkBlue = Colors.Blue.Shade(); + /// </example>
76-80: Improve readability of brightness calculation.Consider using named constants for the luminance coefficients to improve code readability and maintainability.
private static int GetPerceivedBrightness(Color color) { - return (int)(0.299 * color.R + - 0.587 * color.G + - 0.114 * color.B); + const double RedCoefficient = 0.299; + const double GreenCoefficient = 0.587; + const double BlueCoefficient = 0.114; + + return (int)(RedCoefficient * color.R + + GreenCoefficient * color.G + + BlueCoefficient * color.B); }src/Consolonia.Themes.TurboVision/Templates/Controls/Helpers/ConsoloniaTextPresenter.cs (2)
79-79: Improve documentation for CaretBrush initialization.The comment could be more descriptive about why black color was chosen and what "draw own caret" means in this context.
- CaretBrush = new ConsoleBrush(Colors.Black, PixelBackgroundMode.Transparent); // we want to draw own caret + // Initialize caret with black color and transparent background to ensure + // visibility across different text colors while allowing custom caret rendering + CaretBrush = new ConsoleBrush(Colors.Black, PixelBackgroundMode.Transparent);
Timer implementation needs proper handling instead of throwing exception
The code shows that the current implementation is intentionally trying to disable the caret blinking because "terminal caret blinks itself once shown". However, throwing a
NotImplementedExceptionin the timer tick handler is not a proper solution. Instead:
- Since the timer can't be fully disabled, setting
int.MaxValuefor the interval is a reasonable workaround- The tick handler should be a no-op instead of throwing an exception:
caretTickTimer!.Tick += (_, _) => { }; // Do nothing instead of throwingThis approach maintains the intended behavior (preventing double-blinking) while avoiding potential runtime exceptions.
🔗 Analysis chain
Line range hint
73-77: Review timer implementation for potential issues.The current implementation uses
int.MaxValuefor the timer interval and throws an exception in the tick handler. This is a fragile workaround that could lead to runtime issues.Let's check if there's a better way to handle this in the base class:
Consider these alternatives:
- Override the base timer initialization to prevent it from starting
- Implement a proper timer disable mechanism
- Document why this workaround is necessary and any associated risks
The previous pattern might have failed due to syntax. Let's try to understand the inheritance hierarchy and timer implementation better with these broader searches.
Let's try to understand how the base TextPresenter class handles the timer and if there are any documented alternatives or reasons for this implementation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for timer-related code in TextPresenter ast-grep --pattern 'class TextPresenter { $$$ _caretTimer $$$ }'Length of output: 72
Script:
#!/bin/bash # Let's search for TextPresenter class and related timer code more broadly rg -l "TextPresenter" # Then search for any timer-related code in the codebase rg -A 5 "DispatcherTimer" # Also check for any base class that might be involved rg -A 2 "class.*TextPresenter.*:"Length of output: 1968
Script:
#!/bin/bash # Let's examine the base TextPresenter class to understand the timer implementation rg -A 10 "class TextPresenter" # Check for any TickTimer field or property declarations rg -B 2 -A 2 "TickTimer" # Look for any documentation or comments about timer handling rg -B 2 -A 2 "(?i)timer|caret.*blink"Length of output: 8519
src/Tests/Consolonia.TestsCore/PixelTests.cs (1)
1-134: Consider reorganizing tests for better maintainability.The test class could benefit from better organization:
- Group related tests using nested classes (e.g.,
BasicOperations,RelativeOperations,BoundaryTests)- Add a test category attribute to integrate with the test runner
- Consider separating slow-running boundary tests
Example structure:
[TestFixture] [Category("ColorManipulation")] public class PixelTests { [TestFixture] public class BasicOperations { // TestShade, TestBrighten } [TestFixture] public class RelativeOperations { // TestRelativeShade, TestRelativeBrighten } [TestFixture] [Category("SlowTests")] public class BoundaryTests { // TestShadeBoundaries, TestBrightenBoundaries } }src/Tests/Consolonia.TestsCore/UnitTestConsole.cs (1)
56-57: Add parameter validationConsider adding null checks for the text decoration and string parameters to prevent potential NullReferenceExceptions.
void IConsole.Print(PixelBufferCoordinate bufferPoint, Color background, Color foreground, FontStyle style, FontWeight weight, TextDecorationCollection textDecorations, string str) { + if (str == null) + throw new ArgumentNullException(nameof(str)); + + textDecorations ??= new TextDecorationCollection(); + (ushort x, ushort y) = bufferPoint;src/Consolonia.Core/Drawing/RenderTarget.cs (2)
23-24: Consider extracting default text decorations as a constant.The cache initialization looks good and properly supports the new styling properties. Consider extracting the empty
TextDecorationCollectionas a private static readonly field to avoid creating new instances.+ private static readonly TextDecorationCollection EmptyTextDecorations = new(); - private (Color background, Color foreground, FontWeight weight, FontStyle style, TextDecorationCollection - textDecorations, char character)?[,] _cache; + private (Color background, Color foreground, FontWeight weight, FontStyle style, TextDecorationCollection + textDecorations, char character)?[,] _cache;Also applies to: 99-101
205-207: Document the rationale for character filtering.The code includes commented checks for specific Unicode characters ('保', '哥'). If these characters cause issues in some terminals, consider documenting this in a comment or moving the character filtering logic to a dedicated method with proper documentation.
+ /// <summary> + /// Filters out control characters and problematic Unicode characters that may not render correctly in some terminals. + /// </summary> + private static char FilterCharacter(char character) + { + // Some terminals do not print \0 or specific Unicode characters correctly + if (char.IsControl(character) /*|| character is '保' or '哥'*/) + return ' '; + return character; + } - if (char.IsControl(character) /*|| character is '保' or '哥'*/) - character = ' '; // some terminals do not print \0 + character = FilterCharacter(character);src/Consolonia.Core/Drawing/ConsoleBrush.cs (2)
9-47: Add XML documentation for public members.The class and its public members lack XML documentation, which is important for API consumers. Consider adding documentation especially for:
- The
ConsoleBrushclass itself- Public constructors
ModeandColorpropertiesConsider making Transform and TransformOrigin configurable.
The hardcoded values for
TransformandTransformOriginproperties might limit functionality in scenarios where transformations are needed.Consider making these properties configurable if transform support is planned for future releases.
166-204: Consider improving color blending accuracy.The current color blending implementation uses simple averaging, which might not produce visually accurate results. Consider:
- Implementing proper gamma correction
- Supporting different color spaces (like LAB) for more accurate blending
- Adding different blend modes (multiply, screen, etc.)
Example implementation of gamma-correct blending:
private static Color BlendColors(Color color1, Color color2) { - int r = (color1.R + color2.R) / 2; - int g = (color1.G + color2.G) / 2; - int b = (color1.B + color2.B) / 2; - int a = (color1.A + color2.A) / 2; + // Convert to linear space + double r1 = Math.Pow(color1.R / 255.0, 2.2); + double g1 = Math.Pow(color1.G / 255.0, 2.2); + double b1 = Math.Pow(color1.B / 255.0, 2.2); + double r2 = Math.Pow(color2.R / 255.0, 2.2); + double g2 = Math.Pow(color2.G / 255.0, 2.2); + double b2 = Math.Pow(color2.B / 255.0, 2.2); + + // Blend in linear space + double r = (r1 + r2) / 2; + double g = (g1 + g2) / 2; + double b = (b1 + b2) / 2; + + // Convert back to sRGB + return Color.FromArgb( + (byte)((color1.A + color2.A) / 2), + (byte)(Math.Pow(r, 1/2.2) * 255), + (byte)(Math.Pow(g, 1/2.2) * 255), + (byte)(Math.Pow(b, 1/2.2) * 255) + );🧰 Tools
🪛 GitHub Check: build
[notice] 182-182:
"[ConvertIfStatementToSwitchStatement] Convert 'if' statement into 'switch' statement" on /home/runner/work/Consolonia/Consolonia/src/Consolonia.Core/Drawing/ConsoleBrush.cs(182,13)src/Consolonia.Core/Drawing/DrawingContextImpl.cs (2)
266-271: Simplify nested property patterns.The pattern matching can be made more concise by merging the nested property patterns.
Apply this optimization:
- if (pen is not - { - Brush: ConsoleBrush or LineBrush or ImmutableSolidColorBrush, - Thickness: 1, - DashStyle: null or { Dashes: { Count: 0 } }, - LineCap: PenLineCap.Flat, - LineJoin: PenLineJoin.Miter - }) + if (pen is not + { + Brush: ConsoleBrush or LineBrush or ImmutableSolidColorBrush, + Thickness: 1, + DashStyle.Dashes.Count: 0, + LineCap: PenLineCap.Flat, + LineJoin: PenLineJoin.Miter + })🧰 Tools
🪛 GitHub Check: build
[notice] 268-268:
"[MergeNestedPropertyPatterns] Merge nested property patterns" on /home/runner/work/Consolonia/Consolonia/src/Consolonia.Core/Drawing/DrawingContextImpl.cs(268,50)
Line range hint
374-389: Extract tab handling logic to a separate method.The tab handling logic could be extracted to improve readability and maintainability.
Consider extracting the tab handling logic:
+ private void HandleTab(Point characterPoint, int currentXPosition, Color foregroundColor) + { + const int tabSize = 8; + var consolePixel = new Pixel(' ', foregroundColor); + for (int j = 0; j < tabSize; j++) + { + Point newCharacterPoint = characterPoint.WithX(characterPoint.X + j); + CurrentClip.ExecuteWithClipping(newCharacterPoint, () => + { + _pixelBuffer.Set((PixelBufferCoordinate)newCharacterPoint, + (oldPixel, cp) => oldPixel.Blend(cp), consolePixel); + }); + } + return tabSize - 1; + } case '\t': - { - const int tabSize = 8; - var consolePixel = new Pixel(' ', foregroundColor); - for (int j = 0; j < tabSize; j++) - { - Point newCharacterPoint = characterPoint.WithX(characterPoint.X + j); - CurrentClip.ExecuteWithClipping(newCharacterPoint, () => - { - _pixelBuffer.Set((PixelBufferCoordinate)newCharacterPoint, - (oldPixel, cp) => oldPixel.Blend(cp), consolePixel); - }); - } - currentXPosition += tabSize - 1; - } + currentXPosition += HandleTab(characterPoint, currentXPosition, foregroundColor); break;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (20)
src/Consolonia.Core/Drawing/ConsoleBrush.cs(1 hunks)src/Consolonia.Core/Drawing/DrawingContextImpl.cs(9 hunks)src/Consolonia.Core/Drawing/PixelBufferImplementation/Pixel.cs(3 hunks)src/Consolonia.Core/Drawing/PixelBufferImplementation/PixelBackground.cs(2 hunks)src/Consolonia.Core/Drawing/PixelBufferImplementation/PixelForeground.cs(1 hunks)src/Consolonia.Core/Drawing/PixelBufferImplementation/PixelOperations.cs(1 hunks)src/Consolonia.Core/Drawing/RenderTarget.cs(7 hunks)src/Consolonia.Core/Infrastructure/IConsole.cs(2 hunks)src/Consolonia.Core/Infrastructure/InputLessDefaultNetConsole.cs(3 hunks)src/Consolonia.Core/Text/FontManagerImpl.cs(1 hunks)src/Consolonia.Core/Text/GlyphRunImpl.cs(1 hunks)src/Consolonia.Core/Text/GlyphTypeface.cs(2 hunks)src/Consolonia.Core/Text/TextShaper.cs(1 hunks)src/Consolonia.PlatformSupport/WindowsConsole.cs(4 hunks)src/Consolonia.Themes.TurboVision/Templates/Controls/Helpers/ConsoloniaTextPresenter.cs(2 hunks)src/Consolonia.Themes.TurboVision/Templates/TurboVisionDefaultColors.axaml(1 hunks)src/Consolonia.Themes.TurboVision/Themes/TurboVisionBlack/TurboVisionBlackColors.axaml(1 hunks)src/Consolonia.Themes.TurboVision/Themes/TurboVisionDark/TurboVisionDarkColors.axaml(1 hunks)src/Tests/Consolonia.TestsCore/PixelTests.cs(1 hunks)src/Tests/Consolonia.TestsCore/UnitTestConsole.cs(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
- src/Consolonia.Core/Drawing/PixelBufferImplementation/PixelBackground.cs
- src/Consolonia.Core/Infrastructure/IConsole.cs
- src/Consolonia.Core/Text/FontManagerImpl.cs
- src/Consolonia.Core/Text/GlyphRunImpl.cs
- src/Consolonia.Core/Text/TextShaper.cs
- src/Consolonia.PlatformSupport/WindowsConsole.cs
- src/Consolonia.Themes.TurboVision/Templates/TurboVisionDefaultColors.axaml
- src/Consolonia.Themes.TurboVision/Themes/TurboVisionBlack/TurboVisionBlackColors.axaml
- src/Consolonia.Themes.TurboVision/Themes/TurboVisionDark/TurboVisionDarkColors.axaml
🧰 Additional context used
🪛 GitHub Check: build
src/Consolonia.Core/Drawing/ConsoleBrush.cs
[notice] 182-182:
"[ConvertIfStatementToSwitchStatement] Convert 'if' statement into 'switch' statement" on /home/runner/work/Consolonia/Consolonia/src/Consolonia.Core/Drawing/ConsoleBrush.cs(182,13)
src/Consolonia.Core/Drawing/DrawingContextImpl.cs
[notice] 142-142:
"[ConvertToLambdaExpression] Use lambda expression" on /home/runner/work/Consolonia/Consolonia/src/Consolonia.Core/Drawing/DrawingContextImpl.cs(142,46)
[notice] 268-268:
"[MergeNestedPropertyPatterns] Merge nested property patterns" on /home/runner/work/Consolonia/Consolonia/src/Consolonia.Core/Drawing/DrawingContextImpl.cs(268,50)
src/Consolonia.Core/Drawing/PixelBufferImplementation/PixelOperations.cs
[failure] 10-10:
Field '_colorAdjustment' is declared as 'readonly' but is initialized with a constant value. Mark this field as 'const' instead.
[failure] 10-10:
Field '_colorAdjustment' is declared as 'readonly' but is initialized with a constant value. Mark this field as 'const' instead.
src/Consolonia.Core/Infrastructure/InputLessDefaultNetConsole.cs
[notice] 86-86:
"[ConvertIfStatementToSwitchStatement] Convert 'if' statement into 'switch' statement" on /home/runner/work/Consolonia/Consolonia/src/Consolonia.Core/Infrastructure/InputLessDefaultNetConsole.cs(86,13)
[notice] 88-88:
"[MergeIntoLogicalPattern] Merge into logical pattern" on /home/runner/work/Consolonia/Consolonia/src/Consolonia.Core/Infrastructure/InputLessDefaultNetConsole.cs(88,48)
[notice] 90-90:
"[MergeIntoLogicalPattern] Merge into logical pattern" on /home/runner/work/Consolonia/Consolonia/src/Consolonia.Core/Infrastructure/InputLessDefaultNetConsole.cs(90,50)
src/Consolonia.Core/Text/GlyphTypeface.cs
[notice] 64-64:
"[PropertyCanBeMadeInitOnly.Global] Property can be made init-only" on /home/runner/work/Consolonia/Consolonia/src/Consolonia.Core/Text/GlyphTypeface.cs(64,41)
[notice] 65-65:
"[PropertyCanBeMadeInitOnly.Global] Property can be made init-only" on /home/runner/work/Consolonia/Consolonia/src/Consolonia.Core/Text/GlyphTypeface.cs(65,39)
🔇 Additional comments (18)
src/Consolonia.Core/Drawing/PixelBufferImplementation/PixelForeground.cs (5)
2-2: LGTM: Required import for new styling capabilities
The addition of Avalonia.Media import is appropriate for the new RGB color and text decoration support.
21-27: LGTM: Well-structured property implementations
The properties are correctly implemented as read-only and provide the necessary styling capabilities.
30-32: LGTM: Clean implementation of Shade method
The method correctly maintains immutability and properly handles the new Color type.
20-20: Verify the Unicode support limitation
The TODO comment suggests potential limitations with Unicode support. This might need attention, especially if it affects the new styling features.
36-46: Consider additional validation in Blend method
While the implementation is generally good, there are a few points to consider:
- The TODO comment about checking default(char) should be addressed
- Consider validating TextDecorations before applying them
Consider adding validation for TextDecorations:
public PixelForeground Blend(PixelForeground pixelAboveForeground)
{
ISymbol symbolAbove = pixelAboveForeground.Symbol;
ArgumentNullException.ThrowIfNull(symbolAbove);
if (symbolAbove.IsWhiteSpace()) return this;
ISymbol newSymbol = Symbol.Blend(ref symbolAbove);
+ // Ensure TextDecorations are valid before applying
+ var decorations = pixelAboveForeground.TextDecorations?.Count > 0
+ ? pixelAboveForeground.TextDecorations
+ : null;
return new PixelForeground(newSymbol, pixelAboveForeground.Weight, pixelAboveForeground.Style,
- pixelAboveForeground.TextDecorations, pixelAboveForeground.Color);
+ decorations, pixelAboveForeground.Color);
}src/Consolonia.Core/Text/GlyphTypeface.cs (1)
76-78: Verify the underline implementation approach.
Drawing the underline over the text (-1 position) is unconventional and might affect readability. Traditional underlines are typically drawn below the text.
Please consider:
- How this affects readability with different characters and font sizes
- Whether this approach is consistent with platform conventions
- If there should be a way to configure the position
src/Consolonia.Core/Drawing/PixelBufferImplementation/Pixel.cs (3)
2-2: LGTM! New imports and properties align with PR objectives.
The addition of the Avalonia.Media import and the new immutable properties (Background and IsCaret) are well-structured and support the enhanced color functionality.
Also applies to: 12-14
22-24: Redundant default parameters in constructor chain.
The constructor parameters for FontStyle.Normal and FontWeight.Normal are passed explicitly even though they are the default values.
Also applies to: 28-37
Line range hint 1-100: Implementation successfully meets PR objectives.
The changes effectively implement:
- Full RGB color support through the transition to
Colortype - Text decoration support with underline detection
- Font weight and style support through new parameters
The implementation is robust and well-integrated with the existing codebase.
src/Tests/Consolonia.TestsCore/PixelTests.cs (2)
10-28: Enhance test coverage for basic color operations.
The basic tests for Shade and Brighten operations could be more comprehensive:
- Single test case with gray color limits coverage
- No validation of alpha channel preservation
- Missing validation of actual color values or expected ranges
- No edge cases (pure colors, black, white)
Previous review already suggested improvements using [TestCase] attribute for better coverage. That suggestion is still valid.
66-132: Address reliability concerns in boundary tests.
The boundary tests have several issues:
- No timeout protection for potentially long-running operations
- Risk of infinite loops if colors never reach boundaries
- Significant code duplication between Shade and Brighten tests
- No validation that color changes are gradual and consistent
Previous review already suggested improvements including timeout protection, iteration limits, and better validation. That suggestion is still valid.
Let's verify the color manipulation implementation:
#!/bin/bash
# Description: Find and analyze the color manipulation implementation
# to understand if it guarantees convergence to boundary colors
# Look for Shade/Brighten implementation
ast-grep --pattern 'public static Color Shade($color, $_) {
$$$
}'
# Look for any existing timeout or iteration limit constants
rg -i '(timeout|maxiter|maximum.*iteration)'src/Tests/Consolonia.TestsCore/UnitTestConsole.cs (2)
11-11: LGTM: Required import for new text styling features
The addition of Avalonia.Media import is necessary for the new color and text decoration types.
56-67: Previous review comment is still applicable
The suggestion to introduce a parameter object for better maintainability from the previous review is still valid and would improve the code's maintainability.
src/Consolonia.Core/Infrastructure/InputLessDefaultNetConsole.cs (4)
8-8: LGTM: Required imports for enhanced color support.
The new imports support the transition from ConsoleColor to RGB colors and text styling capabilities.
Also applies to: 10-10
83-85: LGTM: Clean text decoration implementation.
The underline decoration check and application is straightforward and effective.
93-95: LGTM: Proper implementation of RGB color output.
The implementation correctly uses Crayon's RGB support for both foreground and background colors.
Line range hint 69-96: Verify color handling across the application.
The changes look good, but let's verify the color handling implementation across the codebase.
✅ Verification successful
Let me gather more specific information about color handling implementations.
Based on the search results, I can now provide a final response:
Color handling implementation is consistent and properly integrated
The verification shows:
- Color operations (Shade/Brighten) are properly implemented with thorough test coverage
- No legacy ConsoleColor usage remains in the codebase
- Color handling is consistently implemented across drawing contexts and pixel operations
- The RGB color implementation aligns with the Crayon library integration
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining ConsoleColor usage that might need updating
# and verify consistent color handling across the codebase
echo "Checking for remaining ConsoleColor usage..."
rg "ConsoleColor" --type cs
echo "Checking for consistent color parameter usage in IConsole implementations..."
ast-grep --pattern 'interface IConsole {
$$$
Print($$$) {
$$$
}
$$$
}'
Length of output: 374
Script:
#!/bin/bash
# Search for Color class usage and implementations
echo "Checking Color class usage..."
rg "Color\s+\w+\s*[=;]|Color\s+\w+\s*\(" --type cs -A 2
echo "Checking color-related method implementations..."
ast-grep --pattern 'void Print($$$, Color $_,$$$)'
echo "Checking for any color enum or struct definitions..."
ast-grep --pattern 'enum Color {
$$$
}'
ast-grep --pattern 'struct Color {
$$$
}'
Length of output: 9933
🧰 Tools
🪛 GitHub Check: build
[notice] 86-86:
"[ConvertIfStatementToSwitchStatement] Convert 'if' statement into 'switch' statement" on /home/runner/work/Consolonia/Consolonia/src/Consolonia.Core/Infrastructure/InputLessDefaultNetConsole.cs(86,13)
[notice] 88-88:
"[MergeIntoLogicalPattern] Merge into logical pattern" on /home/runner/work/Consolonia/Consolonia/src/Consolonia.Core/Infrastructure/InputLessDefaultNetConsole.cs(88,48)
[notice] 90-90:
"[MergeIntoLogicalPattern] Merge into logical pattern" on /home/runner/work/Consolonia/Consolonia/src/Consolonia.Core/Infrastructure/InputLessDefaultNetConsole.cs(90,50)
src/Consolonia.Core/Drawing/ConsoleBrush.cs (1)
58-88: 🛠️ Refactor suggestion
Verify error handling for unsupported brush types.
The method uses ConsoloniaPlatform.RaiseNotSupported(6) but returns null afterward. This could lead to null reference exceptions if the caller doesn't handle the null return value.
Consider throwing an exception instead of returning null:
- ConsoloniaPlatform.RaiseNotSupported(6);
- return null;
+ throw new NotSupportedException("Unsupported brush type");✅ Verification successful
Let me gather more information about the error handling mechanism.
Based on the findings, I can now provide the final response:
The error handling approach using RaiseNotSupported is intentional and safe
The implementation of RaiseNotSupported in ConsoloniaPlatform shows it's a platform-specific error handling mechanism that:
- Raises a
NotSupportedRequestwith an error code - Invokes handlers through the
NotSupportedevent - Calls
CheckHandled()to ensure the error was handled
The null return after RaiseNotSupported is consistently used across the codebase, and the calling code (as seen in DrawingContextImpl.cs) properly handles these cases. This pattern appears to be a deliberate design choice for graceful degradation of unsupported features in the console-based UI framework.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for direct calls to FromBrush to verify null handling
rg -l "FromBrush\(" | xargs rg -A 2 "FromBrush\("
Length of output: 1626
Script:
#!/bin/bash
# Search for RaiseNotSupported usage and implementation
rg -A 5 "RaiseNotSupported"
# Search for any exception handling around FromBrush calls
rg -B 2 -A 5 "FromBrush\(" | rg -B 2 -A 5 "try|catch"
Length of output: 8058
src/Consolonia.Core/Drawing/PixelBufferImplementation/PixelOperations.cs
Show resolved
Hide resolved
src/Consolonia.Core/Drawing/PixelBufferImplementation/PixelOperations.cs
Show resolved
Hide resolved
src/Consolonia.Themes.TurboVision/Templates/Controls/Helpers/ConsoloniaTextPresenter.cs
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
src/Consolonia.Core/Drawing/PixelBufferImplementation/PixelOperations.cs (2)
10-10: Document the ColorAdjustment constant value.The value 32 seems arbitrary. Consider adding XML documentation explaining how this value was determined and its impact on color adjustments.
76-81: Add reference to the perceived brightness formula.Consider adding XML documentation that references the source or standard for this perceived brightness formula (known as the Rec. 601 luma coefficients).
+ /// <summary> + /// Calculates perceived brightness using Rec. 601 luma coefficients. + /// </summary> + /// <param name="color">The color to calculate brightness for</param> + /// <returns>Perceived brightness value between 0 and 255</returns> private static int GetPerceivedBrightness(Color color)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/Consolonia.Core/Drawing/PixelBufferImplementation/PixelOperations.cs(1 hunks)
🔇 Additional comments (3)
src/Consolonia.Core/Drawing/PixelBufferImplementation/PixelOperations.cs (3)
12-30: LGTM! Well-documented convenience methods.
The implementation provides clean default behavior using black as the background color.
60-73: LGTM with previous suggestions applied.
The implementation is correct but should incorporate the same floating-point comparison fixes suggested for the Shade method.
38-52: 🛠️ Refactor suggestion
Fix floating-point comparison and improve readability.
The floating-point comparison issue flagged in previous reviews should be addressed. Additionally, the logic can be made more readable by extracting the brightness comparison into a well-named variable.
Apply this diff to improve the code:
public static Color Shade(this Color foreground, Color background)
{
int foregroundBrightness = GetPerceivedBrightness(foreground);
int backgroundBrightness = GetPerceivedBrightness(background);
- if (foregroundBrightness > backgroundBrightness ||
- foregroundBrightness == backgroundBrightness && foregroundBrightness > 0)
+ const double epsilon = 0.0001;
+ bool shouldDarken = foregroundBrightness > backgroundBrightness - epsilon
+ && foregroundBrightness > 0;
+ if (shouldDarken)
|
ok, maybe the coderabbit is a bit verbose. Pretty cool tech though |
|
I think you need to approve again, your comments reset the approval |
So many things to mearn here. I'd written comments before hitting "approve") |
You can use any colors for brushes
supports full RGB(255,255,255)
TextDecorations.Underline and FontWeight support
Changes:
Summary by CodeRabbit
New Features
ConsoleBrushfor enhanced color handling across various controls.GalleryColors,GalleryGradientBrush, andGalleryAnimatedLinesfor interactive color manipulation and gradient applications.Improvements
Printmethod to supportColortypes instead ofConsoleColor, allowing for more flexible console output.Bug Fixes
TextBlockTests.Style Changes
FourBitColorBrushwith direct color values.Tests
PixelTests.