Add support for Surrogate Unicode sequences #139
Conversation
Core Changes * Changed symbols to Rune class * Threaded Rune through the pipeline * Added gallery samples with surrogate, emoji and right to left examples. Misc changes * fixed type with strikthorugh=>strikethrough * Added debugDisplay attributes to help with debugging
WalkthroughThis pull request introduces multiple changes across various files in the Consolonia project, focusing on improving text rendering and handling, particularly for complex emojis and glyphs. Key modifications include the correction of constants, the introduction of new properties and methods to support emoji rendering, and enhancements to existing methods for better clarity and functionality. The project dependencies are also updated, reflecting a shift in library usage. Changes
Possibly related PRs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (11)
src/Consolonia.Core/Drawing/PixelBufferImplementation/ISymbol.cs (1)
14-18: Consider enhancing method documentation.While the addition of XML documentation is good, the method's documentation could be more descriptive:
- What does "blend" mean in this context?
- What is the expected behavior when blending symbols?
- What are the constraints or requirements for the symbolAbove parameter?
- What type of ISymbol is returned?
Consider expanding the documentation like this:
/// <summary> -/// Blend 2 symbols together +/// Combines two symbols to create a composite symbol, typically used for overlapping UI elements /// </summary> -/// <param name="symbolAbove"></param> +/// <param name="symbolAbove">The symbol to blend on top of the current symbol</param> -/// <returns></returns> +/// <returns>A new ISymbol representing the blended result</returns>src/Consolonia.Core/Drawing/PixelBufferImplementation/SimpleSymbol.cs (1)
21-21: Consider using an auto-propertyThe current property implementation can be simplified.
- public Rune Rune => _rune; + public Rune Rune { get; }🧰 Tools
🪛 GitHub Check: build
[notice] 21-21:
"[ConvertToAutoPropertyWhenPossible] Convert into auto-property" on /home/runner/work/Consolonia/Consolonia/src/Consolonia.Core/Drawing/PixelBufferImplementation/SimpleSymbol.cs(21,21)src/Consolonia.Gallery/Gallery/GalleryViews/GalleryTextBlock.axaml (2)
33-37: LGTM! Consider adding tooltips for accessibility.The surrogate characters demonstration is well-structured and includes good examples of both emoji and complex script characters.
Consider adding tooltips to explain what each character represents, especially for the Osage script example:
- <SelectableTextBlock Text="𐓏𐓘𐓻𐓘𐓻𐓟 𐒻𐓟"/> + <SelectableTextBlock Text="𐓏𐓘𐓻𐓘𐓻𐓟 𐒻𐓟" ToolTip="Example of Osage script (Native American language)"/>
38-41: Consider improving the RTL demonstration clarity.While the RTL implementation is correct, the current layout might be confusing to users. The label "RightToLeft:" appears at the end due to the RTL flow direction.
Consider this alternative structure for better clarity:
- <WrapPanel FlowDirection="RightToLeft"> - <SelectableTextBlock Text="من اليمين إلى اليسار" /> - <SelectableTextBlock Text="RightToLeft:"/> - </WrapPanel> + <StackPanel> + <TextBlock Text="Right-to-Left Text Example:" /> + <WrapPanel FlowDirection="RightToLeft"> + <SelectableTextBlock Text="من اليمين إلى اليسار" ToolTip="Arabic text: 'From right to left'" /> + </WrapPanel> + </StackPanel>This change would:
- Keep the label outside the RTL context for better readability
- Add a tooltip explaining the Arabic text
- Use a more descriptive label
src/Tests/Consolonia.TestsCore/UnitTestConsole.cs (2)
147-147: Document the caret character choiceThe Unicode character 'Ꮖ' (Cherokee Letter Tsi) is being used as a caret symbol. Consider adding a comment explaining why this specific character was chosen, or define it as a named constant for better maintainability.
- Rune rune = pixel.IsCaret ? new Rune('Ꮖ') : pixel.Foreground.Symbol.Rune; + private const char CARET_CHAR = 'Ꮖ'; // Cherokee Letter Tsi - chosen for its distinct visual appearance + Rune rune = pixel.IsCaret ? new Rune(CARET_CHAR) : pixel.Foreground.Symbol.Rune;
148-148: Address the cursor drawing TODO commentThe comment indicates an unresolved issue with cursor drawing. This should be investigated and resolved, especially since it's related to the caret functionality we're currently modifying.
Would you like me to help investigate the cursor drawing issue or create a GitHub issue to track this?
src/Consolonia.Core/Drawing/RenderTarget.cs (2)
137-141: Improve tuple declaration readability.While the change to use
Runeis correct, the tuple declaration could be more readable.Consider this more concise and aligned format:
-(Color background, Color foreground, FontWeight weight, FontStyle style, TextDecorationCollection - textDecorations, Rune rune) - pixelSpread = (pixel.Background.Color, pixel.Foreground.Color, pixel.Foreground.Weight, - pixel.Foreground.Style, pixel.Foreground.TextDecorations, - pixel.Foreground.Symbol.Rune); +(Color background, Color foreground, FontWeight weight, FontStyle style, + TextDecorationCollection textDecorations, Rune rune) pixelSpread = ( + pixel.Background.Color, + pixel.Foreground.Color, + pixel.Foreground.Weight, + pixel.Foreground.Style, + pixel.Foreground.TextDecorations, + pixel.Foreground.Symbol.Rune +);
24-24: Consider performance implications of Rune usage.While the switch to
Runeis necessary for proper Unicode support, it's worth noting that operations onRunemight have different performance characteristics compared tochar. Consider:
- Benchmarking the rendering performance with various Unicode ranges
- Adding performance tests to catch any regressions
- Documenting any performance trade-offs made for Unicode support
Would you like help setting up performance benchmarks for these changes?
Also applies to: 137-141, 207-210
src/Consolonia.Core/Drawing/DrawingContextImpl.cs (2)
165-177: Consider simplifying the nested loop structureWhile the current implementation is correct, consider extracting the pixel drawing logic into a separate method to reduce nesting and improve readability.
-for (int x = 0; x < width; x++) - for (int y = 0; y < height; y++) - { - int px = (int)(r2.TopLeft.X + x); - int py = (int)(r2.TopLeft.Y + y); - - ConsoleBrush backgroundBrush = ConsoleBrush.FromPosition(brush, x, y, (int)width, (int)height); - CurrentClip.ExecuteWithClipping(new Point(px, py), () => - { - _pixelBuffer.Set(new PixelBufferCoordinate((ushort)px, (ushort)py), - (pixel, bb) => pixel.Blend(new Pixel(new PixelBackground(bb.Mode, bb.Color))), - backgroundBrush); - }); - } +private void DrawRectanglePixel(int x, int y, Rect r2, IBrush brush, double width, double height) +{ + int px = (int)(r2.TopLeft.X + x); + int py = (int)(r2.TopLeft.Y + y); + + ConsoleBrush backgroundBrush = ConsoleBrush.FromPosition(brush, x, y, (int)width, (int)height); + CurrentClip.ExecuteWithClipping(new Point(px, py), () => + { + _pixelBuffer.Set(new PixelBufferCoordinate((ushort)px, (ushort)py), + (pixel, bb) => pixel.Blend(new Pixel(new PixelBackground(bb.Mode, bb.Color))), + backgroundBrush); + }); +} + +for (int x = 0; x < width; x++) + for (int y = 0; y < height; y++) + DrawRectanglePixel(x, y, r2, brush, width, height);
513-514: Extract tab size as a constantThe tab size (8) should be extracted as a named constant at the class level for better maintainability and configuration.
+ private const int TabSize = 8; - const int tabSize = 8;src/Consolonia.Core/Drawing/PixelBufferImplementation/DrawingBoxSymbol.cs (1)
24-24: Simplify object creation using target-typed 'new'Since the type
Runeis evident from the property return type, you can use a target-typednewexpression to make the code more concise.-public Rune Rune => new Rune(GetBoxSymbol()); +public Rune Rune => new(GetBoxSymbol());🧰 Tools
🪛 GitHub Check: build
[notice] 24-24:
"[ArrangeObjectCreationWhenTypeEvident] Redundant type specification" on /home/runner/work/Consolonia/Consolonia/src/Consolonia.Core/Drawing/PixelBufferImplementation/DrawingBoxSymbol.cs(24,33)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (12)
src/Consolonia.Core/Drawing/DrawingContextImpl.cs(7 hunks)src/Consolonia.Core/Drawing/PixelBufferImplementation/DrawingBoxSymbol.cs(4 hunks)src/Consolonia.Core/Drawing/PixelBufferImplementation/ISymbol.cs(1 hunks)src/Consolonia.Core/Drawing/PixelBufferImplementation/Pixel.cs(2 hunks)src/Consolonia.Core/Drawing/PixelBufferImplementation/PixelBackground.cs(1 hunks)src/Consolonia.Core/Drawing/PixelBufferImplementation/PixelForeground.cs(1 hunks)src/Consolonia.Core/Drawing/PixelBufferImplementation/SimpleSymbol.cs(1 hunks)src/Consolonia.Core/Drawing/RenderTarget.cs(4 hunks)src/Consolonia.Core/Infrastructure/InputLessDefaultNetConsole.cs(1 hunks)src/Consolonia.Core/Text/GlyphTypeface.cs(1 hunks)src/Consolonia.Gallery/Gallery/GalleryViews/GalleryTextBlock.axaml(1 hunks)src/Tests/Consolonia.TestsCore/UnitTestConsole.cs(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- src/Consolonia.Core/Drawing/PixelBufferImplementation/PixelBackground.cs
- src/Consolonia.Core/Text/GlyphTypeface.cs
🧰 Additional context used
🪛 GitHub Check: build
src/Consolonia.Core/Drawing/PixelBufferImplementation/DrawingBoxSymbol.cs
[notice] 24-24:
"[ArrangeObjectCreationWhenTypeEvident] Redundant type specification" on /home/runner/work/Consolonia/Consolonia/src/Consolonia.Core/Drawing/PixelBufferImplementation/DrawingBoxSymbol.cs(24,33)
src/Consolonia.Core/Drawing/PixelBufferImplementation/PixelForeground.cs
[warning] 14-14:
"[ConstantNullCoalescingCondition] '??' left operand is never null" on /home/runner/work/Consolonia/Consolonia/src/Consolonia.Core/Drawing/PixelBufferImplementation/PixelForeground.cs(14,32)
src/Consolonia.Core/Drawing/PixelBufferImplementation/SimpleSymbol.cs
[notice] 21-21:
"[ConvertToAutoPropertyWhenPossible] Convert into auto-property" on /home/runner/work/Consolonia/Consolonia/src/Consolonia.Core/Drawing/PixelBufferImplementation/SimpleSymbol.cs(21,21)
🔇 Additional comments (19)
src/Consolonia.Core/Drawing/PixelBufferImplementation/ISymbol.cs (2)
1-1: LGTM! Appropriate namespace import for Rune support.
The addition of System.Text import is necessary for the Rune type, which provides better Unicode handling capabilities including surrogate pairs.
7-10: Excellent improvement to Unicode handling.
The transition from GetCharacter() to Rune property is a solid architectural choice that better supports Unicode surrogate pairs and follows modern C# practices.
Let's verify the impact of this breaking change:
✅ Verification successful
Breaking change has been properly implemented across the codebase
The verification shows that all implementations of ISymbol (SimpleSymbol and DrawingBoxSymbol) have correctly implemented the Rune property, and there are no remaining references to the old GetCharacter() method. The change has been consistently applied throughout the codebase:
SimpleSymbolimplementsRuneproperty returning_runefieldDrawingBoxSymbolimplementsRuneproperty usingGetBoxSymbol()- All usages across the codebase reference the new
Runeproperty
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all implementations of ISymbol to ensure they're updated
# Expected: All implementations should now implement the Rune property instead of GetCharacter
# Search for ISymbol implementations
ast-grep --pattern 'class $_ : $$$ISymbol$$$'
# Search for any remaining GetCharacter references that might need updating
rg "GetCharacter"
Length of output: 65
Script:
#!/bin/bash
# Let's try a more comprehensive search approach
# Find all files that might contain ISymbol implementations
fd "\.cs$" | xargs rg -l "ISymbol"
# Search for class implementations with different syntax
rg "class.*:.*ISymbol" -A 5
# Look for files containing "Rune" to see the implementation
rg "Rune" -A 2
Length of output: 7419
src/Consolonia.Core/Drawing/PixelBufferImplementation/SimpleSymbol.cs (3)
1-6: LGTM! Good debugging improvements
The addition of the DebuggerDisplay attribute will improve debugging experience by showing the actual Unicode character.
23-28: LGTM! Clean and efficient implementation
The simplified implementations of IsWhiteSpace() and Blend() are clear and leverage the Rune type's built-in functionality correctly.
9-19: Verify surrogate pair handling
The switch to Rune type is a good improvement for Unicode support. The implementation looks correct, but let's verify the surrogate pair handling.
src/Consolonia.Core/Drawing/PixelBufferImplementation/PixelForeground.cs (2)
7-7: LGTM: Improved debugging display for Unicode characters
The new DebuggerDisplay attribute enhances debugging by showing the actual Unicode character through the Rune property, which is particularly useful when working with surrogate pairs and emojis.
Line range hint 39-46: Verify Unicode blending behavior
The Blend method should be tested with various Unicode scenarios:
- Surrogate pairs
- Right-to-left text
- Combining characters
Let's verify the Unicode handling in the codebase:
✅ Verification successful
Let me gather more specific information about the Unicode handling implementation.
Based on the gathered information, I can now provide the final response:
Unicode handling is properly implemented with Rune support
The codebase correctly handles Unicode through the following implementations:
- Uses
System.Text.Runesfor character representation instead of rawchar - Implements proper blending through
ISymbolinterface withRuneproperty - Handles control characters and zero-width characters (e.g.,
\u200B) - Supports box-drawing characters with proper Unicode code points
The concerns about RTL and surrogate pairs are addressed by the framework-level Rune implementation, which inherently handles these cases correctly.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for Unicode-related test cases and handling
# Look for Unicode test cases
echo "Searching for Unicode-related tests..."
rg -l "surrogate|emoji|RTL|combining\scharacter" --type cs
# Check for Unicode-specific handling in Symbol implementations
echo "Checking Symbol implementations for Unicode handling..."
ast-grep --pattern 'class $_ : ISymbol {
$$$
Rune $_ { get; }
$$$
}'
Length of output: 386
Script:
#!/bin/bash
# Check Symbol.Blend implementation and related tests
echo "Checking Symbol.Blend implementation..."
rg -A 10 "Blend\(" --type cs
# Look for Rune-related tests
echo "Checking Rune-related tests..."
rg -A 5 "Rune" --type cs --glob "*Test*.cs"
# Check for any Unicode character handling
echo "Checking Unicode character handling..."
rg -A 5 "char|Char|Unicode" --type cs --glob "src/Consolonia.Core/Drawing/**/*.cs"
Length of output: 29330
🧰 Tools
🪛 GitHub Check: build
[warning] 14-14:
"[ConstantNullCoalescingCondition] '??' left operand is never null" on /home/runner/work/Consolonia/Consolonia/src/Consolonia.Core/Drawing/PixelBufferImplementation/PixelForeground.cs(14,32)
src/Consolonia.Core/Drawing/PixelBufferImplementation/Pixel.cs (3)
2-11: LGTM! Well-structured debugging support added.
The DebuggerDisplay attribute provides a clear and informative view of the pixel's state during debugging, showing both the Unicode character (via Rune) and its associated colors.
25-27: LGTM! Good improvement for Unicode support.
The switch to Rune from char is a positive change that properly handles surrogate pairs and other Unicode sequences.
25-27: Verify the removal of byte-based drawing box symbol constructor.
The removal of public Pixel(byte drawingBoxSymbol, Color foregroundColor) constructor might affect existing box-drawing functionality. Let's verify all usages have been properly updated.
✅ Verification successful
Based on the search results, I can now generate the final response:
The removal of byte-based constructor is safe as the box-drawing functionality has been properly migrated
The codebase shows that box-drawing symbols are now handled through the DrawingBoxSymbol struct (src/Consolonia.Core/Drawing/PixelBufferImplementation/DrawingBoxSymbol.cs), which implements ISymbol. The removed constructor is not used anywhere in the codebase, and box drawing is now done through the SimpleSymbol or DrawingBoxSymbol types, which are properly passed to the remaining Pixel constructors.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for potential usages of the removed constructor with box drawing symbols
rg -l "new Pixel\(\s*0x[0-9a-fA-F]+\s*,|new Pixel\(\s*[0-9]+\s*,"
# Search for box drawing related code that might need updates
rg -i "box.*draw|draw.*box|[│─┌┐└┘├┤┬┴┼]"
Length of output: 4879
src/Tests/Consolonia.TestsCore/UnitTestConsole.cs (2)
Line range hint 89-92: LGTM: KeyInput method signature enhancement
The addition of the optional modifiers parameter with a default value maintains backward compatibility while providing enhanced input handling capabilities.
147-149: LGTM: Proper Unicode handling with Rune
The switch to using Rune instead of char is a solid improvement that properly supports Unicode surrogate pairs and complex characters. The implementation correctly handles both the caret and regular symbols.
src/Consolonia.Core/Drawing/RenderTarget.cs (2)
207-210: Clarify handling of specific Unicode characters.
The control character handling is correct, but there's a commented condition about specific Chinese characters ('保' or '哥'). This suggests potential issues with certain Unicode ranges that should be addressed.
Let's check for similar patterns in tests:
#!/bin/bash
# Description: Look for Unicode-related test cases
# Expect: Comprehensive Unicode testing including Chinese characters
rg -g '*Test*.cs' '保|哥|Rune.IsControl'Consider:
- Documenting why these specific characters were problematic
- Adding comprehensive tests for various Unicode ranges
- Creating a configuration option for character substitution rules
24-24: LGTM! Verify cache initialization.
The switch from char to Rune in the cache type is appropriate for proper Unicode support. This change enables handling of surrogate pairs and other Unicode sequences correctly.
Let's verify the cache initialization is consistent:
✅ Verification successful
The previous command had syntax issues. Let's try a broader search to understand the cache initialization and usage patterns. We'll look for:
- How
_cacheis initialized - Any Rune-related array initialization
- The field declaration pattern itself
This will help us verify if the change from char to Rune is consistently implemented throughout the codebase.
Let's try a broader search pattern to catch any cache-related code and Rune array initialization. The previous patterns might have been too specific. Also, let's look at the actual file content to understand the context better.
Cache initialization and usage is properly implemented with Rune type
The verification shows that:
- The cache field is correctly declared with Rune type
- Cache initialization in
InitializeCachemethod properly uses the same tuple type with Rune - Cache usage in
RenderToDevicemethod correctly compares and assigns Rune values - The implementation is consistent throughout the file with no remnants of char-based cache
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining char usages in cache-related code
# Expect: No direct char usage in cache initialization or access
rg -g '*.cs' 'new.*char.*\[,\]|_cache.*=.*char'
Length of output: 134
Script:
#!/bin/bash
# Let's check the cache initialization and usage patterns more broadly
rg -g '*.cs' '_cache\s*=\s*new' -A 3
# Also check for Rune-related array initialization
rg -g '*.cs' 'new\s+(?:Rune|System\.Text\.Rune).*\[,\]' -A 3
# Look for cache field declaration and initialization
ast-grep --pattern 'private $_[,] _cache;'
Length of output: 146
Script:
#!/bin/bash
# Let's try a simpler approach to find cache-related code
rg -g '*.cs' '_cache' -B 2 -A 2
# Also search for any Rune array initialization with less strict pattern
rg -g '*.cs' 'new.*Rune.*\[' -B 2 -A 2
# Look at the RenderTarget.cs file directly to see initialization
cat src/Consolonia.Core/Drawing/RenderTarget.cs
Length of output: 9779
src/Consolonia.Core/Drawing/DrawingContextImpl.cs (4)
27-27: LGTM: Spelling correction
The constant name has been correctly updated from StrikthroughThickness to StrikethroughThickness.
73-102: LGTM: Improved bitmap drawing implementation
The changes enhance code readability through:
- Clear comments explaining the quad pixel logic
- Better variable naming
- Structured code organization
153-157: LGTM: Proper ISceneBrush content handling
The implementation correctly handles the scene brush content with proper null checking and scoping.
503-504: LGTM: Improved Unicode support
The implementation now correctly uses EnumerateRunes() for proper handling of Unicode surrogate pairs and emojis.
src/Consolonia.Core/Infrastructure/InputLessDefaultNetConsole.cs (1)
101-114: Unicode handling enhancements are correctly implemented
The updates in the Print method effectively handle complex Unicode characters, ensuring accurate caret positioning when printing text that includes multi-byte characters.
src/Consolonia.Core/Drawing/PixelBufferImplementation/PixelForeground.cs
Outdated
Show resolved
Hide resolved
src/Consolonia.Core/Drawing/PixelBufferImplementation/DrawingBoxSymbol.cs
Outdated
Show resolved
Hide resolved
src/Consolonia.Core/Infrastructure/InputLessDefaultNetConsole.cs
Outdated
Show resolved
Hide resolved
* Fix caret
Co-authored-by: <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (13)
src/Consolonia.Core/Drawing/PixelBufferImplementation/SimpleSymbol.cs (3)
9-9: Consider adding XML documentation.While the field implementation is correct, adding XML documentation would help explain the purpose of using Rune and the significance of the null character initialization.
+/// <summary> +/// Represents a Unicode character using a Rune type for proper surrogate pair support. +/// Initialized to null character by default. +/// </summary> private readonly Rune _rune = new Rune('\0');
11-13: Remove redundant initialization in parameterless constructor.The field is already initialized with
new Rune('\0'), making the constructor's initialization redundant.public SimpleSymbol() { - _rune = new Rune('\0'); }
27-27: Consider improving Blend method readability.While the logic is correct, the expression could be more explicit about its intent.
-=> (symbolAbove.Rune.Value != '\0') ? symbolAbove : this; +=> !symbolAbove.Rune.IsEmpty ? symbolAbove : this;src/Consolonia.Core/Drawing/PixelBufferImplementation/PixelForeground.cs (2)
Line range hint
22-22: Update outdated Unicode documentationThe comment "working with 16 bit unicode only" is now incorrect since the Symbol property uses Rune which supports the full Unicode range including surrogate pairs.
- public ISymbol Symbol { get; } //now working with 16 bit unicode only for simplicity //todo: reference here + public ISymbol Symbol { get; } // Supports full Unicode range including surrogate pairs
Line range hint
39-47: Clean up redundant checks and outdated commentsThe Blend method contains:
- A redundant null check since PixelForeground constructor already ensures Symbol is never null
- An outdated TODO comment about default(char) which is no longer relevant with Rune implementation
public PixelForeground Blend(PixelForeground pixelAboveForeground) { - //todo: check default(char) is there ISymbol symbolAbove = pixelAboveForeground.Symbol; - ArgumentNullException.ThrowIfNull(symbolAbove); ISymbol newSymbol = Symbol.Blend(ref symbolAbove);src/Consolonia.Core/Drawing/PixelBufferImplementation/PixelBuffer.cs (2)
21-21: Consider adding parameter validation in the constructor.While the initialization of
_caretPositionis correct, the constructor should validate thewidthandheightparameters to prevent creation of invalid buffers.public PixelBuffer(ushort width, ushort height) { + if (width == 0 || height == 0) + throw new ArgumentException("Width and height must be greater than zero"); Width = width; Height = height; _buffer = new Pixel[width, height]; _caretPosition = new PixelBufferCoordinate(0, 0); }
100-104: Consider parallel processing for large buffers.While the loop restructuring improves readability, consider adding parallel processing support for better performance with large buffers.
public void ForeachReadonly(Action<PixelBufferCoordinate, Pixel> action) { - for (ushort j = 0; j < Height; j++) - for (ushort i = 0; i < Width; i++) - { - Pixel pixel = this[(PixelBufferCoordinate)(i, j)]; - action(new PixelBufferCoordinate(i, j), pixel); - } + Parallel.For(0, Height, j => + { + for (ushort i = 0; i < Width; i++) + { + Pixel pixel = this[(PixelBufferCoordinate)(i, (ushort)j)]; + action(new PixelBufferCoordinate(i, (ushort)j), pixel); + } + }); }Note: Only use parallel processing if the
actiondelegate is thread-safe and the buffer size justifies the overhead.src/Consolonia.Core/Drawing/PixelBufferImplementation/Pixel.cs (1)
69-73: Consider improving transparency handling readability.While the logic is correct, consider these improvements:
- The comment could be more precise: "Skip blending if the foreground is transparent"
- The complex transparency check could be extracted to a descriptive method
Consider this refactor:
- // if the foreground is transparent, ignore pixelAbove foreground. - newForeground = pixelAbove.Foreground.Color != Colors.Transparent ? Foreground.Blend(pixelAbove.Foreground) : Foreground; - - // background is transparent, ignore pixelAbove background. - newBackground = Background; + // Skip blending if the foreground is transparent + newForeground = ShouldBlendForeground(pixelAbove) + ? Foreground.Blend(pixelAbove.Foreground) + : Foreground; + // Background remains unchanged when transparent + newBackground = Background; +private bool ShouldBlendForeground(Pixel pixelAbove) => + pixelAbove.Foreground.Color != Colors.Transparent;src/Tests/Consolonia.TestsCore/UnitTestConsole.cs (1)
151-153: Consider documenting the caret character choice.While the change to Rune is good, the choice of 'Ꮖ' (Cherokee letter 'TLI') as a caret character is unusual. Consider:
- Documenting why this specific character was chosen
- Using a more conventional caret character like '^' or '▊' (block cursor)
Also, there's a TODO comment about cursor drawing issues that should be tracked.
Would you like me to:
- Suggest alternative caret characters that are more conventional?
- Help create an issue to track the cursor drawing problem mentioned in the TODO?
src/Consolonia.Core/Drawing/DrawingContextImpl.cs (4)
153-157: Consider consistent brace formatting.The opening brace could be on the same line as the case statement to match the codebase style.
- case ISceneBrush sceneBrush: - { + case ISceneBrush sceneBrush: {
165-177: Consider addressing the ReSharper TODO comment.The comment indicates that the closure access should be replaced with a parameter. This would improve code quality and remove the need for the ReSharper disable directive.
- // ReSharper disable once AccessToModifiedClosure todo: pass as a parameter - _pixelBuffer.Set((PixelBufferCoordinate)h, - (pixel, bb) => pixel.Blend(new Pixel(new PixelBackground(bb.Mode, bb.Color))), - backgroundBrush); + var currentBrush = backgroundBrush; + _pixelBuffer.Set((PixelBufferCoordinate)h, + (pixel, bb) => pixel.Blend(new Pixel(new PixelBackground(bb.Mode, bb.Color))), + currentBrush);
512-524: Consider making tab size configurable.The tab size is currently hardcoded to 8. Consider making this configurable through a constant or configuration setting for better flexibility.
- const int tabSize = 8; + private const int DefaultTabSize = 8; + private readonly int _tabSize;
528-534: Remove commented code for newline handling.The commented code for newline handling should be removed as it's not being used and includes uncertainty about its necessity.
- { - /* 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 handling is managed by cursor positioning + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
src/Consolonia.Core/Drawing/DrawingContextImpl.cs(8 hunks)src/Consolonia.Core/Drawing/PixelBufferImplementation/Pixel.cs(3 hunks)src/Consolonia.Core/Drawing/PixelBufferImplementation/PixelBuffer.cs(3 hunks)src/Consolonia.Core/Drawing/PixelBufferImplementation/PixelForeground.cs(1 hunks)src/Consolonia.Core/Drawing/PixelBufferImplementation/SimpleSymbol.cs(1 hunks)src/Consolonia.Core/Drawing/RenderTarget.cs(4 hunks)src/Tests/Consolonia.TestsCore/UnitTestConsole.cs(2 hunks)
🔇 Additional comments (19)
src/Consolonia.Core/Drawing/PixelBufferImplementation/SimpleSymbol.cs (3)
1-6: LGTM! Good addition of debugging support.
The DebuggerDisplay attribute will improve debugging experience by showing the actual Rune value.
21-21: LGTM! Clean property implementation.
The read-only property provides appropriate access to the internal Rune value.
1-28: Verify Unicode support implementation across the codebase.
Let's ensure the Rune type is consistently used across the codebase for Unicode character handling.
✅ Verification successful
Let me gather more information about the Unicode handling implementation.
Based on the gathered information, I can now provide the final response:
Unicode support implementation is properly integrated across the codebase
The implementation consistently uses Rune type for Unicode character handling:
- Core implementation in
SimpleSymbolcorrectly usesRunefor character storage and operations - Proper Unicode handling in drawing context and render targets
- Special handling for complex Unicode characters in console output
- Correct implementation of control character handling and whitespace detection
- Box-drawing characters properly implemented using
Rune
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify consistent Unicode handling implementation
# Check for any remaining direct char usage in symbol-related files
echo "Checking for potential missed char to Rune conversions..."
rg -t cs "char\s+\w+\s*[;=]" --glob "**/*Symbol*.cs"
# Look for potential Unicode handling in tests
echo "Checking for Unicode test coverage..."
rg -t cs "TestMethod|Theory|Fact" -C 5 --glob "**/test/**/*Unicode*.cs"
# Verify Rune usage in related components
echo "Verifying Rune usage in related components..."
ast-grep --pattern 'class $_ {
$$$
char $_
$$$
}'
Length of output: 648
Script:
#!/bin/bash
# Search for all Rune usages to verify consistent implementation
echo "Checking Rune usage across the codebase..."
rg -t cs "Rune" -B 2 -A 2
# Check for any Unicode-related tests
echo "Checking for any Unicode-related tests..."
rg -t cs "Unicode|Rune" --glob "**/test/**/*.cs"
# Look for other symbol implementations
echo "Checking other symbol implementations..."
ast-grep --pattern 'struct $_ : ISymbol {
$$$
}'
Length of output: 12422
src/Consolonia.Core/Drawing/PixelBufferImplementation/PixelForeground.cs (2)
2-2: LGTM: Improved debugging visibility for Unicode characters
The new DebuggerDisplay attribute effectively shows the Unicode character representation using the Rune property, which aligns well with the PR's Unicode support objectives.
Also applies to: 7-7
Line range hint 7-47: Verify consistent Rune implementation across related components
Since this struct is transitioning to use Rune for Unicode support, we should verify that all related components handle Rune properly.
✅ Verification successful
Rune implementation is consistent across the codebase
The verification shows that:
ISymbolinterface properly definesRuneproperty- All implementations (
SimpleSymbol,DrawingBoxSymbol) correctly useRune - The char usages found are for specific purposes:
- Input handling in platform-specific code
- Drawing context's quad pixel characters (block elements for drawing)
- These usages are appropriate and don't need conversion to Rune
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential inconsistencies in Rune implementation
# Look for other implementations of ISymbol to ensure they're updated for Rune
echo "Checking ISymbol implementations:"
ast-grep --pattern 'class $_ : ISymbol'
# Look for direct char usage that might need to be converted to Rune
echo "Checking for potential char usage that might need Rune conversion:"
rg "char\s+\w+\s*[=;]" --type csharp
Length of output: 913
Script:
#!/bin/bash
# Let's check ISymbol interface and its usages to understand the Rune implementation
echo "Checking ISymbol interface definition:"
rg "interface ISymbol" -A 10 --type csharp
# Check for Rune usage in the codebase
echo "Checking Rune usage:"
rg "Rune" --type csharp
# Check specific files that showed char usage for context
echo "Checking DrawingContextImpl.cs for context:"
rg "GetQuadPixelCharacter|GetColorsPattern" -A 5 --type csharp
Length of output: 6677
src/Consolonia.Core/Drawing/PixelBufferImplementation/PixelBuffer.cs (1)
14-14: LGTM: Field addition follows best practices.
The new private field _caretPosition is appropriately encapsulated and follows C# naming conventions.
src/Consolonia.Core/Drawing/PixelBufferImplementation/Pixel.cs (2)
2-3: LGTM! Good debugging enhancement.
The addition of the DebuggerDisplay attribute with Rune property access will improve debugging experience when dealing with Unicode characters.
Also applies to: 11-11
20-22: LGTM! Good Unicode support enhancement.
The switch from char to Rune is the correct approach for proper Unicode support, especially for surrogate pairs and emojis.
Let's verify the impact of removing the byte-based constructor:
src/Tests/Consolonia.TestsCore/UnitTestConsole.cs (2)
61-71: LGTM! Improved Unicode handling with Rune support.
The change from character-based to Rune-based iteration is a solid improvement that properly handles surrogate pairs and other complex Unicode sequences. The use of EnumerateRunes() ensures correct Unicode character boundaries are respected.
Line range hint 134-134: LGTM! Enhanced input testing capabilities.
The addition of the optional modifiers parameter with a default value maintains backward compatibility while enabling more comprehensive input testing scenarios.
src/Consolonia.Core/Drawing/RenderTarget.cs (5)
24-24: LGTM: Appropriate type change for Unicode support
The change from char to Rune in the cache array is correct and necessary for proper Unicode surrogate pair support.
100-101: LGTM: Consistent cache initialization
The initialization change correctly reflects the type update from char to Rune.
134-135: LGTM: Proper fallback character handling
Good choice of using '░' as a fallback character with proper Rune construction.
137-141: LGTM: Improved tuple structure and naming
Good improvements:
- Proper
Runetype usage for Unicode support - Better tuple field naming for improved readability
207-210: Clarify character handling strategy
The control character handling looks good, but there are a few points to consider:
- There's a commented code fragment suggesting special handling for specific characters ('保', '哥'). Should this still be implemented?
- The comment "some terminals do not print \0" could be more specific about which terminals and conditions.
Let's check for any other special character handling in the codebase:
Consider:
- Adding a comment explaining which terminals have issues with control characters
- Creating a dedicated method for character substitution logic
- If needed, restore special handling for CJK characters with proper documentation
Example refactor:
- var rune = pixel.Foreground.Symbol.Rune;
- if (Rune.IsControl(rune)) /*|| character is '保' or '哥'*/
- rune = new Rune(' '); // some terminals do not print \0
- _stringBuilder.Append(rune);
+ var rune = pixel.Foreground.Symbol.Rune;
+ _stringBuilder.Append(GetSafeCharacter(rune));
+
+ private static Rune GetSafeCharacter(Rune rune)
+ {
+ // Some terminal emulators (e.g., Windows Console) cannot properly handle control characters
+ if (Rune.IsControl(rune))
+ return new Rune(' ');
+ return rune;
+ }✅ Verification successful
Control character handling is appropriate, commented code can be removed
The current implementation of control character handling is correct and consistent with the codebase:
- The control character check using
Rune.IsControl()is the standard approach used throughout the codebase - The space character substitution is a proper fallback, as evidenced by similar patterns in test cases
- The commented code about CJK characters ('保', '哥') appears to be obsolete and can be safely removed, as there's no special handling of these characters elsewhere in the codebase
- The comment about terminals not printing '\0' is accurate, as shown by the test cases where control characters are explicitly skipped
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for special character handling patterns
rg -l "IsControl|Rune.*['\u4fdd\u54e5']|// some terminals"
Length of output: 468
Script:
#!/bin/bash
# Let's check the actual usage of Rune.IsControl and special character handling
rg -A 3 "IsControl|Rune.*['\u4fdd\u54e5']|// some terminals" --no-filename
# Also check for any related tests or documentation
fd -e md -e cs -x grep -l "terminal.*character\|control.*character\|special.*character"
Length of output: 2312
Script:
#!/bin/bash
# Let's check the test cases and documentation for character handling
rg -A 5 "control.*character|special.*character" ./src/Tests/Consolonia.TestsCore/UnitTestConsole.cs
# Also check GlyphTypeface for any relevant character handling documentation
rg -A 5 "control.*character|special.*character" ./src/Consolonia.Core/Text/GlyphTypeface.cs
Length of output: 779
src/Consolonia.Core/Drawing/DrawingContextImpl.cs (4)
27-27: LGTM: Spelling correction from "Strikthorugh" to "Strikethrough".
The constant name has been corrected to use proper spelling, improving code readability.
73-102: LGTM: Improved code readability with better comments and formatting.
The changes enhance code maintainability by:
- Adding clear comments explaining the quad pixel logic
- Using more descriptive variable names
- Better formatting of the nested loops
502-503: LGTM: Improved Unicode support using Rune.
The change to use EnumerateRunes() properly handles Unicode characters, including surrogate pairs and emojis.
541-547: LGTM: Proper handling of typeface styles.
The pixel creation now correctly includes typeface style and weight information, ensuring proper rendering of styled text.
src/Consolonia.Core/Drawing/PixelBufferImplementation/PixelBuffer.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (5)
src/Consolonia.Core/Drawing/PixelBufferImplementation/ISymbol.cs (2)
7-10: Enhance property documentationWhile the documentation is present, it could be more descriptive to help implementers understand:
- The purpose of this property in the context of Unicode support
- Any special handling required for surrogate pairs
Consider expanding the documentation:
/// <summary> - The rune for the symbol + The Unicode character (rune) representing this symbol. Supports full Unicode range + including surrogate pairs and emoji sequences. /// </summary>
14-18: Improve Blend method documentationThe current documentation lacks essential details about the blending operation.
Consider expanding the documentation:
/// <summary> - Blend 2 symbols together + Combines this symbol with another symbol placed above it, following + specific blending rules for Unicode characters and their visual representation. /// </summary> /// <param name="symbolAbove"> + The symbol to blend with the current symbol. This symbol is considered + to be visually above the current symbol. /// </param> -/// <returns></returns> +/// <returns>A new symbol representing the result of blending the two symbols</returns>src/Consolonia.Core/Drawing/PixelBufferImplementation/SimpleSymbol.cs (2)
19-19: Consider adding XML documentation.The Rune property implementation is correct and immutable. Consider adding XML documentation to describe its purpose and the significance of the '\0' default value.
+/// <summary> +/// Gets the Unicode character represented by this symbol. +/// A value of '\0' indicates no character (empty symbol). +/// </summary> public Rune Rune { get; } = new('\0');
22-29: Consider improving Blend method implementation.While the logic is correct, there are a few suggestions for improvement:
- The
refkeyword on the parameter appears unnecessary since the value isn't modified- The method's purpose could be more explicit with XML documentation
- public ISymbol Blend(ref ISymbol symbolAbove) + /// <summary> + /// Combines this symbol with the symbol above it, preferring the above symbol if it's not empty. + /// </summary> + /// <param name="symbolAbove">The symbol to blend with</param> + /// <returns>The resulting blended symbol</returns> + public ISymbol Blend(ISymbol symbolAbove) { return symbolAbove.Rune.Value != '\0' ? symbolAbove : this; }src/Tests/Consolonia.TestsCore/UnitTestConsole.cs (1)
151-153: Review caret character choice and cursor drawing.A few observations:
- Using Cherokee letter TSI ('Ꮖ') as a caret character seems unusual. Consider using a more conventional character like '▊' (U+258A) or '│' (U+2502).
- The TODO comment about cursor drawing should be addressed or tracked.
Would you like me to:
- Suggest alternative caret characters that are more conventional for console applications?
- Help create an issue to track the cursor drawing problem?
- Rune rune = pixel.IsCaret ? new Rune('Ꮖ') : pixel.Foreground.Symbol.Rune; + Rune rune = pixel.IsCaret ? new Rune('▊') : pixel.Foreground.Symbol.Rune;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (9)
src/Consolonia.Core/Drawing/DrawingContextImpl.cs(7 hunks)src/Consolonia.Core/Drawing/PixelBufferImplementation/DrawingBoxSymbol.cs(3 hunks)src/Consolonia.Core/Drawing/PixelBufferImplementation/ISymbol.cs(1 hunks)src/Consolonia.Core/Drawing/PixelBufferImplementation/Pixel.cs(3 hunks)src/Consolonia.Core/Drawing/PixelBufferImplementation/PixelBuffer.cs(2 hunks)src/Consolonia.Core/Drawing/PixelBufferImplementation/SimpleSymbol.cs(1 hunks)src/Consolonia.Core/Drawing/RenderTarget.cs(4 hunks)src/Consolonia.Core/Infrastructure/InputLessDefaultNetConsole.cs(1 hunks)src/Tests/Consolonia.TestsCore/UnitTestConsole.cs(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- src/Consolonia.Core/Drawing/DrawingContextImpl.cs
- src/Consolonia.Core/Drawing/PixelBufferImplementation/Pixel.cs
- src/Consolonia.Core/Drawing/RenderTarget.cs
- src/Consolonia.Core/Infrastructure/InputLessDefaultNetConsole.cs
🧰 Additional context used
🪛 GitHub Check: build
src/Consolonia.Core/Drawing/PixelBufferImplementation/PixelBuffer.cs
[warning] 81-81:
"[RedundantArgumentDefaultValue] The parameter 'isCaret' has the same default value" on /home/runner/work/Consolonia/Consolonia/src/Consolonia.Core/Drawing/PixelBufferImplementation/PixelBuffer.cs(81,79)
🔇 Additional comments (11)
src/Consolonia.Core/Drawing/PixelBufferImplementation/ISymbol.cs (2)
1-1: LGTM: Required import for Rune type
The System.Text namespace import is necessary for the Rune type usage.
10-10: Verify impact of breaking interface change
The replacement of GetCharacter() with Rune property is a breaking change. Let's verify all implementations are updated.
✅ Verification successful
Breaking interface change has been properly implemented
From the git history and codebase analysis, we can confirm:
- The change from
GetCharacter()toRuneproperty was part of a planned feature to "Add support for Surrogate sequences" (commit f087bf5) - All implementations of
ISymbol(SimpleSymbol, DrawingBoxSymbol) have been updated to use the newRuneproperty - No remaining references to
GetCharacter()exist in the codebase
The breaking change was properly implemented across all dependent code.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for existing implementations of ISymbol
ast-grep --pattern 'class $_ : $$$ISymbol$$$'
rg "class.*:.*ISymbol" -A 5
Length of output: 77
Script:
#!/bin/bash
# Let's try a broader search for ISymbol implementations and usages
rg "ISymbol" -l
rg "ISymbol" -A 5
# Also search for files containing 'GetCharacter' to compare old vs new usage
rg "GetCharacter" -A 5
Length of output: 6970
Script:
#!/bin/bash
# Let's check git history for the ISymbol interface to see the change from GetCharacter to Rune
git log -p src/Consolonia.Core/Drawing/PixelBufferImplementation/ISymbol.cs
# Also check implementations for any remaining GetCharacter references
rg "GetCharacter\(\)" -A 5
Length of output: 4055
src/Consolonia.Core/Drawing/PixelBufferImplementation/SimpleSymbol.cs (2)
1-6: LGTM! Good debugging support added.
The DebuggerDisplay attribute will improve debugging experience by showing the Rune value directly.
9-17: LGTM! Verify all constructor usages are updated.
The constructors properly handle the transition to Rune. However, since the char-based constructor was removed, we should verify that all existing usages have been updated.
✅ Verification successful
All SimpleSymbol constructor usages have been properly updated to use Rune
Based on the search results, all instances of SimpleSymbol constructor calls are using either the parameterless constructor or properly passing a Rune parameter. The found usages in:
UnitTestConsole.cs: Usesnew SimpleSymbol(rune)DrawingContextImpl.cs: Usesnew SimpleSymbol(new Rune(quadPixel))Pixel.cs: Uses bothnew SimpleSymbol(rune)and parameterlessnew SimpleSymbol()RenderTarget.cs: Usesnew SimpleSymbol(new Rune('░'))
No char-based constructor calls were found, confirming all usages have been updated correctly.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining char-based SimpleSymbol constructor calls that need updating
# Search for potential char-based constructor calls
rg -p "new SimpleSymbol\([^R]"
# Search for any direct char assignments that might need updating
rg -p "SimpleSymbol.*=.*'[^']'"
Length of output: 779
Script:
#!/bin/bash
# Let's check the actual implementation of SimpleSymbol to understand its usage pattern
ast-grep --pattern 'class SimpleSymbol {
$$$
}'
# Also check for any test files that might be using SimpleSymbol
rg -p "SimpleSymbol" "src/Tests" -A 2 -B 2
Length of output: 561
src/Consolonia.Core/Drawing/PixelBufferImplementation/PixelBuffer.cs (2)
73-87: Previous review comments are still applicable.
The implementation still needs to address the concerns raised in the previous review regarding bounds validation, thread safety, and documentation completeness.
Additionally, there's a redundant explicit false value being passed to the Pixel constructor where it's already the default.
- new Pixel(oldCaretPixel.Foreground, oldCaretPixel.Background, false);
+ new Pixel(oldCaretPixel.Foreground, oldCaretPixel.Background);🧰 Tools
🪛 GitHub Check: build
[warning] 81-81:
"[RedundantArgumentDefaultValue] The parameter 'isCaret' has the same default value" on /home/runner/work/Consolonia/Consolonia/src/Consolonia.Core/Drawing/PixelBufferImplementation/PixelBuffer.cs(81,79)
73-87: Verify Unicode character handling with caret positioning.
Since this PR aims to add support for Surrogate Unicode sequences, we should verify that the caret positioning works correctly with multi-byte Unicode characters.
✅ Verification successful
Based on the code analysis, I can now generate the final response:
The caret positioning implementation is Unicode-aware and safe
The codebase shows proper Unicode handling:
- Uses
Runetype from .NET for Unicode character representation - Handles surrogate pairs correctly in
InputLessDefaultNetConsole.cs: "if we have complex unicode runes then we need to calculate the position manually, EnumerateRunes is not enough as some runes (like '🥰') are a single rune, but are emitted as two console characters" - The
PixelBufferoperates on buffer coordinates that are already properly calculated accounting for Unicode width
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for Unicode/Rune-related tests that verify caret positioning
rg -l "SetCaretPosition.*emoji|surrogate|unicode" --type=cs
# Search for related Unicode handling code
ast-grep --pattern 'class $_ {
$$$
Rune $_ {
$$$
}
$$$
}'
Length of output: 333
Script:
#!/bin/bash
# Let's examine the content of these files to understand the Unicode handling
rg "SetCaretPosition.*emoji|surrogate|unicode" -B 2 -A 2 --type=cs
# Look for any Rune-related implementations
rg "Rune" -B 2 -A 2 --type=cs
# Check for any buffer coordinate calculations that might handle Unicode
rg "PixelBufferCoordinate.*width|length|size" -B 2 -A 2 --type=cs
Length of output: 47658
🧰 Tools
🪛 GitHub Check: build
[warning] 81-81:
"[RedundantArgumentDefaultValue] The parameter 'isCaret' has the same default value" on /home/runner/work/Consolonia/Consolonia/src/Consolonia.Core/Drawing/PixelBufferImplementation/PixelBuffer.cs(81,79)
src/Consolonia.Core/Drawing/PixelBufferImplementation/DrawingBoxSymbol.cs (4)
10-10: LGTM! Good improvements for Unicode support and debugging.
The changes effectively support Unicode handling by introducing the Rune property, and the DebuggerDisplay attribute enhances debugging visibility.
Also applies to: 24-24
Line range hint 29-106: Add descriptive exception message.
The previous review comment about adding an informative message to the InvalidOperationException is still valid and should be addressed.
Line range hint 117-132: LGTM! Good improvement in type safety.
The change from returning raw bytes to DrawingBoxSymbol instances improves type safety and encapsulation.
24-24: Verify surrogate pair handling.
While the change to Rune improves Unicode support, let's verify that surrogate pairs are correctly handled in the box-drawing characters implementation.
src/Tests/Consolonia.TestsCore/UnitTestConsole.cs (1)
61-71: Add test coverage for Unicode edge cases.
While the implementation now properly uses Rune for Unicode support, we should add test coverage for:
- Surrogate pairs (e.g., 𝄞 or 🎵)
- Emoji sequences (e.g., 👨👩👧👦 or 🏳️🌈)
- Right-to-left text (e.g., Arabic or Hebrew)
Also applies to: 151-153
src/Consolonia.Core/Drawing/PixelBufferImplementation/PixelBuffer.cs
Outdated
Show resolved
Hide resolved
tomlm
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (4)
src/Consolonia.Core/Text/GlyphRunImpl.cs (2)
1-1: Consider removing unused System namespaceThe System namespace appears to be unused in this file. However, verify there are no implicit type usages before removing it.
-using System;🧰 Tools
🪛 GitHub Check: build
[warning] 1-1:
"[RedundantUsingDirective] Using directive is not required by the code and can be safely removed" on /home/runner/work/Consolonia/Consolonia/src/Consolonia.Core/Text/GlyphRunImpl.cs(1,1)
23-23: Good architectural improvementThe switch from
GlyphIndicestoGlyphInfosis a positive change that:
- Provides richer glyph information
- Better supports Unicode surrogate pairs
- Keeps all glyph data in one cohesive structure
Consider adding XML documentation to describe the structure and purpose of GlyphInfo for better maintainability.
src/Consolonia.Themes.TurboVision/Templates/Controls/Helpers/SymbolsControl.cs (1)
73-78: Improve robustness of text shaping implementation.Consider these enhancements:
- Add null/empty validation before accessing value.AsMemory()
- Make font size configurable instead of hardcoding to 1
- Add error handling for text shaping failures
Here's a suggested implementation:
public string Text { get => _text; set { _text = value; - var glyphs = new TextShaper().ShapeText(value.AsMemory(), new TextShaperOptions(new GlyphTypeface(), 1)); - _shapedText = new GlyphRun(new GlyphTypeface(), - 1, - (_text ?? string.Empty).AsMemory(), - glyphs, - default(Point)); + if (string.IsNullOrEmpty(value)) + { + _shapedText?.Dispose(); + _shapedText = null; + return; + } + + try + { + var fontSize = 1.0f; // Consider making this configurable + var glyphs = new TextShaper().ShapeText( + value.AsMemory(), + new TextShaperOptions(new GlyphTypeface(), fontSize) + ); + _shapedText = new GlyphRun( + new GlyphTypeface(), + fontSize, + value.AsMemory(), + glyphs, + default(Point) + ); + } + catch (Exception ex) + { + // Consider logging or handling the error appropriately + _shapedText = null; + } } }src/Consolonia.Core/Text/TextShaper.cs (1)
3-4: Remove redundant using directivesThe using directives
using System.Runtime.InteropServices;andusing Avalonia.Controls.Documents;are not required and can be safely removed to clean up the code.Apply this diff to remove them:
-using System.Runtime.InteropServices; -using Avalonia.Controls.Documents;🧰 Tools
🪛 GitHub Check: build
[warning] 3-3:
"[RedundantUsingDirective] Using directive is not required by the code and can be safely removed" on /home/runner/work/Consolonia/Consolonia/src/Consolonia.Core/Text/TextShaper.cs(3,1)
[warning] 4-4:
"[RedundantUsingDirective] Using directive is not required by the code and can be safely removed" on /home/runner/work/Consolonia/Consolonia/src/Consolonia.Core/Text/TextShaper.cs(4,1)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
src/Consolonia.Core/Drawing/DrawingContextImpl.cs(9 hunks)src/Consolonia.Core/Text/GlyphRunImpl.cs(2 hunks)src/Consolonia.Core/Text/TextShaper.cs(2 hunks)src/Consolonia.Gallery/Gallery/GalleryViews/GalleryTextBlock.axaml(1 hunks)src/Consolonia.Themes.TurboVision/Templates/Controls/Helpers/SymbolsControl.cs(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/Consolonia.Core/Drawing/DrawingContextImpl.cs
- src/Consolonia.Gallery/Gallery/GalleryViews/GalleryTextBlock.axaml
🧰 Additional context used
🪛 GitHub Check: build
src/Consolonia.Core/Text/GlyphRunImpl.cs
[warning] 1-1:
"[RedundantUsingDirective] Using directive is not required by the code and can be safely removed" on /home/runner/work/Consolonia/Consolonia/src/Consolonia.Core/Text/GlyphRunImpl.cs(1,1)
src/Consolonia.Core/Text/TextShaper.cs
[warning] 3-3:
"[RedundantUsingDirective] Using directive is not required by the code and can be safely removed" on /home/runner/work/Consolonia/Consolonia/src/Consolonia.Core/Text/TextShaper.cs(3,1)
[warning] 4-4:
"[RedundantUsingDirective] Using directive is not required by the code and can be safely removed" on /home/runner/work/Consolonia/Consolonia/src/Consolonia.Core/Text/TextShaper.cs(4,1)
🔇 Additional comments (4)
src/Consolonia.Core/Text/GlyphRunImpl.cs (1)
19-20:
Review the simplified Bounds calculation
The new Bounds width calculation using glyphInfos.Count assumes all glyphs have a width of 1 unit. This is problematic because:
- It doesn't account for variable-width fonts
- Complex Unicode characters (including surrogate pairs and emojis) often have different widths
- RTL text layout might be affected
The previous implementation likely used GlyphAdvance values to account for actual glyph widths.
Let's verify how this might affect text layout:
src/Consolonia.Themes.TurboVision/Templates/Controls/Helpers/SymbolsControl.cs (2)
9-9: LGTM: Required namespace for enhanced text formatting.
The addition of Avalonia.Media.TextFormatting namespace aligns with the PR's objective to improve Unicode support.
73-78: Verify RTL text handling and Unicode integration.
Since this PR adds support for right-to-left text, we should verify the integration with RTL-specific components.
src/Consolonia.Core/Text/TextShaper.cs (1)
17-17:
Avoid casting rune.Value to char to prevent data loss
Casting rune.Value to char can lead to data loss for Unicode code points above U+FFFF, which cannot be represented in a single char. Consider updating ConsoleText.IsWideChar to accept Rune or modify the logic to correctly handle all Unicode characters.
Run the following script to check the definition of ConsoleText.IsWideChar:
This script will help you verify the parameter type of IsWideChar and ensure it accurately processes all Unicode code points.
src/Consolonia.Themes.TurboVision/Templates/Controls/Helpers/SymbolsControl.cs
Outdated
Show resolved
Hide resolved
* Added code to measure the actual width taken for emoticons and ligatures * added GlyphMetric cache so we only have to compute glyph widths once. * Changed ISymbol to have width * Adjust positiong appropriately * Fixed bug so we are correctly hadnling \r in multiline textblocks * Added Unicode library to identify emoticons and such. * Changed DrawString to compute widths to deal with fact emoticons are wide * Changed RenderTarget to use symbol widths to properly skip overlapped cells.
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (8)
src/Consolonia.Core/Drawing/PixelBufferImplementation/ISymbol.cs (2)
7-10: Consider performance optimization for ASCII characters.The change from
chartostringis excellent for Unicode support. However, since most text might be ASCII, consider adding a fast path for single-character cases.Consider implementing an additional property:
/// <summary> /// Provides fast access for single-character symbols /// </summary> bool IsSingleChar { get; }
12-15: Enhance documentation for surrogate pairs.The Width property is well-designed, but the documentation could be more specific about Unicode handling.
Consider updating the documentation:
- /// The number of characters the symbol takes up + /// The display width of the symbol in character cells. + /// For surrogate pairs and combining characters, this may differ from the string length.src/Consolonia.Core/Drawing/PixelBufferImplementation/SimpleSymbol.cs (2)
11-27: Consider adding validation and maintaining width consistency.The constructors could benefit from some improvements:
- The string+width constructor should validate that width > 0
- Consider calculating width based on string length for surrogate pairs and combining characters
Here's a suggested improvement:
public SimpleSymbol(string text, ushort width) { + if (width == 0) + throw new ArgumentException("Width must be greater than 0", nameof(width)); Text = text; Width = width; }
34-41: Improve consistency and performance of string checks.
- Consider caching the IsWhiteSpace result for performance
- Use consistent null checking method (IsNullOrWhiteSpace vs IsNullOrEmpty)
- Use consistent string casing (String vs string)
Here's a suggested improvement:
+private bool? _isWhiteSpace; public bool IsWhiteSpace() { - return string.IsNullOrWhiteSpace(Text); + return _isWhiteSpace ??= string.IsNullOrWhiteSpace(Text); } public ISymbol Blend(ref ISymbol symbolAbove) { - return !String.IsNullOrEmpty(symbolAbove.Text) ? symbolAbove : this; + return !string.IsNullOrWhiteSpace(symbolAbove.Text) ? symbolAbove : this; }src/Consolonia.Core/Drawing/PixelBufferImplementation/DrawingBoxSymbol.cs (1)
Line range hint
31-98: The box-drawing character mapping implementation is robust and well-documented.The implementation correctly handles:
- All box-drawing character combinations
- Both single and double-line styles
- Special cases like bold and empty symbols
Consider adding unit tests to verify the character mappings, especially for edge cases and different line styles, if not already covered.
src/Tests/Consolonia.TestsCore/UnitTestConsole.cs (1)
151-153: Ensure caret visibility across different fontsThe hard-coded Unicode character 'Ꮖ' (U+13D6 CHEROKEE LETTER TSI) for the caret might not be visible in all fonts. Consider:
- Using a more commonly supported character
- Adding a fallback mechanism
- string text = pixel.IsCaret ? "Ꮖ" : pixel.Foreground.Symbol.Text; + string text = pixel.IsCaret + ? IsCharacterSupported('Ꮖ') ? "Ꮖ" : "█" // Use block character as fallback + : pixel.Foreground.Symbol.Text;Add this helper method:
private static bool IsCharacterSupported(char c) { // TODO: Implement font support check // Could use TextRenderer or similar to verify the character can be displayed return true; }src/Consolonia.Core/Drawing/RenderTarget.cs (2)
210-213: Consider optimizing string operations and adding RTL support.The current implementation has two potential areas for improvement:
- Performance: Multiple string concatenations in StringBuilder could be optimized for Unicode operations.
- RTL Support: The PR objectives mention right-to-left text support, but there's no explicit handling for it here.
Consider these improvements:
- if (pixel.Foreground.Symbol.Text.Length == 0) - _stringBuilder.Append(' '); - else - _stringBuilder.Append(pixel.Foreground.Symbol.Text); + // Preallocate StringBuilder capacity for better performance + if (_stringBuilder.Capacity < _stringBuilder.Length + 4) // 4 for typical surrogate pair + _stringBuilder.EnsureCapacity(_stringBuilder.Length + 4); + + var text = pixel.Foreground.Symbol.Text; + if (text.Length == 0) + _stringBuilder.Append(' '); + else { + // Handle RTL text if needed + if (IsRightToLeft(text)) + HandleRTLText(_stringBuilder, text); + else + _stringBuilder.Append(text); + }Would you like me to provide the implementation for
IsRightToLeftandHandleRTLTextmethods?
Line range hint
1-232: Consider documenting Unicode handling strategy.The transition to string-based rendering is a significant architectural change that affects the entire rendering pipeline. To ensure maintainability and proper handling of all Unicode cases, consider:
- Adding documentation about supported Unicode ranges
- Documenting performance implications of string-based rendering
- Creating test cases for various Unicode scenarios (surrogate pairs, RTL, combining characters)
Would you like me to help create a documentation template or test suite for Unicode handling?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (15)
src/Consolonia.Core/Consolonia.Core.csproj(1 hunks)src/Consolonia.Core/Drawing/ConsoleBrush.cs(1 hunks)src/Consolonia.Core/Drawing/DrawingContextImpl.cs(9 hunks)src/Consolonia.Core/Drawing/PixelBufferImplementation/DrawingBoxSymbol.cs(4 hunks)src/Consolonia.Core/Drawing/PixelBufferImplementation/ISymbol.cs(1 hunks)src/Consolonia.Core/Drawing/PixelBufferImplementation/Pixel.cs(2 hunks)src/Consolonia.Core/Drawing/PixelBufferImplementation/PixelBuffer.cs(2 hunks)src/Consolonia.Core/Drawing/PixelBufferImplementation/PixelForeground.cs(1 hunks)src/Consolonia.Core/Drawing/PixelBufferImplementation/SimpleSymbol.cs(1 hunks)src/Consolonia.Core/Drawing/RenderTarget.cs(4 hunks)src/Consolonia.Core/Infrastructure/InputLessDefaultNetConsole.cs(4 hunks)src/Consolonia.Core/Text/GlyphTypeface.cs(2 hunks)src/Consolonia.Core/Text/TextShaper.cs(1 hunks)src/Consolonia.Gallery/Gallery/GalleryViews/GalleryTextBlock.axaml(1 hunks)src/Tests/Consolonia.TestsCore/UnitTestConsole.cs(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/Consolonia.Core/Drawing/ConsoleBrush.cs
🚧 Files skipped from review as they are similar to previous changes (7)
- src/Consolonia.Core/Drawing/DrawingContextImpl.cs
- src/Consolonia.Core/Drawing/PixelBufferImplementation/Pixel.cs
- src/Consolonia.Core/Drawing/PixelBufferImplementation/PixelBuffer.cs
- src/Consolonia.Core/Drawing/PixelBufferImplementation/PixelForeground.cs
- src/Consolonia.Core/Infrastructure/InputLessDefaultNetConsole.cs
- src/Consolonia.Core/Text/GlyphTypeface.cs
- src/Consolonia.Gallery/Gallery/GalleryViews/GalleryTextBlock.axaml
🔇 Additional comments (13)
src/Consolonia.Core/Consolonia.Core.csproj (2)
10-10: LGTM! Addition of Unicode.net package aligns with PR objectives.
The addition of Unicode.net package will provide better support for handling surrogate Unicode sequences and emojis.
Line range hint 4-11: Verify complete removal of NullLib.ConsoleEx.
Since we're transitioning from NullLib.ConsoleEx to Unicode.net for text handling, let's verify that all references to the old package have been properly migrated.
✅ Verification successful
NullLib.ConsoleEx has been successfully removed from the current codebase
The verification shows that while NullLib.ConsoleEx was previously used (as seen in git history), it has been completely removed from the current codebase. The transition to Unicode.net appears to be complete as:
- No current package references to NullLib.ConsoleEx exist in any .csproj files
- No active code imports or usages of NullLib.ConsoleEx remain
- The new Unicode.net package is properly referenced in Consolonia.Core.csproj
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining references to NullLib.ConsoleEx in the codebase
# Test 1: Search for any remaining package references
echo "Checking for NullLib.ConsoleEx package references..."
rg -l "NullLib\.ConsoleEx"
# Test 2: Search for any using statements or type references
echo "Checking for NullLib.ConsoleEx usage in code..."
rg "using.*NullLib\.ConsoleEx|NullLib\.ConsoleEx\."
Length of output: 302
Script:
#!/bin/bash
# Let's try a more comprehensive search approach
# Check for any mentions of NullLib in the codebase (case-insensitive)
echo "Checking for any mentions of NullLib..."
rg -i "nulllib"
# Check specifically in csproj files for historical package references
echo "Checking csproj files..."
fd -e csproj --exec cat {}
# Check git history for removed references
echo "Checking git history for NullLib..."
git log -p | rg -i "nulllib"
Length of output: 5715
src/Consolonia.Core/Drawing/PixelBufferImplementation/ISymbol.cs (2)
1-1: LGTM! Required namespace for Unicode handling.
The addition of System.Text namespace is appropriate for the Unicode-related changes.
1-24: Verify consistent implementation across all ISymbol implementations.
The interface changes look good for Unicode support. Let's verify all implementations are updated correctly.
✅ Verification successful
All ISymbol implementations are correctly aligned with interface changes
Both implementations (DrawingBoxSymbol and SimpleSymbol) properly implement:
string Text { get; }property with Unicode supportushort Width { get; }property for character widthIsWhiteSpace()methodBlend(ref ISymbol symbolAbove)method
The implementations are consistent with the interface changes and handle text appropriately:
SimpleSymbolsupports arbitrary text strings with explicit widthDrawingBoxSymbolhandles box-drawing characters with proper Unicode output
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Find all implementations of ISymbol
ast-grep --pattern 'class $_ : $$$ISymbol$$$' || ast-grep --pattern 'struct $_ : $$$ISymbol$$$'
# Verify Text property implementation
ast-grep --pattern 'string Text { get; $$$}'
# Verify Width property implementation
ast-grep --pattern 'ushort Width { get; $$$}'
Length of output: 142
Script:
#!/bin/bash
# Let's try a different approach using ripgrep to find implementations
rg "class.*:.*ISymbol" -l
rg "struct.*:.*ISymbol" -l
# Search for files containing ISymbol interface usage
rg "ISymbol" --type cs -l
# Look at actual implementations
rg "class.*:.*ISymbol" -A 20
Length of output: 602
Script:
#!/bin/bash
# Let's examine the actual implementation of both classes we found
echo "=== DrawingBoxSymbol.cs ==="
cat src/Consolonia.Core/Drawing/PixelBufferImplementation/DrawingBoxSymbol.cs
echo -e "\n=== SimpleSymbol.cs ==="
cat src/Consolonia.Core/Drawing/PixelBufferImplementation/SimpleSymbol.cs
Length of output: 6721
src/Consolonia.Core/Text/TextShaper.cs (1)
4-4: LGTM: Unicode namespace import aligns with PR objectives.
src/Consolonia.Core/Drawing/PixelBufferImplementation/SimpleSymbol.cs (3)
8-10: LGTM! Good addition of DebuggerDisplay attribute.
The DebuggerDisplay attribute will improve debugging experience by showing the actual text content.
29-31: LGTM! Properties are well-designed.
The read-only properties with proper defaults ensure thread-safety and null-safety.
1-42: Verify handling of surrogate pairs and combining characters.
While the transition to string-based storage enables Unicode support, we should verify the implementation handles all Unicode cases correctly.
#!/bin/bash
# Look for test cases covering surrogate pairs and combining characters
rg -l "surrogate|emoji|combining.*character" --type cs
# Check for Unicode-related test files
fd -e cs test.*unicode -x cat {}src/Consolonia.Core/Drawing/PixelBufferImplementation/DrawingBoxSymbol.cs (3)
2-2: LGTM! The new debugging and Unicode support changes look good.
The added debugging support and new properties align well with the PR objectives:
- DebuggerDisplay attribute improves debugging visibility
- Text property supports Unicode character representation
- Width property correctly handles the fixed-width nature of box-drawing characters
Also applies to: 10-10, 24-26
119-130: Great improvement in type safety!
The change from returning raw bytes to DrawingBoxSymbol instances is a solid improvement that:
- Enhances type safety
- Makes the API more intuitive
- Reduces the chance of misuse
24-24: Verify Unicode compatibility of box-drawing characters.
Let's ensure the box-drawing characters are properly handled with the new Unicode support:
✅ Verification successful
Box-drawing characters are safely handled with Unicode support
The implementation is safe because:
- The
DrawingBoxSymbolstruct uses fixed single-width box-drawing characters (║│═─┼╬╪╫) - Characters are explicitly defined as constants and handled through a well-defined pattern system
- The
Widthproperty is explicitly set to 1, ensuring consistent rendering - The implementation follows Code Page 437 box-drawing character specifications, which are part of the Unicode standard's compatibility area
The ToString() usage in Text property is safe as these box-drawing characters are single code points that don't involve surrogate pairs or complex Unicode sequences.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if any box-drawing characters are used with surrogate pairs
# or other Unicode sequences that might affect rendering.
# Search for Unicode-related tests or usage
rg -l "DrawingBoxSymbol.*[║│═─┼╬╪╫]" --type=cs
# Look for any Unicode-related configuration or handling
rg -l "Unicode.*DrawingBoxSymbol" --type=cs
Length of output: 94
Script:
#!/bin/bash
# Let's check the implementation of DrawingBoxSymbol to understand how it handles characters
ast-grep --pattern 'class DrawingBoxSymbol {
$$$
}'
# Also check for any box drawing related constants or character definitions
rg "BoxSymbol|[║│═─┼╬╪╫]" -A 5 -B 5 --type=cs
Length of output: 12254
src/Tests/Consolonia.TestsCore/UnitTestConsole.cs (1)
Line range hint 124-124: LGTM: Clean method signature update
The addition of the optional RawInputModifiers parameter with a default value maintains backward compatibility while adding flexibility.
src/Consolonia.Core/Drawing/RenderTarget.cs (1)
24-24: LGTM: Cache field type change supports Unicode surrogate pairs.
The change from char to string in the cache tuple is essential for proper Unicode support, particularly for characters that require surrogate pairs.
Let's verify there are no remaining char-based caches in the codebase:
✅ Verification successful
The change from char to string in the cache field is appropriate and safe
The codebase analysis shows that while there are still char usages in the codebase, they are used in different contexts:
- For box-drawing characters and quad-pixel characters which are single-character symbols
- In the
SimpleSymbolconstructor which already converts the char to string viaToString() - In documentation and parameter descriptions
The cache field change is consistent with the codebase's evolution towards better Unicode support, and there are no other similar cache fields that would need updating.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining char-based caches that might need similar updates
ast-grep --pattern 'char[,]?\s*\](\s*,\s*\])*'
Length of output: 48
Script:
#!/bin/bash
# Let's try a different approach using ripgrep to find char array declarations
rg "char\[.*\]" --type cs
# Also search for char-based collections that might need updates
rg "(List|Array|IEnumerable)<char>" --type cs
Length of output: 75
Script:
#!/bin/bash
# Let's search for any remaining usages of char in the codebase, focusing on the Drawing namespace
rg "char" src/Consolonia.Core/Drawing/ -B 2 -A 2
# Also check for any other cache fields that might need similar updates
ast-grep --pattern '$_[] _cache;'
Length of output: 12484
src/Consolonia.Core/Drawing/PixelBufferImplementation/SimpleSymbol.cs
Outdated
Show resolved
Hide resolved
tomlm
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
src/Consolonia.Gallery/Gallery/GalleryViews/GalleryTextBlock.axaml (2)
35-41: Excellent Unicode support implementation.The examples effectively demonstrate the new Unicode capabilities including surrogate pairs, emojis, and international text. Consider adding FlowDirection="RightToLeft" for RTL script examples to ensure proper text layout.
<StackPanel Spacing="0"> <TextBlock Text="International chars" TextDecorations="Underline"/> <TextBlock Text="Chinese: 中国人 "/> <TextBlock Text="Japanese: 漢字学"/> <TextBlock Text="Korean: 한국인" /> + <TextBlock Text="Arabic: مرحبا بالعالم" FlowDirection="RightToLeft" /> </StackPanel>
42-55: Well-structured style demonstrations.The systematic approach to demonstrating text styles is excellent. Consider adding comments to document the purpose of each color combination for future maintenance.
<WrapPanel > + <!-- Light text on dark background demonstrations --> <TextBlock Text="BOLD" Background="DarkMagenta" Foreground="Yellow" FontWeight="Bold" Margin="1 0"/> <TextBlock Text="NORMAL" Background="DarkMagenta" Foreground="Yellow" FontWeight="Normal" Margin="1 0"/> <TextBlock Text="LIGHT" Background="DarkMagenta" Foreground="Yellow" FontWeight="Light" Margin="1 0"/> </WrapPanel>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/Consolonia.Gallery/Gallery/GalleryViews/GalleryTextBlock.axaml(1 hunks)
🔇 Additional comments (1)
src/Consolonia.Gallery/Gallery/GalleryViews/GalleryTextBlock.axaml (1)
13-14: LGTM! Good accessibility consideration.
The addition of ScrollViewer with explicit focus support ensures proper keyboard navigation and handles content overflow appropriately.
src/Consolonia.Gallery/Gallery/GalleryViews/GalleryTextBlock.axaml
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (6)
src/Consolonia.Core/Text/TextShaper.cs (2)
20-21: Consider using Buffer.BlockCopy for better performanceThe current implementation copies glyphs one by one in a loop. For large text blocks, this could be inefficient.
Consider using a more efficient bulk copy operation:
-for (int i = 0; i < shapedBuffer.Length; i++) - shapedBuffer[i] = glyphInfos[i]; +System.Buffer.BlockCopy(glyphInfos, 0, shapedBuffer.Spans, 0, glyphInfos.Length * sizeof(GlyphInfo));
13-21: Implement comprehensive Unicode supportThe current implementation doesn't fully address the PR's objective of supporting Unicode surrogate sequences. A complete solution should handle:
- Proper Unicode character preservation
- Correct character width calculation
- Combining characters
- Grapheme clusters
- Complex script shaping
Consider implementing a more robust text shaping pipeline:
- Use a proper text shaping engine like HarfBuzz
- Handle grapheme clusters properly using
StringInfo- Support combining characters
- Implement proper width calculation for CJK, emoji, etc.
Would you like me to provide a detailed implementation proposal that addresses all these aspects?
src/Consolonia.Core/Drawing/PixelBufferImplementation/SimpleSymbol.cs (2)
15-19: Consider using char.ToString(IFormatProvider) for culture-aware string conversion.While the current implementation works, using
char.ToString()might not handle all culture-specific cases correctly. Consider usingchar.ToString(CultureInfo.InvariantCulture)for more robust Unicode handling.- Text = character.ToString(); + Text = character.ToString(System.Globalization.CultureInfo.InvariantCulture);
38-38: Improve Blend method with pattern matching and string.IsNullOrEmpty.The current implementation can be improved for consistency and readability.
- return !String.IsNullOrEmpty(symbolAbove.Text) ? symbolAbove : this; + return symbolAbove is { Text: { Length: > 0 } } ? symbolAbove : this;Alternatively, if you prefer the null check approach:
- return !String.IsNullOrEmpty(symbolAbove.Text) ? symbolAbove : this; + return !string.IsNullOrEmpty(symbolAbove.Text) ? symbolAbove : this;src/Tests/Consolonia.GalleryTests/TextBlockTests.cs (1)
24-28: Consider adding RTL text examples and improving test organization.While the current tests cover various Unicode sequences, consider these improvements:
- Add right-to-left text examples as mentioned in the PR objectives
- Organize tests into logical groups with descriptive comments
- Add assertions for mixed RTL/LTR text scenarios
Example structure:
// multiline "│Lorem ipsum dolor sit amet, consectetur adipiscing", "│elit. Vivamus magna. Cras in mi at felis aliquet", "│congue. Ut a est eget ligula molestie gravida.", -// special chars, emojis, etc. -"𐓏𐓘𐓻𐓘𐓻𐓟 𐒻𐓟", "𝄞","🎵", ""𝔉𝔞𝔫𝔠𝔶"","ff","fi","½"); +// Indigenous languages (Osage script) +"𐓏𐓘𐓻𐓘𐓻𐓟 𐒻𐓟", +// Musical notation +"𝄞", "🎵", +// Mathematical notation (Fraktur) +""𝔉𝔞𝔫𝔠𝔶"", +// Typography (Ligatures and fractions) +"ff", "fi", "½", +// Right-to-left text +"שָׁלוֹם", // Hebrew +"مرحبا", // Arabic +// Mixed RTL/LTR +"Hello مرحبا World");src/Consolonia.Core/Drawing/PixelBufferImplementation/DrawingBoxSymbol.cs (1)
9-9: LGTM! Consider enhancing the DebuggerDisplay format.The new properties and debugging support look good. The Text property aligns well with Unicode support goals, and the Width property correctly reflects the single-character nature of box symbols.
Consider making the DebuggerDisplay more informative:
-[DebuggerDisplay("DrawingBox {Text}")] +[DebuggerDisplay("DrawingBox: {Text} (0x{_upRightDownLeft:X2})")]Also applies to: 23-25
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
src/Consolonia.Core/Drawing/DrawingContextImpl.cs(9 hunks)src/Consolonia.Core/Drawing/PixelBufferImplementation/DrawingBoxSymbol.cs(4 hunks)src/Consolonia.Core/Drawing/PixelBufferImplementation/ISymbol.cs(1 hunks)src/Consolonia.Core/Drawing/PixelBufferImplementation/Pixel.cs(2 hunks)src/Consolonia.Core/Drawing/PixelBufferImplementation/SimpleSymbol.cs(1 hunks)src/Consolonia.Core/Text/GlyphRunImpl.cs(1 hunks)src/Consolonia.Core/Text/TextShaper.cs(1 hunks)src/Tests/Consolonia.GalleryTests/TextBlockTests.cs(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- src/Consolonia.Core/Drawing/DrawingContextImpl.cs
- src/Consolonia.Core/Drawing/PixelBufferImplementation/ISymbol.cs
- src/Consolonia.Core/Drawing/PixelBufferImplementation/Pixel.cs
- src/Consolonia.Core/Text/GlyphRunImpl.cs
🔇 Additional comments (6)
src/Consolonia.Core/Drawing/PixelBufferImplementation/SimpleSymbol.cs (3)
1-6: LGTM! Clean imports and helpful debug display.
The imports are minimal and the DebuggerDisplay attribute will improve debugging experience by showing the Text content.
27-29: LGTM! Well-designed immutable properties.
The properties are correctly implemented as read-only, maintaining immutability of the struct.
21-24: Verify Width handling for surrogate pairs.
The Width parameter is crucial for correct rendering of surrogate pairs and combining characters. Ensure that the width calculation in the calling code correctly accounts for the full Unicode grapheme clusters.
src/Tests/Consolonia.GalleryTests/TextBlockTests.cs (1)
1-1: LGTM: Required imports added for Unicode text handling.
The new imports are necessary for supporting the enhanced text rendering capabilities and TextBlock controls.
Also applies to: 3-4
src/Consolonia.Core/Drawing/PixelBufferImplementation/DrawingBoxSymbol.cs (2)
Line range hint 118-133: Great improvement in type safety!
The change from returning raw bytes to returning DrawingBoxSymbol instances is a solid improvement that:
- Enhances type safety
- Makes the API more intuitive
- Maintains consistency with the struct's design
Line range hint 30-97: LGTM! The box symbol mapping is comprehensive.
The private GetBoxSymbol method provides a robust implementation for box drawing characters. The switch expressions are well-organized and cover all valid combinations of the _upRightDownLeft patterns.
Let's verify that all box drawing characters are properly handled:
✅ Verification successful
The box symbol mapping implementation is well-structured and complete
Based on the code shown in the review comment, the implementation comprehensively handles all valid combinations of box drawing characters through:
- Simple cases for vertical (│, ║) and horizontal (─, ═) lines
- Complex intersections (┼, ╬, ╪, ╫)
- All corner variants (└, ┌, ┐, ┘) with their bold/double-line counterparts
- T-junction variants (├, ┤, ┬, ┴) with their bold/double-line counterparts
- Special cases for empty and bold symbols
The switch expressions are well-organized and the bit manipulation approach using _upRightDownLeft is an efficient way to determine the appropriate box drawing character.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any missing box drawing character cases
# Look for switch expressions without corresponding box drawing characters
ast-grep --pattern 'switch ($_) {
$$$
_ => throw new InvalidOperationException()
$$$
}'
Length of output: 95
Script:
#!/bin/bash
# Let's try a different approach to find the switch expression in the GetBoxSymbol method
ast-grep --pattern 'return $_ switch
{
$$$
_ => throw new InvalidOperationException()
$$$
}'
Length of output: 100
Script:
#!/bin/bash
# Let's search for the file content directly using ripgrep
rg -U "private char GetBoxSymbol.*?\{.*?throw new InvalidOperationException\(\)" -A 100 --multiline
Length of output: 101
|
I have tried it, when switching to TextBlock in gallery application stops responding, nothing redrawn. I could not figure out why. Sorry, can not debug these days. |
moved width calculation into SimpleSymbol
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (10)
src/Tests/Consolonia.GalleryTests/TextBlockTests.cs (1)
2-3: Verify the necessity of newly added using directives.The static analysis indicates that the following using directives are currently unused:
Avalonia.ControlsAvalonia.MediaNUnit.Framework.InternalIf these are intended for future test additions, consider adding a comment explaining their purpose. Otherwise, they can be safely removed.
Also applies to: 7-7
🧰 Tools
🪛 GitHub Check: build
[warning] 2-2:
"[RedundantUsingDirective] Using directive is not required by the code and can be safely removed" on /home/runner/work/Consolonia/Consolonia/src/Tests/Consolonia.GalleryTests/TextBlockTests.cs(2,1)
[warning] 3-3:
"[RedundantUsingDirective] Using directive is not required by the code and can be safely removed" on /home/runner/work/Consolonia/Consolonia/src/Tests/Consolonia.GalleryTests/TextBlockTests.cs(3,1)src/Consolonia.Core/Helpers/Extensions.cs (1)
18-27: Consider adding parameter validation.While the implementation looks good and aligns with the Unicode support objectives, consider adding null checks for the parameters to prevent potential runtime exceptions.
public static void Print(this IConsole console, PixelBufferCoordinate point, Pixel pixel) { + if (console == null) + throw new ArgumentNullException(nameof(console)); + if (pixel.Foreground == null) + throw new ArgumentNullException(nameof(pixel), "Pixel.Foreground cannot be null"); + console.Print(point, pixel.Background.Color, pixel.Foreground.Color, pixel.Foreground.Style, pixel.Foreground.Weight, pixel.Foreground.TextDecorations, pixel.Foreground.Symbol.Text); }src/Consolonia.Core/Drawing/PixelBufferImplementation/SimpleSymbol.cs (2)
12-17: Consider documenting the width=1 design decision.While using String.Empty with width=1 is a good approach for maintaining layout, it would be helpful to document why this specific width was chosen for empty symbols.
public SimpleSymbol() { - // we use String.Empty to represent an empty symbol. It still takes up space, but it's invisible + // Empty symbols use String.Empty with width=1 to maintain consistent layout spacing. + // This ensures empty cells still occupy space in the buffer while remaining invisible. Text = string.Empty; Width = 1; }
47-47: Fix String casing for consistency.Use
stringinstead ofStringto match the codebase style.- return !String.IsNullOrEmpty(symbolAbove.Text) ? symbolAbove : this; + return !string.IsNullOrEmpty(symbolAbove.Text) ? symbolAbove : this;🧰 Tools
🪛 GitHub Check: build
[notice] 47-47:
"[BuiltInTypeReferenceStyleForMemberAccess] Built-in type reference is inconsistent with code style settings" on /home/runner/work/Consolonia/Consolonia/src/Consolonia.Core/Drawing/PixelBufferImplementation/SimpleSymbol.cs(47,21)src/Consolonia.Core/Drawing/PixelBufferImplementation/PixelBuffer.cs (2)
73-76: Improve XML documentation.The XML documentation is incomplete. Add a description for the
pointparameter and document any exceptions that may be thrown.
79-82: Clean up commented code and standardize variable declarations.
- Remove the commented parameter
/*, false*/as it's no longer needed.- Consider standardizing the use of
varvs explicit type declarations for consistency.- Pixel oldCaretPixel = _buffer[_caretPosition.X, _caretPosition.Y]; - _buffer[_caretPosition.X, _caretPosition.Y] = - new Pixel(oldCaretPixel.Foreground, oldCaretPixel.Background /*, false*/); + var oldCaretPixel = _buffer[_caretPosition.X, _caretPosition.Y]; + _buffer[_caretPosition.X, _caretPosition.Y] = + new Pixel(oldCaretPixel.Foreground, oldCaretPixel.Background);src/Consolonia.Core/Infrastructure/InputLessDefaultNetConsole.cs (2)
2-2: Remove unused import.The
System.Diagnosticsnamespace is not used in this file and can be safely removed.-using System.Diagnostics;🧰 Tools
🪛 GitHub Check: build
[warning] 2-2:
"[RedundantUsingDirective] Using directive is not required by the code and can be safely removed" on /home/runner/work/Consolonia/Consolonia/src/Consolonia.Core/Infrastructure/InputLessDefaultNetConsole.cs(2,1)
Line range hint
71-108: Consider performance optimizations to address reported unresponsiveness.The
- Cache the StringBuilder initial capacity based on common use cases
- Batch console writes where possible
- Consider using a buffer pool for frequently used string operations
Would you like me to provide a detailed implementation of these optimizations?
src/Tests/Consolonia.TestsCore/UnitTestConsole.cs (2)
151-151: Consider using standard caret symbolUsing the Cherokee letter 'Ꮖ' as a caret symbol is not ideal:
- It may not be supported in all fonts
- It could be confused with actual text content
- Traditional caret symbols like '▊' or '█' would be more appropriate
- string text = pixel.IsCaret ? "Ꮖ" : pixel.Foreground.Symbol.Text; + string text = pixel.IsCaret ? "█" : pixel.Foreground.Symbol.Text;
152-152: Address TODO comment about cursor drawingThe TODO comment indicates an unresolved issue with cursor drawing. This should be investigated and fixed as it's directly related to the Unicode support changes.
Would you like me to help investigate the cursor drawing issue or create a GitHub issue to track this?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (12)
src/Consolonia.Core/Consolonia.Core.csproj(1 hunks)src/Consolonia.Core/Drawing/DrawingContextImpl.cs(9 hunks)src/Consolonia.Core/Drawing/PixelBufferImplementation/PixelBuffer.cs(2 hunks)src/Consolonia.Core/Drawing/PixelBufferImplementation/SimpleSymbol.cs(1 hunks)src/Consolonia.Core/Helpers/Extensions.cs(2 hunks)src/Consolonia.Core/Infrastructure/InputLessDefaultNetConsole.cs(3 hunks)src/Consolonia.Core/Text/GlyphTypeface.cs(2 hunks)src/Consolonia.Core/Text/TextShaper.cs(1 hunks)src/Consolonia.Gallery/Gallery/GalleryViews/GalleryTextBlock.axaml(2 hunks)src/Consolonia.Themes.TurboVision/Templates/Controls/Helpers/SymbolsControl.cs(2 hunks)src/Tests/Consolonia.GalleryTests/TextBlockTests.cs(2 hunks)src/Tests/Consolonia.TestsCore/UnitTestConsole.cs(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- src/Consolonia.Core/Consolonia.Core.csproj
- src/Consolonia.Core/Drawing/DrawingContextImpl.cs
- src/Consolonia.Core/Text/GlyphTypeface.cs
- src/Consolonia.Gallery/Gallery/GalleryViews/GalleryTextBlock.axaml
- src/Consolonia.Themes.TurboVision/Templates/Controls/Helpers/SymbolsControl.cs
🧰 Additional context used
🪛 GitHub Check: build
src/Consolonia.Core/Drawing/PixelBufferImplementation/SimpleSymbol.cs
[notice] 47-47:
"[BuiltInTypeReferenceStyleForMemberAccess] Built-in type reference is inconsistent with code style settings" on /home/runner/work/Consolonia/Consolonia/src/Consolonia.Core/Drawing/PixelBufferImplementation/SimpleSymbol.cs(47,21)
src/Consolonia.Core/Infrastructure/InputLessDefaultNetConsole.cs
[warning] 2-2:
"[RedundantUsingDirective] Using directive is not required by the code and can be safely removed" on /home/runner/work/Consolonia/Consolonia/src/Consolonia.Core/Infrastructure/InputLessDefaultNetConsole.cs(2,1)
[warning] 12-12:
"[RedundantUsingDirective] Using directive is not required by the code and can be safely removed" on /home/runner/work/Consolonia/Consolonia/src/Consolonia.Core/Infrastructure/InputLessDefaultNetConsole.cs(12,1)
src/Tests/Consolonia.GalleryTests/TextBlockTests.cs
[warning] 2-2:
"[RedundantUsingDirective] Using directive is not required by the code and can be safely removed" on /home/runner/work/Consolonia/Consolonia/src/Tests/Consolonia.GalleryTests/TextBlockTests.cs(2,1)
[warning] 3-3:
"[RedundantUsingDirective] Using directive is not required by the code and can be safely removed" on /home/runner/work/Consolonia/Consolonia/src/Tests/Consolonia.GalleryTests/TextBlockTests.cs(3,1)
[warning] 7-7:
"[RedundantUsingDirective] Using directive is not required by the code and can be safely removed" on /home/runner/work/Consolonia/Consolonia/src/Tests/Consolonia.GalleryTests/TextBlockTests.cs(7,1)
🔇 Additional comments (13)
src/Consolonia.Core/Text/TextShaper.cs (1)
13-20: Verify text shaping performance impact
Let's analyze the codebase for potential performance bottlenecks and similar implementations.
✅ Verification successful
Based on the shell script results, I can now provide a final response regarding the performance impact of the text shaping changes:
Text shaping implementation is performance-efficient
The current implementation is actually optimal for the console-based rendering context:
- It uses
EnumerateRunes()which is the most efficient way to process Unicode text in .NET - The
ShapedBufferallocation happens once with a pre-calculated size - The implementation avoids multiple allocations by using array indexing instead of creating new collections
The codebase shows this pattern is consistently used across other performance-critical paths:
DrawingContextImpl.csuses the same rune enumeration patternSimpleSymbol.csfollows similar efficient text processing approach- No redundant string allocations or conversions are found
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for other text shaping implementations and potential bottlenecks
echo "Searching for other text shaping implementations..."
ast-grep --pattern 'class $_ : ITextShaperImpl {
$$$
}'
echo "Looking for similar string processing patterns..."
rg -A 5 "EnumerateRunes|ShapedBuffer|GlyphInfo" --type cs
echo "Checking for performance-critical paths..."
rg -A 5 "TextBlock|TextShaper" --type cs
Length of output: 11926
src/Tests/Consolonia.GalleryTests/TextBlockTests.cs (1)
24-27:
Verify text rendering performance with large Unicode sequences.
Given the reported unresponsiveness when switching to TextBlock in the gallery application, we should verify the rendering performance with various text combinations.
Consider adding performance tests that:
- Measure rendering time with large Unicode sequences
- Test memory usage with extensive surrogate pairs
- Include stress tests with rapid text changes
src/Consolonia.Core/Helpers/Extensions.cs (2)
4-5: LGTM! Required namespaces for pixel buffer handling.
The added namespaces are necessary for the new Print method implementation.
26-26: Verify surrogate pair handling in Symbol.Text.
Let's ensure the Unicode surrogate pairs are properly handled throughout the processing pipeline.
✅ Verification successful
Let me gather more specific information about the Unicode handling implementation.
Let me check one more crucial piece - the actual Symbol class implementation and any Unicode-specific handling code.
Unicode surrogate pairs are properly handled in the codebase
The implementation shows robust Unicode support:
- ISymbol interface explicitly documents support for emoji and Unicode characters
- The codebase uses NeoSmart.Unicode library for proper Unicode handling
- SimpleSymbol implementation correctly processes Unicode width using UnicodeCalculator
- Tests include verification with surrogate pairs, emojis, and complex Unicode characters (𐓏𐓘𐓻𐓘𐓻𐓟, 𝄞, 🎵)
- Proper UTF-8 encoding is set for console output
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for Symbol class definition and usage to verify Unicode handling
echo "Checking Symbol class implementation..."
ast-grep --pattern 'class Symbol {
$$$
}'
echo "Checking Symbol.Text property usage..."
rg -l "Symbol\.Text" --type cs
echo "Checking for Unicode-related tests..."
rg -l "surrogate|emoji|Unicode" --glob "**/*Test*.cs"
Length of output: 719
Script:
#!/bin/bash
# Check Symbol class implementation details
echo "Checking Symbol class content..."
rg -A 10 "class Symbol" --type cs
# Check TextBlock tests for Unicode handling
echo "Checking Unicode-related test content..."
rg -C 5 "Unicode|surrogate|emoji" src/Tests/Consolonia.GalleryTests/TextBlockTests.cs
# Check actual Symbol.Text usage in context
echo "Checking Symbol.Text usage context..."
rg -C 3 "Symbol\.Text" --type cs
Length of output: 6428
Script:
#!/bin/bash
# Find and check Symbol interface/class implementation
echo "Looking for ISymbol interface and implementation..."
rg -A 15 "interface ISymbol|class.*:.*ISymbol" --type cs
# Check for any Unicode/string handling methods
echo "Checking string handling methods..."
rg -C 5 "string.*Text.*{.*get" --type cs
# Look for any character encoding related code
echo "Checking encoding related code..."
rg -C 3 "Encoding|UTF|Unicode" --type cs
Length of output: 10196
src/Consolonia.Core/Drawing/PixelBufferImplementation/SimpleSymbol.cs (5)
1-6: Remove unused import System.Text
9-11: LGTM! Good debugging support.
The DebuggerDisplay attribute will help with debugging by showing the Text content.
24-34: LGTM! Well-structured Unicode support.
The string and Rune constructors properly handle width calculation for Unicode characters.
36-38: LGTM! Good immutability design.
The readonly properties in a readonly struct ensure thread-safety and prevent mutation.
51-67: LGTM! Robust Unicode width calculation.
The MeasureGlyph method correctly handles:
- Complex emoji sequences with zero-width joiners
- Object replacement characters
- Multi-width Unicode characters
This aligns well with the PR objective of supporting surrogate sequences.
src/Consolonia.Core/Drawing/PixelBufferImplementation/PixelBuffer.cs (3)
14-14: LGTM: Field declaration follows conventions.
The private field declaration for caret position tracking is well-structured and follows naming conventions.
21-21: LGTM: Proper field initialization.
The caret position is correctly initialized to the origin (0,0).
77-90: Consider validating pixel states during caret updates.
The method directly modifies pixels without validating their current state. Consider adding checks to ensure pixels are in a valid state before modification, especially since this is part of Unicode support changes.
Consider adding validation:
public void SetCaretPosition(PixelBufferCoordinate point)
{
+ ValidatePixelState(_buffer[_caretPosition.X, _caretPosition.Y]);
// clear old caret position by merging in IsCaret = false
var oldCaretPixel = _buffer[_caretPosition.X, _caretPosition.Y];
_buffer[_caretPosition.X, _caretPosition.Y] =
new Pixel(oldCaretPixel.Foreground, oldCaretPixel.Background);
_caretPosition = point;
+ ValidatePixelState(_buffer[point.X, point.Y]);
// set new caret position by merging in IsCaret = true
var newCaretPixel = _buffer[_caretPosition.X, _caretPosition.Y];
_buffer[_caretPosition.X, _caretPosition.Y] =
new Pixel(newCaretPixel.Foreground, newCaretPixel.Background, true);
}
+private void ValidatePixelState(Pixel pixel)
+{
+ // Add validation logic here
+ // Example: Check if the pixel's current state is valid for caret positioning
+}src/Consolonia.Core/Infrastructure/InputLessDefaultNetConsole.cs (1)
Line range hint 71-71: Previous review comment about exception handling is still applicable.
The concern about handling potential IOException when calling Console.GetCursorPosition() remains unaddressed.
🧰 Tools
🪛 GitHub Check: build
[warning] 2-2:
"[RedundantUsingDirective] Using directive is not required by the code and can be safely removed" on /home/runner/work/Consolonia/Consolonia/src/Consolonia.Core/Infrastructure/InputLessDefaultNetConsole.cs(2,1)
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (3)
src/Consolonia.Core/Drawing/PixelBufferImplementation/SimpleSymbol.cs (1)
25-29: Consider caching glyph width measurements.Both string and Rune constructors calculate width using
MeasureGlyph(). For frequently used glyphs, consider caching these measurements to improve performance.Consider implementing a static cache:
+ private static readonly ConcurrentDictionary<string, ushort> _widthCache = new(); public SimpleSymbol(string glyph) { Text = glyph; - Width = Text.MeasureGlyph(); + Width = _widthCache.GetOrAdd(Text, static text => text.MeasureGlyph()); }Also applies to: 31-34
src/Consolonia.Core/Helpers/Extensions.cs (2)
22-31: LGTM: Clean extension method for pixel printingThe method provides a convenient way to print pixels while maintaining a clean separation of concerns. Consider adding XML documentation to clarify the method's purpose and parameters.
Add documentation like this:
+ /// <summary> + /// Prints a pixel at the specified coordinate using the console. + /// </summary> + /// <param name="console">The console to print to</param> + /// <param name="point">The coordinate where to print</param> + /// <param name="pixel">The pixel containing the styling and content to print</param> public static void Print(this IConsole console, PixelBufferCoordinate point, Pixel pixel)
74-91: Consider performance optimization for large stringsThe method correctly calculates glyph widths while handling special Unicode characters. However, for better performance with large strings, consider pre-calculating the capacity needed for the enumeration.
Add a capacity hint:
public static ushort MeasureGlyph(this string glyph) { + if (string.IsNullOrEmpty(glyph)) + return 0; ushort width = 0; ushort lastWidth = 0; - foreach (var rune in glyph.EnumerateRunes()) + var runes = glyph.EnumerateRunes(); + foreach (var rune in runes)Also, consider adding validation for null input to prevent potential NullReferenceException.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
src/Consolonia.Core/Drawing/DrawingContextImpl.cs(9 hunks)src/Consolonia.Core/Drawing/PixelBufferImplementation/SimpleSymbol.cs(1 hunks)src/Consolonia.Core/Helpers/Extensions.cs(2 hunks)src/Consolonia.Core/Text/TextShaper.cs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Consolonia.Core/Drawing/DrawingContextImpl.cs
🔇 Additional comments (5)
src/Consolonia.Core/Drawing/PixelBufferImplementation/SimpleSymbol.cs (3)
10-12: LGTM! Good debugging support.
The DebuggerDisplay attribute will improve debugging experience by showing the actual text content.
13-18: LGTM! Well-documented default constructor.
The empty constructor's comment clearly explains the semantic meaning of an empty symbol.
37-39: LGTM! Well-designed immutable properties.
The immutable design with proper initialization aligns well with the struct's readonly nature.
src/Consolonia.Core/Helpers/Extensions.cs (2)
1-9: LGTM: Appropriate dependencies added for Unicode support
The new using directives for NeoSmart.Unicode and other required namespaces are correctly added to support the Unicode functionality.
38-72: Verify GetGlyphs performance impact on UI
Given the reported UI freeze when switching to TextBlock in the gallery application, we should verify if the glyph processing could be causing performance issues with large text blocks.
Co-authored-by: <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
src/Consolonia.Core/Infrastructure/InputLessDefaultNetConsole.cs (1)
59-59: Add XML documentation for the SupportsComplexEmoji property.The property should be documented to explain its purpose and behavior.
Add XML documentation:
+ /// <summary> + /// Indicates whether the console supports complex emoji rendering. + /// True if the console properly renders surrogate pairs and combining characters. + /// </summary> public bool SupportsComplexEmoji { get; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/Consolonia.Core/Infrastructure/InputLessDefaultNetConsole.cs(3 hunks)
🔇 Additional comments (1)
src/Consolonia.Core/Infrastructure/InputLessDefaultNetConsole.cs (1)
120-125: Verify MeasureText implementation for Unicode handling.
The switch to MeasureText() is good for Unicode support, but we should verify its implementation handles all Unicode cases correctly.
Let's check the MeasureText implementation:
✅ Verification successful
MeasureText implementation handles Unicode correctly
The implementation in Extensions.cs properly handles Unicode text width calculation:
- Uses
Runefor proper Unicode character enumeration - Handles complex emoji with zero-width joiners (ZWJ) and object replacement characters
- Calculates width using
UnicodeCalculator.GetWidth - Supports complex emoji through
SupportsComplexEmojiflag
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify MeasureText implementation for Unicode handling
# Look for the implementation and any related tests
# Find MeasureText implementation
echo "Looking for MeasureText implementation:"
rg -l "MeasureText.*string" --type cs
# Check for related tests
echo -e "\nLooking for related tests:"
rg -l "MeasureText.*Test" --type cs
# Look for Unicode-specific test cases
echo -e "\nLooking for Unicode test cases:"
rg "MeasureText.*[👨🏻💻🌍]" --type cs
Length of output: 398
Script:
#!/bin/bash
# Let's examine the MeasureText implementation in Extensions.cs
echo "=== MeasureText Implementation ==="
rg -A 10 "MeasureText" src/Consolonia.Core/Helpers/Extensions.cs
# Let's also check for any string width calculation related tests
echo -e "\n=== String Width Related Tests ==="
rg -l "TestStringWidth|StringWidth.*Test|MeasureText.*Test" --type cs
# Let's look for any Unicode-related string handling
echo -e "\n=== Unicode String Handling ==="
rg "StringInfo|TextElementEnumerator|Rune|GetTextElement" --type cs
Length of output: 1759
Script:
#!/bin/bash
# Let's check the UnicodeCalculator implementation
echo "=== UnicodeCalculator Implementation ==="
rg -A 10 "class UnicodeCalculator" --type cs
# Let's also check for emoji support implementation
echo -e "\n=== Emoji Support Implementation ==="
rg -A 5 "SupportsComplexEmoji" --type cs
# Let's look for any width calculation tests
echo -e "\n=== Width Calculation Tests ==="
rg -A 10 "GetWidth.*Test" --type cs
Length of output: 3605
src/Consolonia.Core/Infrastructure/InputLessDefaultNetConsole.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (2)
src/Consolonia.Core/Helpers/Extensions.cs (2)
48-84: Simplify complex emoji processing logic.The nested if conditions make the code harder to follow. Consider extracting emoji processing logic into separate methods.
Here's a suggested refactoring:
+private static bool IsEmojiRelatedCharacter(Rune rune) => + rune.Value == Emoji.ZeroWidthJoiner || + rune.Value == Emoji.ObjectReplacementCharacter || + rune.Value == Codepoints.VariationSelectors.EmojiSymbol || + rune.Value == Codepoints.VariationSelectors.TextSymbol; +private static bool ShouldAppendToEmoji(Rune lastRune, Rune currentRune) => + lastRune.Value == Codepoints.ZWJ || + lastRune.Value == Codepoints.ORC || + Emoji.IsEmoji(currentRune.ToString()); while (runes.MoveNext()) if (console.SupportsComplexEmoji) { - if (lastRune.Value == Codepoints.ZWJ || - lastRune.Value == Codepoints.ORC || - Emoji.IsEmoji(runes.Current.ToString())) + if (ShouldAppendToEmoji(lastRune, runes.Current)) { emoji.Append(runes.Current); } - else if (runes.Current.Value == Emoji.ZeroWidthJoiner || - runes.Current.Value == Emoji.ObjectReplacementCharacter || - rune.Value == Codepoints.VariationSelectors.EmojiSymbol || - rune.Value == Codepoints.VariationSelectors.TextSymbol) + else if (IsEmojiRelatedCharacter(runes.Current)) { emoji.Append(runes.Current); }
104-106: Cache SupportsComplexEmoji property outside the loop.Accessing
console.SupportsComplexEmojiin each iteration is inefficient.var console = AvaloniaLocator.Current.GetService<IConsole>(); +bool supportsComplexEmoji = console.SupportsComplexEmoji; ushort width = 0; ushort lastWidth = 0; foreach (Rune rune in text.EnumerateRunes()) { ushort runeWidth = (ushort)UnicodeCalculator.GetWidth(rune); - if (console.SupportsComplexEmoji && + if (supportsComplexEmoji && (rune.Value == Emoji.ZeroWidthJoiner || rune.Value == Emoji.ObjectReplacementCharacter))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
src/Consolonia.Core/Drawing/DrawingContextImpl.cs(10 hunks)src/Consolonia.Core/Helpers/Extensions.cs(2 hunks)src/Consolonia.Core/Infrastructure/InputLessDefaultNetConsole.cs(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Consolonia.Core/Drawing/DrawingContextImpl.cs
🧰 Additional context used
🪛 GitHub Check: build
src/Consolonia.Core/Infrastructure/InputLessDefaultNetConsole.cs
[warning] 29-29:
"[CSharpWarnings::CS1587] XML comment is not placed on a valid language element" on /home/runner/work/Consolonia/Consolonia/src/Consolonia.Core/Infrastructure/InputLessDefaultNetConsole.cs(29,13)
🔇 Additional comments (5)
src/Consolonia.Core/Helpers/Extensions.cs (2)
22-31: LGTM: Clean and well-structured Print method.
The method follows good practices by:
- Properly unpacking Pixel properties
- Following single responsibility principle
- Using clear parameter names
39-39: Use more specific return type.
As suggested in past review comments, using IReadOnlyCollection<string> or string[] would better indicate immutability.
src/Consolonia.Core/Infrastructure/InputLessDefaultNetConsole.cs (3)
60-60: LGTM: Well-designed property for emoji support detection.
The read-only property correctly encapsulates the emoji support detection result.
121-126: LGTM: Improved text width calculation for Unicode support.
The change from string.Length to MeasureText() correctly handles the width of Unicode characters, including surrogate pairs and combining characters. This should resolve potential cursor positioning issues with complex Unicode text.
121-126: Verify performance impact of text measurement.
Given the reported TextBlock unresponsiveness, let's verify if there are any performance-critical calls to MeasureText().
src/Consolonia.Core/Infrastructure/InputLessDefaultNetConsole.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
src/Consolonia.Core/Infrastructure/InputLessDefaultNetConsole.cs (1)
60-60: Add XML documentation for public API.Please add XML documentation for the
SupportsComplexEmojiproperty to describe its purpose and behavior.+ /// <summary> + /// Gets whether the console supports complex emoji rendering with surrogate pairs. + /// </summary> public bool SupportsComplexEmoji { get; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/Consolonia.Core/Infrastructure/InputLessDefaultNetConsole.cs(3 hunks)
🔇 Additional comments (3)
src/Consolonia.Core/Infrastructure/InputLessDefaultNetConsole.cs (3)
10-10: LGTM!
The addition of the Text namespace import is appropriate for the Unicode text handling functionality.
121-126: LGTM! Improved text width calculation.
The switch to MeasureText() is a good improvement that properly handles Unicode text width measurement, including surrogate pairs and complex emoji sequences.
24-38: Consider thread safety in console initialization.
The emoji support detection involves multiple console operations that could interfere with other threads accessing the console. Consider adding a lock or synchronization mechanism to ensure thread safety during initialization.
…ging regular pixels by falling through the the isCaret section instead of returning the blended pixel.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
src/Consolonia.Core/Text/GlyphTypeface.cs(3 hunks)src/Consolonia.Core/Text/TextShaper.cs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Consolonia.Core/Text/GlyphTypeface.cs
🔇 Additional comments (2)
src/Consolonia.Core/Text/TextShaper.cs (2)
1-4: Missing required imports for Unicode support
A previous review comment about adding System.Text import for proper Unicode handling is still valid.
12-12:
Performance and Unicode validation concerns
The current implementation has two issues:
ToString()creates unnecessary string allocation- No validation for well-formed Unicode sequences
Apply this diff to fix both issues:
-var glyphs = text.Span.ToString().GetGlyphs();
+if (!System.Text.Unicode.Utf16.IsWellFormed(text.Span))
+ throw new ArgumentException("Invalid Unicode sequence", nameof(text));
+var glyphs = text.Span.GetGlyphs();Likely invalid or redundant comment.
Fix for #112
Core Changes
Misc changes
Summary by CodeRabbit
Release Notes
New Features
TextBlockelements showcasing diverse text features, including emojis and international characters.TextandWidth, across various structures.Bug Fixes
Documentation
Chores