Add support for richer console colors and FontWeight and Bold TextDecorator #122
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.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (3)
src/Consolonia.Core/Drawing/ConsoleBrush.cs (3)
11-18: Add XML documentation for class and propertiesThe class and its styled properties lack XML documentation. Consider adding documentation to improve code maintainability.
Add the following documentation:
/// <summary> /// Represents a brush that provides color and mode information for console rendering. /// </summary> public class ConsoleBrush : AvaloniaObject, IImmutableBrush { /// <summary> /// Defines the Color property that represents the brush's color. /// </summary> public static readonly StyledProperty<Color> ColorProperty = AvaloniaProperty.Register<ConsoleBrush, Color>(nameof(Color)); /// <summary> /// Defines the Mode property that determines how the background is rendered. /// </summary> public static readonly StyledProperty<PixelBackgroundMode> ModeProperty = AvaloniaProperty.Register<ConsoleBrush, PixelBackgroundMode>(nameof(Mode));
34-42: Improve XML documentation for FromBrush methodThe current documentation lacks parameter and return value descriptions.
Update the documentation as follows:
/// <summary> /// Converts an IBrush to a ConsoleBrush. /// </summary> /// <remarks> /// If the input is a ConsoleBrush, it will be passed through unchanged, unless mode is specified. /// </remarks> /// <param name="brush">The brush to convert.</param> /// <param name="mode">Optional mode to apply. If not specified, defaults to Colored for new brushes.</param> /// <returns>A ConsoleBrush instance representing the input brush.</returns> /// <exception cref="ArgumentNullException">Thrown when brush is null.</exception> /// <exception cref="ArgumentException">Thrown when the brush type is not supported.</exception>
78-78: Add XML documentation for FromPosition methodThe method lacks XML documentation describing its purpose and parameters.
Add the following documentation:
/// <summary> /// Creates a ConsoleBrush by sampling a color at a specific position within a brush's bounds. /// </summary> /// <param name="brush">The source brush to sample from.</param> /// <param name="x">The x-coordinate of the sampling position.</param> /// <param name="y">The y-coordinate of the sampling position.</param> /// <param name="width">The total width of the brush's bounds.</param> /// <param name="height">The total height of the brush's bounds.</param> /// <returns>A new ConsoleBrush with the sampled color.</returns> /// <exception cref="ArgumentNullException">Thrown when brush is null.</exception> /// <exception cref="ArgumentOutOfRangeException">Thrown when coordinates or dimensions are invalid.</exception>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
src/Consolonia.Core/Drawing/ConsoleBrush.cs(1 hunks)src/Consolonia.Core/Drawing/PixelBufferImplementation/Pixel.cs(4 hunks)src/Consolonia.Core/Drawing/RenderTarget.cs(8 hunks)src/Tests/Consolonia.TestsCore/UnitTestConsole.cs(2 hunks)
🔇 Additional comments (8)
src/Consolonia.Core/Drawing/PixelBufferImplementation/Pixel.cs (3)
Line range hint 2-17: LGTM! New imports and property changes align with PR objectives.
The addition of Avalonia.Controls.Documents and Media namespaces, along with the new Background property, properly supports the enhanced text formatting capabilities.
25-40: LGTM! Constructor changes properly implement enhanced text formatting.
The constructors have been updated to:
- Replace ConsoleColor with Color
- Add support for FontStyle, FontWeight, and TextDecorations
- Use proper constructor chaining
78-80: LGTM! Text decoration handling is well-implemented.
The code correctly converts drawing lines through text into underline text decorations, with clear comments explaining the behavior.
src/Tests/Consolonia.TestsCore/UnitTestConsole.cs (2)
11-11: LGTM: Required import for enhanced text styling.
The added import supports the transition from ConsoleColor to Color and enables rich text formatting features.
56-65: 🛠️ Refactor suggestion
Extract pixel creation logic to improve readability.
The inline lambda with the ReSharper comment makes the code harder to read. Consider extracting the pixel creation logic to a separate method.
Apply this diff to improve readability:
void IConsole.Print(PixelBufferCoordinate bufferPoint, Color background, Color foreground, FontStyle style, FontWeight weight, TextDecorationCollection textDecorations, string str)
{
(ushort x, ushort y) = bufferPoint;
for (int i = 0; i < str.Length; i++)
- PixelBuffer.Set(new PixelBufferCoordinate((ushort)(x + i), y), _ =>
- // ReSharper disable once AccessToModifiedClosure we are sure about inline execution
- new Pixel(
- new PixelForeground(new SimpleSymbol(str[i]), color: foreground, style: style, weight: weight, textDecorations: textDecorations),
- new PixelBackground(PixelBackgroundMode.Colored, background)));
+ PixelBuffer.Set(
+ new PixelBufferCoordinate((ushort)(x + i), y),
+ CreatePixel(str[i], foreground, background, style, weight, textDecorations));
}
+ private static Pixel CreatePixel(
+ char c,
+ Color foreground,
+ Color background,
+ FontStyle style,
+ FontWeight weight,
+ TextDecorationCollection textDecorations)
+ {
+ return new Pixel(
+ new PixelForeground(
+ new SimpleSymbol(c),
+ color: foreground,
+ style: style,
+ weight: weight,
+ textDecorations: textDecorations),
+ new PixelBackground(PixelBackgroundMode.Colored, background));
+ }src/Consolonia.Core/Drawing/RenderTarget.cs (3)
8-9: LGTM: Import and field updates align with enhanced formatting support
The addition of Avalonia.Media import and the updated cache field structure properly support the new rich formatting capabilities including colors, font weights, styles, and text decorations.
Also applies to: 24-24
92-92: LGTM: Cache initialization properly handles new formatting attributes
The initialization correctly sets up the cache array with the expanded tuple structure.
213-218: LGTM: Drawing context implementation properly restored
The CreateDrawingContext and IsCorrupted implementations are correctly restored and properly integrated with the enhanced rendering capabilities.
|
@tomlm This is new one:
By the way, do you see log of the builds? |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (4)
src/Consolonia.Gallery/Gallery/GalleryViews/GalleryGradientBrush.axaml (3)
1-9: Consider removing unused namespace import.The
drawingnamespace is imported but not utilized in this XAML file. Consider removing it if it's not needed:- xmlns:drawing="clr-namespace:Consolonia.Core.Drawing;assembly=Consolonia.Core"
18-22: Add accessibility attributes to buttons.Consider adding tooltips and automation properties to improve accessibility:
- <Button Click="Linear_Click" Background="Gray">Linear</Button> + <Button Click="Linear_Click" Background="Gray" + ToolTip.Tip="Switch to linear gradient" + AutomationProperties.Name="Linear gradient"> + Linear + </Button>
1-24: Consider adding gradient presets and documentation.Since this is a gallery demonstration for console colors and gradients:
- Consider adding preset gradient combinations that work well in console environments
- Add comments explaining how these gradients translate to console output
- Consider showing both color and monochrome gradient examples for different terminal capabilities
This would help users understand how gradients behave in various console environments.
src/Consolonia.Gallery/Gallery/GalleryViews/GalleryGradientBrush.xaml.cs (1)
12-14: Add XML documentation for the public class.Since this is a public class in a gallery view, adding XML documentation would improve maintainability and help other developers understand its purpose and usage.
+ /// <summary> + /// Demonstrates various gradient brush capabilities in the console environment. + /// </summary> [GalleryOrder(1000)] public partial class GalleryGradientBrush : UserControl
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
src/Consolonia.Core/Drawing/ConsoleBrush.cs(1 hunks)src/Consolonia.Gallery/Consolonia.Gallery.csproj(1 hunks)src/Consolonia.Gallery/Gallery/GalleryViews/GalleryGradientBrush.axaml(1 hunks)src/Consolonia.Gallery/Gallery/GalleryViews/GalleryGradientBrush.xaml.cs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Consolonia.Core/Drawing/ConsoleBrush.cs
🔇 Additional comments (5)
src/Consolonia.Gallery/Consolonia.Gallery.csproj (3)
20-24: LGTM: Proper XAML configuration for GalleryGradientBrush.
The XAML configuration correctly sets up the GalleryGradientBrush component with the MSBuild compiler, which is essential for the new gradient brush functionality.
26-31: LGTM: Proper code-behind configuration with correct dependency.
The code-behind file is correctly linked to its AXAML file through the DependentUpon property, ensuring proper build order and maintainability.
20-32: Verify the existence of referenced files.
Let's ensure the referenced files exist in the correct locations.
✅ Verification successful
Files exist at the expected locations ✓
Both the AXAML file and its code-behind file are present in the correct directory structure:
src/Consolonia.Gallery/Gallery/GalleryViews/GalleryGradientBrush.axamlsrc/Consolonia.Gallery/Gallery/GalleryViews/GalleryGradientBrush.xaml.cs
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the existence of the new GalleryGradientBrush files
# Test: Check if the AXAML and code-behind files exist
fd --type f "GalleryGradientBrush\.(axaml|xaml\.cs)$" src/Consolonia.Gallery/Gallery/GalleryViews/
Length of output: 244
src/Consolonia.Gallery/Gallery/GalleryViews/GalleryGradientBrush.axaml (1)
19-21: Verify click handler implementations.
Ensure that the Click handlers (Linear_Click, Radial_Click, Conic_Click) are properly implemented in the code-behind file.
✅ Verification successful
The previous ast-grep pattern might have failed due to syntax issues. Let's first locate the relevant files and then analyze their content.
Click handlers are properly implemented
All three click handlers (Linear_Click, Radial_Click, and Conic_Click) are correctly implemented in the code-behind file. Each handler:
- Has the correct method signature
- Sets appropriate gradient brushes on the "MyGrid" element
- Contains valid gradient configurations specific to each brush type
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for the click handler implementations
ast-grep --pattern 'void $_(object $_, RoutedEventArgs $_) {
$$$
}'
Length of output: 71
Script:
#!/bin/bash
# Search for the code-behind file first
fd "GalleryGradientBrush.axaml.cs"
# Also try to find any file containing these click handlers using ripgrep
rg -l "Linear_Click|Radial_Click|Conic_Click"
Length of output: 226
Script:
#!/bin/bash
# Let's examine the content of the code-behind file to verify the click handler implementations
cat "src/Consolonia.Gallery/Gallery/GalleryViews/GalleryGradientBrush.xaml.cs"
Length of output: 2685
src/Consolonia.Gallery/Gallery/GalleryViews/GalleryGradientBrush.xaml.cs (1)
25-37: 🛠️ Refactor suggestion
Verify console color support and consider extracting gradient stops.
The linear gradient implementation looks correct, but there are two considerations:
Consider extracting the gradient stops as constants or a reusable collection, especially if they're used across different examples:
+ private static readonly GradientStops BlackWhiteGradient = new()
+ {
+ new GradientStop { Color = Colors.White, Offset = 0 },
+ new GradientStop { Color = Colors.Black, Offset = 1 }
+ };
+
private void Linear_Click(object sender, Avalonia.Interactivity.RoutedEventArgs e)
{
this.FindControl<Grid>("MyGrid").Background = new LinearGradientBrush
{
StartPoint = new RelativePoint(0, 0, RelativeUnit.Relative),
EndPoint = new RelativePoint(1, 1, RelativeUnit.Relative),
- GradientStops = new GradientStops
- {
- new GradientStop { Color = Colors.White, Offset = 0 },
- new GradientStop { Color = Colors.Black, Offset = 1 }
- }
+ GradientStops = BlackWhiteGradient
};
}✅ Verification successful
Let me check the color handling implementation in more detail to verify if the Colors.White and Colors.Black are properly supported in the console environment.
Let me check one more thing about the color handling in the console environment, specifically how the ConsoleBrush handles these colors.
Based on the analysis of the ConsoleBrush implementation, I can now provide the final response:
The linear gradient implementation with White and Black colors is fully supported
The ConsoleBrush implementation properly handles gradient interpolation and color blending. The code shows that:
- Colors.White and Colors.Black are properly handled through the color interpolation system
- The gradient implementation includes proper color space conversion and blending
- The ConsoleBrush class can handle both solid colors and gradients through its FromPosition method
While extracting constants could improve code maintainability, the current implementation is technically correct and fully functional in the console environment.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if ConsoleColorBrush or similar console-specific color handling is used elsewhere
rg -l "ConsoleColorBrush|ConsoleBrush"
Length of output: 1136
Script:
#!/bin/bash
# Check how colors are handled in the core drawing implementation
rg -A 10 "Colors\.(White|Black)" src/Consolonia.Core/
# Check the specific gallery implementation context
cat src/Consolonia.Gallery/Gallery/GalleryViews/GalleryGradientBrush.xaml.cs
Length of output: 8239
Script:
#!/bin/bash
# Check ConsoleBrush implementation for color handling
cat src/Consolonia.Core/Drawing/ConsoleBrush.cs
# Check if there are any specific color mappings or restrictions
rg -A 5 "class.*Colors" src/Consolonia.Core/
Length of output: 8638
src/Consolonia.Gallery/Gallery/GalleryViews/GalleryGradientBrush.axaml
Outdated
Show resolved
Hide resolved
src/Consolonia.Gallery/Gallery/GalleryViews/GalleryGradientBrush.xaml.cs
Outdated
Show resolved
Hide resolved
src/Consolonia.Gallery/Gallery/GalleryViews/GalleryGradientBrush.xaml.cs
Show resolved
Hide resolved
|
Wow, this coderabbit tool is unbelievable! |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
src/Consolonia.Gallery/Gallery/GalleryViews/GalleryGradientBrush.axaml (2)
11-17: Consider using console-optimized colors for the gradient.While the gradient configuration is correct, consider using colors that are optimized for console rendering. The current White to Red gradient might not render optimally in all console environments.
<LinearGradientBrush EndPoint="0%,0%" StartPoint="0%,100%"> - <GradientStop Color="White" Offset="0" /> - <GradientStop Color="Red" Offset="1" /> + <GradientStop Color="{x:Static drawing:Colors.Gray}" Offset="0" /> + <GradientStop Color="{x:Static drawing:Colors.DarkRed}" Offset="1" /> </LinearGradientBrush>
18-23: Enhance button accessibility and user guidance.Consider adding tooltips and accessibility attributes to improve user experience and make the controls more accessible.
<StackPanel Orientation="Horizontal" HorizontalAlignment="Center"> - <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" Background="Gray" + ToolTip.Tip="Switch to linear gradient" + AutomationProperties.Name="Linear gradient" + AutomationProperties.HelpText="Changes the background to a linear gradient pattern"> + Linear + </Button> + <Button Click="Radial_Click" Background="Gray" + ToolTip.Tip="Switch to radial gradient" + AutomationProperties.Name="Radial gradient" + AutomationProperties.HelpText="Changes the background to a radial gradient pattern"> + Radial + </Button> + <Button Click="Conic_Click" Background="Gray" + ToolTip.Tip="Switch to conic gradient" + AutomationProperties.Name="Conic gradient" + AutomationProperties.HelpText="Changes the background to a conic gradient pattern"> + Conic + </Button> </StackPanel>src/Consolonia.Core/Drawing/ConsoleBrush.cs (1)
11-18: Add XML documentation for the class and propertiesThe class and its styled properties lack XML documentation. Consider adding documentation to improve code maintainability.
Add the following documentation:
/// <summary> /// Represents a console-specific brush that supports color and background mode styling. /// </summary> public class ConsoleBrush : AvaloniaObject, IImmutableBrush { /// <summary> /// Defines the Color styled property. /// </summary> public static readonly StyledProperty<Color> ColorProperty = AvaloniaProperty.Register<ConsoleBrush, Color>(nameof(Color)); /// <summary> /// Defines the Mode styled property. /// </summary> public static readonly StyledProperty<PixelBackgroundMode> ModeProperty = AvaloniaProperty.Register<ConsoleBrush, PixelBackgroundMode>(nameof(Mode));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
src/Consolonia.Core/Drawing/ConsoleBrush.cs(1 hunks)src/Consolonia.Gallery/Gallery/GalleryViews/GalleryColors.axaml(1 hunks)src/Consolonia.Gallery/Gallery/GalleryViews/GalleryGradientBrush.axaml(1 hunks)src/Consolonia.Gallery/Gallery/GalleryViews/GalleryGradientBrush.xaml.cs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/Consolonia.Gallery/Gallery/GalleryViews/GalleryColors.axaml
- src/Consolonia.Gallery/Gallery/GalleryViews/GalleryGradientBrush.xaml.cs
🔇 Additional comments (3)
src/Consolonia.Gallery/Gallery/GalleryViews/GalleryGradientBrush.axaml (2)
1-10: LGTM! Proper namespace setup and design-time configuration.
The UserControl declaration follows Avalonia conventions and includes necessary namespaces, with proper integration of Consolonia's drawing capabilities.
19-21: Verify click handler implementations.
Ensure that Linear_Click, Radial_Click, and Conic_Click handlers are properly implemented in the code-behind file.
✅ Verification successful
The previous command didn't work as expected. Let's try a different approach to first locate the code-behind file and then examine its content. The XAML file usually has a corresponding code-behind file with the same name but a different extension (typically .cs or .axaml.cs).
Click handlers are properly implemented
All three click handlers (Linear_Click, Radial_Click, and Conic_Click) are correctly implemented in the code-behind file. Each handler sets an appropriate gradient brush (Linear, Radial, or Conic) as the background of the "MyGrid" element with proper configuration of gradient stops and parameters.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for the click handler implementations
ast-grep --pattern 'void $_(object sender, RoutedEventArgs e)
{
$$$
}'
Length of output: 74
Script:
#!/bin/bash
# Let's try a different approach to find the code-behind file and its content
fd "GalleryGradientBrush" --type f
# After finding the file, we'll examine its content
Length of output: 178
Script:
#!/bin/bash
# Now that we found the code-behind file, let's examine its content
cat src/Consolonia.Gallery/Gallery/GalleryViews/GalleryGradientBrush.xaml.cs
Length of output: 2681
src/Consolonia.Core/Drawing/ConsoleBrush.cs (1)
1-214: Implementation successfully adds support for richer console colors
The ConsoleBrush implementation effectively supports full RGB colors and various brush types, aligning well with the PR objectives. The code is well-structured and handles different brush scenarios appropriately.
In a good or bad way? I was thinking to set it to be less verbose/sensitive |
|
In a very good way. I recently retired and one of the things I miss is the valuable feedback I would get from colleagues. The quality of the code review is sooooo much better than crappy linting tools. I'm super impressed! |
|
I think I've resolved all of the coderabbit issues, I'm ready to go if the build works and you are satisfied. |
|
huh. Code inspection build is failing and I don't understand why |
May be that because this action does not work with forks. Can you rebase to this repo branch? Or as an option i can run code inspections/cleanup in a separate PR afterwards. |
|
I'll try. I always get a bit nervous doing that. |
|
BTW: I've got another PR I'm working on: |
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (13)
src/Consolonia.Core/Text/TextShaper.cs (1)
Wide character handling needs proper implementation
The current implementation replaces wide characters with '?' as a temporary solution, but this is problematic because:
- There's an open issue (AXAML custom color parsing #2) in the referenced repository indicating incorrect wide character detection
- The placeholder replacement affects text rendering quality
- The implementation is spread across multiple files without proper handling
Required changes:
- Remove the '?' placeholder replacement in
src/Consolonia.Core/Text/TextShaper.cs- Implement proper wide character detection and rendering
- Consider adding tests for wide character scenarios
🔗 Analysis chain
Line range hint
31-35: Improve wide character handlingCurrently, wide characters are replaced with '?', which provides a poor user experience, especially given this PR's focus on enhanced console capabilities.
Let's check the status of the referenced issue and related code:
Would you like me to:
- Research alternative wide character handling solutions?
- Create a follow-up issue for proper wide character support?
Let me gather more context about the ConsoleText.IsWideChar implementation to better understand the current handling.
Let me check the referenced GitHub issue to understand if there's an existing plan or discussion about wide character support.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for wide character handling across the codebase echo "Searching for wide character handling..." rg -i "widechar|wide.?char|double.?width" # Check for other character replacement patterns echo "Searching for character replacement patterns..." ast-grep --pattern 'c => $$$\?$$$'Length of output: 853
Script:
#!/bin/bash # Check ConsoleText.IsWideChar implementation ast-grep --pattern 'IsWideChar($$$) { $$$ }' # Check for any related tests or documentation rg -l "IsWideChar" --type-add 'test:*{Test,test,Tests,tests}*' --type test # Look for any configuration or constants related to wide char handling rg "WideChar" -B 2 -A 2Length of output: 1681
Script:
#!/bin/bash # Try to fetch the referenced GitHub issue gh issue view "https://github.com/SlimeNull/NullLib.ConsoleEx/issues/2" || echo "Unable to fetch external issue" # Check commit history for any recent changes related to wide character support git log -p --grep="wide.*char" --all || echo "No commits found" # Look for any configuration files that might contain character encoding settings fd -e json -e config -e xml -e ini | xargs rg -i "encoding|charset" || echo "No encoding configuration found"Length of output: 932
🧰 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/Drawing/PixelBufferImplementation/PixelForeground.cs (1)
37-37: Consider addressing the TODO comment about default char validation.The comment suggests that default char validation needs to be implemented. This could affect the robustness of the blending operation.
Would you like me to help implement the default char validation or create a GitHub issue to track this task?
src/Consolonia.Core/Drawing/PixelBufferImplementation/Pixel.cs (1)
71-77: Clean up redundant 'this' qualifiersSeveral redundant 'this' qualifiers can be removed to improve code cleanliness.
Apply this diff to clean up the qualifiers:
- if (pixelAbove.Foreground.Symbol is DrawingBoxSymbol && - this.Foreground.Symbol is SimpleSymbol simpleSymbol) - { - newForeground = new PixelForeground(Foreground.Symbol, Foreground.Weight, Foreground.Style, TextDecorations.Underline, Foreground.Color); + if (pixelAbove.Foreground.Symbol is DrawingBoxSymbol && + Foreground.Symbol is SimpleSymbol simpleSymbol) + { + newForeground = new PixelForeground(Foreground.Symbol, Foreground.Weight, Foreground.Style, TextDecorations.Underline, Foreground.Color);🧰 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)src/Consolonia.Core/Infrastructure/InputLessDefaultNetConsole.cs (2)
86-91: Consider adding input validation for FontWeight.While the font weight handling is well-implemented, consider adding validation to handle potential null or invalid FontWeight values gracefully.
+if (weight == null) + weight = FontWeight.Normal; + if (weight == FontWeight.Normal) foreground = foreground.Shade(background);
92-94: Consider adding color validation.While the RGB output is well-implemented, consider adding validation for the Color parameters to handle potential null values or invalid color components gracefully.
+if (background == null) + background = Colors.Black; +if (foreground == null) + foreground = Colors.White; + Console.Write(Crayon.Output.Rgb(foreground.R, foreground.G, foreground.B) .Background.Rgb(background.R, background.G, background.B) .Text(str));src/Consolonia.Core/Drawing/RenderTarget.cs (2)
Line range hint
153-172: Consider converting FlushingBuffer to a classThe current struct design has several characteristics that make it more suitable as a class:
- Contains multiple fields
- Is mutable
- Uses a constructor pattern typically seen in classes
- Manages disposable resources (StringBuilder)
Convert the struct to a class:
-private struct FlushingBuffer +private class FlushingBuffer { private readonly IConsole _console; private readonly StringBuilder _stringBuilder; // ... public FlushingBuffer(IConsole console) { - this = new FlushingBuffer(); _console = console; _stringBuilder = new StringBuilder(); + _lastStyle = FontStyle.Normal; + _lastWeight = FontWeight.Normal; + _lastTextDecorations = new TextDecorationCollection(); }
216-216: Document the purpose of IsCorrupted propertyThe property is hardcoded to return false without any clear indication of its purpose or when it might return true.
Add documentation:
+/// <summary> +/// Indicates whether the render target's state has been corrupted and needs reset. +/// Currently always returns false as corruption detection is not implemented. +/// </summary> public bool IsCorrupted => false;src/Consolonia.Core/Drawing/ConsoleBrush.cs (4)
9-15: Add XML documentation for class and properties.The class and its public properties lack XML documentation, which is important for API consumers.
Add documentation like this:
/// <summary> /// Represents a brush that provides console-specific color rendering capabilities. /// </summary> public class ConsoleBrush : AvaloniaObject, IImmutableBrush { /// <summary> /// Defines the Color property that represents the brush's color. /// </summary> public static readonly StyledProperty<Color> ColorProperty = AvaloniaProperty.Register<ConsoleBrush, Color>(nameof(Color)); /// <summary> /// Defines the Mode property that determines how the background is rendered. /// </summary> public static readonly StyledProperty<PixelBackgroundMode> ModeProperty = AvaloniaProperty.Register<ConsoleBrush, PixelBackgroundMode>(nameof(Mode)); }
28-30: Initialize default values in parameterless constructor.The parameterless constructor should set default values for Color and Mode to ensure consistent behavior.
public ConsoleBrush() { + Color = Colors.White; // or any other suitable default + Mode = PixelBackgroundMode.Colored; // or any other suitable default }
80-81: Fix typo in error message.The error message contains a typo: "out bounds" should be "out of bounds".
-throw new ArgumentOutOfRangeException(nameof(y), "y is out bounds"); +throw new ArgumentOutOfRangeException(nameof(y), "y is out of bounds");
205-212: Consider using unchecked block for color blending.The color blending operations are safe from overflow due to division by 2, but using unchecked can make this explicit.
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; + unchecked + { + 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; + return Color.FromArgb((byte)a, (byte)r, (byte)g, (byte)b); + } - return Color.FromArgb((byte)a, (byte)r, (byte)g, (byte)b); }src/Consolonia.Core/Drawing/DrawingContextImpl.cs (2)
Line range hint
266-285: Improve error handling in color extractionThe method throws a generic ArgumentOutOfRangeException when an unsupported PixelBackgroundMode is encountered. Consider providing more specific error handling for each unsupported case.
Apply this diff to improve error handling:
switch (consoleColorBrush.Mode) { case PixelBackgroundMode.Colored: return consoleColorBrush.Color; case PixelBackgroundMode.Transparent: return null; case PixelBackgroundMode.Shaded: ConsoloniaPlatform.RaiseNotSupported(8); return null; default: - throw new ArgumentOutOfRangeException(nameof(pen)); + throw new ArgumentOutOfRangeException( + nameof(pen), + $"Unsupported PixelBackgroundMode: {consoleColorBrush.Mode}"); }
396-402: Remove or properly document commented codeThe commented code for newline handling should either be removed if it's no longer needed or properly documented if it's intended for future implementation.
Apply this diff to clean up the code:
- { - /* it's not clear if we need to draw anything. Cursor can be placed at the end of the line - var consolePixel = new Pixel(' ', foregroundColor); - - _pixelBuffer.Set((PixelBufferCoordinate)characterPoint, - (oldPixel, cp) => oldPixel.Blend(cp), consolePixel);*/ - } + // Skip newline characters as cursor positioning is handled by the console + break;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (9)
src/Consolonia.Core/Drawing/ConsoleBrush.cs(1 hunks)src/Consolonia.Core/Drawing/DrawingContextImpl.cs(8 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/RenderTarget.cs(7 hunks)src/Consolonia.Core/Infrastructure/InputLessDefaultNetConsole.cs(3 hunks)src/Consolonia.Core/Text/GlyphRunImpl.cs(1 hunks)src/Consolonia.Core/Text/TextShaper.cs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/Consolonia.Core/Drawing/PixelBufferImplementation/PixelBackground.cs
- src/Consolonia.Core/Text/GlyphRunImpl.cs
🧰 Additional context used
🪛 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/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/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)
🔇 Additional comments (15)
src/Consolonia.Core/Text/TextShaper.cs (2)
17-17:
Add null check for options.Typeface
Given that this method handles text shaping fundamentals, it should defensively handle null Typeface scenarios.
Consider adding a null check:
- var shapedBuffer = new ShapedBuffer(text, glyphInfos.Length,
- options.Typeface, 1, 0 /*todo: must be 1 for right to left?*/);
+ var typeface = options.Typeface ?? throw new ArgumentException("Typeface is required for text shaping", nameof(options));
+ var shapedBuffer = new ShapedBuffer(text, glyphInfos.Length,
+ typeface, 1, 0 /*todo: must be 1 for right to left?*/);Likely invalid or redundant comment.
🧰 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)
17-17: Consider implementing proper RTL text support
The TODO comment suggests that right-to-left text support is incomplete. This could lead to incorrect text rendering for RTL languages.
Let me help investigate the RTL text handling requirements. Would you like me to:
- Research proper BiDi implementation requirements?
- Open a GitHub issue to track RTL support?
🧰 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/Drawing/PixelBufferImplementation/PixelForeground.cs (3)
9-27: LGTM! Well-structured immutable design with rich text formatting support.
The enhanced constructor and new properties for Weight, Style, and TextDecorations provide a robust foundation for rich text formatting while maintaining immutability. The default color of White is a sensible choice.
🧰 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)
30-32: LGTM! Clean implementation preserving immutability.
The method correctly maintains all text formatting properties while creating a new instance with the shaded color.
44-45: Verify the text decoration blending behavior.
The implementation takes all styling properties (weight, style, decorations) from the above pixel. Let's verify this is the intended behavior across the codebase.
✅ Verification successful
The text decoration blending behavior is correctly implemented
Looking at the implementation in PixelForeground.cs, the blending behavior is intentionally designed to:
- Keep the original pixel's properties if the above symbol is whitespace
- Take all styling properties (weight, style, decorations) from the above pixel when blending non-whitespace symbols
This is consistent with the overall blending strategy seen in the codebase where:
- The
Blendmethod is part of a larger blending system involvingPixel,ISymbol, and their implementations - The behavior is used consistently across different pixel types (simple, drawing box)
- The implementation follows a clear pattern where the above pixel's properties take precedence during blending
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other instances of text decoration blending to ensure consistency
ast-grep --pattern 'new PixelForeground($_, $weight, $style, $decorations, $_)'
Length of output: 81
Script:
#!/bin/bash
# Let's try a broader search for PixelForeground constructor usage
rg "new PixelForeground" -A 3
# Also search for the class definition to understand the intended behavior
rg "class PixelForeground" -A 10
Length of output: 3334
Script:
#!/bin/bash
# Let's find the PixelForeground class definition and its Blend method
rg -U "class PixelForeground.*?Blend" -A 20 --multiline
# Also search for any tests related to blending behavior
rg -l "PixelForeground.*Blend|Blend.*PixelForeground" src/Tests
Length of output: 123
Script:
#!/bin/bash
# Let's try a simpler approach to find the class and its usage
cat src/Consolonia.Core/Drawing/PixelBufferImplementation/PixelForeground.cs
# Also look for any documentation or comments about blending behavior
rg -B 2 -A 2 "Blend" src/Consolonia.Core/Drawing/PixelBufferImplementation/
Length of output: 6018
src/Consolonia.Core/Drawing/PixelBufferImplementation/Pixel.cs (3)
12-14: LGTM: Background property addition
The new Background property aligns with the PR's objective to enhance console color support.
69-83: LGTM: Text decoration handling with minor suggestions
The implementation correctly detects and handles cases where a drawing box symbol overlaps with text, converting it to an underline decoration. However, the comment could be more precise.
Consider updating the comment to be more specific:
-// when a textdecoration of underline happens a DrawLine() is called over the top of the a pixel with non-zero symbol.
-// this detects this situation and eats the draw line, turning it into a textdecoration
+// When a box drawing symbol overlaps with a non-empty text character,
+// convert the box drawing into an underline text decoration to maintain text readabilityNote: The redundant condition check was already addressed in a previous review comment.
🧰 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)
[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)
22-40: Verify Color type migration across the codebase
The constructors have been updated to use Color instead of ConsoleColor. Let's verify that all usages have been updated accordingly.
✅ Verification successful
Based on the search results, I can now generate the final response:
Color type migration appears complete and consistent
The codebase scan shows:
- No remaining references to
ConsoleColortype - All
Pixelconstructor calls use the newColortype consistently - The changes appear to be properly propagated across the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for any remaining ConsoleColor usages that might need updating
# Look for ConsoleColor usages in the codebase
echo "Searching for remaining ConsoleColor usages..."
rg "ConsoleColor" -l
# Look for potential constructor calls that might need updating
echo "Searching for potential Pixel constructor calls..."
rg "new Pixel\(" -A 2
Length of output: 3523
🧰 Tools
🪛 GitHub Check: build
[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)
src/Consolonia.Core/Infrastructure/InputLessDefaultNetConsole.cs (2)
68-68: LGTM: Method signature enhancement aligns with PR objectives.
The updated signature properly introduces support for rich colors and text decorations while maintaining a clear and logical parameter order.
81-84: LGTM: Clean implementation of text decoration handling.
The implementation properly handles null checks and uses the Crayon library effectively for underline decoration.
src/Consolonia.Core/Drawing/RenderTarget.cs (1)
211-214:
Add error handling in CreateDrawingContext
The method creates a new DrawingContextImpl instance without checking if the buffer is in a valid state.
Add validation:
public IDrawingContextImpl CreateDrawingContext()
{
+ if (_bufferBuffer == null)
+ throw new InvalidOperationException("Buffer not initialized");
return new DrawingContextImpl(_consoleWindow, _bufferBuffer);
}src/Consolonia.Core/Drawing/ConsoleBrush.cs (2)
41-73: LGTM! Previous issues have been addressed.
The implementation looks good with proper null checks and pattern matching.
144-166: LGTM! Properties and interface implementation are correct.
The properties are properly implemented using the Avalonia property system.
src/Consolonia.Core/Drawing/DrawingContextImpl.cs (2)
380-390: 🛠️ Refactor suggestion
Make tab size configurable
The tab size is currently hardcoded to 8 characters. Consider making this configurable to support different console environments and user preferences.
Apply this diff to make tab size configurable:
+ private const int DefaultTabSize = 8;
+ private readonly int _tabSize;
+
+ public DrawingContextImpl(ConsoleWindow consoleWindow, PixelBuffer pixelBuffer, int? tabSize = null)
+ {
+ _consoleWindow = consoleWindow;
+ _pixelBuffer = pixelBuffer;
+ _tabSize = tabSize ?? DefaultTabSize;
+ _clipStack.Push(pixelBuffer.Size);
+ }
case '\t':
{
- const int tabSize = 8;
var consolePixel = new Pixel(' ', foregroundColor);
- for (int j = 0; j < tabSize; j++)
+ 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 += _tabSize - 1;
}Likely invalid or redundant comment.
133-140:
Consider using integer arithmetic for pixel coordinates
The current implementation uses floating-point arithmetic for pixel coordinates, which could lead to precision issues and inconsistent rendering. Consider using integer arithmetic or proper rounding.
Apply this diff to improve precision handling:
-(double top, double left) = r2.TopLeft;
-var width = r2.Width + (pen?.Thickness ?? 0);
-var height = r2.Height + (pen?.Thickness ?? 0);
+var topLeft = r2.TopLeft;
+int width = (int)Math.Ceiling(r2.Width + (pen?.Thickness ?? 0));
+int height = (int)Math.Ceiling(r2.Height + (pen?.Thickness ?? 0));
for (int x = 0; x < width; x++)
for (int y = 0; y < height; y++)
{
- int px = (int)(top + x);
- int py = (int)(left + y);
+ int px = (int)Math.Round(topLeft.X + x);
+ int py = (int)Math.Round(topLeft.Y + y);Likely invalid or redundant comment.
|
|
||
| public Pixel(ISymbol symbol, ConsoleColor foregroundColor) : this( | ||
| new PixelForeground(symbol, foregroundColor), | ||
| public Pixel(ISymbol symbol, Color foregroundColor, FontStyle style = FontStyle.Normal, FontWeight weight = FontWeight.Normal, TextDecorationCollection textDecorations = null) : this( | ||
| new PixelForeground(symbol, weight, style, textDecorations, foregroundColor), | ||
| new PixelBackground(PixelBackgroundMode.Transparent)) | ||
| { |
There was a problem hiding this comment.
Remove redundant default values in constructor parameters
The default values for style and weight parameters are redundant as they're passed through to another constructor that already defines these defaults.
Apply this diff to clean up the constructor:
-public Pixel(ISymbol symbol, Color foregroundColor, FontStyle style = FontStyle.Normal, FontWeight weight = FontWeight.Normal, TextDecorationCollection textDecorations = null) : this(
+public Pixel(ISymbol symbol, Color foregroundColor, FontStyle style, FontWeight weight, TextDecorationCollection textDecorations = null) : this(
new PixelForeground(symbol, weight, style, textDecorations, foregroundColor),
new PixelBackground(PixelBackgroundMode.Transparent))
{
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public Pixel(ISymbol symbol, ConsoleColor foregroundColor) : this( | |
| new PixelForeground(symbol, foregroundColor), | |
| public Pixel(ISymbol symbol, Color foregroundColor, FontStyle style = FontStyle.Normal, FontWeight weight = FontWeight.Normal, TextDecorationCollection textDecorations = null) : this( | |
| new PixelForeground(symbol, weight, style, textDecorations, foregroundColor), | |
| new PixelBackground(PixelBackgroundMode.Transparent)) | |
| { | |
| public Pixel(ISymbol symbol, Color foregroundColor, FontStyle style, FontWeight weight, TextDecorationCollection textDecorations = null) : this( | |
| new PixelForeground(symbol, weight, style, textDecorations, foregroundColor), | |
| new PixelBackground(PixelBackgroundMode.Transparent)) | |
| { |
🧰 Tools
🪛 GitHub Check: build
[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)
| { | ||
| _cache = new (ConsoleColor background, ConsoleColor foreground, char character)?[width, height]; | ||
| _cache = new (Color background, Color foreground, FontWeight weight, FontStyle style, TextDecorationCollection textDecorations, char character)?[width, height]; |
There was a problem hiding this comment.
Add parameter validation in InitializeCache
The method accepts ushort parameters but doesn't validate if they are non-zero. Invalid dimensions could lead to array allocation issues.
Add validation:
private void InitializeCache(ushort width, ushort height)
{
+ if (width == 0 || height == 0)
+ throw new ArgumentException("Buffer dimensions must be greater than zero");
_cache = new (Color background, Color foreground, FontWeight weight, FontStyle style, TextDecorationCollection textDecorations, char character)?[width, height];
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| { | |
| _cache = new (ConsoleColor background, ConsoleColor foreground, char character)?[width, height]; | |
| _cache = new (Color background, Color foreground, FontWeight weight, FontStyle style, TextDecorationCollection textDecorations, char character)?[width, height]; | |
| { | |
| if (width == 0 || height == 0) | |
| throw new ArgumentException("Buffer dimensions must be greater than zero"); | |
| _cache = new (Color background, Color foreground, FontWeight weight, FontStyle style, TextDecorationCollection textDecorations, char character)?[width, height]; |
| else | ||
| { | ||
| throw; | ||
| pixel = new Pixel(new PixelForeground(new SimpleSymbol('░')), new PixelBackground(PixelBackgroundMode.Colored)); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider making the fallback character configurable
The hardcoded character '░' used for uninitialized pixels could be made configurable to support different visual preferences or console capabilities.
Consider adding a constant or configuration option:
+private const char DefaultFallbackCharacter = '░';
// ...
-pixel = new Pixel(new PixelForeground(new SimpleSymbol('░')), new PixelBackground(PixelBackgroundMode.Colored));
+pixel = new Pixel(new PixelForeground(new SimpleSymbol(DefaultFallbackCharacter)), new PixelBackground(PixelBackgroundMode.Colored));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| pixel = new Pixel(new PixelForeground(new SimpleSymbol('░')), new PixelBackground(PixelBackgroundMode.Colored)); | |
| private const char DefaultFallbackCharacter = '░'; | |
| pixel = new Pixel(new PixelForeground(new SimpleSymbol(DefaultFallbackCharacter)), new PixelBackground(PixelBackgroundMode.Colored)); |
| var pixelSpread = (pixel.Background.Color, pixel.Foreground.Color, pixel.Foreground.Weight, pixel.Foreground.Style, pixel.Foreground.TextDecorations, pixel.Foreground.Symbol.GetCharacter()); | ||
| //todo: indexOutOfRange during resize | ||
| if (_cache[x, y] == pixelSpread) | ||
| continue; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize tuple allocation in rendering loop
Creating a new tuple for each pixel in the rendering loop could impact performance. Consider reusing a tuple instance.
+private (Color background, Color foreground, FontWeight weight, FontStyle style, TextDecorationCollection textDecorations, char character) _pixelSpread;
// ...
-var pixelSpread = (pixel.Background.Color, pixel.Foreground.Color, pixel.Foreground.Weight, pixel.Foreground.Style, pixel.Foreground.TextDecorations, pixel.Foreground.Symbol.GetCharacter());
+_pixelSpread.background = pixel.Background.Color;
+_pixelSpread.foreground = pixel.Foreground.Color;
+_pixelSpread.weight = pixel.Foreground.Weight;
+_pixelSpread.style = pixel.Foreground.Style;
+_pixelSpread.textDecorations = pixel.Foreground.TextDecorations;
+_pixelSpread.character = pixel.Foreground.Symbol.GetCharacter();
-if (_cache[x, y] == pixelSpread)
+if (_cache[x, y] == _pixelSpread)Committable suggestion was skipped due to low confidence.
| double horizontalRelativePosition = (double)x / width; | ||
| double verticalRelativePosition = (double)y / height; |
There was a problem hiding this comment.
Prevent division by zero in position calculations.
When width or height is 0, division by zero will occur.
-double horizontalRelativePosition = (double)x / width;
-double verticalRelativePosition = (double)y / height;
+double horizontalRelativePosition = width == 0 ? 0 : (double)x / width;
+double verticalRelativePosition = height == 0 ? 0 : (double)y / height;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| double horizontalRelativePosition = (double)x / width; | |
| double verticalRelativePosition = (double)y / height; | |
| double horizontalRelativePosition = width == 0 ? 0 : (double)x / width; | |
| double verticalRelativePosition = height == 0 ? 0 : (double)y / height; |
| private static Color InterpolateColor(IGradientBrush brush, double relativePosition) | ||
| { | ||
| IGradientStop before = null; | ||
| IGradientStop after = null; | ||
|
|
||
| foreach (var stop in brush.GradientStops) | ||
| { | ||
| if (stop.Offset <= relativePosition) | ||
| { | ||
| before = stop; | ||
| } | ||
| else if (stop.Offset >= relativePosition) | ||
| { | ||
| after = stop; | ||
| break; | ||
| } | ||
| } | ||
| if (before == null && after == null) | ||
| throw new ArgumentException("no gradientstops defined"); | ||
|
|
||
| if (before == null) | ||
| { | ||
| return after.Color; | ||
| } | ||
| if (after == null) | ||
| { | ||
| return before.Color; | ||
| } | ||
|
|
||
| double ratio = (relativePosition - before.Offset) / (after.Offset - before.Offset); | ||
| byte r = (byte)(before.Color.R + ratio * (after.Color.R - before.Color.R)); | ||
| byte g = (byte)(before.Color.G + ratio * (after.Color.G - before.Color.G)); | ||
| byte b = (byte)(before.Color.B + ratio * (after.Color.B - before.Color.B)); | ||
| byte a = (byte)(before.Color.A + ratio * (after.Color.A - before.Color.A)); | ||
|
|
||
| return Color.FromArgb(a, r, g, b); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize color interpolation using LINQ.
The current implementation can be simplified and made more efficient using LINQ.
private static Color InterpolateColor(IGradientBrush brush, double relativePosition)
{
- IGradientStop before = null;
- IGradientStop after = null;
-
- foreach (var stop in brush.GradientStops)
- {
- if (stop.Offset <= relativePosition)
- {
- before = stop;
- }
- else if (stop.Offset >= relativePosition)
- {
- after = stop;
- break;
- }
- }
+ var stops = brush.GradientStops.OrderBy(s => s.Offset).ToList();
+ if (!stops.Any())
+ throw new ArgumentException("No gradient stops defined", nameof(brush));
+
+ var before = stops.Where(s => s.Offset <= relativePosition).MaxBy(s => s.Offset);
+ var after = stops.Where(s => s.Offset > relativePosition).MinBy(s => s.Offset);
if (before == null && after == null)
throw new ArgumentException("no gradientstops defined");Committable suggestion was skipped due to low confidence.
🧰 Tools
🪛 GitHub Check: build
[warning] 189-189:
"[PossibleNullReferenceException] Possible 'System.NullReferenceException'" on /home/runner/work/Consolonia/Consolonia/src/Consolonia.Core/Drawing/ConsoleBrush.cs(189,24)
| (pixel, bb) => | ||
| { | ||
| return pixel.Blend(new Pixel(new PixelBackground(bb.Mode, bb.Color))); | ||
| }, backgroundBrush); |
There was a problem hiding this comment.
Fix closure variable capture in pixel blending
The lambda passed to _pixelBuffer.Set captures variables that are modified in the outer scope, which could lead to race conditions or unexpected behavior.
Apply this diff to pass the required values as parameters:
_pixelBuffer.Set(new PixelBufferCoordinate((ushort)px, (ushort)py),
- (pixel, bb) =>
- {
- return pixel.Blend(new Pixel(new PixelBackground(bb.Mode, bb.Color)));
- }, backgroundBrush);
+ (pixel, bb) => pixel.Blend(new Pixel(new PixelBackground(bb.Mode, bb.Color))),
+ backgroundBrush);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| (pixel, bb) => | |
| { | |
| return pixel.Blend(new Pixel(new PixelBackground(bb.Mode, bb.Color))); | |
| }, backgroundBrush); | |
| (pixel, bb) => pixel.Blend(new Pixel(new PixelBackground(bb.Mode, bb.Color))), | |
| backgroundBrush); |
🧰 Tools
🪛 GitHub Check: build
[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)
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (8)
src/Consolonia.Core/Drawing/PixelBufferImplementation/PixelForeground.cs (2)
8-26: Add XML documentation for public membersThe struct and its public members lack XML documentation, which is important for API consumers.
Add documentation like this:
+/// <summary> +/// Represents the foreground properties of a console pixel including color, font weight, style and decorations. +/// </summary> public readonly struct PixelForeground { + /// <summary> + /// Initializes a new instance of the <see cref="PixelForeground"/> struct. + /// </summary> + /// <param name="symbol">The symbol to display.</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 foreground color. Defaults to White if null.</param> public PixelForeground(...)
46-47: Remove extra empty linesMultiple empty lines at the end of the file should be removed.
src/Consolonia.Core/Drawing/PixelBufferImplementation/PixelOperations.cs (3)
10-10: Consider improving the field declaration.The magic number 32 should be better documented and possibly made into a named constant. Consider:
- Adding XML documentation explaining the significance of this value
- Making it
private constsince it appears to be immutable- Using a more descriptive name like
ColorAdjustmentStep- private static int _colorAdjustment = 32; + /// <summary> + /// The step size for color adjustments when shading or brightening. + /// A larger value creates more contrast, while a smaller value creates subtler changes. + /// </summary> + private const int ColorAdjustmentStep = 32;
12-30: Enhance XML documentation for color manipulation methods.The current documentation is minimal. Consider adding:
- More detailed descriptions of what the methods do
- Example usage scenarios
- Expected behavior with different input colors
/// <summary> - /// Make color darker + /// Darkens a color by reducing its RGB values by a fixed step. + /// This is a convenience method that uses black as the background color. /// </summary> - /// <param name="color"></param> - /// <returns></returns> + /// <param name="color">The color to shade</param> + /// <returns>A darker version of the input color</returns> + /// <example> + /// var darkRed = Colors.Red.Shade(); + /// </example>
86-91: Improve precision and formatting of GetPerceivedBrightness.Consider:
- Using
doublereturn type for better precision- Fixing formatting inconsistencies
- Adding XML documentation
+ /// <summary> + /// Calculates the perceived brightness of a color using the ITU-R BT.709 coefficients. + /// </summary> + /// <param name="color">The color to calculate brightness for</param> + /// <returns>The perceived brightness value between 0 and 255</returns> - private static int GetPerceivedBrightness(Color color) + private static double GetPerceivedBrightness(Color color) { - return (int)(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; }src/Consolonia.Core/Drawing/RenderTarget.cs (1)
213-213: Document the IsCorrupted property.The hardcoded
falsereturn value needs documentation explaining why corruption is impossible in this context or under what conditions it might change in the future.src/Consolonia.Core/Drawing/DrawingContextImpl.cs (2)
121-128: Improve error handling for unsupported sceneBrushContent.When
sceneBrushContentis null, the method silently returns. Consider raising a platform-specific error to maintain consistency with other unsupported scenarios in the codebase.if (sceneBrushContent != null) { sceneBrushContent.Render(this, Matrix.Identity); +} else { + ConsoloniaPlatform.RaiseNotSupported(18, "Null scene brush content"); } return;
395-401: Clarify newline handling with documentation.The comment about newline handling could be more explicit about the design decision.
-/* it's not clear if we need to draw anything. Cursor can be placed at the end of the line - var consolePixel = new Pixel(' ', foregroundColor); - - _pixelBuffer.Set((PixelBufferCoordinate)characterPoint, - (oldPixel, cp) => oldPixel.Blend(cp), consolePixel);*/ +/* Newline characters (\n) are handled by cursor positioning logic rather than drawing. + This ensures consistent behavior across different console implementations and + maintains compatibility with the native console's line wrapping behavior. */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
src/Consolonia.Core/Drawing/DrawingContextImpl.cs(8 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)
🧰 Additional context used
🪛 GitHub Check: build
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,88)
[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,91)
[warning] 143-143:
"[RedundantAssignment] The value passed to the method is never used because it is overwritten in the method body before being read" on /home/runner/work/Consolonia/Consolonia/src/Consolonia.Core/Drawing/DrawingContextImpl.cs(143,41)
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] 53-53:
"[BuiltInTypeReferenceStyleForMemberAccess] Built-in type reference is inconsistent with code style settings" on /home/runner/work/Consolonia/Consolonia/src/Consolonia.Core/Drawing/PixelBufferImplementation/PixelOperations.cs(53,53)
[notice] 54-54:
"[BuiltInTypeReferenceStyleForMemberAccess] Built-in type reference is inconsistent with code style settings" on /home/runner/work/Consolonia/Consolonia/src/Consolonia.Core/Drawing/PixelBufferImplementation/PixelOperations.cs(54,53)
[notice] 55-55:
"[BuiltInTypeReferenceStyleForMemberAccess] Built-in type reference is inconsistent with code style settings" on /home/runner/work/Consolonia/Consolonia/src/Consolonia.Core/Drawing/PixelBufferImplementation/PixelOperations.cs(55,53)
[notice] 70-70:
"[BuiltInTypeReferenceStyleForMemberAccess] Built-in type reference is inconsistent with code style settings" on /home/runner/work/Consolonia/Consolonia/src/Consolonia.Core/Drawing/PixelBufferImplementation/PixelOperations.cs(70,136)
🔇 Additional comments (3)
src/Consolonia.Core/Drawing/PixelBufferImplementation/PixelForeground.cs (2)
29-31: LGTM!
The Shade method correctly maintains immutability while creating a new instance with the shaded color.
34-44:
Add parameter validation and resolve TODO comment
The method needs parameter validation and the TODO comment should be addressed.
The previous review comment about adding null check for pixelAboveForeground is still valid. Additionally:
- The TODO comment about checking
default(char)needs to be resolved - The method's blending behavior should be documented
Apply this diff:
+/// <summary>
+/// Blends this foreground with the foreground above it, taking styling properties from the above foreground.
+/// </summary>
+/// <param name="pixelAboveForeground">The foreground to blend with.</param>
+/// <returns>A new PixelForeground instance with blended properties.</returns>
+/// <exception cref="ArgumentNullException">Thrown when pixelAboveForeground is null.</exception>
public PixelForeground Blend(PixelForeground pixelAboveForeground)
{
+ ArgumentNullException.ThrowIfNull(pixelAboveForeground);
- //todo: check default(char) is there
ISymbol symbolAbove = pixelAboveForeground.Symbol;
ArgumentNullException.ThrowIfNull(symbolAbove);src/Consolonia.Core/Drawing/RenderTarget.cs (1)
170-172: LGTM! Improved encapsulation with WritePixel method.
The change from WriteCharacter to WritePixel improves encapsulation by passing the entire Pixel object instead of individual attributes. This makes the code more maintainable and reduces the risk of parameter misalignment.
| public PixelForeground(ISymbol symbol, FontWeight weight = FontWeight.Normal, FontStyle style = FontStyle.Normal, TextDecorationCollection textDecorations = null, Color? color = null) | ||
| { | ||
| ArgumentNullException.ThrowIfNull(symbol); | ||
| Symbol = symbol; | ||
| Color = color; | ||
| Color = color ?? Colors.White; | ||
| Weight = weight; | ||
| Style = style; | ||
| TextDecorations = textDecorations; |
There was a problem hiding this comment.
Add null check and validation for constructor parameters
The constructor should validate the TextDecorations parameter to prevent potential null reference issues later. Consider adding parameter validation for FontWeight and FontStyle values.
Apply this diff:
public PixelForeground(ISymbol symbol, FontWeight weight = FontWeight.Normal, FontStyle style = FontStyle.Normal, TextDecorationCollection textDecorations = null, Color? color = null)
{
ArgumentNullException.ThrowIfNull(symbol);
+ textDecorations ??= new TextDecorationCollection();
+ if (!Enum.IsDefined(typeof(FontStyle), style))
+ throw new ArgumentException("Invalid font style value", nameof(style));
Symbol = symbol;
Color = color ?? Colors.White;
Weight = weight;
Style = style;
TextDecorations = textDecorations;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public PixelForeground(ISymbol symbol, FontWeight weight = FontWeight.Normal, FontStyle style = FontStyle.Normal, TextDecorationCollection textDecorations = null, Color? color = null) | |
| { | |
| ArgumentNullException.ThrowIfNull(symbol); | |
| Symbol = symbol; | |
| Color = color; | |
| Color = color ?? Colors.White; | |
| Weight = weight; | |
| Style = style; | |
| TextDecorations = textDecorations; | |
| public PixelForeground(ISymbol symbol, FontWeight weight = FontWeight.Normal, FontStyle style = FontStyle.Normal, TextDecorationCollection textDecorations = null, Color? color = null) | |
| { | |
| ArgumentNullException.ThrowIfNull(symbol); | |
| if (!Enum.IsDefined(typeof(FontWeight), weight)) | |
| throw new ArgumentException("Invalid font weight value", nameof(weight)); | |
| if (!Enum.IsDefined(typeof(FontStyle), style)) | |
| throw new ArgumentException("Invalid font style value", nameof(style)); | |
| textDecorations ??= new TextDecorationCollection(); | |
| Symbol = symbol; | |
| Color = color ?? Colors.White; | |
| Weight = weight; | |
| Style = style; | |
| TextDecorations = textDecorations; | |
| } |
| /// <summary> | ||
| /// Make color less contrast with background color | ||
| /// </summary> | ||
| /// <param name="foreground"></param> | ||
| /// <param name="background"></param> | ||
| /// <returns></returns> | ||
| public static Color Shade(this Color foreground, Color background) | ||
| { | ||
| var foregroundBrightness = GetPerceivedBrightness(foreground); | ||
| var backgroundBrightness = GetPerceivedBrightness(background); | ||
| if (foregroundBrightness > backgroundBrightness || (foregroundBrightness == backgroundBrightness && foregroundBrightness > 0)) | ||
| { | ||
| // if color is lighter than background shading should make it more dark (less contrast) | ||
| return Color.FromRgb((byte)Math.Max(0, foreground.R - _colorAdjustment), | ||
| (byte)Math.Max(0, foreground.G - _colorAdjustment), | ||
| (byte)Math.Max(0, foreground.B - _colorAdjustment)); | ||
|
|
||
| } | ||
| else | ||
| { | ||
| // if color is darker than background shading should make it more bright (less contrast) | ||
| return Color.FromRgb((byte)Math.Min(Byte.MaxValue, foreground.R + _colorAdjustment), | ||
| (byte)Math.Min(Byte.MaxValue, foreground.G + _colorAdjustment), | ||
| (byte)Math.Min(Byte.MaxValue, foreground.B + _colorAdjustment)); | ||
|
|
||
| } | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Make color more contrast with background color | ||
| /// </summary> | ||
| /// <param name="foreground"></param> | ||
| /// <param name="background"></param> | ||
| /// <returns></returns> | ||
| public static Color Brighten(this Color foreground, Color background) | ||
| { | ||
| // ReSharper disable once SwitchStatementHandlesSomeKnownEnumValuesWithDefault | ||
| switch (color) | ||
| var foregroundBrightness = GetPerceivedBrightness(foreground); | ||
| var backgroundBrightness = GetPerceivedBrightness(background); | ||
| if (foregroundBrightness > backgroundBrightness || (foregroundBrightness == backgroundBrightness && foregroundBrightness < Byte.MaxValue)) | ||
| { | ||
| // if color is lighter than background brighten should make it more bright (more contrast) | ||
| return Color.FromRgb((byte)Math.Min(Byte.MaxValue, foreground.R + _colorAdjustment), | ||
| (byte)Math.Min(Byte.MaxValue, foreground.G + _colorAdjustment), | ||
| (byte)Math.Min(Byte.MaxValue, foreground.B + _colorAdjustment)); | ||
| } | ||
| else | ||
| { | ||
| case ConsoleColor.Black: return color; | ||
| case ConsoleColor.White: return ConsoleColor.Gray; | ||
| default: | ||
| string name = Enum.GetName(color) ?? throw new NotImplementedException(); | ||
| const string dark = "Dark"; | ||
| return !name.Contains(dark, StringComparison.Ordinal) | ||
| ? Enum.Parse<ConsoleColor>(dark + name) | ||
| : ConsoleColor.Black; | ||
| // if color is darker than background brighten should make it more dark (more contrast) | ||
| return Color.FromRgb((byte)Math.Max(0, foreground.R - _colorAdjustment), | ||
| (byte)Math.Max(0, foreground.G - _colorAdjustment), | ||
| (byte)Math.Max(0, foreground.B - _colorAdjustment)); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor color adjustment logic to reduce duplication.
The color adjustment logic is duplicated across methods. Consider:
- Extracting the color adjustment logic into a helper method
- Fixing typos in comments
- Maintaining consistent formatting in Color.FromRgb calls
+ private static Color AdjustColor(Color color, int adjustment)
+ {
+ return Color.FromRgb(
+ (byte)Math.Clamp(color.R + adjustment, 0, byte.MaxValue),
+ (byte)Math.Clamp(color.G + adjustment, 0, byte.MaxValue),
+ (byte)Math.Clamp(color.B + adjustment, 0, byte.MaxValue));
+ }
public static Color Shade(this Color foreground, Color background)
{
var foregroundBrightness = GetPerceivedBrightness(foreground);
var backgroundBrightness = GetPerceivedBrightness(background);
- if (foregroundBrightness > backgroundBrightness || (foregroundBrightness == backgroundBrightness && foregroundBrightness > 0))
+ if (foregroundBrightness > backgroundBrightness ||
+ (foregroundBrightness == backgroundBrightness && foregroundBrightness > 0))
{
- // if color is lighter than background shading should make it more dark (less contrast)
- return Color.FromRgb((byte)Math.Max(0, foreground.R - _colorAdjustment),
- (byte)Math.Max(0, foreground.G - _colorAdjustment),
- (byte)Math.Max(0, foreground.B - _colorAdjustment));
+ // If color is lighter than background, shading should make it darker (less contrast)
+ return AdjustColor(foreground, -ColorAdjustmentStep);
}
- // if color is darker than background shading should make it more bright (less contrast)
- return Color.FromRgb((byte)Math.Min(Byte.MaxValue, foreground.R + _colorAdjustment),
- (byte)Math.Min(Byte.MaxValue, foreground.G + _colorAdjustment),
- (byte)Math.Min(Byte.MaxValue, foreground.B + _colorAdjustment));
+ // If color is darker than background, shading should make it brighter (less contrast)
+ return AdjustColor(foreground, ColorAdjustmentStep);Committable suggestion was skipped due to low confidence.
🧰 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)
[notice] 70-70:
"[ArrangeRedundantParentheses] Redundant parentheses" on /home/runner/work/Consolonia/Consolonia/src/Consolonia.Core/Drawing/PixelBufferImplementation/PixelOperations.cs(70,64)
[notice] 53-53:
"[BuiltInTypeReferenceStyleForMemberAccess] Built-in type reference is inconsistent with code style settings" on /home/runner/work/Consolonia/Consolonia/src/Consolonia.Core/Drawing/PixelBufferImplementation/PixelOperations.cs(53,53)
[notice] 54-54:
"[BuiltInTypeReferenceStyleForMemberAccess] Built-in type reference is inconsistent with code style settings" on /home/runner/work/Consolonia/Consolonia/src/Consolonia.Core/Drawing/PixelBufferImplementation/PixelOperations.cs(54,53)
[notice] 55-55:
"[BuiltInTypeReferenceStyleForMemberAccess] Built-in type reference is inconsistent with code style settings" on /home/runner/work/Consolonia/Consolonia/src/Consolonia.Core/Drawing/PixelBufferImplementation/PixelOperations.cs(55,53)
[notice] 70-70:
"[BuiltInTypeReferenceStyleForMemberAccess] Built-in type reference is inconsistent with code style settings" on /home/runner/work/Consolonia/Consolonia/src/Consolonia.Core/Drawing/PixelBufferImplementation/PixelOperations.cs(70,136)
| (pixel, backgroundBrush) => | ||
| { | ||
| backgroundBrush = ConsoleBrush.FromPosition(brush, x, y, (int)width, (int)height); | ||
| return pixel.Blend(new Pixel(new PixelBackground(backgroundBrush.Mode, backgroundBrush.Color))); | ||
| }, new ConsoleBrush(Colors.Transparent)); |
There was a problem hiding this comment.
Fix closure variable modification and redundant parameter.
The backgroundBrush parameter is immediately overwritten in the lambda, making the initial value redundant. Additionally, modifying the closure variable can lead to race conditions.
-_pixelBuffer.Set(new PixelBufferCoordinate((ushort)px, (ushort)py),
- (pixel, backgroundBrush) =>
- {
- backgroundBrush = ConsoleBrush.FromPosition(brush, x, y, (int)width, (int)height);
- return pixel.Blend(new Pixel(new PixelBackground(backgroundBrush.Mode, backgroundBrush.Color)));
- }, new ConsoleBrush(Colors.Transparent));
+_pixelBuffer.Set(new PixelBufferCoordinate((ushort)px, (ushort)py),
+ (pixel, _) =>
+ {
+ var pixelBrush = ConsoleBrush.FromPosition(brush, x, y, (int)width, (int)height);
+ return pixel.Blend(new Pixel(new PixelBackground(pixelBrush.Mode, pixelBrush.Color)));
+ }, null);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| (pixel, backgroundBrush) => | |
| { | |
| backgroundBrush = ConsoleBrush.FromPosition(brush, x, y, (int)width, (int)height); | |
| return pixel.Blend(new Pixel(new PixelBackground(backgroundBrush.Mode, backgroundBrush.Color))); | |
| }, new ConsoleBrush(Colors.Transparent)); | |
| (pixel, _) => | |
| { | |
| var pixelBrush = ConsoleBrush.FromPosition(brush, x, y, (int)width, (int)height); | |
| return pixel.Blend(new Pixel(new PixelBackground(pixelBrush.Mode, pixelBrush.Color))); | |
| }, null); |
🧰 Tools
🪛 GitHub Check: build
[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,88)
[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,91)
[warning] 143-143:
"[RedundantAssignment] The value passed to the method is never used because it is overwritten in the method body before being read" on /home/runner/work/Consolonia/Consolonia/src/Consolonia.Core/Drawing/DrawingContextImpl.cs(143,41)
| if (pen.Brush is LineBrush lineBrush) | ||
| lineStyle = lineBrush.LineStyle; | ||
| } | ||
|
|
||
| ConsoleBrush consoleColorBrush = ConsoleBrush.FromBrush(pen.Brush); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify null check pattern using pattern matching.
The current implementation can be simplified using C# pattern matching for better readability.
-if (pen.Brush is LineBrush lineBrush)
- lineStyle = lineBrush.LineStyle;
-
-ConsoleBrush consoleColorBrush = ConsoleBrush.FromBrush(pen.Brush);
+lineStyle = pen.Brush switch
+{
+ LineBrush lb => lb.LineStyle,
+ _ => null
+};
+
+var consoleColorBrush = ConsoleBrush.FromBrush(pen.Brush);Committable suggestion was skipped due to low confidence.
| @@ -371,49 +370,49 @@ | |||
| foreach (char c in str) | |||
| { | |||
| Point characterPoint = whereToDraw.Transform(Matrix.CreateTranslation(currentXPosition++, 0)); | |||
| ConsoleColor foregroundColor = consoleColorBrush.Color; | |||
| Color foregroundColor = consoleColorBrush.Color; | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider caching frequently used colors.
Creating new Color instances frequently can impact performance. Consider caching frequently used colors.
+private static readonly Dictionary<ConsoleBrush, Color> _colorCache = new();
+
private void DrawStringInternal(IBrush foreground, string str, IGlyphTypeface typeface, Point origin = new())
{
foreground = ConsoleBrush.FromBrush(foreground);
if (foreground is not ConsoleBrush { Mode: PixelBackgroundMode.Colored } consoleColorBrush)
{
ConsoloniaPlatform.RaiseNotSupported(4);
return;
}
- Color foregroundColor = consoleColorBrush.Color;
+ if (!_colorCache.TryGetValue(consoleColorBrush, out var foregroundColor))
+ {
+ foregroundColor = consoleColorBrush.Color;
+ _colorCache[consoleColorBrush] = foregroundColor;
+ }Committable suggestion was skipped due to low confidence.
There was an issue regarding this btw, will find later |
Im thinking eventually we need to setup the pipeline to properly validate forks as well. But also i was thinking may be better to migrate to qodana instead of jetbrains cli. Thus, if we can, let's do pull requests right in the repo for now. |
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 management across various components.Bug Fixes
Documentation
Chores
Style
ConsoleBrushinstead ofFourBitColorBrush.