Refactor tests, splitting out Consolonia.NUnit library and a ton of unit tests #143
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
* Fix caret
Co-authored-by: <[email protected]>
* 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.
moved width calculation into SimpleSymbol
…r the Nunit.Framework.Internal as a unused reference but the linter does. This was driving me crazy!
Co-authored-by: <[email protected]>
Co-authored-by: <[email protected]>
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 21
🧹 Outside diff range and nitpick comments (60)
src/Consolonia.Core/Assembly.cs (1)
6-6: Consider addressing CLS compliance as part of test improvements.While not part of the current changes, the TODO comment raises an important architectural consideration. CLS compliance would ensure better cross-language compatibility. Consider:
- Evaluating the need for CLS compliance based on the project's usage patterns
- Adding test cases to verify cross-language compatibility if deciding to make it compliant
Would you like me to help create a GitHub issue to track the CLS compliance evaluation?
src/Consolonia.PlatformSupport/Assembly.cs (1)
6-6: Consider addressing the CLS compliance TODO.The TODO comment raises a valid question about CLS compliance. This could impact cross-language interoperability.
Would you like me to create a GitHub issue to track the CLS compliance investigation?
src/Consolonia.GuiCS/Assembly.cs (1)
7-7: Consider addressing CLS compliance as part of test refactoring.The TODO comment raises a valid question about CLS compliance. Since this PR involves significant test restructuring, it might be a good opportunity to evaluate and potentially implement CLS compliance.
Would you like me to help analyze the implications of making the assembly CLS compliant and create a GitHub issue to track this task?
src/Tests/Consolonia.Gallery.Tests/ScrollViewerTests.cs (1)
Line range hint
15-20: Improve test clarity and maintainability.While the test implementation works, consider these improvements:
- Add XML documentation or descriptive test name explaining the test's purpose
- Extract magic number "30" into a named constant with explanation
- Consider adding setup context about the initial UI state
- Move the expected cow eyes pattern into a constant with clear naming
Example improvement:
+ /// <summary> + /// Verifies that horizontal scrolling in the viewer correctly reveals the cow animation + /// by navigating to the scroll viewer and moving right to see the full pattern. + /// </summary> [Test] public async Task PerformSingleTest() { + const int STEPS_TO_REVEAL_PATTERN = 30; // Number of right scrolls needed to reveal the full cow pattern + const string EXPECTED_COW_EYES = @"\(o\) \(o \}"; + await UITest.KeyInput(Key.Tab); await UITest.KeyInput(Key.Tab, Key.Tab, Key.Tab); - await UITest.KeyInput(30, Key.Right); + await UITest.KeyInput(STEPS_TO_REVEAL_PATTERN, Key.Right); // Checking cow eyes visible - await UITest.AssertHasText(@"\(o\) \(o \}"); + await UITest.AssertHasText(EXPECTED_COW_EYES); }src/Tests/Consolonia.Gallery.Tests/ProgressBarTests.cs (2)
9-13: Consider implementing animation control mechanisms for testing.The TODO comment highlights a significant testing challenge with animated components. This could be addressed by:
- Implementing animation pause/disable mechanisms for testing
- Adding explicit wait conditions for animation completion
- Using test-specific progress bar configurations
Would you like me to help design a solution for testing animated components? I can create a GitHub issue to track this enhancement.
14-19: Enhance test method clarity and robustness.Consider the following improvements:
- Rename the test to describe the specific scenario being tested
- Add XML documentation explaining the test's purpose
- Add assertions to verify the Tab key properly focused the progress bar
- Make the percentage assertions more explicit about what they're verifying
Here's a suggested implementation:
[Test] -public async Task PerformSingleTest() +[Description("Verifies progress bar displays correct percentages after focus")] +public async Task ProgressBar_WhenFocused_DisplaysCorrectPercentages() { await UITest.KeyInput(Key.Tab); + await UITest.AssertHasFocus("progressBar"); // Verify focus await UITest.AssertHasText("5%", "50%"); }src/Tests/Consolonia.Gallery.Tests/ListBoxTests.cs (1)
Line range hint
15-19: Add test documentation to clarify the test scenario.While the test implementation is clear, adding XML documentation would help explain:
- The purpose of the keyboard navigation sequence
- The expected behavior
- Why "Item 53" is the expected result
[Test] +/// <summary> +/// Verifies that keyboard navigation (Tab, Shift+Tab, PageDown) works correctly in the ListBox +/// and allows reaching Item 53 through pagination. +/// </summary> public async Task PerformSingleTest()src/Tests/Consolonia.Core.Tests/Assembly.cs (1)
12-21: Consider improving test setup configuration and cleanup.A few suggestions to enhance the test setup:
- The hardcoded PixelBufferSize (100x100) might be too rigid. Consider making it configurable or documenting why this specific size was chosen.
- Missing cleanup: Add a
[OneTimeTearDown]method to ensure proper cleanup after tests.- Using
.ToConstant()for IConsole binding might affect test isolation if tests need different console configurations.Here's a suggested improvement:
[SetUpFixture] public class AllTests { + private const int DefaultBufferSize = 100; + [OneTimeSetUp] public void OneTimeSetUp() { AvaloniaLocator.Current = new AvaloniaLocator() - .Bind<IConsole>().ToConstant(new UnitTestConsole(new PixelBufferSize(100, 100))); + .Bind<IConsole>().ToConstant(new UnitTestConsole(new PixelBufferSize(DefaultBufferSize, DefaultBufferSize))); } + + [OneTimeTearDown] + public void OneTimeTearDown() + { + // Reset the locator to ensure clean state for other test runs + AvaloniaLocator.Current = new AvaloniaLocator(); + } }src/Tests/Consolonia.Gallery.Tests/TabControlTests.cs (1)
Line range hint
15-21: Consider adding comments to clarify test steps.While the test implementation is correct, adding comments to describe the purpose of each key input and assertion would improve readability and maintenance.
public async Task PerformSingleTest() { + // Set initial focus await UITest.KeyInput(Key.Tab); await UITest.AssertHasText("Arch", "Leaf"); + // Navigate to second tab await UITest.KeyInput(Key.Right); await UITest.AssertHasText("Arch", "Leaf", "This is the second page"); + // Navigate within second tab content await UITest.KeyInput(Key.Tab, Key.Tab, Key.Down); await UITest.AssertHasText("Arch", "Leaf", "This is the sec"); }src/Consolonia.Core/Drawing/PixelBufferImplementation/ISymbol.cs (1)
10-13: Consider documenting Width property edge cases.The Width property documentation could benefit from additional details about handling edge cases:
- How it behaves with zero-width characters
- Treatment of combining characters
- Handling of wide characters (CJK, emojis)
/// <summary> -/// The number of characters the symbol takes up +/// The number of characters the symbol takes up. +/// For complex Unicode characters: +/// - Combining characters are counted as part of the base character +/// - Wide characters (CJK, emojis) may occupy multiple width units +/// - Zero-width characters (e.g., ZWJ) are counted as width 0 /// </summary>src/Consolonia.Core/Consolonia.Core.csproj (1)
10-11: Consider using version variables for new packages.The package references look good. However, for consistency with how Avalonia versions are managed (using $(AvaloniaVersion)), consider moving the Unicode.net and Wcwidth versions to variables in the Directory.Core.Build.props file.
- <PackageReference Include="Unicode.net" Version="2.0.0" /> - <PackageReference Include="Wcwidth" Version="2.0.0" /> + <PackageReference Include="Unicode.net" Version="$(UnicodeNetVersion)" /> + <PackageReference Include="Wcwidth" Version="$(WcwidthVersion)" />src/Tests/Consolonia.Gallery.Tests/DataGridTests.cs (2)
15-15: Document the purpose of the initial Tab key input.Consider adding a comment explaining why this Tab key input is necessary (e.g., if it's for focus management).
- await UITest.KeyInput(Key.Tab); + // Set initial focus to the DataGrid + await UITest.KeyInput(Key.Tab);
18-18: Address the magic number TODO comment.The magic number for PageDown operations should be defined as a named constant with proper documentation.
+ // Number of PageDown operations needed to reach the bottom of the DataGrid + private const int PageDownToBottom = 50; + [Test] public async Task PerformSingleTest() { await UITest.KeyInput(Key.Tab); await UITest.AssertHasText("Co.+Region.+Population.+Area", @"Afg.+ASIA \(EX. NEAR"); await UITest.KeyInput(Key.Tab, Key.Tab); - await UITest.KeyInput(50 /*todo: Magic number of pagedown*/, Key.PageDown); + await UITest.KeyInput(PageDownToBottom, Key.PageDown);src/Tests/Consolonia.Gallery.Tests/MenuTests.cs (1)
15-15: Consider adding test documentation.The test covers important menu navigation scenarios. Consider adding XML documentation to describe:
- The purpose of the initial Tab navigation
- Expected menu state transitions
- Test prerequisites (if any)
[Test] +/// <summary> +/// Verifies menu navigation, focusing behavior, and state transitions: +/// 1. Initial focus navigation +/// 2. Menu item selection +/// 3. Submenu navigation +/// 4. Menu dismissal +/// </summary> public async Task PerformSingleTest()src/Tests/Consolonia.Core.Tests/PixelBackgroundTests.cs (1)
10-16: Consider adding edge cases to the Constructor test.While the basic case is covered, consider adding tests for:
- Transparent colors
- Colors with different alpha values
- Colors.Default or null scenarios
Example addition:
[Test] public void Constructor() { var pixelBackground = new PixelBackground(Colors.Red); Assert.That(pixelBackground.Color, Is.EqualTo(Colors.Red)); Assert.That(pixelBackground.Mode, Is.EqualTo(PixelBackgroundMode.Colored)); + + // Test transparent color + var transparentBackground = new PixelBackground(Colors.Transparent); + Assert.That(transparentBackground.Mode, Is.EqualTo(PixelBackgroundMode.None)); + + // Test semi-transparent color + var semiTransparent = Color.FromArgb(128, 255, 0, 0); + var semiBackground = new PixelBackground(semiTransparent); + Assert.That(semiBackground.Mode, Is.EqualTo(PixelBackgroundMode.Colored)); }src/Tests/Consolonia.Gallery.Tests/RadioButtonTests.cs (1)
Line range hint
15-29: Consider splitting into smaller, focused test cases.The current test method covers multiple behaviors (focus navigation, Space key selection, Enter key selection). Consider refactoring into separate test methods for better maintainability and clarity:
-[Test] -public async Task PerformSingleTest() +[Test] +public async Task InitialFocusNavigationSelectsFirstOption() +{ + await UITest.KeyInput(Key.Tab); + await UITest.AssertHasText(@"\(\*\).+Option 1", + @"\( \).+Option 2", + @"\(■\).+Option 3"); +} + +[Test] +public async Task SpaceKeySelectsOption() +{ + await UITest.KeyInput(Key.Tab, Key.Tab, Key.Space); + await UITest.AssertHasText(@"\( \).+Option 1", + @"\(\*\).+Option 2", + @"\(■\).+Option 3"); +} + +[Test] +public async Task EnterKeySelectsOption() +{ + await UITest.KeyInput(Key.Tab, Key.Tab, Key.Tab, Key.Enter); + await UITest.AssertHasText(@"\( \).+Option 1", + @"\( \).+Option 2", + @"\(\*\).+Option 3"); +}src/Tests/Consolonia.Gallery.Tests/CalendarTests.cs (1)
Line range hint
16-26: Consider improving test organization and documentation.While the test implementation is functional, consider these improvements:
- Use a more descriptive test name that indicates the scenario being tested
- Split multiple assertions into separate test methods for better isolation
- Add XML documentation to describe the test's purpose and expected behavior
Example refactor:
- [Test] - public async Task PerformSingleTest() + /// <summary> + /// Validates calendar navigation through different views (day, month, year) + /// using keyboard interactions. + /// </summary> + [Test] + public async Task Calendar_KeyboardNavigation_ShouldCycleViewsCorrectly()Consider splitting into focused test methods:
[Test] public async Task Calendar_DayView_ShouldDisplayCorrectWeekDays() [Test] public async Task Calendar_MonthView_ShouldDisplayAllMonths() [Test] public async Task Calendar_YearView_ShouldDisplayYearRange()src/Tests/Consolonia.Core.Tests/Consolonia.Core.Tests.csproj (1)
5-7: Consider adding additional test project properties.While setting
IsPackableto false is correct, consider adding these common test project properties for better configuration:<PropertyGroup> <IsPackable>false</IsPackable> + <IsTestProject>true</IsTestProject> + <NoWarn>$(NoWarn);CS1591</NoWarn> </PropertyGroup>
IsTestProject: Explicitly marks this as a test projectNoWarn: Suppresses XML documentation warnings in test projectssrc/Tests/Consolonia.Gallery.Tests/CalendarPickerTests.cs (1)
Line range hint
15-28: Consider improving test readability with helper methods.While the test is functional, it could benefit from some readability improvements:
- The sequence of key inputs could be grouped into meaningful helper methods
- The magic date values could be extracted into constants
- Comments could explain the purpose of each interaction sequence
Here's a suggested refactoring:
+ private static readonly DateTime InitialDate = new(2022, 2, 16); + private static readonly DateTime FinalDate = new(2026, 9, 15); + [Test] public async Task PerformSingleTest() { await UITest.KeyInput(Key.Tab); - await UITest.AssertHasText("2/16/2022"); - await UITest.KeyInput(Key.Right); - await UITest.KeyInput(9, Key.Left); - await UITest.KeyInput(Key.Delete); - await UITest.StringInput("1"); - await UITest.KeyInput(Key.Enter); + // Verify initial date + await AssertDate(InitialDate); + + // Navigate to next day and modify month + await NavigateAndModifyDate(); + + // Verify calendar view await UITest.AssertHasText("January", "2022", "16 17 18 19 20 21 22"); - await UITest.KeyInput(Key.Right); - await UITest.AssertHasText("1/17/2022"); - await UITest.KeyInput(2, Key.OemMinus); - await UITest.KeyInput(Key.Down, Key.Enter, Key.Down, Key.Down, Key.Enter, Key.Down, Key.Down, Key.Enter); - await UITest.AssertHasText("9/15/2026"); + + // Navigate to future date + await NavigateToFutureDate(); + await AssertDate(FinalDate); } + + private async Task AssertDate(DateTime date) => + await UITest.AssertHasText(date.ToString("M/d/yyyy")); + + private async Task NavigateAndModifyDate() + { + await UITest.KeyInput(Key.Right); + await UITest.KeyInput(9, Key.Left); + await UITest.KeyInput(Key.Delete); + await UITest.StringInput("1"); + await UITest.KeyInput(Key.Enter); + } + + private async Task NavigateToFutureDate() + { + await UITest.KeyInput(Key.Right); + await UITest.KeyInput(2, Key.OemMinus); + await UITest.KeyInput( + Key.Down, Key.Enter, + Key.Down, Key.Down, Key.Enter, + Key.Down, Key.Down, Key.Enter); + }src/Tests/Consolonia.Gallery.Tests/TextBlockTests.cs (1)
9-13: Consider enhancing the class documentation.While the current documentation indicates the purpose, it could be more descriptive by including:
- What aspects of TextBlock are being tested
- Any prerequisites or setup requirements
- Expected behavior patterns being verified
Example enhancement:
- /// <summary> - /// Unit test for TextBlock view - /// </summary> + /// <summary> + /// Unit tests for TextBlock view functionality, verifying: + /// - Text trimming behavior + /// - Text alignment variations + /// - Multiline text rendering + /// - Special character and emoji display + /// </summary>src/Tests/Consolonia.Gallery.Tests/ComboBoxTests.cs (2)
12-13: LGTM! Consider adding test documentation.The method signature change and [Test] attribute addition improve test accessibility and discoverability. The test comprehensively covers ComboBox interactions.
Consider adding XML documentation to describe:
- Test's purpose
- Key scenarios being tested
- Expected behaviors
+ /// <summary> + /// Validates ComboBox navigation, selection, and cancellation behaviors: + /// - Initial state verification + /// - Item selection and navigation + /// - Escape key cancellation + /// - Boundary conditions (navigating past last item) + /// </summary> [Test] public async Task PerformSingleTest()
13-13: Consider using a more descriptive test method name.The current name "PerformSingleTest" doesn't indicate what aspects of the ComboBox are being tested. Consider renaming to better reflect the test's purpose.
- public async Task PerformSingleTest() + public async Task ComboBox_NavigationAndSelection_WorksAsExpected()src/Tests/Consolonia.Gallery.Tests/CheckBoxTests.cs (1)
Line range hint
15-41: Consider enhancing test documentation and readability.While the test implementation is solid, consider these improvements:
- Add XML documentation to describe the test's purpose and expected behavior
- Consider using named constants for the repeated iteration count (6)
- Consider breaking down the assertions into smaller, more focused test methods
Here's a suggested improvement:
[Test] + /// <summary> + /// Verifies that checkbox states cycle correctly through unchecked, checked, + /// and indeterminate states when interacted with via keyboard input. + /// </summary> public async Task PerformSingleTest() { + const int STATE_CYCLE_COUNT = 6; await UITest.KeyInput(Key.Tab); await UITest.AssertHasText( "[ ].+Unchecked", "[V].+Checked", @"[■].+Indeterminate", "Disabled", "[ ].+Three State: Unchecked", "[V].+Three State: Checked", @"[■].+Three State: Indeterminate", @"[■].+Three State: Disabled" ); - for (int i = 0; i < 6; i++) await UITest.KeyInput(Key.Space, Key.Tab); + for (int i = 0; i < STATE_CYCLE_COUNT; i++) + await UITest.KeyInput(Key.Space, Key.Tab);src/Consolonia.Core/Drawing/PixelBufferImplementation/SimpleSymbol.cs (3)
10-15: Consider enhancing documentation for empty symbol behavior.While the inline comment explains the empty symbol concept, this behavior might be surprising to consumers of the ISymbol interface. Consider adding XML documentation to clarify that an empty symbol still occupies space with Width=1.
+ /// <summary> + /// Creates an empty symbol that occupies one character width but is invisible. + /// This is useful for maintaining spacing in layouts while having no visible content. + /// </summary> public SimpleSymbol()
34-36: Remove redundant default value from Text property.The default value of
string.Emptyon the Text property is redundant since it's always set in constructors.- public string Text { get; } = string.Empty; + public string Text { get; }
44-46: Consider removing unnecessary ref parameter.The
refparameter onsymbolAbovein the Blend method appears unnecessary as ISymbol is likely a readonly interface and the parameter is not modified.- public ISymbol Blend(ref ISymbol symbolAbove) + public ISymbol Blend(ISymbol symbolAbove)src/Consolonia.Core/Infrastructure/IConsole.cs (1)
19-22: Consider documenting emoji support requirements.Since this property affects text rendering behavior, it would be helpful to add more detailed XML documentation about:
- What constitutes a "complex emoji"
- Examples of supported/unsupported emoji sequences
- Any platform-specific considerations
Here's a suggested documentation enhancement:
/// <summary> /// This is true if console supports composing multiple emojis together (like: 👨👩👧👦). + /// Complex emojis are sequences that combine multiple Unicode code points, such as: + /// - Family emojis (e.g., 👨👩👧👦) + /// - Professional emojis with skin tones (e.g., 👨🏽💻) + /// - Flag emojis (e.g., 🏳️🌈) + /// </summary> + /// <remarks> + /// Support for complex emojis may vary by platform and terminal emulator. + /// When false, complex emoji sequences may be rendered as separate characters. + /// </remarks> bool SupportsComplexEmoji { get; }src/Consolonia.Core/Drawing/PixelBufferImplementation/PixelBackground.cs (1)
Line range hint
26-45: Consider adding XML documentation for the Shade method.The Shade method implements a state machine for transitioning between background modes. While the implementation is clear, adding XML documentation would help developers understand the expected transitions and use cases.
Here's a suggested documentation addition:
+ /// <summary> + /// Creates a new PixelBackground with modified shade state following the transition rules: + /// - Colored -> Colored (with shaded color) + /// - Transparent -> Shaded + /// - Shaded -> Colored (black) + /// </summary> + /// <returns>A new PixelBackground instance with the next shade state.</returns> + /// <exception cref="ArgumentOutOfRangeException">Thrown when Mode is not a valid PixelBackgroundMode value.</exception> public PixelBackground Shade()src/Consolonia.Core/Drawing/PixelBufferImplementation/PixelForeground.cs (1)
34-34: Consider additional validation in Blend method.The updates to
Shade()andBlend()methods correctly align with the new constructor signature. However, theBlend()method could benefit from additional validation.Consider adding validation for the incoming color:
public PixelForeground Blend(PixelForeground pixelAboveForeground) { //todo: check default(char) is there ISymbol symbolAbove = pixelAboveForeground.Symbol; ArgumentNullException.ThrowIfNull(symbolAbove); + if (pixelAboveForeground.Color == default) + throw new ArgumentException("Invalid color in pixelAboveForeground", nameof(pixelAboveForeground)); ISymbol newSymbol = Symbol.Blend(ref symbolAbove); return new PixelForeground(newSymbol, pixelAboveForeground.Color, pixelAboveForeground.Weight, pixelAboveForeground.Style, pixelAboveForeground.TextDecorations); }Also applies to: 45-46
src/Tests/Consolonia.Gallery.Tests/TextBoxTests.cs (1)
Line range hint
15-42: Consider breaking down this large UI test into smaller, focused test cases.While the implementation is functional, the test covers multiple scenarios (regular input, readonly state, disabled state, watermark behavior) in a single method. This could make it harder to diagnose failures and maintain the test.
Consider splitting this into multiple focused test methods:
-[Test] -public async Task PerformSingleTest() +[Test] +public async Task TextBox_ShouldHandleBasicInputAndNavigation() +{ + await UITest.KeyInput(Key.Tab); + await UITest.KeyInput(Key.Right); + await UITest.AssertHasText(@"elit\. "); + await UITest.KeyInput(Key.Right); + await UITest.StringInput("asd"); + // ... rest of basic input tests +} + +[Test] +public async Task TextBox_ShouldRespectReadOnlyState() +{ + await UITest.KeyInput(Key.Tab); + await UITest.StringInput("asdf"); + await UITest.AssertHasText("This is read only"); + await UITest.AssertHasText("ReadOnly watermark"); +} + +[Test] +public async Task TextBox_ShouldHandleWatermarkBehavior() +{ + await UITest.KeyInput(Key.Tab); + await UITest.KeyInput(Key.Tab); + await UITest.KeyInput(Key.Delete); + await UITest.AssertHasText("Floating Watermark"); + // ... rest of watermark tests +}src/Tests/Consolonia.Core.Tests/DrawingBoxSymbolTests.cs (2)
10-15: Enhance constructor test coverage.While the basic test is good, consider:
- Adding more test cases with different box drawing characters
- Testing edge cases and invalid inputs
- Renaming the test to be more descriptive (e.g.,
Constructor_WithValidBoxDrawingCode_SetsExpectedText)- [Test] - public void Constructor() + [TestCase(0b0000_1111, "┼", Description = "Cross")] + [TestCase(0b0000_0101, "│", Description = "Vertical line")] + [TestCase(0b0000_1010, "─", Description = "Horizontal line")] + [TestCase(0b0000_0000, " ", Description = "Empty space")] + public void Constructor_WithBoxDrawingCode_SetsExpectedText(byte code, string expected) { - ISymbol symbol = new DrawingBoxSymbol(0b0000_1111); - Assert.That(symbol.Text, Is.EqualTo("┼")); + ISymbol symbol = new DrawingBoxSymbol(code); + Assert.That(symbol.Text, Is.EqualTo(expected)); } + + [Test] + public void Constructor_WithInvalidCode_ThrowsException() + { + Assert.Throws<ArgumentException>(() => new DrawingBoxSymbol(0b1111_1111)); + }
17-26: Improve Blend test clarity and coverage.The test verifies basic blending functionality but could be enhanced:
- Add XML documentation explaining the blending behavior
- Use TestCase attribute for multiple blend scenarios
- Rename test to better describe the scenario (e.g.,
Blend_HorizontalAndVerticalLines_ProducesCross)- [Test] - public void Blend() + /// <summary> + /// Verifies that blending two box drawing symbols combines their characteristics + /// to produce the expected composite symbol. + /// </summary> + [TestCase(0b0000_0101, "─", 0b0000_1010, "│", "┼", Description = "Horizontal + Vertical = Cross")] + [TestCase(0b0000_0001, "╵", 0b0000_0100, "╷", "│", Description = "Bottom + Top = Vertical")] + public void Blend_TwoSymbols_ProducesExpectedComposite( + byte code1, string expected1, + byte code2, string expected2, + string expectedBlend) { - ISymbol symbol = new DrawingBoxSymbol(0b0000_0101); - Assert.That(symbol.Text, Is.EqualTo("─")); - ISymbol symbolAbove = new DrawingBoxSymbol(0b0000_1010); - Assert.That(symbolAbove.Text, Is.EqualTo("│")); - ISymbol newSymbol = symbol.Blend(ref symbolAbove); - Assert.That(newSymbol.Text, Is.EqualTo("┼")); + ISymbol symbol1 = new DrawingBoxSymbol(code1); + ISymbol symbol2 = new DrawingBoxSymbol(code2); + + Assert.That(symbol1.Text, Is.EqualTo(expected1), "First symbol should match expected"); + Assert.That(symbol2.Text, Is.EqualTo(expected2), "Second symbol should match expected"); + + ISymbol blended = symbol1.Blend(ref symbol2); + Assert.That(blended.Text, Is.EqualTo(expectedBlend), "Blended symbol should match expected"); }src/Tests/Consolonia.Core.Tests/GlyphTests.cs (1)
9-104: Great test coverage with room for enhancement.The test suite provides comprehensive coverage of various scenarios including empty strings, single characters, multiple characters, complex Unicode characters, emojis, and feature flag testing. The tests follow the Arrange-Act-Assert pattern consistently.
Consider these enhancements:
- Add negative test cases:
- Invalid Unicode sequences
- Null input handling
- Maximum string length scenarios
- Use parameterized tests for similar scenarios:
- Different Unicode ranges
- Various emoji combinations
- Consider more descriptive test names:
GetGlyphsWithComplexEmoji→GetGlyphsWithFamilyEmojiReturnsSingleGlyphGetGlyphsWithOutComplexEmojiSupport→GetGlyphsWithoutComplexEmojiSupportSplitsEmojiComponentsExample of a parameterized test:
[TestCase("", 0, Description = "Empty string")] [TestCase("a", 1, Description = "Single ASCII character")] [TestCase("👍", 1, Description = "Single emoji")] [TestCase("𝔉", 1, Description = "Single Unicode character")] public void GetGlyphs_ReturnsExpectedGlyphCount(string input, int expectedCount) { var glyphs = input.GetGlyphs(true); Assert.AreEqual(expectedCount, glyphs.Count); }src/Tests/Consolonia.Core.Tests/SimpleSymbolTests.cs (2)
12-51: Consider adding more edge cases to constructor tests.While the current tests cover the basic scenarios well, consider adding the following test cases:
- Null string input
- Empty string constructor
- Invalid Unicode sequences
- Multiple TestCases using [TestCase] attribute for similar tests
Example implementation:
[TestCase(null, "")] [TestCase("", "")] [TestCase("a", "a")] public void ConstructorString_Various(string input, string expected) { ISymbol symbol = new SimpleSymbol(input); Assert.That(symbol.Text, Is.EqualTo(expected)); } [Test] public void ConstructorString_InvalidUnicode_ShouldThrow() { Assert.Throws<ArgumentException>(() => new SimpleSymbol("\uD800")); // Unpaired surrogate }
61-99: Enhance Blend tests with additional scenarios.The current Blend tests are good but could be more comprehensive. Consider adding:
- Documentation comments explaining the expected behavior
- Tests for blending symbols of different widths
- Null input handling test
Example additions:
/// <summary> /// Tests that blending with null throws appropriate exception /// </summary> [Test] public void BlendWithNull() { ISymbol symbol = new SimpleSymbol("a"); ISymbol? symbolAbove = null; Assert.Throws<ArgumentNullException>(() => symbol.Blend(ref symbolAbove!)); } /// <summary> /// Tests blending symbols of different widths /// </summary> [Test] public void BlendDifferentWidths() { ISymbol symbol = new SimpleSymbol("a"); // width 1 ISymbol symbolAbove = new SimpleSymbol("👨👩👧👦"); // width 2 ISymbol newSymbol = symbol.Blend(ref symbolAbove); Assert.That(newSymbol.Width, Is.EqualTo(2)); }src/Consolonia.Themes.TurboVision/Templates/Controls/Helpers/SymbolsControl.cs (1)
Line range hint
1-116: Consider architectural improvements for better maintainability.The current implementation mixes text shaping with rendering concerns. Consider:
Extract text shaping logic into a separate service:
- Create
ITextShapingServicefor better separation of concerns- Inject the service to make the class more testable
- Cache shaped text results for better performance
Implement factory pattern for resource management:
- Create
GlyphRunFactoryto handleGlyphRuncreation and disposal- Centralize text shaping configuration
Example interface:
public interface ITextShapingService { GlyphRun ShapeText(string text, GlyphTypeface typeface, double fontSize); void Dispose(); }Would you like me to provide a detailed implementation of these architectural improvements?
src/Consolonia.Gallery/Gallery/GalleryViews/GalleryTextBlock.axaml (1)
34-40: Consider expanding internationalization coverage.The addition of CJK (Chinese, Japanese, Korean) text examples is excellent. However, consider adding examples for:
- Right-to-left (RTL) languages (Arabic, Hebrew)
- Complex script systems (Thai, Hindi)
- Bidirectional text mixing
This would ensure comprehensive testing of text rendering capabilities across different writing systems.
Here's a suggested addition:
<StackPanel Spacing="0"> <TextBlock Text="International chars" TextDecorations="Underline"/> <TextBlock Text="Chinese: 中国人 "/> <TextBlock Text="Japanese: 漢字学"/> <TextBlock Text="Korean: 한국인" /> + <TextBlock Text="Arabic: مرحبا بالعالم" /> + <TextBlock Text="Hebrew: שלום עולם" /> + <TextBlock Text="Thai: สวัสดีชาวโลก" /> + <TextBlock Text="Hindi: नमस्ते दुनिया" /> + <TextBlock Text="Mixed: Hello مرحبا שלום" /> </StackPanel>src/Consolonia.Core/Drawing/PixelBufferImplementation/PixelBuffer.cs (1)
73-76: Enhance XML documentation.The current documentation is minimal. Consider adding:
<exception>tags for potential exceptions- Parameter description including valid range
<returns>tag if you add a return value (recommended)src/Tests/Consolonia.Core.Tests/PixelTests.cs (2)
11-56: Consider adding edge cases and negative tests.The constructor tests are well-structured and cover the main scenarios comprehensively. Consider enhancing the test suite with:
Edge cases:
- Empty or whitespace symbols
- Special characters in symbols
- Null colors
Negative tests:
- Invalid drawing box patterns
- Null symbol instances
Related scenarios using TestCase attributes:
[TestCase("a", "Red", "Transparent")] [TestCase("", "Blue", "Green")] public void ConstructorWithVariousInputs(string symbol, Color foreground, Color background)
58-78: Enhance blending tests coverage.The blending tests cover basic scenarios well, but consider adding:
- Documentation comments explaining the expected blending behavior
- Tests for:
- Blending with null pixels
- Complex foreground properties (style, weight, decorations)
- Commutative property (A.Blend(B) vs B.Blend(A))
- Chained blending (A.Blend(B).Blend(C))
Example structure:
/// <summary> /// Verifies that blending preserves the commutative property: /// A.Blend(B) should equal B.Blend(A) /// </summary> [Test] public void BlendingIsCommutative() { Pixel a = new Pixel(new PixelBackground(Colors.Green)); Pixel b = new Pixel(new PixelForeground(new SimpleSymbol('a'), Colors.Red)); Assert.That(a.Blend(b), Is.EqualTo(b.Blend(a))); }src/Tests/Consolonia.Core.Tests/ColorTests.cs (3)
Line range hint
11-28: Consider adding edge case tests for basic operations.While the basic tests for
Shade()andBrighten()are good, consider adding test cases for edge colors (pure black/white) to ensure proper handling of boundary conditions. These could complement your existing boundary tests.Example addition:
+[Test] +public void ShadeWithBlack() +{ + Color foreground = Colors.Black; + Color newColor = foreground.Shade(); + Assert.That(newColor == Colors.Black); +}
Line range hint
31-65: Add null safety test for relative operations.The relative color operations are well tested for normal cases, but consider adding a test to verify behavior when background color is null or default.
Example addition:
+[Test] +public void RelativeShadeWithDefaultBackground() +{ + Color foreground = Colors.Gray; + Color newColor = foreground.Shade(default); + Assert.That(newColor, Is.Not.EqualTo(foreground)); +}
Line range hint
67-135: Optimize boundary test iterations.The boundary tests are thorough but could be optimized. The current implementation uses
byte.MaxValueiterations with early breaks, which might be excessive since color components are bytes (0-255).Consider:
- Reducing the maximum iterations to match color depth (255)
- Adding a maximum iteration guard to prevent infinite loops
- Using a more deterministic approach
Example optimization:
-for (int i = 0; i < byte.MaxValue; i++) +const int MaxIterations = 255; +for (int i = 0; i < MaxIterations; i++) { newColor = newColor.Shade(background); + // Guard against potential infinite loop + if (i == MaxIterations - 1) + Assert.Fail("Failed to reach target color within expected iterations"); Assert.That(newColor.R < foreground.R);src/Consolonia.Core/Helpers/Extensions.cs (1)
40-87: Consider refactoring for better maintainability.The method has high cyclomatic complexity and could benefit from:
- Extracting the emoji detection logic into a separate method
- Using constants for Unicode values
- Splitting the complex and simple emoji handling into separate methods
Here's a suggested refactor:
+ private const int ZWJ = 0x200D; // Zero Width Joiner + private const int ORC = 0xFFFC; // Object Replacement Character + + private static bool IsEmojiRelatedCharacter(Rune rune) => + rune.Value == ZWJ || + rune.Value == ORC || + rune.Value == Codepoints.VariationSelectors.EmojiSymbol || + rune.Value == Codepoints.VariationSelectors.TextSymbol; + public static IReadOnlyList<string> GetGlyphs(this string text, bool supportsComplexEmoji) { var glyphs = new List<string>(); + return supportsComplexEmoji + ? GetComplexEmojiGlyphs(text, glyphs) + : GetSimpleEmojiGlyphs(text, glyphs); + } + + private static IReadOnlyList<string> GetComplexEmojiGlyphs(string text, List<string> glyphs) + { var emoji = new StringBuilder(); StringRuneEnumerator runes = text.EnumerateRunes(); var lastRune = new Rune();src/Consolonia.Core/Drawing/PixelBufferImplementation/DrawingBoxSymbol.cs (3)
17-25: Consider adding XML documentation for public members.The implementation looks good with proper immutability through init-only properties. Consider adding XML documentation for the public
TextandWidthproperties to improve API documentation.+ /// <summary> + /// Gets the string representation of the box-drawing character. + /// </summary> public string Text { get; private init; } + /// <summary> + /// Gets the width of the box-drawing character, which is always 1. + /// </summary> public ushort Width { get; } = 1;
Line range hint
30-107: Consider improving switch expression readability.The implementation is correct, but the large switch expression could be more maintainable. Consider grouping similar cases and extracting helper methods.
private static char GetBoxSymbol(byte upRightDownLeft) { + if (IsSpecialSymbol(upRightDownLeft)) + return GetSpecialSymbol(upRightDownLeft); + + if (IsSimpleLine(upRightDownLeft)) + return GetSimpleLine(upRightDownLeft, hasLeftPart); + + return GetComplexBoxSymbol(upRightDownLeft); } + + private static bool IsSpecialSymbol(byte value) => + value is EmptySymbol or BoldSymbol; + + private static char GetSpecialSymbol(byte value) => + value switch + { + EmptySymbol => char.MinValue, + BoldSymbol => '█', + _ => throw new InvalidOperationException() + };
Line range hint
119-134: Consider extracting bit manipulation constants.The implementation is correct and aligns well with the immutable design. Consider extracting the bit shift value to a named constant for better readability.
public struct DrawingBoxSymbol : ISymbol { + private const int LeftPartShift = 4; // ... other constants ... public static DrawingBoxSymbol UpRightDownLeftFromPattern(byte pattern, LineStyle lineStyle) { // ... other cases ... case LineStyle.DoubleLine: - byte leftPart = (byte)(pattern << 4); + byte leftPart = (byte)(pattern << LeftPartShift); return new DrawingBoxSymbol((byte)(leftPart | pattern));src/Tests/Consolonia.Core.Tests/PixelForegroundTests.cs (4)
11-21: Consider adding edge cases to the basic constructor test.While the current test covers the happy path well, consider adding test cases for:
- Null symbol handling
- Empty character handling
- Invalid color values
Example test method:
[Test] public void ConstructorWithNullSymbol() { Assert.Throws<ArgumentNullException>(() => new PixelForeground(null, Colors.Red)); }
23-58: Consider using TestCase attribute for parameter variations.The constructor tests follow a similar pattern and could be optimized using NUnit's TestCase attribute to reduce code duplication.
Example refactor:
[TestCase(FontWeight.Bold, FontStyle.Normal, null, Description = "Weight only")] [TestCase(FontWeight.Normal, FontStyle.Italic, null, Description = "Style only")] [TestCase(FontWeight.Normal, FontStyle.Normal, TextDecorations.Underline, Description = "Decorations only")] public void ConstructorWithParameters(FontWeight weight, FontStyle style, TextDecorations? decorations) { var symbol = new SimpleSymbol('a'); var pixelForeground = new PixelForeground(symbol, Colors.Red, weight, style, decorations); Assert.That(pixelForeground.Color, Is.EqualTo(Colors.Red)); Assert.That(pixelForeground.Symbol.Text, Is.EqualTo("a")); Assert.That(pixelForeground.Weight, Is.EqualTo(weight)); Assert.That(pixelForeground.Style, Is.EqualTo(style)); Assert.That(pixelForeground.TextDecorations, Is.EqualTo(decorations)); }
60-71: Enhance wide character testing coverage.While the emoji test is good, consider expanding it to cover:
- Multiple emoji types (single character vs compound)
- Other wide Unicode characters (CJK characters)
- Verification of character width property
Example additional test:
[TestCase("🏳️🌈", "Complex emoji")] [TestCase("世界", "CJK characters")] [TestCase("👨👩👧👦", "Family emoji")] public void ConstructorWithVariousWideCharacters(string input, string description) { var symbol = new SimpleSymbol(input); var pixelForeground = new PixelForeground(symbol, Colors.Red); Assert.That(pixelForeground.Symbol.Text, Is.EqualTo(input)); // Add assertion for character width property once available }
73-110: Expand blend operation test coverage.The blend tests are good but consider adding:
- Null handling test case
- Same character blend test
- Different width character blend test
- Performance test for multiple consecutive blends
Also, consider using
[TestCase]to combine similar blend tests.Example additional tests:
[Test] public void BlendWithNull() { var symbol = new SimpleSymbol('a'); var pixelForeground = new PixelForeground(symbol, Colors.Red); Assert.Throws<ArgumentNullException>(() => pixelForeground.Blend(null)); } [Test] public void BlendSameCharacter() { var symbol = new SimpleSymbol('a'); var pixelForeground1 = new PixelForeground(symbol, Colors.Red); var pixelForeground2 = new PixelForeground(symbol, Colors.Blue); var result = pixelForeground1.Blend(pixelForeground2); Assert.That(result.Symbol.Text, Is.EqualTo("a")); Assert.That(result.Color, Is.EqualTo(Colors.Blue)); } [Test] public void BlendDifferentWidthCharacters() { var narrow = new SimpleSymbol('a'); var wide = new SimpleSymbol("世"); var pixelForeground1 = new PixelForeground(narrow, Colors.Red); var pixelForeground2 = new PixelForeground(wide, Colors.Blue); var result = pixelForeground1.Blend(pixelForeground2); Assert.That(result.Symbol.Text, Is.EqualTo("世")); }src/Consolonia.NUnit/UnitTestConsole.cs (1)
153-155: Address cursor drawing issue.While the change to use
Symbol.Textis good, there's an unresolved cursor drawing issue noted in the TODO comment. This should be investigated and fixed, especially since this is a test console implementation where cursor visibility might be important for testing UI interactions.Would you like me to help investigate the cursor drawing issue or create a GitHub issue to track this?
src/Consolonia.sln (1)
Line range hint
1-111: Solution restructuring follows good architectural practices.The reorganization of test projects demonstrates good architectural decisions:
- Clear separation between test support (Consolonia.NUnit) and actual tests
- Logical grouping of tests (Core.Tests and Gallery.Tests)
- Well-structured solution hierarchy with proper project organization
This structure will improve maintainability and make it easier to add new tests in the future.
src/Consolonia.Core/Infrastructure/InputLessDefaultNetConsole.cs (3)
17-17: Consider improving the emoji detection mechanism.The emoji detection logic is clever but could be enhanced:
- Consider moving the detection to a separate method for better testability
- Add fallback handling for environments where emoji testing fails
private const string TestEmoji = "👨👩👧👦"; +private bool DetectComplexEmojiSupport() +{ + try + { + Console.SetCursorPosition(0, 0); + Console.Write(TestEmoji); + (int left, _) = Console.GetCursorPosition(); + return left == 2; + } + catch (Exception) + { + return false; // Fallback to no complex emoji support + } + finally + { + Console.SetCursorPosition(0, 0); + Console.Write(" "); // Clean up test character + } +}Also applies to: 29-34
24-38: Document the console initialization sequence.The constructor performs several critical operations (encoding setup, emoji detection, screen clearing) that should be documented for maintainability.
protected InputLessDefaultNetConsole() { + // Configure console for Unicode support Console.OutputEncoding = Encoding.UTF8; + // Switch to alternate buffer to preserve original console state Console.Write(ConsoleUtils.EnableAlternateBuffer); // Detect complex emoji support by writing a complex emoji and checking cursor position. // If the cursor moves 2 positions, it indicates proper rendering of composite surrogate pairs. Console.SetCursorPosition(0, 0); Console.Write(TestEmoji); (int left, _) = Console.GetCursorPosition(); SupportsComplexEmoji = left == 2; + // Initialize console state Console.CursorVisible = false; Console.Clear(); ActualizeSize(); }
121-126: Consider enhancing the Print method's robustness.While the switch to
MeasureText()improves accuracy, consider these enhancements:
- Add validation for the measured text width
- Simplify the cursor position calculation
- ushort textWidth = str.MeasureText(); - if (_headBufferPoint.X < Size.Width - textWidth) - _headBufferPoint = - new PixelBufferCoordinate((ushort)(_headBufferPoint.X + textWidth), _headBufferPoint.Y); - else - _headBufferPoint = (PixelBufferCoordinate)((ushort)0, (ushort)(_headBufferPoint.Y + 1)); + ushort textWidth = str.MeasureText(); + if (textWidth == 0) return; // Early return for empty strings + + // Calculate new position, wrapping to next line if needed + ushort newX = (ushort)(_headBufferPoint.X + textWidth); + ushort newY = _headBufferPoint.Y; + + if (newX >= Size.Width) + { + newX = 0; + newY++; + } + + _headBufferPoint = new PixelBufferCoordinate(newX, newY);src/Consolonia.Core/Drawing/RenderTarget.cs (1)
209-212: Consider adding string length validation.While handling empty strings with spaces is good, there's no upper bound check on the string length. This could cause issues with very long strings or special characters.
Consider adding validation:
- if (pixel.Foreground.Symbol.Text.Length == 0) + var text = pixel.Foreground.Symbol.Text; + if (text.Length == 0) _stringBuilder.Append(' '); + else if (text.Length > maxLength) + _stringBuilder.Append(text.Substring(0, maxLength)); else - _stringBuilder.Append(pixel.Foreground.Symbol.Text); + _stringBuilder.Append(text);src/Consolonia.Core/Drawing/PixelBufferImplementation/Pixel.cs (1)
19-32: Add descriptions to XML documentation parameters.The constructor's XML comments lack descriptions for the parameters
symbol,foregroundColor,style,weight, andtextDecorations. Providing descriptions will enhance code readability and maintainability.src/Consolonia.Core/Drawing/DrawingContextImpl.cs (2)
309-313: Consider refactoring repeated code for moving console caret position.The condition checking for
pen.Brush is MoveConsoleCaretToPositionBrushand setting the caret position occurs in multiple methods (DrawLineInternal,DrawRectangleLineInternal). Consider extracting this logic into a separate method to reduce code duplication and improve maintainability.Also applies to: 375-379
491-491: Remove commented-out code to improve readability.The commented-out line
// if (!Transform.IsTranslateOnly()) ConsoloniaPlatform.RaiseNotSupported(15);can be removed if it's no longer needed to keep the code clean.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (58)
src/Consolonia.Core/Assembly.cs(1 hunks)src/Consolonia.Core/Consolonia.Core.csproj(1 hunks)src/Consolonia.Core/Drawing/ConsoleBrush.cs(1 hunks)src/Consolonia.Core/Drawing/DrawingContextImpl.cs(11 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/PixelBackground.cs(2 hunks)src/Consolonia.Core/Drawing/PixelBufferImplementation/PixelBuffer.cs(2 hunks)src/Consolonia.Core/Drawing/PixelBufferImplementation/PixelForeground.cs(3 hunks)src/Consolonia.Core/Drawing/PixelBufferImplementation/SimpleSymbol.cs(1 hunks)src/Consolonia.Core/Drawing/RenderTarget.cs(5 hunks)src/Consolonia.Core/Helpers/Extensions.cs(2 hunks)src/Consolonia.Core/Infrastructure/DefaultNetConsole.cs(0 hunks)src/Consolonia.Core/Infrastructure/IConsole.cs(1 hunks)src/Consolonia.Core/Infrastructure/InputLessDefaultNetConsole.cs(3 hunks)src/Consolonia.Core/Text/GlyphRunImpl.cs(1 hunks)src/Consolonia.Core/Text/GlyphTypeface.cs(2 hunks)src/Consolonia.Core/Text/TextShaper.cs(1 hunks)src/Consolonia.Gallery/Assembly.cs(1 hunks)src/Consolonia.Gallery/Gallery/GalleryViews/GalleryTextBlock.axaml(2 hunks)src/Consolonia.GuiCS/Assembly.cs(1 hunks)src/Consolonia.NUnit/Consolonia.NUnit.csproj(1 hunks)src/Consolonia.NUnit/ConsoloniaAppTestBase.cs(1 hunks)src/Consolonia.NUnit/TestHelpers.cs(1 hunks)src/Consolonia.NUnit/UnitTestConsole.cs(4 hunks)src/Consolonia.PlatformSupport/Assembly.cs(1 hunks)src/Consolonia.PlatformSupport/WindowsConsole.cs(0 hunks)src/Consolonia.Themes.TurboVision/Templates/Controls/Helpers/SymbolsControl.cs(2 hunks)src/Consolonia.sln(3 hunks)src/Tests/Consolonia.Core.Tests/Assembly.cs(1 hunks)src/Tests/Consolonia.Core.Tests/ColorTests.cs(6 hunks)src/Tests/Consolonia.Core.Tests/Consolonia.Core.Tests.csproj(1 hunks)src/Tests/Consolonia.Core.Tests/DrawingBoxSymbolTests.cs(1 hunks)src/Tests/Consolonia.Core.Tests/GlyphTests.cs(1 hunks)src/Tests/Consolonia.Core.Tests/PixelBackgroundTests.cs(1 hunks)src/Tests/Consolonia.Core.Tests/PixelForegroundTests.cs(1 hunks)src/Tests/Consolonia.Core.Tests/PixelTests.cs(1 hunks)src/Tests/Consolonia.Core.Tests/SimpleSymbolTests.cs(1 hunks)src/Tests/Consolonia.Gallery.Tests/Base/GalleryTestsBaseBase.cs(1 hunks)src/Tests/Consolonia.Gallery.Tests/ButtonTests.cs(1 hunks)src/Tests/Consolonia.Gallery.Tests/CalendarPickerTests.cs(1 hunks)src/Tests/Consolonia.Gallery.Tests/CalendarTests.cs(1 hunks)src/Tests/Consolonia.Gallery.Tests/CheckBoxTests.cs(1 hunks)src/Tests/Consolonia.Gallery.Tests/ComboBoxTests.cs(1 hunks)src/Tests/Consolonia.Gallery.Tests/Consolonia.Gallery.Tests.csproj(1 hunks)src/Tests/Consolonia.Gallery.Tests/DataGridTests.cs(1 hunks)src/Tests/Consolonia.Gallery.Tests/DialogTests.cs(1 hunks)src/Tests/Consolonia.Gallery.Tests/FlyoutTests.cs(1 hunks)src/Tests/Consolonia.Gallery.Tests/ListBoxTests.cs(1 hunks)src/Tests/Consolonia.Gallery.Tests/MenuTests.cs(1 hunks)src/Tests/Consolonia.Gallery.Tests/ProgressBarTests.cs(1 hunks)src/Tests/Consolonia.Gallery.Tests/RadioButtonTests.cs(1 hunks)src/Tests/Consolonia.Gallery.Tests/ScrollViewerTests.cs(1 hunks)src/Tests/Consolonia.Gallery.Tests/TabControlTests.cs(1 hunks)src/Tests/Consolonia.Gallery.Tests/TextBlockTests.cs(1 hunks)src/Tests/Consolonia.Gallery.Tests/TextBoxTests.cs(1 hunks)src/Tests/Consolonia.GalleryTests/TextBlockTests.cs(0 hunks)
💤 Files with no reviewable changes (3)
- src/Consolonia.Core/Infrastructure/DefaultNetConsole.cs
- src/Consolonia.PlatformSupport/WindowsConsole.cs
- src/Tests/Consolonia.GalleryTests/TextBlockTests.cs
✅ Files skipped from review due to trivial changes (4)
- src/Consolonia.Core/Drawing/ConsoleBrush.cs
- src/Consolonia.Gallery/Assembly.cs
- src/Consolonia.NUnit/ConsoloniaAppTestBase.cs
- src/Consolonia.NUnit/TestHelpers.cs
🔇 Additional comments (96)
src/Consolonia.Core/Assembly.cs (1)
4-4: LGTM! Verify consistency across assemblies.
The assembly attribute update aligns with the PR's test reorganization objectives.
Let's verify that this change is consistent across other related assemblies:
src/Consolonia.PlatformSupport/Assembly.cs (1)
4-4: LGTM! Verify coordination with other assemblies.
The assembly attribute change aligns with the test reorganization objectives.
Let's verify that all related assembly files have been updated consistently:
✅ Verification successful
The results show that there are no remaining references to the old assembly name "Consolonia.TestsCore" and the new assembly name "Consolonia.Core.Tests" is consistently used across the solution. Let me verify the actual content of the InternalsVisibleTo attributes to ensure they're properly configured.
InternalsVisibleTo attributes are properly coordinated across assemblies
The verification confirms that the InternalsVisibleTo attribute has been consistently updated to "Consolonia.Core.Tests" in all relevant assemblies (Core, GuiCS, and PlatformSupport). The change is properly coordinated and there are no remaining references to the old test assembly name.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining references to the old test assembly name
# and confirm the new assembly name is used consistently
echo "Checking for old assembly references..."
rg -l "Consolonia\.TestsCore"
echo "Verifying new assembly references..."
rg -l "Consolonia\.Core\.Tests"
Length of output: 768
Script:
#!/bin/bash
# Check the actual InternalsVisibleTo attribute declarations
rg -p "InternalsVisibleTo.*Consolonia\.Core\.Tests" src/*/Assembly.cs
Length of output: 559
src/Consolonia.GuiCS/Assembly.cs (1)
4-5: LGTM! Assembly attribute update aligns with test restructuring.
The update to InternalsVisibleTo attribute correctly reflects the new test library structure, allowing Consolonia.Core.Tests to access internal members. This change is consistent with the PR objectives of separating and reorganizing the test libraries.
src/Consolonia.NUnit/Consolonia.NUnit.csproj (3)
6-6: LGTM: Package activation aligns with library's new role.
Setting IsPackable to true is appropriate as this is now a reusable test support library that other projects can reference.
9-11: LGTM: Appropriate package references for a test support library.
The removal of Microsoft.NET.Test.Sdk and NUnit3TestAdapter is correct as these are test runner dependencies that should be included in the actual test projects, not in a test support library. Keeping only the NUnit reference is appropriate for this library's purpose.
14-14: Verify the project reference path.
The project reference path has been updated to use a relative path one level up. Let's verify this path is correct.
✅ Verification successful
Project reference path is correct and valid ✅
The referenced project Consolonia.Core.csproj exists at the exact relative path specified in the project reference (../Consolonia.Core/Consolonia.Core.csproj), and there are no other copies of this project file elsewhere in the codebase that might be more appropriate.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the Consolonia.Core project exists at the specified path
# Expected: The project file should exist at the relative path
# Test 1: Check if the project file exists at the new relative path
fd -p "src/Consolonia.Core/Consolonia.Core.csproj" --type f
# Test 2: Verify there are no other copies of the project file that might be more appropriate
fd "Consolonia.Core.csproj" --type f
Length of output: 182
src/Tests/Consolonia.Gallery.Tests/ScrollViewerTests.cs (2)
3-4: LGTM! Namespace changes align with the test reorganization.
The updated namespaces properly reflect the new test structure and follow standard naming conventions.
12-13: LGTM! Method signature changes improve test accessibility.
The addition of the [Test] attribute and public visibility properly integrates with NUnit's test discovery mechanism.
src/Tests/Consolonia.Gallery.Tests/ProgressBarTests.cs (1)
2-4: LGTM! Import changes align with the refactoring objectives.
The namespace updates correctly reflect the migration from GalleryTests to Gallery.Tests and the transition to the new Consolonia.NUnit library.
src/Tests/Consolonia.Gallery.Tests/ButtonTests.cs (3)
3-4: LGTM! Using statements are well-organized.
The addition of the Base namespace aligns with the PR's objective of improving test organization and maintainability.
12-13: LGTM! Method visibility and attribute changes improve test discoverability.
The changes align with the PR's objective of improving test organization and NUnit integration.
15-15: Verify the intentionality of adding Tab navigation.
The addition of Tab key input before Enter changes the test's behavior by first moving focus before clicking. Please confirm if this change is intentional and whether it should be applied consistently across other button tests.
✅ Verification successful
Let me gather more context about the test patterns to understand if this is a consistent testing approach.
Based on the search results, I can now generate the final response as we have sufficient context showing that Tab navigation is a consistent pattern across all UI tests in the codebase.
Tab navigation is a standard testing pattern in this codebase
The addition of Tab key input is consistent with the testing approach used throughout the codebase. Every UI test (ButtonTests, DialogTests, MenuTests, RadioButtonTests, etc.) starts with Tab navigation to focus the element before interaction, making this a deliberate and standardized testing pattern.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if other button tests follow the same pattern of Tab+Enter
# Expected: Consistent pattern across button tests
# Search for similar test patterns in other test files
rg -l "UITest\.KeyInput\(Key\.Enter\)" | xargs rg "UITest\.KeyInput\(Key\.Tab\)"
# Search for other button test classes
ast-grep --pattern 'class $_ButtonTests : $_'
Length of output: 898
Script:
#!/bin/bash
# Get more context around the Tab+Enter patterns in test files
rg -B 2 -A 2 "UITest\.KeyInput\(Key\.Tab\)" src/Tests/Consolonia.Gallery.Tests/
# Look specifically at ButtonTests class implementation
ast-grep --pattern 'class ButtonTests {
$$$
}'
Length of output: 9264
src/Tests/Consolonia.Gallery.Tests/ListBoxTests.cs (2)
3-4: LGTM! Import changes align with test framework restructuring.
The updated imports correctly reflect the new namespace structure mentioned in the PR objectives.
12-13: Verify the base class hierarchy after removing 'override'.
The removal of the override modifier suggests a change in the test hierarchy. Please ensure this doesn't break any existing test inheritance patterns.
✅ Verification successful
Removing 'override' modifier is safe - no base class implementation found
The verification shows that:
ListBoxTestsand many other test classes inherit directly fromGalleryTestsBaseBaseGalleryTestsBaseBaseis an abstract class but doesn't declare any virtual/abstractPerformSingleTestmethod- The
PerformSingleTestmethod inListBoxTestsis a standalone test method, not overriding any base implementation
Therefore, removing the override modifier and making the method public is a correct refactoring that doesn't break any inheritance patterns.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for other classes that might be affected by the hierarchy change
# Search for classes that inherit from GalleryTestsBaseBase and override PerformSingleTest
ast-grep --pattern 'class $_ : GalleryTestsBaseBase {
$$$
protected override $$ PerformSingleTest() {
$$$
}
$$$
}'
# Check if PerformSingleTest is defined in the base class
ast-grep --pattern 'class GalleryTestsBaseBase {
$$$
protected virtual $$ PerformSingleTest() {
$$$
}
$$$
}'
Length of output: 251
Script:
#!/bin/bash
# Let's try a broader search to understand the class hierarchy and method implementation
# Find any class that contains PerformSingleTest method
ast-grep --pattern 'class $_ {
$$$
$$ PerformSingleTest($$$) {
$$$
}
$$$
}'
# Find any base class references
rg -l "GalleryTestsBaseBase" --type cs
# Find class inheritance patterns
rg "class.*:.*GalleryTestsBase" --type cs
Length of output: 2817
Script:
#!/bin/bash
# Let's examine the base class and the specific test class in detail
# Check the base class implementation
rg -A 10 "class GalleryTestsBaseBase" src/Tests/Consolonia.Gallery.Tests/Base/GalleryTestsBaseBase.cs
# Check the ListBoxTests class implementation
rg -A 10 "class ListBoxTests" src/Tests/Consolonia.Gallery.Tests/ListBoxTests.cs
# Look for any virtual/abstract method declarations of PerformSingleTest
rg "virtual.*PerformSingleTest|abstract.*PerformSingleTest" --type cs
Length of output: 1041
src/Tests/Consolonia.Gallery.Tests/DialogTests.cs (3)
3-4: LGTM: Framework migration looks correct.
The change from Consolonia.TestsCore to Consolonia.NUnit aligns with the PR objectives of separating and reorganizing the test infrastructure.
12-13: Verify test discovery and execution.
The change from protected override to public with [Test] attribute makes this method directly executable by NUnit. However, since this class inherits from GalleryTestsBaseBase, we should verify that this change doesn't break any existing test inheritance patterns.
✅ Verification successful
Let me gather more information about the base class to understand the inheritance pattern better.
Let me check if there's any implementation of PerformSingleTest in the base class.
Based on the gathered information, I can now provide the final response:
The change from protected override to public with [Test] attribute is safe
The evidence shows that:
GalleryTestsBaseBaseis an internal abstract class that doesn't define or requirePerformSingleTest- All test classes inherit from
GalleryTestsBaseBaseand implement their ownpublicversion ofPerformSingleTest - The removal of
overridemodifier is correct since there's no base implementation to override
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for similar test methods in base classes and derived classes
# to ensure the visibility change doesn't break inheritance patterns
# Check the base class implementation
echo "Checking base class implementation:"
ast-grep --pattern 'class GalleryTestsBaseBase {
$$$
PerformSingleTest() {
$$$
}
$$$
}'
# Check for other derived classes
echo "Checking other derived classes:"
rg -l "GalleryTestsBaseBase" | xargs rg "PerformSingleTest"
Length of output: 1888
Script:
#!/bin/bash
# Check the base class content and inheritance hierarchy
echo "Checking GalleryTestsBaseBase class definition and content:"
rg -A 10 "class GalleryTestsBaseBase"
echo "Checking inheritance pattern in test classes:"
rg "class \w+Tests\s*:\s*\w+"
Length of output: 3106
Script:
#!/bin/bash
# Search for PerformSingleTest implementation in GalleryTestsBaseBase
echo "Checking PerformSingleTest in base class:"
rg -A 10 "PerformSingleTest" src/Tests/Consolonia.Gallery.Tests/Base/GalleryTestsBaseBase.cs
# Check if the method is abstract or virtual in base class
echo "Checking for abstract or virtual methods:"
rg "(abstract|virtual).*PerformSingleTest" src/Tests/Consolonia.Gallery.Tests/Base/GalleryTestsBaseBase.cs
Length of output: 387
Line range hint 15-20: Verify tab navigation behavior.
The addition of Tab key input suggests this test is verifying focus navigation. We should ensure:
- The initial tab navigation is necessary and intentional
- The dialog focus behavior is consistent across all dialog tests
✅ Verification successful
Let me gather more context about dialog-related tests and focus handling.
Let me check the dialog implementation to understand the expected focus behavior.
Let me check the dialog test implementation to understand the test's context better.
Let me check the dialog window implementation and its XAML to understand the UI structure.
Tab navigation behavior is correctly implemented and verified
The tab navigation in the test is intentional and correct because:
- The first Tab navigates to the "Show Dialog" button in GalleryDialog
- Enter opens the dialog window
- The second Tab is unnecessary as the dialog window already sets focus to the "One More" button in its Loaded event (confirmed in SomeDialogWindow.cs)
- The test correctly verifies the dialog's presence and dismissal
While the second Tab keystroke is redundant due to automatic focus handling, it doesn't affect the test's validity since the button is already focused. The dialog's focus behavior is consistently implemented across the codebase through the DialogWindow base class.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check other dialog-related tests for consistent focus handling
# Look for other dialog test classes
echo "Checking dialog-related tests for focus handling:"
rg -B 2 -A 2 "KeyInput\(Key\.Tab\)" --glob "**/*Dialog*.cs"
Length of output: 809
Script:
#!/bin/bash
# Search for dialog test files and their content to understand focus behavior
echo "Dialog test files:"
fd "Dialog.*\.cs$" --type f
echo -e "\nChecking for dialog-related assertions and key inputs:"
rg -B 3 -A 3 "UITest\.(Assert|KeyInput)" --glob "**/*Dialog*.cs"
Length of output: 1657
Script:
#!/bin/bash
# Check dialog implementation for focus handling
echo "Dialog implementation focus handling:"
rg -B 3 -A 3 "Focus|TabIndex|NavigationDirection" --glob "src/Consolonia.Themes.TurboVision/Templates/Controls/Dialog/*.cs"
# Check dialog XAML for focus-related attributes
echo -e "\nDialog XAML focus attributes:"
rg -B 2 -A 2 "TabIndex|KeyboardNavigation" --glob "**/*Dialog*.axaml"
Length of output: 4565
Script:
#!/bin/bash
# Check the complete dialog test implementation
echo "Complete DialogTests implementation:"
cat "src/Tests/Consolonia.Gallery.Tests/DialogTests.cs"
# Check the dialog view implementation
echo -e "\nDialog view implementation:"
cat "src/Consolonia.Gallery/Gallery/GalleryViews/GalleryDialog.axaml.cs"
Length of output: 1573
Script:
#!/bin/bash
# Check the dialog window implementation and XAML
echo "Dialog window implementation:"
cat "src/Consolonia.Gallery/Gallery/GalleryViews/SomeDialogWindow.axaml.cs"
echo -e "\nDialog window XAML:"
cat "src/Consolonia.Gallery/Gallery/GalleryViews/SomeDialogWindow.axaml"
echo -e "\nGallery dialog XAML:"
cat "src/Consolonia.Gallery/Gallery/GalleryViews/GalleryDialog.axaml"
Length of output: 4460
src/Tests/Consolonia.Core.Tests/Assembly.cs (1)
10-23: Document test setup requirements and organization.
Given that this is part of the new Consolonia.Core.Tests project mentioned in the PR objectives, it would be helpful to:
- Add XML documentation comments explaining the purpose of this setup class and its role in the test infrastructure.
- Consider adding a README.md in the test project root documenting the test organization and setup requirements.
Let's check for existing test documentation:
src/Tests/Consolonia.Gallery.Tests/Consolonia.Gallery.Tests.csproj (2)
16-16: LGTM! Project reference update aligns with test reorganization.
The change from TestsCore to Consolonia.NUnit aligns well with the PR objective of separating the test support package.
Line range hint 1-20: Verify test project configuration consistency.
Let's ensure all test projects follow the same configuration pattern and use consistent package versions.
✅ Verification successful
Based on the output, I can generate the final response as we have sufficient information about the test project configurations.
Test project configurations are consistent across the codebase
All test projects (Consolonia.Gallery.Tests and Consolonia.Core.Tests) follow the same configuration pattern:
- Use identical NUnit package versions (NUnit 3.13.1, NUnit3TestAdapter 3.17.0)
- Reference the Consolonia.NUnit project consistently
- Include standard test infrastructure packages (Microsoft.NET.Test.Sdk, coverlet.collector)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistency across test projects
# Expected results:
# 1. All test projects should reference Consolonia.NUnit
# 2. NUnit package versions should be consistent
# Find all test project files
echo "=== Test Projects ==="
fd -e csproj -e fsproj --exec grep -l "Test" {}
# Check NUnit package versions across all test projects
echo -e "\n=== NUnit Package Versions ==="
fd -e csproj -e fsproj --exec grep -A 1 "NUnit" {}
# Check Consolonia.NUnit references
echo -e "\n=== Consolonia.NUnit References ==="
fd -e csproj -e fsproj --exec grep -l "Consolonia.NUnit.csproj" {}
Length of output: 1451
src/Tests/Consolonia.Gallery.Tests/TabControlTests.cs (2)
3-4: LGTM! Import changes align with test library reorganization.
The namespace updates correctly reflect the new test library structure mentioned in the PR objectives.
12-13: Verify base class implementation after signature change.
The removal of override modifier suggests changes in the inheritance hierarchy. Let's verify the base class implementation.
✅ Verification successful
Base class change is valid and consistent with the codebase
The base class GalleryTestsBaseBase doesn't define or require the PerformSingleTest method. All test classes inherit from it and implement their own test methods without overriding. The removal of the override modifier and making the method public aligns with the NUnit test pattern seen across the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if there are any other classes that might be affected by
# the removal of the override modifier
# Search for base class implementation
ast-grep --pattern 'class GalleryTestsBaseBase {
$$$
protected $_ PerformSingleTest() {
$$$
}
$$$
}'
# Search for other derived classes that might need similar updates
ast-grep --pattern 'class $_ : GalleryTestsBaseBase {
$$$
protected override $_ PerformSingleTest() {
$$$
}
$$$
}'
Length of output: 243
Script:
#!/bin/bash
# Let's try a broader search to understand the inheritance hierarchy and method implementation
# Search for any class containing PerformSingleTest method
ast-grep --pattern 'class $_ {
$$$
$_ PerformSingleTest($$$) {
$$$
}
$$$
}'
# Search for any class inheriting from another class that has PerformSingleTest
rg -A 2 "class.*:.*" | rg -B 2 "PerformSingleTest"
# Search for base class references
rg "GalleryTestsBaseBase" -A 2
Length of output: 4532
Script:
#!/bin/bash
# Let's check the base class implementation and other test implementations
# Check the base class (GalleryTestsBaseBase) content
cat src/Tests/Consolonia.Gallery.Tests/Base/GalleryTestsBaseBase.cs
# Check a few test implementations to see the pattern
cat src/Tests/Consolonia.Gallery.Tests/ButtonTests.cs
cat src/Tests/Consolonia.Gallery.Tests/TabControlTests.cs
Length of output: 2631
src/Consolonia.Core/Drawing/PixelBufferImplementation/ISymbol.cs (2)
5-14: LGTM! Well-documented properties that improve Unicode support.
The new Text and Width properties are well-designed and documented, providing better support for both single characters and Unicode encodings like emojis.
5-14: Verify impact of removing GetCharacter() method.
The removal of GetCharacter() method is a breaking change. Let's verify all implementations have been updated.
✅ Verification successful
Let me try a different search pattern to find ISymbol implementations and any potential GetCharacter references.
No impact found from GetCharacter() removal
Based on the comprehensive search results, there are no remaining references to the GetCharacter() method in the codebase. The interface ISymbol is implemented by SimpleSymbol and DrawingBoxSymbol, and both implementations are already aligned with the current interface definition that only includes Text and Width properties. The removal of GetCharacter() method appears to be a clean change with no remaining dependencies.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for any remaining usages of GetCharacter() that need updating
# Test 1: Look for any remaining GetCharacter() calls
echo "Searching for GetCharacter() calls that need updating..."
rg "GetCharacter\(\)" --type cs
# Test 2: Find all implementations of ISymbol to verify they've been updated
echo "Finding ISymbol implementations..."
ast-grep --pattern 'class $_ : $$$ISymbol$$$'
Length of output: 283
Script:
#!/bin/bash
# Search for ISymbol implementations with broader pattern and context
echo "Finding ISymbol implementations..."
rg "ISymbol" -A 5 --type cs
# Search for files containing both ISymbol and GetCharacter
echo "Searching for files with both ISymbol and GetCharacter..."
rg "GetCharacter" --type cs -l | xargs -I {} rg "ISymbol" {} --with-filename || true
Length of output: 14921
src/Consolonia.Core/Consolonia.Core.csproj (1)
10-11: Verify the replacement of NullLib.ConsoleEx functionality.
The replacement of NullLib.ConsoleEx with Unicode.net and Wcwidth suggests a shift to more specialized Unicode handling. Please ensure that all necessary console functionality previously provided by NullLib.ConsoleEx is covered by these new packages.
✅ Verification successful
The replacement of NullLib.ConsoleEx is properly implemented with Unicode.net and Wcwidth
The verification shows that the new packages are effectively integrated:
- Wcwidth is used for accurate character width calculations via
UnicodeCalculator.GetWidth() - Unicode.net (NeoSmart.Unicode) provides proper Unicode handling with
Emoji,Codepoints, and other Unicode-specific functionality - The implementation properly handles complex cases like emoji combinations, zero-width joiners, and variation selectors
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for any remaining usages of NullLib.ConsoleEx functionality that might need migration
# Test: Look for any remaining references to NullLib types or namespaces
rg "NullLib\.ConsoleEx"
# Test: Look for potential console-related functionality that might need review
ast-grep --pattern 'using NullLib.$$$'
Length of output: 66
Script:
#!/bin/bash
# Let's try a broader search to understand the console-related functionality
# Search for console-related types and methods
rg -l "Console|Terminal" --type cs
# Look for any Unicode or width calculation related code
rg -l "Unicode|Width" --type cs
# Check git history for the specific commit that removed NullLib.ConsoleEx
git log -p -- "**/**.csproj" | rg -A 5 -B 5 "NullLib\.ConsoleEx"
Length of output: 7358
Script:
#!/bin/bash
# Let's examine the specific text/Unicode handling implementations
# Look for Unicode/width calculation implementations in Core
rg -A 10 "Unicode|Width" "src/Consolonia.Core/Text/" "src/Consolonia.Core/Drawing/"
# Check for any console text rendering related code that might use these new packages
rg -A 10 "using (Unicode|Wcwidth)" --type cs
# Look specifically at text shaping and console utils implementations
cat "src/Consolonia.Core/Text/TextShaper.cs" "src/Consolonia.Core/Text/ConsoleUtils.cs" 2>/dev/null
Length of output: 32393
Script:
#!/bin/bash
# Let's check the text measurement implementation that uses the new packages
# Look for text measurement implementations
rg -A 10 "MeasureText" src/Consolonia.Core/
# Check Extensions.cs which imports Wcwidth
cat src/Consolonia.Core/Helpers/Extensions.cs
Length of output: 8817
src/Tests/Consolonia.Gallery.Tests/DataGridTests.cs (2)
3-4: LGTM: Import changes align with test restructuring.
The updated imports correctly reflect the new namespace structure and the migration to the Consolonia.NUnit testing framework.
12-13: Verify base class implementation after signature changes.
The method signature change from protected override to public looks good for NUnit integration, but let's verify the base class implementation.
src/Tests/Consolonia.Gallery.Tests/MenuTests.cs (2)
3-4: LGTM! Namespace changes align with the test restructuring.
The updated namespaces follow standard .NET conventions and properly reflect the new test library organization.
12-13: Verify the impact of removing override modifier.
The change from protected override to public suggests this method is no longer part of a test template pattern. Please verify that:
- The base class
GalleryTestsBaseBaseno longer requires this method to be overridden - Other derived classes have been updated similarly
✅ Verification successful
Removal of override modifier is correct
Based on the evidence:
- The base class
GalleryTestsBaseBasedoes not contain any virtual/abstractPerformSingleTestmethod - All test classes (ButtonTests, MenuTests, etc.) have consistently changed to use the same non-override public method pattern with
[Test]attribute - This appears to be a coordinated refactoring to move away from the template pattern to independent test methods
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if base class still has the abstract/virtual method
ast-grep --pattern 'class GalleryTestsBaseBase {
$$$
$_ PerformSingleTest() {
$$$
}
$$$
}'
# Check for any remaining override implementations
ast-grep --pattern 'class $_ {
$$$
override $_ PerformSingleTest() {
$$$
}
$$$
}'
Length of output: 200
Script:
#!/bin/bash
# Let's try a broader search for the base class and any related inheritance
rg -l "GalleryTestsBaseBase" .
# Search for any class that contains PerformSingleTest
rg "PerformSingleTest" -A 2 -B 2
# Search for files containing both GalleryTestsBaseBase and PerformSingleTest
rg -l "GalleryTestsBaseBase.*PerformSingleTest|PerformSingleTest.*GalleryTestsBaseBase" .
Length of output: 7268
Script:
#!/bin/bash
# Let's check the content of the base class to see if it previously had the virtual/abstract method
cat "./src/Tests/Consolonia.Gallery.Tests/Base/GalleryTestsBaseBase.cs"
Length of output: 1183
src/Tests/Consolonia.Core.Tests/PixelBackgroundTests.cs (2)
1-6: LGTM! Clean and minimal imports.
The namespace hierarchy correctly follows the new project structure mentioned in the PR objectives.
7-9: LGTM! Proper test class setup.
The test class follows NUnit conventions with appropriate attributes.
src/Tests/Consolonia.Gallery.Tests/RadioButtonTests.cs (3)
3-4: LGTM: Namespace and imports are properly organized.
The namespace and import changes align with the PR's objective of separating and reorganizing the test structure.
12-13: LGTM: Test method properly decorated and accessible.
The change from protected override to public with [Test] attribute improves test discoverability and follows NUnit best practices.
Line range hint 24-24: Investigate Space key behavior issue.
The TODO comment indicates a potential bug where the Space key doesn't work as expected. This should be investigated as it might affect the user experience.
Would you like me to create an issue to track this investigation?
src/Tests/Consolonia.Gallery.Tests/CalendarTests.cs (2)
1-8: LGTM! Import and namespace changes align with PR objectives.
The updates to imports and namespace properly reflect the test library separation and follow standard .NET naming conventions.
13-14: LGTM! Test method declaration follows NUnit conventions.
The change to make the test method public with [Test] attribute properly aligns with NUnit testing patterns and allows direct test execution.
src/Tests/Consolonia.Core.Tests/Consolonia.Core.Tests.csproj (1)
1-3: LGTM! Project SDK and build properties import look good.
The project setup correctly uses Microsoft.NET.Sdk and properly imports shared build properties using MSBuild path resolution.
src/Tests/Consolonia.Gallery.Tests/FlyoutTests.cs (3)
3-4: LGTM: Import changes align with test reorganization.
The updated imports correctly reflect the new test structure, with the separation of test support into the Consolonia.NUnit package.
12-13: LGTM: Method signature changes improve test accessibility.
The changes to make the test method public and add the [Test] attribute improve test discovery and align with standard NUnit practices.
15-15: Verify the impact of the new Tab key input.
The addition of an initial Tab key input likely improves test reliability by ensuring proper focus state. However, we should verify this doesn't affect other Flyout-related tests.
✅ Verification successful
Let me gather more specific information about the test implementations.
Let me check how Tab navigation is used in other test files to ensure consistency.
Initial Tab navigation is consistent with other UI tests
The addition of the Tab key input in the Flyout test follows the established pattern across the codebase. Other UI tests (TextBox, RadioButton, Button) consistently use an initial Tab key input to set focus on the first interactive element before performing their main test actions. This change aligns with the existing test practices and improves test reliability.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other Flyout-related tests to verify consistent behavior
rg -l "UITest.KeyInput.*Tab.*Enter" --type cs
# Look for any existing Flyout-related tests
ast-grep --pattern 'class $_Tests : GalleryTestsBaseBase {
$$$
$_ Flyout$_() {
$$$
}
}'
Length of output: 197
Script:
#!/bin/bash
# Get the content of FlyoutTests.cs to analyze all test methods
cat src/Tests/Consolonia.Gallery.Tests/FlyoutTests.cs
# Search for any other test files that might be affected by Tab navigation
rg -l "UITest.KeyInput.*Tab" --type cs
Length of output: 1996
Script:
#!/bin/bash
# Look at a few key test files to understand their Tab navigation patterns
echo "=== TextBoxTests.cs ==="
rg "UITest.KeyInput.*Tab" src/Tests/Consolonia.Gallery.Tests/TextBoxTests.cs -B 1 -A 1
echo "=== RadioButtonTests.cs ==="
rg "UITest.KeyInput.*Tab" src/Tests/Consolonia.Gallery.Tests/RadioButtonTests.cs -B 1 -A 1
echo "=== ButtonTests.cs ==="
rg "UITest.KeyInput.*Tab" src/Tests/Consolonia.Gallery.Tests/ButtonTests.cs -B 1 -A 1
Length of output: 1269
src/Tests/Consolonia.Gallery.Tests/CalendarPickerTests.cs (2)
1-8: LGTM: Import and namespace changes align with refactoring objectives.
The namespace change from GalleryTests to Gallery.Tests and the switch to the new Consolonia.NUnit package are consistent with the PR's goal of better test organization.
12-14: Verify the base class implementation after removing 'override'.
The removal of the override modifier suggests a change in the test hierarchy. Let's verify that this doesn't break any test inheritance patterns.
✅ Verification successful
Removal of 'override' modifier is correct
The base class GalleryTestsBaseBase does not define a PerformSingleTest method, and looking at other test classes, they all implement this method as a non-override public method. The removal of the override modifier aligns with the rest of the test suite's pattern where PerformSingleTest is a standalone test method in each test class, not an override of a base class method.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the base class hierarchy and other implementations of PerformSingleTest
# Check the base class implementation
ast-grep --pattern 'class GalleryTestsBaseBase {
$$$
PerformSingleTest() {
$$$
}
$$$
}'
# Check for other classes that might be affected by this change
ast-grep --pattern 'class $_ : GalleryTestsBaseBase {
$$$
PerformSingleTest() {
$$$
}
$$$
}'
Length of output: 208
Script:
#!/bin/bash
# Let's try a broader search to understand the class hierarchy and test patterns
# Find all files containing CalendarPickerTests
fd "CalendarPickerTests" --type f
# Search for base class and inheritance patterns
rg -l "class.*GalleryTestsBase"
# Look for any PerformSingleTest method definitions
rg "PerformSingleTest" -A 2
# Check the content of the test file
fd "CalendarPickerTests.cs" --type f --exec cat {}
Length of output: 6352
Script:
#!/bin/bash
# Let's check the base class implementation to understand if PerformSingleTest was originally virtual/abstract
cat src/Tests/Consolonia.Gallery.Tests/Base/GalleryTestsBaseBase.cs
Length of output: 1181
src/Tests/Consolonia.Gallery.Tests/Base/GalleryTestsBaseBase.cs (2)
7-7: LGTM! Clean namespace reorganization.
The namespace change and new dependency on Consolonia.NUnit align well with the PR objectives of separating test concerns and improving test organization.
Also applies to: 10-10
Line range hint 13-38: Verify the impact of removed test methods.
The removal of SingleTest and PerformSingleTest() methods from this base class suggests these responsibilities have been moved elsewhere. Let's verify that all derived test classes have been properly updated to handle this change.
✅ Verification successful
Let me check the actual content of these files to see how they're using these methods.
Let me check if these are new test method implementations rather than references to the base class methods.
Based on the evidence gathered, I can now provide a final response:
The removal of base methods appears safe as derived classes implement their own versions
The investigation shows that while multiple test files contain references to PerformSingleTest, these are actually independent test method implementations in each derived class, not calls to the removed base methods. This is evidenced by:
- No calls to
base.SingleTestorbase.PerformSingleTestwere found - The sample implementations we saw in CalendarPickerTests.cs and ButtonTests.cs show their own
PerformSingleTestmethods with specific implementations - Each test class appears to handle its own test logic independently
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining references to the removed methods
# and verify that derived classes have been updated accordingly
# Search for any remaining references to SingleTest or PerformSingleTest
echo "Checking for any lingering references to removed methods..."
rg -l "SingleTest|PerformSingleTest" --type cs
# Find all test classes that inherit from GalleryTestsBaseBase
echo "Finding all derived test classes..."
ast-grep --pattern 'class $_ : GalleryTestsBaseBase'
Length of output: 1152
Script:
#!/bin/bash
# Let's examine one of the files that contains these references to understand the context
echo "Checking CalendarPickerTests.cs for context..."
rg "SingleTest|PerformSingleTest" src/Tests/Consolonia.Gallery.Tests/CalendarPickerTests.cs -B 2 -A 2
echo -e "\nChecking ButtonTests.cs for additional context..."
rg "SingleTest|PerformSingleTest" src/Tests/Consolonia.Gallery.Tests/ButtonTests.cs -B 2 -A 2
# Let's also check if there's a new base test class that might have inherited these methods
echo -e "\nChecking for potential new base test classes..."
ast-grep --pattern 'class $_ : $_TestsBase'
Length of output: 816
Script:
#!/bin/bash
# Let's check the inheritance hierarchy and method implementations
echo "Checking test class definitions..."
ast-grep --pattern 'class $_ : GalleryTestsBaseBase {
$$$
public async Task PerformSingleTest() {
$$$
}
$$$
}'
# Let's also verify if there are any direct calls to base.PerformSingleTest()
echo -e "\nChecking for base method calls..."
rg "base\.(SingleTest|PerformSingleTest)" --type cs
Length of output: 340
src/Tests/Consolonia.Gallery.Tests/TextBlockTests.cs (1)
1-6: LGTM! Clean and focused imports.
The imports are minimal and properly organized, containing only the necessary dependencies for the test implementation.
src/Consolonia.Core/Text/GlyphRunImpl.cs (2)
21-21: LGTM! Good choice of IReadOnlyList for the property type.
The change from ushort[] to IReadOnlyList<GlyphInfo> is a good improvement:
- Provides immutability guarantees
- Encapsulates more comprehensive glyph information
- Follows modern C# API design practices
16-16: Verify consistent GlyphInfo usage across the codebase.
Let's ensure the GlyphInfo type is consistently used across related components:
✅ Verification successful
Let me gather more information about the GlyphInfo type definition and its relationship with GlyphIndices.
Let me search for the GlyphInfo interface or record definition since we haven't found its class or struct definition yet.
Let me try one final search to find the GlyphInfo definition by looking for its usage patterns and imports.
Based on the code analysis, I can now generate the final response:
GlyphInfo usage is consistent across the codebase
The GlyphInfo property in GlyphRunImpl is correctly used as part of the text rendering pipeline:
- Used by TextShaper to create shaped text buffers
- Consumed by DrawingContextImpl for text rendering
- Properly integrated with the ConsoloniaRenderInterface
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for GlyphInfo usage patterns
echo "Checking GlyphInfo usage patterns:"
rg "GlyphInfo" -A 3
echo -e "\nChecking for any remaining ushort[] GlyphIndices:"
rg "GlyphIndices.*ushort\[\]"
Length of output: 2348
Script:
#!/bin/bash
# Search for GlyphInfo type definition
echo "Checking GlyphInfo type definition:"
ast-grep --pattern 'class GlyphInfo'
echo -e "\nChecking for struct GlyphInfo:"
ast-grep --pattern 'struct GlyphInfo'
echo -e "\nChecking for any references to GlyphIndices in the codebase:"
rg "GlyphIndices" -A 3
Length of output: 20679
Script:
#!/bin/bash
# Search for GlyphInfo interface or record
echo "Checking for GlyphInfo interface definition:"
ast-grep --pattern 'interface GlyphInfo'
echo -e "\nChecking for GlyphInfo record definition:"
ast-grep --pattern 'record GlyphInfo'
echo -e "\nChecking all files containing GlyphInfo:"
fd GlyphInfo
Length of output: 582
Script:
#!/bin/bash
# Search for files importing or using GlyphInfo
echo "Checking for GlyphInfo imports and usages:"
rg -l "GlyphInfo" | xargs cat
echo -e "\nChecking for files in Text directory:"
fd . src/Consolonia.Core/Text/
Length of output: 40391
src/Tests/Consolonia.Gallery.Tests/ComboBoxTests.cs (1)
3-4: LGTM! Namespace changes align with PR objectives.
The namespace updates and the switch to Consolonia.NUnit are consistent with the PR's goals of restructuring the test organization and adopting the new test library.
Also applies to: 7-7
src/Tests/Consolonia.Gallery.Tests/CheckBoxTests.cs (2)
3-4: LGTM: Namespace updates align with project restructuring.
The updated namespaces correctly reflect the new test organization structure mentioned in the PR objectives, where test packages have been separated and renamed.
12-13: LGTM: Method signature improvements enhance test discoverability.
The changes to make the test method public and add the [Test] attribute improve test discoverability and align with NUnit best practices. This is consistent with the PR's goal of improving test organization and navigation.
src/Consolonia.Core/Drawing/PixelBufferImplementation/SimpleSymbol.cs (2)
1-7: Great addition of DebuggerDisplay attribute!
The DebuggerDisplay attribute will improve debugging experience by showing the Text content directly in the debugger.
17-20: LGTM! Good use of constructor chaining.
The delegation to the string constructor reduces code duplication and maintains consistency.
src/Consolonia.Core/Infrastructure/IConsole.cs (1)
19-22: Property addition looks good, but verify implementations.
The new SupportsComplexEmoji property is well-documented and serves a clear purpose for handling complex emoji sequences.
Let's verify that all implementations of IConsole have been updated:
Additionally, since this is a breaking change:
- Ensure all implementations provide the new property
- Consider adding unit tests specifically for this property
Would you like me to help generate unit tests for verifying the emoji support functionality?
✅ Verification successful
Property addition is properly implemented across the codebase
The verification shows that all implementations of IConsole (InputLessDefaultNetConsole and UnitTestConsole) have correctly implemented the SupportsComplexEmoji property. The property is also being used consistently in several places:
- Text shaping in
TextShaper.cs - Drawing context in
DrawingContextImpl.cs - Extensions in
Helpers/Extensions.cs
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all implementations of IConsole interface
# and verify they implement the new property
# Find all implementations of IConsole
ast-grep --pattern 'class $_ : $$$IConsole$$$'
# Find direct references to SupportsComplexEmoji
rg "SupportsComplexEmoji"
Length of output: 848
src/Consolonia.Core/Drawing/PixelBufferImplementation/PixelBackground.cs (3)
7-7: LGTM! Good debugging enhancement.
The addition of the DebuggerDisplay attribute improves the debugging experience by showing the essential state (Color and Mode) in a concise format, which will be particularly helpful during unit testing.
Line range hint 11-13: LGTM! Improved transparency handling.
The automatic Mode setting based on the alpha channel is a logical improvement that ensures consistency between the Color and Mode properties.
18-19: Verify impact of changing default color to Transparent.
The change from Colors.Black to Colors.Transparent as the default color could affect existing code that relies on the previous behavior.
Let's check for potential impacts:
src/Consolonia.Core/Drawing/PixelBufferImplementation/PixelForeground.cs (2)
2-2: LGTM: Good debugging enhancement!
The addition of DebuggerDisplay attribute with symbol text and color improves the debugging experience, making it easier to inspect PixelForeground instances during testing.
Also applies to: 7-7
10-15: Verify impact of breaking change in constructor.
Making color parameter required improves type safety by ensuring all PixelForeground instances have an explicit color. However, this is a breaking change that needs careful verification.
Let's verify the impact:
✅ Verification successful
No breaking changes detected - all usages already provide explicit colors
After analyzing all PixelForeground instantiations in the codebase, every usage already provides an explicit color parameter, either directly or through the named parameter color:. This indicates that making the color parameter required is not a breaking change in practice.
Examples of existing usage patterns found:
- Test files:
new PixelForeground(symbol, Colors.Red) - Core implementation:
new PixelForeground(new SimpleSymbol(quadPixel), color: foreground) - Default transparent case:
new PixelForeground(new SimpleSymbol(), Colors.Transparent)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find potential breaking changes from the constructor modification
# Search for PixelForeground instantiations that might be affected
echo "Searching for PixelForeground instantiations that might need updates:"
rg -l "new PixelForeground\(" | while read -r file; do
echo "=== $file ==="
rg -A 2 "new PixelForeground\(" "$file"
done
Length of output: 6144
src/Tests/Consolonia.Gallery.Tests/TextBoxTests.cs (2)
3-4: LGTM! Namespace changes align with project restructuring.
The updated imports correctly reflect the new package structure, moving from TestsCore to NUnit and updating the base namespace. This aligns with the PR objectives of separating and reorganizing the test libraries.
12-13: LGTM! Method accessibility change improves test organization.
Making the test method public and adding the [Test] attribute improves test discoverability and organization, which aligns with the PR objective of "allowing for better navigation and the ability to have multiple unit tests per class."
src/Tests/Consolonia.Core.Tests/DrawingBoxSymbolTests.cs (1)
1-9: LGTM! Well-structured test class setup.
The file structure follows good testing practices with appropriate imports and test fixture decoration.
src/Consolonia.Core/Text/GlyphTypeface.cs (2)
78-78: LGTM!
Good consistency improvement by using the DrawingContextImpl constant for StrikethroughThickness.
29-31: Review the design decision to use placeholder characters.
The current implementation replaces all characters with 'X', which could have several implications:
- Loss of character width information for proper text layout
- Potential issues with special characters and Unicode handling
- Impact on text measurement and positioning
Consider implementing proper glyph mapping or documenting why this simplification is acceptable.
Let's check if this affects any visual tests or layout-dependent code:
src/Tests/Consolonia.Core.Tests/GlyphTests.cs (2)
1-8: LGTM! Well-structured test class setup.
The file follows testing best practices with proper namespace organization and NUnit test structure.
43-78: Verify Unicode and emoji handling across platforms.
The tests for complex characters (𝔉𝔞𝔫𝔠𝔶) and emojis (👨👩👧👦) might behave differently across:
- Different operating systems
- Various terminal emulators
- Different culture settings
Consider:
- Adding culture-specific test cases
- Documenting platform requirements
- Adding tests with larger Unicode sequences to verify memory handling
src/Tests/Consolonia.Core.Tests/SimpleSymbolTests.cs (1)
1-10: LGTM! Well-structured test class setup.
The file structure follows testing best practices with proper imports and test fixture decoration.
src/Consolonia.Themes.TurboVision/Templates/Controls/Helpers/SymbolsControl.cs (2)
8-8: LGTM: Required import for text formatting.
The added import is necessary for the new text shaping implementation using ShapedBuffer.
72-77:
Several improvements needed in text shaping implementation.
The current implementation has potential issues:
- Memory leak: The old
_shapedTextshould be disposed before reassignment - Performance: Creating a new
TextShaperinstance on every set is inefficient - Missing null checks: Direct access to
valuewithout validation - Resource management: Unclear if
ShapedBufferneeds disposal
Consider applying these improvements:
public string Text
{
get => _text;
set
{
+ if (value == _text)
+ return;
+
+ _shapedText?.Dispose();
_text = value;
- ShapedBuffer glyphs =
- new TextShaper().ShapeText(value.AsMemory(), new TextShaperOptions(new GlyphTypeface(), 1));
+ if (string.IsNullOrEmpty(_text))
+ {
+ _shapedText = null;
+ return;
+ }
+
+ using var shaper = new TextShaper();
+ ShapedBuffer glyphs = shaper.ShapeText(
+ _text.AsMemory(),
+ new TextShaperOptions(new GlyphTypeface(), 1));
+
_shapedText = new GlyphRun(new GlyphTypeface(),
1,
- (_text ?? string.Empty).AsMemory(),
+ _text.AsMemory(),
glyphs,
default(Point));
}
}Let's verify if ShapedBuffer implements IDisposable:
src/Consolonia.Gallery/Gallery/GalleryViews/GalleryTextBlock.axaml (1)
12-12: LGTM!
The added empty line improves readability by separating the reference comment from the implementation.
src/Consolonia.Core/Drawing/PixelBufferImplementation/PixelBuffer.cs (3)
14-14: LGTM: Field declaration follows best practices.
The private field declaration follows proper naming conventions and uses an appropriate type for coordinate storage.
21-21: LGTM: Proper initialization of caret position.
The caret position is appropriately initialized to the origin (0,0) after buffer creation.
77-90: Verify caret position usage and Pixel construction.
Let's verify the usage of this new functionality and the Pixel constructor pattern.
✅ Verification successful
Based on the results, I can now provide a final response:
Caret position handling and Pixel construction are consistent
The changes in SetCaretPosition align with the codebase patterns:
- The method is properly implemented across the interface (
IConsole) and its implementations - The Pixel constructor usage in
SetCaretPositionfollows the established pattern wherePixelobjects are constructed with foreground, background, and optional caret parameters - The caret-specific constructor calls in
PixelBuffer.csare unique to this class, which is appropriate as it's the primary owner of caret state management
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for SetCaretPosition usage patterns
echo "Checking SetCaretPosition usage patterns:"
rg "SetCaretPosition\(" -A 3
# Check Pixel constructor usage patterns
echo -e "\nChecking Pixel constructor patterns:"
ast-grep --pattern 'new Pixel($$$)'
Length of output: 6797
src/Tests/Consolonia.Core.Tests/PixelTests.cs (1)
1-10: LGTM! Well-structured test class setup.
The file follows testing best practices with proper organization, necessary imports, and appropriate test attributes.
src/Tests/Consolonia.Core.Tests/ColorTests.cs (1)
5-8: LGTM! Class rename improves clarity.
The rename from PixelTests to ColorTests better reflects the actual purpose of these tests, which focus on color manipulation operations.
src/Consolonia.Core/Helpers/Extensions.cs (2)
22-31: LGTM! Clean parameter unpacking.
The method provides a convenient way to print a Pixel object by unpacking its properties into the underlying console Print method.
94-114: Verify width calculation for complex emoji sequences.
The width calculation for complex emoji sequences (using ZWJ) needs verification, especially when multiple emoji characters are combined. Also, consider caching the console service reference to avoid repeated lookups.
Let's verify the width calculation with different emoji sequences:
✅ Verification successful
Let's gather more information about the emoji width calculation tests.
Let's check one more thing to understand the UnicodeCalculator implementation.
Based on the test evidence from GlyphTests.cs and the implementation in Extensions.cs, I can now provide a final response:
Width calculation for complex emoji sequences is correctly implemented
The test suite demonstrates proper handling of emoji width calculations:
- Single emoji (👍) correctly returns width of 2
- Complex emoji sequence (👨👩👧👦) also returns width of 2, accounting for ZWJ
- Tests verify both complex emoji support enabled and disabled scenarios
- Width calculations are consistent across different test cases
The suggestion about caching the console service is valid but is a minor optimization concern, as the lookup cost is negligible compared to the text processing operations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find test files that verify emoji width calculations
# Look for test files related to text measurement or emoji handling
rg -l "MeasureText|emoji|width" --type cs
# Look for specific test cases with complex emoji sequences
ast-grep --pattern 'Assert.$$$("👨👩👧👦"$$$)'
Length of output: 1121
Script:
#!/bin/bash
# Check the content of GlyphTests.cs for emoji width tests
cat "src/Tests/Consolonia.Core.Tests/GlyphTests.cs"
# Look for UnicodeCalculator implementation and usage
rg -g "*.cs" -A 5 "class UnicodeCalculator"
# Find any other emoji-related tests
rg -g "*.cs" -A 5 "SupportsComplexEmoji"
Length of output: 6943
Script:
#!/bin/bash
# Find the UnicodeCalculator implementation
fd -e cs UnicodeCalculator
# If found, display its content
find . -name "UnicodeCalculator.cs" -type f -exec cat {} \;
# Look for any width-related tests that might use UnicodeCalculator
rg -g "*.cs" "GetWidth.*test" -B 2 -A 5
Length of output: 133
src/Consolonia.Core/Drawing/PixelBufferImplementation/DrawingBoxSymbol.cs (2)
2-15: LGTM! Good improvements to debugging and immutability.
The addition of DebuggerDisplay attribute and making _upRightDownLeft readonly are good improvements that enhance debugging experience and enforce immutability.
109-116: LGTM! Good immutability improvements.
The refactored Blend method correctly implements immutability by returning new instances and has proper error handling with pattern matching.
src/Tests/Consolonia.Core.Tests/PixelForegroundTests.cs (2)
1-11: LGTM! Well-structured test class setup.
The file structure follows testing best practices with proper imports and attributes.
1-112: Overall, well-structured and comprehensive test suite!
The test coverage is good and follows testing best practices. The suggested improvements would enhance the robustness of the test suite, but the current implementation is solid and ready for use.
src/Consolonia.NUnit/UnitTestConsole.cs (2)
16-16: LGTM: Namespace change aligns with PR objectives.
The namespace change from Consolonia.TestsCore to Consolonia.NUnit correctly implements the test library separation and renaming described in the PR objectives.
63-73: LGTM: Improved Unicode text handling.
The switch to EnumerateRunes() and Rune-based symbol creation improves Unicode character handling, particularly for complex emoji support. The ReSharper warning suppression is appropriately documented.
src/Consolonia.sln (5)
3-5: LGTM: Visual Studio version information is properly configured.
The solution file correctly specifies Visual Studio 2022 (Version 17) with appropriate version numbers:
- VisualStudioVersion = 17.11.35327.3
- MinimumVisualStudioVersion = 10.0.40219.1
11-14: LGTM: Solution items are properly organized.
The solution items section correctly includes essential files:
- .editorconfig for consistent coding styles
- readme.md for project documentation
90-97: LGTM: Build configurations are properly set up.
The Debug and Release configurations are correctly defined for the new projects:
- Consolonia.NUnit
- Consolonia.Core.Tests
99-109: LGTM: Solution structure is well organized.
The solution properties and nested projects are properly configured:
- Test projects are correctly nested under the Tests folder
- Solution properties are properly set
- Global section includes a valid solution GUID
46-47: Verify test project dependencies.
The new test project structure looks good with:
- Consolonia.Gallery.Tests (renamed from TestsCore)
- Consolonia.NUnit (new test support library)
- Consolonia.Core.Tests (new core test project)
Let's verify the test project references:
Also applies to: 52-54
✅ Verification successful
Test project dependencies are properly configured
The verification confirms that both test projects:
- Reference the required NUnit packages (NUnit, NUnit3TestAdapter)
- Include the Consolonia.NUnit support library
- Have appropriate project references:
- Core.Tests references Core, PlatformSupport, and Themes
- Gallery.Tests references Gallery project
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify test project references to ensure proper dependencies
# Check if test projects reference the NUnit library
echo "Checking NUnit references in test projects..."
fd -e csproj -p "Tests/" | xargs grep -l "Consolonia.NUnit"
# Check if test projects have proper project references
echo "Checking project references..."
fd -e csproj -p "Tests/" | xargs grep -l "<ProjectReference.*Consolonia\."
Length of output: 560
Script:
#!/bin/bash
# Let's examine the actual content of test project files to verify specific dependencies
echo "=== Content of Core Tests project ==="
cat "src/Tests/Consolonia.Core.Tests/Consolonia.Core.Tests.csproj"
echo -e "\n=== Content of Gallery Tests project ==="
cat "src/Tests/Consolonia.Gallery.Tests/Consolonia.Gallery.Tests.csproj"
echo -e "\n=== Content of NUnit support library ==="
fd -e csproj -p "src/" | grep -l "Consolonia.NUnit" | xargs cat
Length of output: 2215
src/Consolonia.Core/Infrastructure/InputLessDefaultNetConsole.cs (1)
60-61: LGTM! Property implementation follows best practices.
The read-only property is well-designed and properly initialized during construction.
src/Consolonia.Core/Drawing/RenderTarget.cs (2)
101-101: LGTM: Cache initialization updated correctly.
The cache initialization has been properly updated to match the new field type.
24-24: Verify the impact of changing _cache field type.
The change from char to string in the cache tuple is a significant modification that affects the entire rendering pipeline. While this change enables more complex text rendering, we should ensure all consumers are updated.
✅ Verification successful
Change from char to string in _cache field is safe to proceed
The verification shows that the _cache field is only used internally within the RenderTarget class for pixel spread comparison and assignment. The change from char to string in the cache tuple is properly contained within the class's implementation, and there are no direct external dependencies on the char-based cache structure. The usage patterns found show proper encapsulation of the cache field.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any direct field access or related char-based operations
rg -l "_cache\[.*\].*char"
Length of output: 1582
Script:
#!/bin/bash
# Let's search for usages of _cache field and related patterns
rg -A 3 "_cache\[" src/Consolonia.Core/Drawing/RenderTarget.cs
# Also search for any char-based operations in the file
rg -A 3 "char\s+\w+" src/Consolonia.Core/Drawing/RenderTarget.cs
# Look for method signatures that might be affected
ast-grep --pattern 'public $_ $_($$$) {
$$$
_cache$$$
$$$
}'
Length of output: 454
src/Consolonia.Core/Drawing/PixelBufferImplementation/Pixel.cs (4)
10-10: Verify the correctness of the DebuggerDisplay attribute.
Ensure that Foreground.Symbol.Text, Foreground.Color, and Background.Color are always properly initialized to prevent potential NullReferenceException during debugging. This will help maintain accurate and reliable debug information.
37-42: Confirm the handling of foreground in background-only pixels.
When creating a Pixel with only a background, the foreground is initialized with a SimpleSymbol and Colors.Transparent. Verify that this approach meets the intended rendering behavior and does not introduce any unexpected issues.
53-55: Default parameter for isCaret in constructor.
The isCaret parameter in the constructor has a default value of false. This seems appropriate for most use cases.
Line range hint 62-87: Review the blending logic in the Blend method.
The Blend method handles different PixelBackgroundMode cases:
- Colored Mode: Merges background colors using alpha blending and replaces the foreground with
pixelAbove.Foreground. - Transparent Mode: Blends foregrounds only if
pixelAbove.Foreground.Coloris notColors.Transparent. The background remains unchanged. - Shaded Mode: Applies shading to both foreground and background before blending the foregrounds.
Ensure that these implementations align with the desired visual outcomes and that there are no edge cases where the blending might not behave as expected.
src/Consolonia.Core/Drawing/DrawingContextImpl.cs (2)
Line range hint 482-544: Updates to DrawStringInternal improve glyph handling.
The enhancements in the DrawStringInternal method improve glyph rendering and appear to be implemented correctly.
29-29: Ensure all references to StrikthroughThickness are updated to StrikethroughThickness.
The constant name has been corrected; please verify that all usages in the codebase have been updated accordingly.
Run the following script to check for any remaining references to the old constant name:
✅ Verification successful
No issues found with StrikethroughThickness usage
The search results show that the constant StrikethroughThickness is consistently used with the correct spelling across the codebase:
- Defined in
DrawingContextImpl.cs - Referenced in
GlyphTypeface.cs - Used in a switch expression in
DrawingContextImpl.cs
There are no instances of the incorrect spelling StrikthroughThickness in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for occurrences of 'StrikthroughThickness' (incorrect spelling)
# Expected result: No matches found.
rg 'StrikthroughThickness'
Length of output: 26
Script:
#!/bin/bash
# Let's search for both spellings to understand the context better
echo "Searching for 'StrikethroughThickness':"
rg "StrikethroughThickness" -A 2
echo -e "\nSearching for any similar constant names with 'strike' or 'through':"
rg -i "strike.*thickness|through.*thickness" -A 2
Length of output: 2062
…how up as a unused reference in visual studio but the linter says it's unused.
Co-authored-by: <[email protected]>
tomlm
changed the title
…ymbols getting rid of grody spread operator on cache Added unit tests for all of things.
Co-authored-by: <[email protected]>
Co-authored-by: <[email protected]>
|
I don't know if you like the name of Consolonia.NUnit but it aligns with Avalonia.Headless.Tests.NUnit/XUnit. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (27)
src/Consolonia.Core/Drawing/PixelBufferImplementation/PixelBufferCoordinate.cs (1)
Line range hint
8-61: Implement complete equality pattern for value types.The struct implements
Equals(PixelBufferCoordinate)but is missing other essential equality members. For value types, it's recommended to implement the complete equality pattern:
IEquatable<T>interface- Override of
Equals(object)==and!=operatorsConsider implementing the complete equality pattern:
-public readonly struct PixelBufferCoordinate +public readonly struct PixelBufferCoordinate : IEquatable<PixelBufferCoordinate> { // ... existing code ... + public override bool Equals(object? obj) + { + return obj is PixelBufferCoordinate other && Equals(other); + } public bool Equals(PixelBufferCoordinate secondPoint) { return X == secondPoint.X && Y == secondPoint.Y; } public override int GetHashCode() { return HashCode.Combine(X, Y); } + public static bool operator ==(PixelBufferCoordinate left, PixelBufferCoordinate right) + { + return left.Equals(right); + } + + public static bool operator !=(PixelBufferCoordinate left, PixelBufferCoordinate right) + { + return !(left == right); + } }src/Consolonia.Core/Drawing/PixelBufferImplementation/SimpleSymbol.cs (1)
50-72: Consider null handling in equality operatorsWhile the equality implementation is generally good and follows best practices, the == and != operators might need to handle null references when used with nullable SimpleSymbol variables.
Consider updating the operators to handle null:
public static bool operator ==(SimpleSymbol left, SimpleSymbol right) { return left.Equals(right); } public static bool operator !=(SimpleSymbol left, SimpleSymbol right) { return !left.Equals(right); }Note: Since SimpleSymbol is a struct, it can't be null itself, but it can be wrapped in a nullable type (SimpleSymbol?). If this is not a common use case in your codebase, you can ignore this suggestion.
src/Tests/Consolonia.Core.Tests/GlyphTests.cs (2)
9-104: Consider using test data attributes for better maintainability.While the tests are well-structured and comprehensive, consider using NUnit's TestCase attributes to reduce code duplication and improve maintainability, especially for similar test cases.
Here's an example refactor for the single character and emoji tests:
- [Test] - public void GetGlyphsSingleCharacterReturnsSingleGlyph() - { - string text = "a"; - Assert.AreEqual(1, text.MeasureText()); - - var glyphs = text.GetGlyphs(true); - Assert.AreEqual(1, glyphs.Count); - Assert.AreEqual("a", glyphs[0]); - } - - [Test] - public void GetGlyphsSingleEmojiReturnsSingleGlyph() - { - string text = "👍"; - Assert.AreEqual(2, text.MeasureText()); - - var glyphs = text.GetGlyphs(true); - Assert.AreEqual(1, glyphs.Count); - Assert.AreEqual("👍", glyphs[0]); - } + [TestCase("a", 1, Description = "Single character")] + [TestCase("👍", 2, Description = "Single emoji")] + public void GetGlyphsSingleGlyphTest(string input, int expectedMeasure) + { + Assert.AreEqual(expectedMeasure, input.MeasureText()); + + var glyphs = input.GetGlyphs(true); + Assert.AreEqual(1, glyphs.Count); + Assert.AreEqual(input, glyphs[0]); + }🧰 Tools
🪛 GitHub Check: build
[notice] 20-20:
"[ConvertToConstant.Local] Convert into constant" on /home/runner/work/Consolonia/Consolonia/src/Tests/Consolonia.Core.Tests/GlyphTests.cs(20,20)
[notice] 31-31:
"[ConvertToConstant.Local] Convert into constant" on /home/runner/work/Consolonia/Consolonia/src/Tests/Consolonia.Core.Tests/GlyphTests.cs(31,20)
[notice] 46-46:
"[ConvertToConstant.Local] Convert into constant" on /home/runner/work/Consolonia/Consolonia/src/Tests/Consolonia.Core.Tests/GlyphTests.cs(46,20)
[notice] 61-61:
"[ConvertToConstant.Local] Convert into constant" on /home/runner/work/Consolonia/Consolonia/src/Tests/Consolonia.Core.Tests/GlyphTests.cs(61,20)
[notice] 72-72:
"[ConvertToConstant.Local] Convert into constant" on /home/runner/work/Consolonia/Consolonia/src/Tests/Consolonia.Core.Tests/GlyphTests.cs(72,20)
[notice] 83-83:
"[ConvertToConstant.Local] Convert into constant" on /home/runner/work/Consolonia/Consolonia/src/Tests/Consolonia.Core.Tests/GlyphTests.cs(83,20)
[notice] 95-95:
"[ConvertToConstant.Local] Convert into constant" on /home/runner/work/Consolonia/Consolonia/src/Tests/Consolonia.Core.Tests/GlyphTests.cs(95,20)
92-104: Consider adding test for mixed complex emoji and text scenario.While the test coverage is comprehensive, consider adding a test case that combines multiple complex emojis with regular text to ensure proper handling of mixed content.
Example test case:
[Test] public void GetGlyphsWithMixedComplexEmojiAndText() { string text = "Hello 👨👩👧👦 World 👩💻"; var glyphs = text.GetGlyphs(true); Assert.AreEqual(4, glyphs.Count); Assert.AreEqual("Hello ", glyphs[0]); Assert.AreEqual("👨👩👧👦", glyphs[1]); Assert.AreEqual(" World ", glyphs[2]); Assert.AreEqual("👩💻", glyphs[3]); }🧰 Tools
🪛 GitHub Check: build
[notice] 95-95:
"[ConvertToConstant.Local] Convert into constant" on /home/runner/work/Consolonia/Consolonia/src/Tests/Consolonia.Core.Tests/GlyphTests.cs(95,20)src/Tests/Consolonia.Core.Tests/PixelBackgroundTests.cs (5)
10-16: Consider using a more descriptive test name.While the test logic is correct, consider renaming the test method to better describe the scenario being tested, e.g.,
DefaultConstructor_ShouldInitializeWithTransparentValues.-public void Constructor() +public void DefaultConstructor_ShouldInitializeWithTransparentValues()
18-24: Consider parameterizing the color test.The test could be enhanced by using TestCase attribute to verify behavior with different colors, including edge cases.
-[Test] -public void ConstructorWithColor() -{ - var pixelBackground = new PixelBackground(Colors.Red); - Assert.That(pixelBackground.Color, Is.EqualTo(Colors.Red)); - Assert.That(pixelBackground.Mode, Is.EqualTo(PixelBackgroundMode.Colored)); -} +[TestCase("Red")] +[TestCase("Transparent")] +[TestCase("White")] +[TestCase("Black")] +public void ConstructorWithColor_ShouldSetColorAndMode(string colorName) +{ + var color = (Color)typeof(Colors).GetProperty(colorName).GetValue(null); + var pixelBackground = new PixelBackground(color); + Assert.That(pixelBackground.Color, Is.EqualTo(color)); + Assert.That(pixelBackground.Mode, Is.EqualTo( + color == Colors.Transparent ? + PixelBackgroundMode.Transparent : + PixelBackgroundMode.Colored)); +}
26-35: Consider using more specific assertions.While the test logic is correct, using direct Equals calls in assertions makes error messages less descriptive. Consider using NUnit's built-in constraints.
-Assert.That(pixelBackground.Color.Equals(Colors.Red)); -Assert.That(pixelBackground.Mode.Equals(mode)); +Assert.That(pixelBackground.Color, Is.EqualTo(Colors.Red)); +Assert.That(pixelBackground.Mode, Is.EqualTo(mode));
37-51: Consider adding null equality test.The equality tests are comprehensive but should include a null comparison test to ensure proper behavior.
[Test] public void Equality() { var pixelBackground = new PixelBackground(Colors.Red); var pixelBackground2 = new PixelBackground(Colors.Red); Assert.That(pixelBackground.Equals((object)pixelBackground2)); Assert.That(pixelBackground.Equals(pixelBackground2)); Assert.That(pixelBackground == pixelBackground2); + Assert.That(pixelBackground.Equals(null), Is.False);
68-83: Consider adding hash code consistency test.While the hash code equality tests are good, consider adding a test to verify that hash codes remain consistent across multiple calls.
[Test] public void HashCode() { var pixelBackground = new PixelBackground(Colors.Red); var pixelBackground2 = new PixelBackground(Colors.Red); Assert.That(pixelBackground.GetHashCode(), Is.EqualTo(pixelBackground2.GetHashCode())); + + // Test hash code consistency + var hash1 = pixelBackground.GetHashCode(); + var hash2 = pixelBackground.GetHashCode(); + Assert.That(hash1, Is.EqualTo(hash2), "Hash code should be consistent");src/Tests/Consolonia.Core.Tests/DrawingBoxSymbolTests.cs (6)
11-16: Enhance Constructor test coverage.The current test only verifies a single case. Consider adding:
- Edge cases (0x00, 0xFF)
- Invalid/boundary values
- Negative test cases
[Test] public void Constructor() { - ISymbol symbol = new DrawingBoxSymbol(0b0000_1111); - Assert.That(symbol.Text, Is.EqualTo("┼")); + Assert.Multiple(() => + { + // Basic case + Assert.That(new DrawingBoxSymbol(0b0000_1111).Text, Is.EqualTo("┼")); + // Edge cases + Assert.That(new DrawingBoxSymbol(0b0000_0000).Text, Is.EqualTo(" ")); + Assert.That(new DrawingBoxSymbol(0b1111_1111).Text, Is.Not.Empty); + }); }
18-27: Improve Blend test method name and assertions.While the test has good intermediate validation, consider:
- Renaming test to better describe the scenario (e.g.,
BlendHorizontalAndVerticalLines)- Verifying that original symbols remain unchanged after blend
[Test] - public void Blend() + public void BlendHorizontalAndVerticalLines() { ISymbol symbol = new DrawingBoxSymbol(0b0000_0101); Assert.That(symbol.Text, Is.EqualTo("─")); ISymbol symbolAbove = new DrawingBoxSymbol(0b0000_1010); Assert.That(symbolAbove.Text, Is.EqualTo("│")); + + string originalSymbolText = symbol.Text; + string originalSymbolAboveText = symbolAbove.Text; + ISymbol newSymbol = symbol.Blend(ref symbolAbove); - Assert.That(newSymbol.Text, Is.EqualTo("┼")); + + Assert.Multiple(() => + { + Assert.That(newSymbol.Text, Is.EqualTo("┼"), "Blended symbol should be a cross"); + Assert.That(symbol.Text, Is.EqualTo(originalSymbolText), "Original symbol should remain unchanged"); + Assert.That(symbolAbove.Text, Is.EqualTo(originalSymbolAboveText), "Original symbol above should remain unchanged"); + }); }
74-81: Remove duplicate assertion in EqualityISymbol test.The same assertion is repeated twice.
public void EqualityISymbol() { ISymbol symbol = new DrawingBoxSymbol(0b0000_1111); ISymbol symbol2 = new DrawingBoxSymbol(0b0000_1111); Assert.That(symbol.Equals(symbol2)); - Assert.That(symbol.Equals(symbol2)); }
64-72: Enhance equality test with GetHashCode verification.When testing equality, it's important to verify that equal objects have equal hash codes.
public void Equality() { var symbol = new DrawingBoxSymbol(0b0000_1111); var symbol2 = new DrawingBoxSymbol(0b0000_1111); - Assert.That(symbol.Equals((object)symbol2)); - Assert.That(symbol.Equals(symbol2)); - Assert.That(symbol == symbol2); + Assert.Multiple(() => + { + Assert.That(symbol.Equals((object)symbol2), "Object equality should work"); + Assert.That(symbol.Equals(symbol2), "Typed equality should work"); + Assert.That(symbol == symbol2, "Equality operator should work"); + Assert.That(symbol.GetHashCode(), Is.EqualTo(symbol2.GetHashCode()), + "Equal objects should have equal hash codes"); + }); }
93-100: Remove duplicate assertion in InequalityISymbol test.The same assertion is repeated twice.
public void InequalityISymbol() { ISymbol symbol = new DrawingBoxSymbol(0b0000_1111); ISymbol symbol2 = new DrawingBoxSymbol(0b0000_0000); Assert.That(!symbol.Equals(symbol2)); - Assert.That(!symbol.Equals(symbol2)); }
102-114: Enhance Hash test coverage.Consider adding verification that different objects have different hash codes and are treated differently in HashSet.
public void Hash() { var set = new HashSet<DrawingBoxSymbol>(); set.Add(new DrawingBoxSymbol(0b0000_1111)); set.Add(new DrawingBoxSymbol(0b0000_1111)); - Assert.That(set.Count, Is.EqualTo(1)); + Assert.Multiple(() => + { + Assert.That(set.Count, Is.EqualTo(1), "Equal objects should be treated as same"); + set.Add(new DrawingBoxSymbol(0b0000_0000)); + Assert.That(set.Count, Is.EqualTo(2), "Different objects should be treated as different"); + }); var set2 = new HashSet<ISymbol>(); set2.Add(new DrawingBoxSymbol(0b0000_1111)); set2.Add(new DrawingBoxSymbol(0b0000_1111)); - Assert.That(set2.Count, Is.EqualTo(1)); + Assert.Multiple(() => + { + Assert.That(set2.Count, Is.EqualTo(1), "Equal ISymbols should be treated as same"); + set2.Add(new DrawingBoxSymbol(0b0000_0000)); + Assert.That(set2.Count, Is.EqualTo(2), "Different ISymbols should be treated as different"); + }); }src/Tests/Consolonia.Core.Tests/SimpleSymbolTests.cs (4)
12-51: Consider adding more edge cases to constructor tests.While the current tests cover the basic scenarios well, consider adding:
- Tests for null/invalid inputs
- Tests for maximum width validation
- Multiple test cases for complex emojis using TestCase attribute
Example addition:
[Test] public void ConstructorString_Null_ThrowsArgumentNullException() { Assert.Throws<ArgumentNullException>(() => new SimpleSymbol((string)null)); } [TestCase("🏳️🌈", 2)] // Rainbow flag [TestCase("👨👩👧👦", 2)] // Family [TestCase("🤦♂️", 2)] // Face palm public void ConstructorComplexEmoji_Various(string emoji, int expectedWidth) { ISymbol symbol = new SimpleSymbol(emoji); Assert.That(symbol.Text, Is.EqualTo(emoji)); Assert.That(symbol.Width, Is.EqualTo(expectedWidth)); }
53-89: Remove redundant equality assertions.In both
EqualityISymbolandInequalityISymbolmethods, the same assertion is made twice:Assert.That(symbol.Equals(symbol2)); Assert.That(symbol.Equals(symbol2)); // DuplicateRemove the duplicate assertions to improve test maintainability.
91-103: Consider additional HashCode test scenarios.The current test verifies basic HashSet behavior, but consider adding:
- Test with different strings having the same hash code
- Test with emoji characters
- Verify GetHashCode contract: equal objects must have equal hash codes
Example addition:
[Test] public void HashCode_EqualObjects_HaveEqualHashCodes() { var symbol1 = new SimpleSymbol("👍"); var symbol2 = new SimpleSymbol("👍"); Assert.That(symbol1.GetHashCode(), Is.EqualTo(symbol2.GetHashCode())); }
113-151: Consider additional Blend test scenarios.The current Blend tests cover basic scenarios well, but consider adding:
- Test blending with null (verify exception handling)
- Test blending two emoji characters
- Test blending characters with different widths
Example addition:
[Test] public void Blend_WithNull_ThrowsArgumentNullException() { ISymbol symbol = new SimpleSymbol("a"); ISymbol nullSymbol = null; Assert.Throws<ArgumentNullException>(() => symbol.Blend(ref nullSymbol)); } [Test] public void Blend_TwoEmojis() { ISymbol symbol = new SimpleSymbol("👍"); ISymbol symbolAbove = new SimpleSymbol("👎"); ISymbol newSymbol = symbol.Blend(ref symbolAbove); Assert.That(newSymbol.Text, Is.EqualTo("👎")); Assert.That(newSymbol.Width, Is.EqualTo(2)); }src/Tests/Consolonia.Core.Tests/PixelTests.cs (4)
11-66: Consider adding edge cases to constructor tests.While the current constructor tests are thorough for happy paths, consider adding tests for:
- Null symbol handling
- Invalid DrawingBoxSymbol patterns (if applicable)
- Constructor exception cases
68-98: Enhance equality tests with additional cases.Consider adding tests for:
[Test] public void Equality_WithNull() { var pixel = new Pixel(new SimpleSymbol("a"), Colors.Red); Assert.That(pixel.Equals(null), Is.False); } [Test] public void Equality_WithSelf() { var pixel = new Pixel(new SimpleSymbol("a"), Colors.Red); Assert.That(pixel.Equals(pixel), Is.True); }
100-122: Add more comprehensive blending test cases.Consider adding tests for:
- Blending with null
- Blending two transparent backgrounds
- Blending identical pixels
Example:
[Test] public void BlendWithNull() { var pixel = new Pixel(new PixelBackground(Colors.Green)); Assert.That(() => pixel.Blend(null), Throws.ArgumentNullException); } [Test] public void BlendTransparentWithTransparent() { var pixel1 = new Pixel(new PixelBackground(Colors.Transparent)); var pixel2 = new Pixel(new PixelBackground(Colors.Transparent)); var result = pixel1.Blend(pixel2); Assert.That(result.Background.Mode, Is.EqualTo(PixelBackgroundMode.Transparent)); }
124-133: Strengthen hash code testing.While the HashSet test is good, consider adding explicit hash code tests:
[Test] public void HashCode_ConsistentForSameValues() { var pixel1 = new Pixel(new SimpleSymbol("a"), Colors.Red); var pixel2 = new Pixel(new SimpleSymbol("a"), Colors.Red); Assert.That(pixel1.GetHashCode(), Is.EqualTo(pixel2.GetHashCode())); } [Test] public void HashCode_DifferentForDifferentValues() { var pixel1 = new Pixel(new SimpleSymbol("a"), Colors.Red); var pixel2 = new Pixel(new SimpleSymbol("b"), Colors.Red); Assert.That(pixel1.GetHashCode(), Is.Not.EqualTo(pixel2.GetHashCode())); }src/Tests/Consolonia.Core.Tests/PixelForegroundTests.cs (4)
12-83: Consider adding negative test cases for constructors.While the constructor tests are comprehensive for valid inputs, consider adding tests for:
- Null symbol handling
- Invalid color values
- Empty or invalid emoji sequences
Example test to add:
[Test] public void Constructor_WithNullSymbol_ShouldThrowArgumentNullException() { Assert.Throws<ArgumentNullException>(() => new PixelForeground(null, Colors.Red)); }
84-112: Enhance equality testing coverage.The equality tests are well-structured, but consider adding:
- Null comparison test
- Explicit IEquatable interface test
- Self-reference equality test (x.Equals(x))
Example additions:
[Test] public void Equality_WithNull_ReturnsFalse() { var pixelForeground = new PixelForeground(new SimpleSymbol('a'), Colors.Red); Assert.That(pixelForeground.Equals(null), Is.False); } [Test] public void Equality_WithSelfReference_ReturnsTrue() { var pixelForeground = new PixelForeground(new SimpleSymbol('a'), Colors.Red); Assert.That(pixelForeground.Equals(pixelForeground), Is.True); }
113-152: Add edge cases to blending tests.The blending tests cover main scenarios well, but consider adding:
- Blending with null (should throw ArgumentNullException)
- Blending with transparent/empty pixel foreground (should retain original)
- Blending with same pixel foreground (should return equivalent result)
Example test:
[Test] public void Blend_WithNull_ShouldThrowArgumentNullException() { var pixelForeground = new PixelForeground(new SimpleSymbol('a'), Colors.Red); Assert.Throws<ArgumentNullException>(() => pixelForeground.Blend(null)); }
153-169: Improve hash code test organization and coverage.While the hash code tests verify the basic contract, consider:
- Using TestCases for better organization of inequality tests
- Adding test for hash code consistency
- Adding test for complex objects (with weight, style, decorations)
Example refactor:
[Test] public void HashCode_ShouldBeConsistent() { var pixelForeground = new PixelForeground(new SimpleSymbol('a'), Colors.Red); var hash1 = pixelForeground.GetHashCode(); var hash2 = pixelForeground.GetHashCode(); Assert.That(hash1, Is.EqualTo(hash2)); } [TestCase('a', 'b', TestName = "Different Symbols")] [TestCase('a', 'a', Colors.Red, Colors.Blue, TestName = "Different Colors")] public void HashCode_ShouldNotBeEqual(char symbol1, char symbol2, Color color1 = Colors.Red, Color color2 = Colors.Red) { var pf1 = new PixelForeground(new SimpleSymbol(symbol1), color1); var pf2 = new PixelForeground(new SimpleSymbol(symbol2), color2); Assert.That(pf1.GetHashCode(), Is.Not.EqualTo(pf2.GetHashCode())); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (19)
src/Consolonia.Core/Drawing/DrawingContextImpl.cs(3 hunks)src/Consolonia.Core/Drawing/PixelBufferImplementation/DrawingBoxSymbol.cs(4 hunks)src/Consolonia.Core/Drawing/PixelBufferImplementation/Pixel.cs(4 hunks)src/Consolonia.Core/Drawing/PixelBufferImplementation/PixelBackground.cs(3 hunks)src/Consolonia.Core/Drawing/PixelBufferImplementation/PixelBufferCoordinate.cs(1 hunks)src/Consolonia.Core/Drawing/PixelBufferImplementation/PixelForeground.cs(3 hunks)src/Consolonia.Core/Drawing/PixelBufferImplementation/SimpleSymbol.cs(2 hunks)src/Consolonia.Core/Drawing/RenderTarget.cs(3 hunks)src/Consolonia.Core/Helpers/Extensions.cs(2 hunks)src/Consolonia.Core/Text/TextShaper.cs(1 hunks)src/Consolonia.NUnit/UnitTestConsole.cs(2 hunks)src/Tests/Consolonia.Core.Tests/Assembly.cs(1 hunks)src/Tests/Consolonia.Core.Tests/Consolonia.Core.Tests.csproj(1 hunks)src/Tests/Consolonia.Core.Tests/DrawingBoxSymbolTests.cs(1 hunks)src/Tests/Consolonia.Core.Tests/GlyphTests.cs(1 hunks)src/Tests/Consolonia.Core.Tests/PixelBackgroundTests.cs(1 hunks)src/Tests/Consolonia.Core.Tests/PixelForegroundTests.cs(1 hunks)src/Tests/Consolonia.Core.Tests/PixelTests.cs(1 hunks)src/Tests/Consolonia.Core.Tests/SimpleSymbolTests.cs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- src/Consolonia.Core/Drawing/DrawingContextImpl.cs
- src/Consolonia.Core/Drawing/PixelBufferImplementation/PixelForeground.cs
- src/Consolonia.Core/Helpers/Extensions.cs
- src/Consolonia.Core/Text/TextShaper.cs
- src/Consolonia.NUnit/UnitTestConsole.cs
- src/Tests/Consolonia.Core.Tests/Assembly.cs
- src/Tests/Consolonia.Core.Tests/Consolonia.Core.Tests.csproj
🧰 Additional context used
🪛 GitHub Check: build
src/Tests/Consolonia.Core.Tests/GlyphTests.cs
[notice] 20-20:
"[ConvertToConstant.Local] Convert into constant" on /home/runner/work/Consolonia/Consolonia/src/Tests/Consolonia.Core.Tests/GlyphTests.cs(20,20)
[notice] 31-31:
"[ConvertToConstant.Local] Convert into constant" on /home/runner/work/Consolonia/Consolonia/src/Tests/Consolonia.Core.Tests/GlyphTests.cs(31,20)
[notice] 46-46:
"[ConvertToConstant.Local] Convert into constant" on /home/runner/work/Consolonia/Consolonia/src/Tests/Consolonia.Core.Tests/GlyphTests.cs(46,20)
[notice] 61-61:
"[ConvertToConstant.Local] Convert into constant" on /home/runner/work/Consolonia/Consolonia/src/Tests/Consolonia.Core.Tests/GlyphTests.cs(61,20)
[notice] 72-72:
"[ConvertToConstant.Local] Convert into constant" on /home/runner/work/Consolonia/Consolonia/src/Tests/Consolonia.Core.Tests/GlyphTests.cs(72,20)
[notice] 83-83:
"[ConvertToConstant.Local] Convert into constant" on /home/runner/work/Consolonia/Consolonia/src/Tests/Consolonia.Core.Tests/GlyphTests.cs(83,20)
[notice] 95-95:
"[ConvertToConstant.Local] Convert into constant" on /home/runner/work/Consolonia/Consolonia/src/Tests/Consolonia.Core.Tests/GlyphTests.cs(95,20)
🔇 Additional comments (32)
src/Consolonia.Core/Drawing/PixelBufferImplementation/SimpleSymbol.cs (4)
10-10: LGTM! Good practice implementing IEquatable for value types.
The implementation of IEquatable is appropriate for a struct and will improve performance by avoiding boxing/unboxing operations.
47-48: LGTM! Improved readability.
The refactored implementation is more concise and clearer in its intent.
Line range hint 24-34: Parameter validation is still needed in constructors
The previous review comment about adding parameter validation in the constructors is still applicable.
42-42: Verify the behavioral change in IsWhiteSpace()
The change from IsNullOrWhiteSpace to IsNullOrEmpty is a significant behavioral change. While this aligns with the comment about empty symbols on line 14, please verify that this change doesn't break existing behavior where whitespace characters (spaces, tabs) were previously considered as white space.
✅ Verification successful
Based on the search results, I can generate the final response as we have sufficient context. The test file SimpleSymbolTests.cs explicitly verifies the expected behavior, showing that the change from IsNullOrWhiteSpace to IsNullOrEmpty is intentional and correctly tested:
The behavioral change in IsWhiteSpace() is intentional and verified by tests
The change from IsNullOrWhiteSpace to IsNullOrEmpty is correct as evidenced by the unit tests in SimpleSymbolTests.cs which explicitly verify that:
- Empty string returns
true - Space character returns
false - Regular character returns
false
This aligns with the implementation's purpose where only null or empty strings should be considered as white space, not actual whitespace characters.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usages of IsWhiteSpace() to understand impact
rg -A 5 "IsWhiteSpace\(\)"
Length of output: 3979
src/Consolonia.Core/Drawing/PixelBufferImplementation/PixelBackground.cs (4)
3-3: LGTM! Good practice implementing IEquatable for a value type.
The implementation of IEquatable<T> is appropriate for a readonly struct, as it provides better performance for equality comparisons by avoiding boxing/unboxing.
Also applies to: 9-9
11-15: LGTM! Sensible default values.
The default constructor provides appropriate initialization values, making the struct's behavior predictable when default-constructed.
55-78: LGTM! Complete and correct equality implementation.
The equality implementation follows all best practices:
- Implements IEquatable for type-safe comparisons
- Correctly overrides Object.Equals
- Implements GetHashCode consistently
- Provides operator overloads
25-25: Verify the impact of changing the default color from Black to Transparent.
This change in default color could affect existing code that relies on the previous behavior.
✅ Verification successful
Let me search for more context about the usage of this constructor to ensure we have a complete picture.
The change in default color from Black to Transparent is safe to proceed
Based on the codebase analysis:
- All explicit constructor calls provide their own color values
- The default constructor is already tested to expect
Colors.Transparentas the default color - No code relies on the implicit Black color behavior
- The change makes the behavior consistent with the parameterless constructor
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for constructor calls that might be affected by the default color change
rg -A 2 "new PixelBackground\(PixelBackgroundMode\."
Length of output: 2222
Script:
#!/bin/bash
# Search for all PixelBackground instantiations to see if any code relies on the default color
rg -A 2 "new PixelBackground\("
# Also check for any tests that might verify the default color behavior
rg -A 5 "PixelBackground.*Default"
Length of output: 11249
src/Tests/Consolonia.Core.Tests/GlyphTests.cs (2)
1-8: LGTM! Well-structured test class setup.
The file structure follows testing best practices with proper namespace organization and test fixture decoration.
20-20: Ignore static analysis suggestions for converting test data to constants.
The static analysis tool suggests converting string literals to constants, but these are test data values. Converting them to constants would reduce test readability without providing significant benefits. It's a common practice to keep test data inline for better clarity and maintainability.
Also applies to: 31-31, 46-46, 61-61, 72-72, 83-83, 95-95
🧰 Tools
🪛 GitHub Check: build
[notice] 20-20:
"[ConvertToConstant.Local] Convert into constant" on /home/runner/work/Consolonia/Consolonia/src/Tests/Consolonia.Core.Tests/GlyphTests.cs(20,20)
src/Tests/Consolonia.Core.Tests/PixelBackgroundTests.cs (2)
1-9: LGTM! Well-structured test class setup.
The file structure follows testing best practices with appropriate imports and test fixture decoration.
53-66: LGTM! Comprehensive inequality testing.
The test covers all necessary inequality scenarios including different colors and modes.
src/Tests/Consolonia.Core.Tests/DrawingBoxSymbolTests.cs (1)
29-62: Past review comment is still applicable.
The previous review comment about improving the BlendAllSymbols test structure and assertions is still valid.
src/Tests/Consolonia.Core.Tests/SimpleSymbolTests.cs (2)
1-11: LGTM! Well-structured test class setup.
The file structure follows NUnit testing best practices with appropriate imports and attributes.
105-111: Skip comment - see previous review.
The previous review comment about the IsWhiteSpace implementation is still valid and applicable.
src/Consolonia.Core/Drawing/PixelBufferImplementation/Pixel.cs (5)
3-3: LGTM! Good practice implementing IEquatable for value types.
The implementation of IEquatable is a good practice for value types as it provides better performance for equality comparisons and proper value semantics.
Also applies to: 12-12
22-63: Well-documented constructors with improved initialization logic.
The changes improve the code in several ways:
- Consistent initialization using default constructors
- Clear XML documentation explaining the purpose of each constructor
- Logical use of transparent colors for default states
70-81: LGTM! Well-documented blending logic.
The XML documentation clearly explains the method's purpose and the early return for caret handling is a good optimization.
143-161: LGTM! Comprehensive equality implementation.
The equality implementation follows best practices:
- Correct use of NotNullWhen attribute
- Proper implementation of GetHashCode using HashCode.Combine
- Consistent operator overloads
12-12: Verify test coverage for equality implementation.
Since IEquatable is a new addition, ensure comprehensive test coverage exists for all equality scenarios.
✅ Verification successful
Let me check the actual test coverage by examining the test files content.
Test coverage for Pixel equality implementation is comprehensive
The test file includes thorough equality testing:
- Tests for equality with both
Equals()and==operator - Tests for inequality with different symbols and colors
- Tests for
GetHashCode()implementation viaHashSetusage - Tests cover both object and strongly-typed equality scenarios
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for Pixel equality tests
echo "Searching for Pixel equality tests..."
rg -l "Pixel.*Equals|Assert.*Equal.*Pixel" --type cs
# Search for existing test files related to Pixel
echo -e "\nSearching for Pixel test files..."
fd -e cs -g "*Pixel*Test*.cs"
Length of output: 723
Script:
#!/bin/bash
# Check content of PixelTests.cs for equality tests
echo "Content of PixelTests.cs:"
cat "src/Tests/Consolonia.Core.Tests/PixelTests.cs"
Length of output: 6147
src/Consolonia.Core/Drawing/PixelBufferImplementation/DrawingBoxSymbol.cs (5)
3-3: LGTM! Good improvements to type safety and immutability.
The implementation of IEquatable<T> is appropriate for a value type, and making the field readonly helps prevent accidental modifications.
Also applies to: 11-11, 16-16
17-24: LGTM! Good improvements to immutability.
The changes properly encapsulate the Text property with a private init setter and ensure consistent initialization in the constructor.
31-38: LGTM! Good refactoring to static method.
The change to a static method with a parameter is a good improvement over using instance state. The switch expression provides clear and maintainable pattern matching.
137-160: LGTM! Complete and correct equality implementation.
The equality implementation follows best practices for value types, including all necessary methods and operators. The implementation is consistent and correct.
111-117: Consider revising the Blend logic when types don't match.
The current implementation returns symbolAbove when it's not a DrawingBoxSymbol, which might not be the desired behavior as it completely ignores the current instance. Consider if this is the intended behavior or if it should handle non-matching types differently.
✅ Verification successful
Based on the gathered information, I can now provide the final response:
The Blend logic when types don't match is consistent with the design
Looking at the implementations across different symbol types:
SimpleSymbolreturns the above symbol unless it's whitespaceDrawingBoxSymbolreturns the above symbol unless it's whitespace or can be merged with another box symbol- This pattern ensures that non-whitespace symbols from above always take precedence unless special merging rules apply
The current implementation follows a consistent pattern where non-matching types take precedence from above, which is the intended behavior for the drawing system.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for Blend method implementations in other symbol types to verify consistency
ast-grep --pattern 'interface ISymbol {
$$$
ISymbol Blend(ref ISymbol symbolAbove);
$$$
}'
ast-grep --pattern 'class $_ : ISymbol {
$$$
public ISymbol Blend(ref ISymbol symbolAbove) {
$$$
}
$$$
}'
Length of output: 218
Script:
#!/bin/bash
# Let's try a different approach to find ISymbol interface and its implementations
rg -l "ISymbol" --type cs
# Then let's look for Blend method implementations
rg "Blend.*ISymbol" -A 5 --type cs
Length of output: 2056
Script:
#!/bin/bash
# Let's look at the IsWhiteSpace implementation and any whitespace-related constants
rg "IsWhiteSpace" -A 5 --type cs
# Let's also check for any tests related to Blend behavior
rg "Blend.*Test" -A 10 --type cs
Length of output: 4018
src/Tests/Consolonia.Core.Tests/PixelTests.cs (2)
1-10: LGTM! Well-structured test class setup.
The file structure follows testing best practices with appropriate imports and namespace organization.
1-135: Overall excellent test coverage!
The test class provides comprehensive coverage of the Pixel class functionality. While there are suggestions for additional test cases, the current implementation follows testing best practices and provides good coverage of the main functionality.
src/Tests/Consolonia.Core.Tests/PixelForegroundTests.cs (2)
1-11: LGTM! Well-structured test class setup.
The file structure follows good practices with proper imports and test class organization.
1-171: Overall excellent test coverage with room for enhancement.
The test suite is well-structured and comprehensive, covering all major functionality of the PixelForeground class. The suggestions provided above would further enhance the robustness of the test suite, but the current implementation is solid and follows good testing practices.
src/Consolonia.Core/Drawing/RenderTarget.cs (3)
23-24: Good refactoring to simplify the cache structure
Changing _cache to use Pixel?[,] enhances code clarity and maintainability by directly storing pixel data instead of complex tuples.
132-138: Efficient caching mechanism improves performance
By comparing the current pixel with the cached one and skipping rendering if they are identical, you optimize performance by avoiding unnecessary writes to the console.
138-138:
Prevent potential infinite loop due to zero Symbol.Width
If pixel.Foreground.Symbol.Width is zero, the increment x += pixel.Foreground.Symbol.Width will not advance x, leading to an infinite loop. Ensure that Symbol.Width is always at least 1, or add a check to handle this case.
Apply this diff to add a safeguard:
- x += pixel.Foreground.Symbol.Width;
+ var symbolWidth = Math.Max(pixel.Foreground.Symbol.Width, 1);
+ x += symbolWidth;Likely invalid or redundant comment.
src/Consolonia.Core/Drawing/PixelBufferImplementation/PixelBufferCoordinate.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
src/Consolonia.GuiCS/Size.cs (2)
Line range hint
272-280: Consider reducing code duplication in Add/Subtract methods.The static
AddandSubtractmethods duplicate the logic of their corresponding operators. Consider refactoring to have the operators call these methods or vice versa to maintain DRY principles.Example refactor:
public static Size operator + (Size sz1, Size sz2) { - return new Size (sz1.Width + sz2.Width, - sz1.Height + sz2.Height); + return Add(sz1, sz2); } public static Size operator - (Size sz1, Size sz2) { - return new Size (sz1.Width - sz2.Width, - sz1.Height - sz2.Height); + return Subtract(sz1, sz2); }Also applies to: 282-290
Line range hint
1-290: Consider implementing IEquatable.The struct implements value equality but could benefit from implementing
IEquatable<Size>to avoid boxing in generic collections and improve performance.Example implementation:
public struct Size : IEquatable<Size> { // ... existing code ... public bool Equals(Size other) { return width == other.width && height == other.height; } public override bool Equals(object obj) { return obj is Size other && Equals(other); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
src/Consolonia.Core/Drawing/PixelBufferImplementation/Pixel.cs(4 hunks)src/Consolonia.Core/Drawing/PixelBufferImplementation/PixelBufferCoordinate.cs(2 hunks)src/Consolonia.GuiCS/Size.cs(1 hunks)src/Tests/Consolonia.Core.Tests/PixelTests.cs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/Consolonia.Core/Drawing/PixelBufferImplementation/Pixel.cs
- src/Consolonia.Core/Drawing/PixelBufferImplementation/PixelBufferCoordinate.cs
- src/Tests/Consolonia.Core.Tests/PixelTests.cs
🔇 Additional comments (1)
src/Consolonia.GuiCS/Size.cs (1)
209-209: LGTM! Improved hash code generation.
The change from bitwise XOR to HashCode.Combine is a good improvement as it:
- Provides better hash distribution
- Reduces collision probability
- Follows modern C# best practices
- Maintains consistency with similar structs in the codebase
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
src/Tests/Consolonia.Gallery.Tests/TextBlockTests.cs (2)
9-13: Enhance class documentation.Consider expanding the XML documentation to include:
- Purpose of the test suite
- Prerequisites or setup requirements
- Description of the base class relationship
Example enhancement:
- /// <summary> - /// Unit test for TextBlock view - /// </summary> + /// <summary> + /// Unit tests validating TextBlock view functionality including basic text display, + /// text trimming, alignment, multiline text, and special character handling. + /// </summary> + /// <remarks> + /// Inherits from GalleryTestsBaseBase to utilize common test infrastructure. + /// Tests require UI context initialization through the base class. + /// </remarks>
15-56: Consider extracting common setup logic.Each test method repeats the Tab key input setup. Consider extracting this to a setup method or base class helper:
+ private async Task FocusTextBlock() + { + await UITest.KeyInput(Key.Tab); + // Consider adding validation that correct control is focused + } [Test] public async Task TextBlock_DisplaysBasicText() { - await UITest.KeyInput(Key.Tab); + await FocusTextBlock(); await UITest.AssertHasText("This is TextBlock"); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/Tests/Consolonia.Gallery.Tests/TextBlockTests.cs(1 hunks)
🔇 Additional comments (1)
src/Tests/Consolonia.Gallery.Tests/TextBlockTests.cs (1)
1-6: LGTM! Imports are clean and aligned with PR objectives.
The imports correctly reference the new Consolonia.NUnit package and include all necessary dependencies.
Second can be found using keyword "test". |
Summary by CodeRabbit
New Features
DrawingBoxSymbol,Pixel,PixelBackground, andPixelForeground.Bug Fixes
StrikethroughThicknessin the drawing context.Tests
DrawingBoxSymbol,Glyphs,Pixel, andPixelForeground.Chores