Add 2 new line styles for brushes Edge and EdgeWide #199
Conversation
added support for non-uniform borders, only avalonia seems to be buggy in calculating it's boundaries with non-uniform borders. Stashing this for a better day.
…nto tomlm/borders
📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces several modifications across multiple files in the Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (5)
src/Consolonia.Core/Drawing/DrawingContextImpl.cs (1)
355-376: Redundant Pen Thickness Calculation inFillRectangleWithBrushIn the
FillRectangleWithBrushmethod, the width and height calculations include the pen thickness even when the pen is null or its thickness is zero:double width = r2.Width + (pen?.Thickness ?? 0); double height = r2.Height + (pen?.Thickness ?? 0);If the pen is not used for filling, including its thickness may lead to incorrect dimensions.
Consider adjusting the calculations to exclude pen thickness when filling with a brush:
-double width = r2.Width + (pen?.Thickness ?? 0); -double height = r2.Height + (pen?.Thickness ?? 0); +double width = r2.Width; +double height = r2.Height;Include pen thickness only when necessary.
src/Consolonia.Core/Drawing/LineBrush.cs (2)
Line range hint
23-25: Incomplete Implementation ofIImmutableBrushInterfaceThe properties
Opacity,Transform, andTransformOriginare stub implementations. ReturningnullforTransformmay cause issues if consumers expect non-null values.Ensure full implementation of the
IImmutableBrushinterface:
- Provide meaningful default values or behaviors.
- Avoid returning
nullunless explicitly allowed.- Remove or resolve TODO comments to clarify implementation status.
Consider this implementation:
public double Opacity => 1; -public ITransform Transform => null; +public ITransform Transform => Transform.Identity; public RelativePoint TransformOrigin => RelativePoint.TopLeft;Adjust according to the expected behaviors in Avalonia.
Line range hint
3-25: Address TODO Comments and Verify FunctionalityThe presence of TODO comments indicates unresolved implementation details. Not implementing the interface properly may lead to unexpected behaviors.
- Review the necessity of implementing
IImmutableBrush.- Ensure that all required interface members are correctly implemented.
- Remove or address TODO comments after resolving the issues.
This will improve code clarity and prevent potential runtime issues.
src/Consolonia.Gallery/Gallery/GalleryViews/GalleryBorders.axaml (2)
45-57: Consider adjusting the ComboBox width for better usability.The current width of 10 units seems too narrow for a ComboBox. While this might be intentional for the gallery demonstration, it could make the items difficult to read and interact with.
Consider adjusting the width or making it dynamic:
- <ComboBox Width="10"> + <ComboBox MinWidth="100">
58-84: Consider using a resource key for the image path.The image path is hardcoded multiple times. Consider defining it as a resource to improve maintainability.
Example refactor:
+ <UserControl.Resources> + <x:String x:Key="HappyImageSource">avares://Consolonia.Gallery/Resources/happy.png</x:String> + </UserControl.Resources> ... - <Image Source="avares://Consolonia.Gallery/Resources/happy.png" Width="40" Height="20" /> + <Image Source="{StaticResource HappyImageSource}" Width="40" Height="20" />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
src/Consolonia.Core/Drawing/DrawingContextImpl.cs(10 hunks)src/Consolonia.Core/Drawing/LineBrush.cs(1 hunks)src/Consolonia.Core/Drawing/LineStyle.cs(1 hunks)src/Consolonia.Core/Drawing/RectangleLinePosition.cs(1 hunks)src/Consolonia.Gallery/Gallery/GalleryViews/GalleryBorders.axaml(1 hunks)src/Consolonia.Gallery/Gallery/GalleryViews/GalleryBorders.axaml.cs(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/Consolonia.Gallery/Gallery/GalleryViews/GalleryBorders.axaml.cs
🔇 Additional comments (6)
src/Consolonia.Core/Drawing/DrawingContextImpl.cs (2)
157-161: Proper Handling of ISceneBrush in DrawRectangle
The addition appropriately handles ISceneBrush by creating its content and rendering it when available. This enhances the flexibility of DrawRectangle.
378-384:
Syntax Error in Array Initialization of _lineChars
The _lineChars array is initialized with incorrect syntax. In C#, arrays should be initialized using curly braces {} instead of square brackets []. Also, jagged arrays require proper initialization.
Apply this diff to fix the syntax error:
-private static char[][] _lineChars =
-[
+private static readonly char[][] _lineChars = new char[][]
+{
// LineStyle=Edge
- ['▕', '▁', '▏', '▔'],
+ new char[] { '▕', '▁', '▏', '▔' },
// LineStyle=EdgeWide
- ['▐', '▄', '▌', '▀'],
+ new char[] { '▐', '▄', '▌', '▀' }
-];
+};Ensure that all arrays are properly initialized with the correct syntax.
Likely invalid or redundant comment.
src/Consolonia.Core/Drawing/RectangleLinePosition.cs (1)
1-11: Introduction of RectangleLinePosition Enum
The new RectangleLinePosition enum provides a clear representation of rectangle line positions, enhancing code readability and maintainability.
src/Consolonia.Gallery/Gallery/GalleryViews/GalleryBorders.axaml (3)
1-7: LGTM: Namespace imports and UserControl declaration are well-structured.
The necessary namespaces are properly imported, including the Consolonia.Core.Drawing namespace for LineBrush usage.
64-64: Verify the existence of the happy.png resource.
Let's ensure the referenced image resource exists in the correct location.
Also applies to: 72-72, 80-80
✅ Verification successful
Image resource 'happy.png' exists at the correct location
The image resource is properly located at 'src/Consolonia.Gallery/Resources/happy.png', which matches the path referenced in the XAML file ('avares://Consolonia.Gallery/Resources/happy.png').
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the existence of the happy.png resource
fd "happy.png" --type f
Length of output: 66
10-44: Well-structured demonstration of border styles.
The WrapPanel effectively demonstrates all available border styles, including the new Edge and EdgeWide styles, with clear visual differentiation.
Let's verify that the new line styles are properly registered in the LineStyle enum:
✅ Verification successful
Border styles are properly registered and demonstrated
The LineStyle enum in src/Consolonia.Core/Drawing/LineStyle.cs correctly includes all the styles shown in the demonstration, including the new Edge and EdgeWide styles. The WrapPanel provides a clear visual representation of each available style.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify LineStyle enum contains the new styles
ast-grep --pattern 'enum LineStyle {
$$$
Edge,
EdgeWide,
$$$
}'
Length of output: 73
Script:
#!/bin/bash
# Let's first find the file containing LineStyle enum
fd -e cs -e fs "LineStyle"
# Then let's search for enum definition with ripgrep for better context
rg "enum\s+LineStyle" -A 10
Length of output: 595
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (5)
src/Example.Blazor.Components/BordersPage.razor (2)
12-18: Consider enhancing the display information.The current implementation only shows the thickness value. For a better demonstration of the Edge line style, it would be helpful to show both thickness and padding values.
Apply this diff to show both values:
<StackPanel Orientation="Orientation.Vertical"> - <TextBlock Text="@Thicknesses[i]" /> + <TextBlock Text="@($"Thickness: {Thicknesses[i]}")" /> + <TextBlock Text="@($"Padding: {Paddings[i]}")" /> </StackPanel>
25-28: Add XML documentation for public members.The
Back()method should have XML documentation to describe its purpose and behavior.Add documentation:
+ /// <summary> + /// Navigates back to the previous page. + /// </summary> + /// <returns>A task representing the asynchronous operation.</returns> public async Task Back()src/Consolonia.Core/Drawing/StreamGeometryImpl.cs (1)
119-123: Consider Throwing NotSupportedException inArcToMethodThe
ArcTomethod is currently implemented as a no-op and silently ignores arc instructions. To make the limitation explicit, consider throwing aNotSupportedExceptionor documenting that arcs are not supported.src/Consolonia.Core/Drawing/ConsoloniaRenderInterface.cs (2)
75-105: Remove or Refactor Commented-Out Code to Improve MaintainabilityThe
CreateCombinedGeometrymethod contains extensive blocks of commented-out code related to layout patches. This can reduce code readability and maintainability. Consider removing the commented-out sections if they are no longer needed, or refactoring them into separate methods or conditional compilation blocks if they may be required in the future.
47-49: Document Limited Support forGeometryCombineModeThe
CreateCombinedGeometrymethod currently supports onlyGeometryCombineMode.Excludeand throws aNotImplementedExceptionfor other modes. Consider documenting this limitation clearly in the method summary or supporting additional combine modes if required.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/Consolonia.Core/Drawing/ConsoloniaRenderInterface.cs(3 hunks)src/Consolonia.Core/Drawing/DrawingContextImpl.cs(11 hunks)src/Consolonia.Core/Drawing/Line.cs(1 hunks)src/Consolonia.Core/Drawing/RectangleLinePosition.cs(1 hunks)src/Consolonia.Core/Drawing/StreamGeometryImpl.cs(1 hunks)src/Consolonia.Gallery/Gallery/GalleryViews/GalleryBorders.axaml(1 hunks)src/Example.Blazor.Components/BordersPage.razor(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/Consolonia.Core/Drawing/RectangleLinePosition.cs
- src/Consolonia.Gallery/Gallery/GalleryViews/GalleryBorders.axaml
🔇 Additional comments (4)
src/Example.Blazor.Components/BordersPage.razor (2)
1-4: LGTM!
The component declaration and dependency injection are properly configured.
53-54: Clarify the exclusion of uniform border configurations.
The arrays have commented-out entries for uniform borders (all 0s and all 1s). If these configurations are intentionally excluded, consider adding a comment explaining why, or include them for completeness.
Let's verify if these configurations are supported with the Edge line style:
Also applies to: 74-75
✅ Verification successful
The commented-out uniform border configurations are intentionally excluded
Looking at the implementation in DrawingContextImpl.cs, the LineStyle.Edge uses special Unicode box-drawing characters ('▕', '▁', '▏', '▔') for rendering borders. A uniform border configuration of "0 0 0 0" would result in no visible border, making it redundant to include. Similarly, "1 1 1 1" would create a complete box which is already achievable with the existing configurations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any restrictions or special handling of uniform borders
rg -l "Edge.*(?:uniform|all.*(?:zeros?|ones?))" --type cs
Length of output: 59
Script:
#!/bin/bash
# Let's look at the broader context of the BordersPage.razor file
cat src/Example.Blazor.Components/BordersPage.razor
Length of output: 1610
Script:
#!/bin/bash
# Let's search for any documentation or tests related to border configurations
rg -i "border.*thickness|linestyle.*edge" --type=cs -A 5 -B 5
Length of output: 6293
src/Consolonia.Core/Drawing/DrawingContextImpl.cs (2)
499-506: Correct Syntax Errors in Array Initialization of _lineChars and _cornerChars
The arrays _lineChars and _cornerChars are using incorrect initialization syntax. In C#, arrays should be initialized using braces {} instead of square brackets [], and the inner arrays should be explicitly defined.
Apply the following diff to fix the syntax errors:
-private static char[][] _lineChars =
-[
+private static readonly char[][] _lineChars = new char[][]
+{
// LineStyle=Edge
- ['▕', '▁', '▏', '▔'],
+ new char[] { '▕', '▁', '▏', '▔' },
// LineStyle=EdgeWide
- ['▐', '▄', '▌', '▀'],
+ new char[] { '▐', '▄', '▌', '▀' }
-];
+};
-private static char[][] _cornerChars =
-{
// LineStyle=Edge we don't draw chars for edge corners
- [' ', ' ', ' ', ' ' ],
+ new char[] { ' ', ' ', ' ', ' ' },
// LineStyle=EdgeWide
- ['▗', '▖', '▘', '▝'],
+ new char[] { '▗', '▖', '▘', '▝' }
-};
+};Also applies to: 508-516
747-760: Potential Off-by-One Error in Tab Handling
In the tab character handling, currentXPosition is incremented by tabSize - 1, which may cause the next character to overlap the tab space.
Adjust the incrementation to correctly position the next character:
- currentXPosition += tabSize - 1;
+ currentXPosition += tabSize;
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
src/Consolonia.Core/Drawing/ConsoloniaRenderInterface.cs (3)
44-46: Document implementation constraints and future plansThe comments indicate this is a specialized implementation. Consider adding more detailed documentation:
- What specific border cases are supported
- What general cases are not supported
- Future plans for making it more robust
73-74: Track Avalonia bug resolutionThe code contains workarounds for an Avalonia layout bug. Consider:
- Adding a TODO to revisit this code when the bug is fixed
- Adding more details about the specific layout issues being worked around
79-86: Extract Vector adjustments into named constantsThe stroke position adjustments use magic numbers that could be difficult to maintain.
Consider extracting these into named constants:
+ private static readonly Vector RIGHT_ADJUSTMENT = new Vector(-1, 0); + private static readonly Vector BOTTOM_RIGHT_ADJUSTMENT = new Vector(-1, -1); + private static readonly Vector BOTTOM_ADJUSTMENT = new Vector(0, -1); + if (hasTopStroke) - AddStroke(ctx, topStroke.PStart, topStroke.PEnd + new Vector(-1, 0)); + AddStroke(ctx, topStroke.PStart, topStroke.PEnd + RIGHT_ADJUSTMENT); if (hasRightStroke) - AddStroke(ctx, rightStroke.PStart + new Vector(-1, 0), rightStroke.PEnd + new Vector(-1, -1)); + AddStroke(ctx, rightStroke.PStart + RIGHT_ADJUSTMENT, rightStroke.PEnd + BOTTOM_RIGHT_ADJUSTMENT);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Consolonia.Core/Drawing/ConsoloniaRenderInterface.cs(3 hunks)
🔇 Additional comments (2)
src/Consolonia.Core/Drawing/ConsoloniaRenderInterface.cs (2)
34-34: LGTM: StreamGeometry implementation added
The implementation correctly enables stream geometry support required for the new line styles.
93-97: LGTM: Clean helper method implementation
The AddStroke helper method is well-designed:
- Single responsibility
- Clear parameters
- Proper stroke creation sequence
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (9)
src/Consolonia.Core/Drawing/ConsoloniaRenderInterface.cs (3)
44-46: Document limitations and assumptionsThe comments indicate this is a specialized implementation. Consider adding more detailed documentation about:
- What types of geometries are supported
- What assumptions are made about stroke ordering
- Why only Exclude mode is supported
57-60: Extract magic numbers into named constantsThe boundary checks use magic numbers (1) which makes the code harder to understand and maintain.
+ private const int StrokeBoundaryOffset = 1; + - var hasLeftStroke = stream2.Bounds.X == 1; - var hasTopStroke = stream2.Bounds.Y == 1; + var hasLeftStroke = stream2.Bounds.X == StrokeBoundaryOffset; + var hasTopStroke = stream2.Bounds.Y == StrokeBoundaryOffset;
77-84: Extract Vector arithmetic into constantsThe stroke positioning logic uses multiple Vector operations with magic numbers. Consider extracting these into named constants for better maintainability.
+ private static readonly Vector HorizontalOffset = new Vector(-1, 0); + private static readonly Vector VerticalOffset = new Vector(0, -1); + private static readonly Vector DiagonalOffset = new Vector(-1, -1); + - AddStroke(ctx, topStroke.PStart, topStroke.PEnd + new Vector(-1, 0)); + AddStroke(ctx, topStroke.PStart, topStroke.PEnd + HorizontalOffset);src/Tests/Consolonia.Gallery.Tests/BorderTests.cs (1)
12-22: Consider expanding test coverage for border styles.While the test validates the visual representation of borders, consider adding more test cases to cover:
- Different border lengths and positions
- Corner cases (e.g., minimum size borders)
- Interaction between different border styles
- Error cases (e.g., invalid border thickness)
Example test case:
+[Test] +public async Task MinimumSizeBorderTest() +{ + await UITest.KeyInput(Key.Tab); + await UITest.AssertHasText( + "▗▖", // EdgeWide minimum size box + "▝▘" + ); +}src/Consolonia.Core/Drawing/DrawingContextImpl.cs (5)
185-186: Remove or implement commented code.Several lines of commented code suggest incomplete or planned features for stroke positioning. Either implement these features or remove the comments to maintain code clarity.
Also applies to: 192-193, 199-200, 206-207
499-515: Improve array initialization readability.Consider using named constants or an enum for array indices to improve code maintainability.
+private const int EDGE_STYLE_INDEX = 0; +private const int EDGE_WIDE_STYLE_INDEX = 1; private static char[][] _lineChars = [ - // LineStyle=Edge - ['▕', '▁', '▏', '▔'], - // LineStyle=EdgeWide - ['▐', '▄', '▌', '▀'], + // EDGE_STYLE_INDEX + ['▕', '▁', '▏', '▔'], + // EDGE_WIDE_STYLE_INDEX + ['▐', '▄', '▌', '▀'], ];
548-550: Add explicit error handling for unknown line styles.Consider adding explicit error handling when lineStyle is null and linePosition is not unknown.
-if (lineStyle == null || linePosition == RectangleLinePosition.Unknown) - lineStyle = LineStyle.SingleLine; +if (lineStyle == null) +{ + if (linePosition == RectangleLinePosition.Unknown) + lineStyle = LineStyle.SingleLine; + else + throw new ArgumentException("Line style must be specified for known positions"); +}
568-572: Add parameter validation for DrawEdgeLine method.Consider adding parameter validation to ensure color is not transparent and lineStyle is valid.
private void DrawEdgeLine(Line line, RectangleLinePosition linePosition, LineStyle lineStyle, Color color, bool includeStartSymbol, bool includeEndSymbol) { + if (color.A == 0) + throw new ArgumentException("Color cannot be transparent", nameof(color)); + if (lineStyle != LineStyle.Edge && lineStyle != LineStyle.EdgeWide) + throw new ArgumentException("Invalid line style", nameof(lineStyle)); if (line.Length == 0) return;
768-826: Improve documentation for wide character handling.The complex logic for handling wide characters would benefit from more detailed documentation explaining:
- The purpose of each pixel manipulation
- The relationship between width and pixel operations
- Edge cases being handled
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/Consolonia.Core/Drawing/ConsoloniaRenderInterface.cs(3 hunks)src/Consolonia.Core/Drawing/DrawingContextImpl.cs(11 hunks)src/Tests/Consolonia.Gallery.Tests/BorderTests.cs(1 hunks)
🔇 Additional comments (6)
src/Consolonia.Core/Drawing/ConsoloniaRenderInterface.cs (4)
34-34: LGTM: Simple and correct implementation
The implementation correctly instantiates and returns a new StreamGeometryImpl instance.
47-51: Add null checks and improve error handling
65-68: Avoid hardcoded stroke indices
91-95: LGTM: Well-structured helper method
The method is focused, well-named, and correctly implements the stroke addition logic.
src/Consolonia.Core/Drawing/DrawingContextImpl.cs (2)
76-106:
Potential index out-of-bounds issue in bitmap processing.
The nested loops accessing bitmap.GetPixel(x + 1, y) and related pixels could cause index out-of-bounds exceptions.
745-758:
Off-by-one error in tab handling.
The tab size calculation may cause incorrect positioning.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (8)
src/Consolonia.Core/Drawing/ConsoloniaRenderInterface.cs (4)
44-46: Document method limitations and intended usageThe comments indicate this is a specialized implementation for border strokes, but this isn't clear from the method signature. Consider adding XML documentation to clearly state:
- The method is specifically designed for border strokes
- Only single thickness line strokes are supported
- Only Exclude combine mode is supported
57-60: Replace magic numbers with named constantsThe stroke position calculations use magic numbers (1) which makes the code harder to understand and maintain.
+ private const double StrokeThickness = 1.0; + private const double StrokeOffset = 1.0; + - var hasLeftStroke = stream2.Bounds.X.IsNearlyEqual(1); - var hasTopStroke = stream2.Bounds.Y.IsNearlyEqual(1); - var hasRightStroke = (stream1.Bounds.Width - stream2.Bounds.Width).IsNearlyEqual(stream2.Bounds.X + 1); - var hasBottomStroke = (stream1.Bounds.Height - stream2.Bounds.Height).IsNearlyEqual(stream2.Bounds.Y + 1); + var hasLeftStroke = stream2.Bounds.X.IsNearlyEqual(StrokeThickness); + var hasTopStroke = stream2.Bounds.Y.IsNearlyEqual(StrokeThickness); + var hasRightStroke = (stream1.Bounds.Width - stream2.Bounds.Width).IsNearlyEqual(stream2.Bounds.X + StrokeOffset); + var hasBottomStroke = (stream1.Bounds.Height - stream2.Bounds.Height).IsNearlyEqual(stream2.Bounds.Y + StrokeOffset);
75-82: Vector adjustments need documentation and constantsThe stroke position adjustments using Vector seem brittle and the reasoning behind the specific offsets isn't clear.
+ // Constants for stroke position adjustments + private static readonly Vector HorizontalOffset = new Vector(-1, 0); + private static readonly Vector VerticalOffset = new Vector(0, -1); + private static readonly Vector CornerOffset = new Vector(-1, -1); + if (hasTopStroke) - AddStroke(ctx, topStroke.PStart, topStroke.PEnd + new Vector(-1, 0)); + AddStroke(ctx, topStroke.PStart, topStroke.PEnd + HorizontalOffset); if (hasRightStroke) - AddStroke(ctx, rightStroke.PStart + new Vector(-1, 0), rightStroke.PEnd + new Vector(-1, -1)); + AddStroke(ctx, rightStroke.PStart + HorizontalOffset, rightStroke.PEnd + CornerOffset); if (hasBottomStroke) - AddStroke(ctx, bottomStroke.PStart + new Vector(0,-1), bottomStroke.PEnd + new Vector(-1, -1)); + AddStroke(ctx, bottomStroke.PStart + VerticalOffset, bottomStroke.PEnd + CornerOffset); if (hasLeftStroke) - AddStroke(ctx, leftStroke.PStart, leftStroke.PEnd + new Vector(0, -1)); + AddStroke(ctx, leftStroke.PStart, leftStroke.PEnd + VerticalOffset);
88-92: Make figure parameters more explicitThe boolean parameters in BeginFigure and EndFigure could be more explicit about their meaning.
- ctx.BeginFigure(start, false); + const bool isFilled = false; + ctx.BeginFigure(start, isFilled); ctx.LineTo(end); - ctx.EndFigure(true); + const bool isClosed = true; + ctx.EndFigure(isClosed);src/Consolonia.Core/Drawing/DrawingContextImpl.cs (4)
129-212: Consider adding documentation for the stroke position inference logic.The focal point calculation and stroke position inference logic is complex and would benefit from detailed documentation explaining the geometric principles behind it.
285-293: Consider refactoring rectangle drawing for better maintainability.The repeated calls to
DrawBoxLineInternalcould be simplified using a loop over an array of line positions and their corresponding points.-DrawBoxLineInternal(pen, new Line(r.TopLeft, false, (int)r.Width), RectangleLinePosition.Top); -DrawBoxLineInternal(pen, new Line(r.BottomLeft, false, (int)r.Width), RectangleLinePosition.Bottom); -DrawBoxLineInternal(pen, new Line(r.TopLeft, true, (int)r.Height), RectangleLinePosition.Left); -DrawBoxLineInternal(pen, new Line(r.TopRight, true, (int)r.Height), RectangleLinePosition.Right); +var lines = new[] { + (new Line(r.TopLeft, false, (int)r.Width), RectangleLinePosition.Top), + (new Line(r.BottomLeft, false, (int)r.Width), RectangleLinePosition.Bottom), + (new Line(r.TopLeft, true, (int)r.Height), RectangleLinePosition.Left), + (new Line(r.TopRight, true, (int)r.Height), RectangleLinePosition.Right) +}; +foreach (var (line, position) in lines) + DrawBoxLineInternal(pen, line, position);
Line range hint
569-716: Add bounds checking for edge line drawing.The edge line drawing implementation should validate the line length and buffer bounds before drawing.
private void DrawEdgeLine(Line line, RectangleLinePosition linePosition, LineStyle lineStyle, Color color, bool includeStartSymbol, bool includeEndSymbol) { if (line.Length == 0) return; + if (line.Length < 0) + throw new ArgumentException("Line length must be non-negative", nameof(line)); + + // Validate that the line fits within the buffer + var endPoint = line.Vertical + ? new Point(line.PStart.X, line.PStart.Y + line.Length - 1) + : new Point(line.PStart.X + line.Length - 1, line.PStart.Y); + if (!_pixelBuffer.Size.Contains(line.PStart) || !_pixelBuffer.Size.Contains(endPoint)) + return;
769-827: Consider simplifying wide character handling.The wide character handling logic is complex and hard to follow. Consider extracting the pixel manipulation logic into separate methods with clear documentation.
+private void HandleEmptyPixel(Point characterPoint) +{ + double targetX = characterPoint.X - 1; + if (targetX >= 0) + _pixelBuffer.Set( + (PixelBufferCoordinate)new Point(targetX, characterPoint.Y), + oldPixel2 => new Pixel( + new PixelForeground(new SimpleSymbol(' '), Colors.Transparent), + oldPixel2.Background)); +} + +private void HandleWidePixel(Point characterPoint, int width) +{ + for (ushort i = 1; i < width; i++) + { + double targetX = characterPoint.X + i; + if (targetX < _pixelBuffer.Size.Width) + _pixelBuffer.Set( + (PixelBufferCoordinate)new Point(targetX, characterPoint.Y), + oldPixel2 => new Pixel( + new PixelForeground(new SimpleSymbol(' '), + Colors.Transparent), oldPixel2.Background)); + } +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/Consolonia.Core/Drawing/ConsoloniaRenderInterface.cs(3 hunks)src/Consolonia.Core/Drawing/DrawingContextImpl.cs(11 hunks)src/Consolonia.Core/Drawing/StreamGeometryImpl.cs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Consolonia.Core/Drawing/StreamGeometryImpl.cs
🔇 Additional comments (6)
src/Consolonia.Core/Drawing/ConsoloniaRenderInterface.cs (2)
34-34: LGTM: Simple and correct implementation
The implementation properly instantiates a new StreamGeometryImpl instance, replacing the previous NotImplementedException.
65-68: Hardcoded stroke indices make the code fragile
This is a duplicate of a previous review comment. The code assumes a specific order of strokes which makes it fragile.
src/Consolonia.Core/Drawing/DrawingContextImpl.cs (4)
76-106: Index out-of-bounds risk in bitmap processing.
The nested loops accessing x + 1 and y + 1 could lead to IndexOutOfRangeException.
499-516: Fix array initialization syntax.
The array initialization syntax for _lineChars and _cornerChars needs correction.
746-759: Fix tab handling off-by-one error.
The tab position calculation needs adjustment.
549-567: Verify Edge and EdgeWide line style rendering.
The implementation of Edge and EdgeWide line styles looks correct, but please ensure:
- The line styles render correctly at different positions
- The line styles work correctly with different background colors
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
src/Consolonia.Core/Drawing/DrawingContextImpl.cs (2)
211-228: Clean up commented code.There are several commented-out code blocks that should be removed if they're no longer needed.
- //if (strokeBottom != null) - // strokeLeft = new Line(strokeLeft.PStart, strokeBottom.PStart, strokeLeft.SourceGeometry, strokeLeft.Transform); DrawBoxLineInternal(pen, strokeLeft, RectangleLinePosition.Left); if (strokeTop != null) - //if (strokeRight != null) - // strokeTop = new Line(strokeTop.PStart, strokeRight.PStart, strokeTop.SourceGeometry, strokeTop.Transform); DrawBoxLineInternal(pen, strokeTop, RectangleLinePosition.Top); if (strokeRight != null) - //if (strokeBottom != null) - // strokeRight = new Line(strokeRight.PStart, strokeBottom.PEnd, strokeRight.SourceGeometry, strokeRight.Transform); DrawBoxLineInternal(pen, strokeRight, RectangleLinePosition.Right); if (strokeBottom != null) - //if (strokeLeft != null) - // strokeBottom = new Line(strokeLeft.PEnd, strokeBottom.PEnd, strokeBottom.SourceGeometry, strokeBottom.Transform); DrawBoxLineInternal(pen, strokeBottom, RectangleLinePosition.Bottom);
Line range hint
567-716: Consider extracting symbol selection logic for better maintainability.While the implementation is thorough, the symbol selection logic could be extracted into a separate method to improve readability and maintainability.
Consider refactoring:
+private (ISymbol startSymbol, ISymbol middleSymbol, ISymbol endSymbol) GetEdgeSymbols( + RectangleLinePosition linePosition, int styleIndex) +{ + return linePosition switch + { + RectangleLinePosition.Left => ( + new SimpleSymbol(CornerChars[styleIndex][TopLeft]), + new SimpleSymbol(EdgeChars[styleIndex][(int)RectangleLinePosition.Left]), + new SimpleSymbol(CornerChars[styleIndex][BottomLeft])), + RectangleLinePosition.Top => ( + new SimpleSymbol(CornerChars[styleIndex][TopLeft]), + new SimpleSymbol(EdgeChars[styleIndex][(int)RectangleLinePosition.Top]), + new SimpleSymbol(CornerChars[styleIndex][TopRight])), + RectangleLinePosition.Right => ( + new SimpleSymbol(CornerChars[styleIndex][TopRight]), + new SimpleSymbol(EdgeChars[styleIndex][(int)RectangleLinePosition.Right]), + new SimpleSymbol(CornerChars[styleIndex][BottomRight])), + RectangleLinePosition.Bottom => ( + new SimpleSymbol(CornerChars[styleIndex][BottomLeft]), + new SimpleSymbol(EdgeChars[styleIndex][(int)RectangleLinePosition.Bottom]), + new SimpleSymbol(CornerChars[styleIndex][BottomRight])), + _ => throw new NotImplementedException("This shouldn't happen") + }; +}Then simplify the DrawEdgeLine method:
private void DrawEdgeLine(Line line, RectangleLinePosition linePosition, LineStyle lineStyle, Color color, bool includeStartSymbol, bool includeEndSymbol) { if (line.Length == 0) return; - ISymbol startSymbol; - ISymbol middleSymbol; - ISymbol endSymbol; int iStyle = (int)lineStyle - (int)LineStyle.Edge; - switch (linePosition) - { - case RectangleLinePosition.Left: - startSymbol = new SimpleSymbol(CornerChars[iStyle][TopLeft]); - // ... removed for brevity - default: - throw new NotImplementedException("This shouldn't happen"); - } + var (startSymbol, middleSymbol, endSymbol) = GetEdgeSymbols(linePosition, iStyle);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Consolonia.Core/Drawing/DrawingContextImpl.cs(10 hunks)
🔇 Additional comments (4)
src/Consolonia.Core/Drawing/DrawingContextImpl.cs (4)
31-52: LGTM! Clean implementation of the new line style characters.
The implementation uses modern C# collection expressions syntax and appropriate Unicode box-drawing characters for both edge styles and corners.
152-231: LGTM! Well-structured implementation of geometry drawing for Edge styles.
The implementation properly handles both Edge/EdgeWide and traditional line styles, with clean separation of concerns between position inference and drawing logic.
272-275: LGTM! Clean update to rectangle drawing.
The implementation correctly uses the new position-aware DrawBoxLineInternal method for each side of the rectangle.
401-438: Consider additional validation for focal point calculation.
While the focal point calculation using averages is clever, it might have edge cases with highly asymmetric geometries. Consider adding validation for such cases.
Consider adding validation:
private static RectangleLinePosition[] InferStrokePositions(StreamGeometryImpl streamGeometry)
{
+ if (streamGeometry.Strokes.Count == 0)
+ return Array.Empty<RectangleLinePosition>();
+
double focalPointX = streamGeometry.Strokes.Average(stroke =>
stroke.PStart.X + Math.Abs(stroke.PStart.X - stroke.PEnd.X) / 2);
double focalPointY = streamGeometry.Strokes.Average(stroke =>
stroke.PStart.Y + Math.Abs(stroke.PStart.Y - stroke.PEnd.Y) / 2);
+
+ // Handle case where all strokes are at the same position
+ if (streamGeometry.Strokes.All(s => s.PStart.X == focalPointX && s.PStart.Y == focalPointY))
+ return Enumerable.Repeat(RectangleLinePosition.Unknown, streamGeometry.Strokes.Count).ToArray();
|
|
||
| var extractColorCheckPlatformSupported = | ||
| ExtractColorOrNullWithPlatformCheck(pen, out var lineStyle); | ||
| if (extractColorCheckPlatformSupported == null) |
There was a problem hiding this comment.
Does it make sense to ignore not supported? Could be better to notify it's not going to be painted with an exception?
|
@tomlm I left questions mostly and disabled auto-merge. Feel free to merge please. |
Edge uses 1/8 wide vertical/horizontal chars
EdgeWide uses 1/4 vertical/horizontal block chars (remember the QuadPixels?)
Benefits of Edge and EdgeWide (and Bold) is that it is tight up next to the edge of content, which means that you don't end up with double background color effects like this:
Non-uniform borders work now too (as long as you set appropriate padding.
Related Avalonia bug AvaloniaUI/Avalonia#17752 filed