Pull symbols into theme resources #290
Conversation
use consolonia namespace Only use SymbolsControl if repeating symbol is intended
Slider thumb are resources
📝 WalkthroughWalkthroughThis pull request encompasses a broad range of updates across multiple XAML and project files. The changes primarily involve updating XML namespace references from the legacy "helpers" namespace to the new "console" namespace across various control templates, thereby standardizing control references such as Suggested labels
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (26)
src/Consolonia.Themes/Templates/Controls/ComboBoxItem.axaml (1)
16-17: Consider adjusting attribute indentation for consistency.While the namespace update to 'console' is correct, the attribute indentation appears to be using 8 spaces instead of the standard 4 spaces.
- <console:CaretControl IsCaretShown="{TemplateBinding IsFocused}" - Background="Transparent"> + <console:CaretControl IsCaretShown="{TemplateBinding IsFocused}" + Background="Transparent">Also applies to: 27-27
src/Tests/Consolonia.Gallery.Tests/TooltipTests.cs (1)
13-20: Consider enhancing test robustness and maintainability.While the test logic is sound, consider the following improvements:
- Add verification that the correct control is focused after Tab key press
- Add cleanup to ensure tooltip is hidden after test
- Move the hardcoded string "A control which pops up a hint" to a constant
- Add timeout handling for async operations
Here's a suggested implementation:
+ private const string TooltipText = "A control which pops up a hint"; + [Test] public async Task PerformSingleTest() { await UITest.KeyInput(Key.Tab); + Assert.That(await UITest.GetFocusedControlName(), Is.EqualTo("TooltipTestControl")); - await UITest.AssertHasNoText("A control which pops up a hint"); + await UITest.AssertHasNoText(TooltipText); await UITest.KeyInput(Key.Space); - await UITest.AssertHasText("A control which pops up a hint"); + await UITest.AssertHasText(TooltipText); + + // Cleanup + await UITest.KeyInput(Key.Escape); + await UITest.AssertHasNoText(TooltipText); }src/Tests/Consolonia.Gallery.Tests/NumericUpDownTests.cs (2)
15-16: Add a comment explaining the magic number.The hard-coded value of 7 tab presses is not immediately clear. Consider adding a comment explaining why this specific number of tabs is needed to reach the NumericUpDown control, or better yet, implement a more robust navigation method.
18-22: Consider making the test more robust.The current implementation has several areas for improvement:
- Verify that the NumericUpDown control is actually focused before performing operations
- Consider extracting the expected text strings into constants to make updates easier
- Add test coverage for edge cases (minimum and maximum values)
Example improvement:
+ private const string MINIMUM_LABEL = "│ Minimum: "; + [Test] public async Task PerformSingleTest() { + // Navigate to the NumericUpDown control for (int i = 0; i < 7; i++) await UITest.KeyInput(Key.Tab); - await UITest.AssertHasText("│ Minimum: 0 ▴▾"); + // Verify initial state + string initialText = await UITest.GetText(); + Assert.That(initialText, Does.Contain(MINIMUM_LABEL), "NumericUpDown control not found"); + await UITest.AssertHasText($"{MINIMUM_LABEL}0 ▴▾"); + // Test increment await UITest.KeyInput(Key.Up); - await UITest.AssertHasText("│ Minimum: 1.0 ▴▾"); + await UITest.AssertHasText($"{MINIMUM_LABEL}1.0 ▴▾"); + // Test decrement await UITest.KeyInput(Key.Down); - await UITest.AssertHasText("│ Minimum: 0.0 ▴▾"); + await UITest.AssertHasText($"{MINIMUM_LABEL}0.0 ▴▾"); + + // TODO: Add tests for minimum and maximum value boundaries }src/Consolonia.Themes/Templates/Controls/ScrollViewer.axaml (2)
54-58: Consider using theme resources for symbols.While the migration from
SymbolsControltoTextBlockis correct, consider using theme resources for the "+" symbol to align with the PR's objective of standardizing symbols in theme resources.- <TextBlock Text="+" + <TextBlock Text="{DynamicResource ThemeScrollViewerCornerSymbol}" Foreground="{DynamicResource ThemeSelectionBackgroundBrush}" />
91-92: Consider using theme resources for arrow symbols.The arrow symbols should be defined in theme resources to maintain consistency with the PR's objective of standardizing symbols.
- <TextBlock Text="^" + <TextBlock Text="{DynamicResource ThemeScrollViewerUpArrowSymbol}" Foreground="{TemplateBinding Foreground}" /> - <TextBlock Text="v" + <TextBlock Text="{DynamicResource ThemeScrollViewerDownArrowSymbol}" Foreground="{TemplateBinding Foreground}" />Also applies to: 112-113
src/Tests/Consolonia.Gallery.Tests/TransitioningContentTests.cs (1)
12-22: Improve test reliability and maintainability.Several improvements could make this test more robust and maintainable:
- Remove or document the commented-out assertion.
- Consider extracting the animation delay to a constant or configuration.
- Document the expected navigation path for the Tab key presses.
Apply this diff to improve the test:
+ private const int AnimationCompletionDelay = 500; + [Test] public async Task PerformSingleTest() { await UITest.AssertHasText("Lorem ipsum"); - // await UITest.AssertHasNoText("Duis aute irure"); + // Navigate to the transition trigger button await UITest.KeyInput(Key.Tab); await UITest.KeyInput(Key.Tab); await UITest.KeyInput(Key.Tab); await UITest.KeyInput(Key.Tab); await UITest.KeyInput(Key.Space); - await Task.Delay(500); // Wait for the animation to finish + await Task.Delay(AnimationCompletionDelay); // Wait for the animation to finishsrc/Tests/Consolonia.Gallery.Tests/SliderTests.cs (2)
13-13: Improve test method name to reflect its purpose.The current name
PerformSingleTestis too generic. Consider renaming it to better describe what aspect of the slider it's testing, e.g.,TestSliderKeyboardNavigationAndVisualFeedback.
14-21: Enhance test coverage and documentation.Consider the following improvements:
- Add XML documentation to describe the test's purpose and expectations
- Add tests for:
- Left key navigation
- Edge cases (minimum and maximum values)
- Other keyboard inputs (e.g., Home, End keys)
Here's a suggested improvement:
+ /// <summary> + /// Verifies that the slider responds correctly to keyboard navigation + /// and updates its visual representation accordingly. + /// </summary> [Test] - public async Task PerformSingleTest() + public async Task TestSliderKeyboardNavigationAndVisualFeedback() { await UITest.KeyInput(Key.Tab); await UITest.KeyInput(Key.Tab); await UITest.AssertHasText(" ◯─────"); await UITest.KeyInput(Key.Right); await UITest.KeyInput(Key.Right); await UITest.AssertHasText(" ─◯────"); + await UITest.KeyInput(Key.Left); + await UITest.AssertHasText(" ◯─────"); } + + [Test] + public async Task TestSliderKeyboardNavigationEdgeCases() + { + await UITest.KeyInput(Key.Tab); + await UITest.KeyInput(Key.Tab); + await UITest.KeyInput(Key.Home); + await UITest.AssertHasText(" ◯─────"); + await UITest.KeyInput(Key.End); + await UITest.AssertHasText(" ─────◯"); + }src/Tests/Consolonia.Gallery.Tests/NotificationsTests.cs (2)
18-21: Consider making the test more robust by avoiding hard-coded delays.The test uses fixed delays (500ms and 6000ms) which could be flaky in CI environments. Consider:
- Using a notification service mock to control timing
- Adding retry logic with timeout for assertions
- Making delays configurable via test settings
Example implementation with retry logic:
- await Task.Delay(500); - await UITest.AssertHasText("It's"); - await Task.Delay(6000); - await UITest.AssertHasNoText("It's"); + // Retry assertion with timeout + await UITest.RetryUntilAsync( + async () => await UITest.AssertHasText("It's"), + timeout: TimeSpan.FromSeconds(1) + ); + + // Retry assertion with timeout + await UITest.RetryUntilAsync( + async () => await UITest.AssertHasNoText("It's"), + timeout: TimeSpan.FromSeconds(7) + );
12-22: Add test cases for different notification scenarios.The current test only verifies a single notification scenario. Consider adding test cases for:
- Different notification types (info, warning, error)
- Different notification durations
- Multiple concurrent notifications
src/Tests/Consolonia.Gallery.Tests/ButtonSpinnerTests.cs (2)
15-18: Consider reducing repetitive Tab key inputs.The test uses three consecutive Tab key inputs which could be simplified.
- await UITest.KeyInput(Key.Tab); - await UITest.KeyInput(Key.Tab); - await UITest.KeyInput(Key.Tab); + for (int i = 0; i < 3; i++) + await UITest.KeyInput(Key.Tab);
12-25: Add test cases for edge cases and rapid input.The current test only verifies basic navigation. Consider adding test cases for:
- Boundary behavior (first/last item)
- Rapid key presses
- Visual state verification
src/Tests/Consolonia.Gallery.Tests/ToggleSwitchTests.cs (2)
20-20: Consider making animation delays configurable.The test uses fixed delays (200ms) for animations which could be flaky in CI environments.
+ private const int AnimationDelay = 200; + [Test] public async Task PerformSingleTest() { // ... test code ... - await Task.Delay(200); // Wait for the animation to finish + await Task.Delay(AnimationDelay); // Wait for the animation to finishAlso applies to: 23-23
12-25: Add test cases for keyboard navigation and focus handling.The current test only verifies basic toggle functionality. Consider adding test cases for:
- Keyboard navigation (Tab/Shift+Tab)
- Focus indicators
- Disabled state
src/Tests/Consolonia.Gallery.Tests/CarouselTests.cs (1)
22-22: Extract animation delay to a constant.The magic number
200for animation delay should be extracted to a constant with a descriptive name for better maintainability.+ private const int CarouselAnimationDelayMs = 200; public async Task PerformSingleTest() { - await Task.Delay(200); // Wait for the animation to finish + await Task.Delay(CarouselAnimationDelayMs); // Wait for the animation to finishsrc/Tests/Consolonia.Gallery.Tests/FlyoutTests.cs (1)
17-17: Use regex pattern for more resilient text matching.The hardcoded spaces in the text assertion
"Item 1 ▸"make the test fragile. Consider using a regex pattern to make the test more resilient to spacing changes.- await UITest.AssertHasText(@"Item 1 ▸", + await UITest.AssertHasText(@"Item 1\s+▸",src/Tests/Consolonia.Gallery.Tests/CheckBoxTests.cs (1)
27-27: Extract loop count to a constant and improve readability.The magic number
6in the loop and the purpose of the loop could be made more explicit.+ private const int CheckboxesToToggle = 6; + public async Task PerformSingleTest() { - for (int i = 0; i < 6; i++) await UITest.KeyInput(Key.Space, Key.Tab); + // Toggle state of first 6 checkboxes + for (int i = 0; i < CheckboxesToToggle; i++) + await UITest.KeyInput(Key.Space, Key.Tab);src/Tests/Consolonia.Gallery.Tests/TreeViewTests.cs (2)
13-13: Rename test method to better reflect its purpose.The current name
PerformSingleTestis too generic. Consider renaming it to describe the specific scenario being tested, e.g.,TreeViewExpandCollapseTest.- public async Task PerformSingleTest() + public async Task TreeViewExpandCollapseTest()
15-32: Consider adding more test cases.The current test only verifies basic expand/collapse functionality. Consider adding tests for:
- Multiple levels of nesting
- Keyboard navigation between items
- Selection behavior
- Edge cases (empty items, deeply nested items)
src/Consolonia.Themes/TurboVision/TurboVisionTheme.axaml (1)
34-40: Ensure consistent spacing in radio button and checkbox symbols.The radio button and checkbox symbols include spaces
( ),(*),[ ],[x]which differ from the Material theme's approach. Consider using single-character symbols for consistency and to prevent potential spacing issues in different terminal widths.- <x:String x:Key="ThemeRadioButtonNotSelectedSymbol">( )</x:String> - <x:String x:Key="ThemeRadioButtonSelectedSymbol">(*)</x:String> - <x:String x:Key="ThemeRadioButtonIndeterminateSymbol">(■)</x:String> + <x:String x:Key="ThemeRadioButtonNotSelectedSymbol">○</x:String> + <x:String x:Key="ThemeRadioButtonSelectedSymbol">●</x:String> + <x:String x:Key="ThemeRadioButtonIndeterminateSymbol">◑</x:String> - <x:String x:Key="ThemeCheckBoxNotSelectedSymbol">[ ]</x:String> - <x:String x:Key="ThemeCheckBoxSelectedSymbol">[x]</x:String> - <x:String x:Key="ThemeCheckBoxIndeterminateSymbol">[■]</x:String> + <x:String x:Key="ThemeCheckBoxNotSelectedSymbol">☐</x:String> + <x:String x:Key="ThemeCheckBoxSelectedSymbol">☒</x:String> + <x:String x:Key="ThemeCheckBoxIndeterminateSymbol">▣</x:String>src/Consolonia.Themes/Material/MaterialTheme.axaml (1)
10-45: LGTM! Well-chosen symbols that align with Material Design principles.The chosen symbols use widely supported Unicode geometric shapes that maintain good visual consistency. Consider using this theme's approach as a reference for standardizing symbols across other themes.
Key benefits of this approach:
- Uses simple geometric shapes (◂, ▸, ▴, ▾, ◯, ⬤) that are widely supported
- Maintains consistent visual weight across different symbols
- Follows Material Design's minimalist aesthetic
src/Tests/Consolonia.Gallery.Tests/ExpanderTests.cs (1)
17-17: Extract Unicode arrow characters into constants.The test is using hardcoded Unicode arrow characters in assertions. Consider extracting these into constants or using the theme resources to make the tests more maintainable.
+private const string UP_ARROW = "▴"; +private const string DOWN_ARROW = "▾"; +private const string LEFT_ARROW = "◂"; +private const string RIGHT_ARROW = "▸"; -await UITest.AssertHasText(" Up ▴ ", "Down ▾", "◂Left", "Right▸"); +await UITest.AssertHasText($" Up {UP_ARROW} ", $"Down {DOWN_ARROW}", $"{LEFT_ARROW}Left", $"Right{RIGHT_ARROW}");Also applies to: 25-25, 31-31, 38-38, 46-46, 63-63
src/Consolonia.Themes/Templates/Controls/CheckBox.axaml (1)
53-76: Consider consolidating disabled styles.The commented-out styles for disabled state could be consolidated with the active disabled style at the bottom of the file for better maintainability.
src/Consolonia.Themes/Templates/Controls/ToggleSwitch.axaml (1)
41-41: Remove unnecessary commented-out tag.The commented-out closing tag
<!--</console:CaretControl>-->appears to be unnecessary and could cause confusion.-<!--</console:CaretControl>-->src/Consolonia.Themes/Templates/Controls/DataGrid.axaml (1)
168-171: Clean up commented-out code blocks.There are multiple blocks of commented-out code throughout the file. Consider removing them to improve code readability and maintenance.
Also applies to: 185-188, 258-270, 298-302, 326-334, 336-345, 359-363, 377-381, 389-392
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (56)
src/Consolonia.Gallery/Gallery/GalleryViews/GalleryMenu.axaml(1 hunks)src/Consolonia.Gallery/Gallery/GalleryViews/GalleryTooltip.axaml(0 hunks)src/Consolonia.Themes/Consolonia.Themes.csproj(1 hunks)src/Consolonia.Themes/Fluent/FluentTheme.axaml(1 hunks)src/Consolonia.Themes/Material/MaterialTheme.axaml(1 hunks)src/Consolonia.Themes/Templates/Controls/Button.axaml(4 hunks)src/Consolonia.Themes/Templates/Controls/ButtonSpinner.axaml(1 hunks)src/Consolonia.Themes/Templates/Controls/Calendar.axaml(1 hunks)src/Consolonia.Themes/Templates/Controls/CalendarDatePicker.axaml(3 hunks)src/Consolonia.Themes/Templates/Controls/CalendarItem.axaml(2 hunks)src/Consolonia.Themes/Templates/Controls/CaretControl.axaml(1 hunks)src/Consolonia.Themes/Templates/Controls/CheckBox.axaml(3 hunks)src/Consolonia.Themes/Templates/Controls/ComboBox.axaml(4 hunks)src/Consolonia.Themes/Templates/Controls/ComboBoxItem.axaml(3 hunks)src/Consolonia.Themes/Templates/Controls/DataGrid.axaml(6 hunks)src/Consolonia.Themes/Templates/Controls/DialogWindow.axaml(5 hunks)src/Consolonia.Themes/Templates/Controls/DropDownButton.axaml(1 hunks)src/Consolonia.Themes/Templates/Controls/Expander.axaml(4 hunks)src/Consolonia.Themes/Templates/Controls/Helpers/LineSeparator.cs(1 hunks)src/Consolonia.Themes/Templates/Controls/ListBox.axaml(2 hunks)src/Consolonia.Themes/Templates/Controls/ListBoxItem.axaml(3 hunks)src/Consolonia.Themes/Templates/Controls/Menu.axaml(4 hunks)src/Consolonia.Themes/Templates/Controls/MenuFlyoutPresenter.axaml(2 hunks)src/Consolonia.Themes/Templates/Controls/MenuItem.axaml(7 hunks)src/Consolonia.Themes/Templates/Controls/NumericUpDown.axaml(1 hunks)src/Consolonia.Themes/Templates/Controls/ProgressBar.axaml(2 hunks)src/Consolonia.Themes/Templates/Controls/RadioButton.axaml(3 hunks)src/Consolonia.Themes/Templates/Controls/ScrollBar.axaml(7 hunks)src/Consolonia.Themes/Templates/Controls/ScrollViewer.axaml(5 hunks)src/Consolonia.Themes/Templates/Controls/SelectableTextBlock.axaml(1 hunks)src/Consolonia.Themes/Templates/Controls/Slider.axaml(4 hunks)src/Consolonia.Themes/Templates/Controls/TabItem.axaml(2 hunks)src/Consolonia.Themes/Templates/Controls/TextBox.axaml(4 hunks)src/Consolonia.Themes/Templates/Controls/ToggleButton.axaml(3 hunks)src/Consolonia.Themes/Templates/Controls/ToggleSwitch.axaml(3 hunks)src/Consolonia.Themes/Templates/Controls/TreeView.axaml(2 hunks)src/Consolonia.Themes/Templates/Controls/TreeViewItem.axaml(4 hunks)src/Consolonia.Themes/Templates/TurboVisionTheme.axaml(0 hunks)src/Consolonia.Themes/TurboVision/TurboVisionTheme.axaml(1 hunks)src/Consolonia.Themes/TurboVisionBlack/Controls/Button.axaml(1 hunks)src/Consolonia.Themes/TurboVisionBlack/TurboVisionBlackTheme.axaml(2 hunks)src/Consolonia.Themes/TurboVisionDark/Controls/DialogWindow.axaml(1 hunks)src/Consolonia.Themes/TurboVisionDark/TurboVisionDarkTheme.axaml(2 hunks)src/Tests/Consolonia.Gallery.Tests/ButtonSpinnerTests.cs(1 hunks)src/Tests/Consolonia.Gallery.Tests/CarouselTests.cs(1 hunks)src/Tests/Consolonia.Gallery.Tests/CheckBoxTests.cs(1 hunks)src/Tests/Consolonia.Gallery.Tests/ExpanderTests.cs(1 hunks)src/Tests/Consolonia.Gallery.Tests/FlyoutTests.cs(1 hunks)src/Tests/Consolonia.Gallery.Tests/NotificationsTests.cs(1 hunks)src/Tests/Consolonia.Gallery.Tests/NumericUpDownTests.cs(1 hunks)src/Tests/Consolonia.Gallery.Tests/RadioButtonTests.cs(1 hunks)src/Tests/Consolonia.Gallery.Tests/SliderTests.cs(1 hunks)src/Tests/Consolonia.Gallery.Tests/ToggleSwitchTests.cs(1 hunks)src/Tests/Consolonia.Gallery.Tests/TooltipTests.cs(1 hunks)src/Tests/Consolonia.Gallery.Tests/TransitioningContentTests.cs(1 hunks)src/Tests/Consolonia.Gallery.Tests/TreeViewTests.cs(1 hunks)
💤 Files with no reviewable changes (2)
- src/Consolonia.Gallery/Gallery/GalleryViews/GalleryTooltip.axaml
- src/Consolonia.Themes/Templates/TurboVisionTheme.axaml
✅ Files skipped from review due to trivial changes (2)
- src/Consolonia.Themes/Consolonia.Themes.csproj
- src/Consolonia.Themes/Templates/Controls/Helpers/LineSeparator.cs
🧰 Additional context used
📓 Learnings (3)
src/Consolonia.Themes/Templates/Controls/SelectableTextBlock.axaml (1)
Learnt from: jinek
PR: jinek/Consolonia#219
File: src/Consolonia.Themes/Templates/Controls/Helpers/SelectTextWithPointerUpExtension.cs:21-25
Timestamp: 2024-12-19T21:20:43.248Z
Learning: When mouseMove is supported by IConsole, the SelectableTextBlock handles selection normally without needing an additional subscription for right-click pointer up events in SelectTextWithPointerUpExtension.
src/Consolonia.Themes/Templates/Controls/Menu.axaml (1)
Learnt from: jinek
PR: jinek/Consolonia#176
File: src/Consolonia.Themes.TurboVision/Templates/Controls/MenuItem.axaml:131-131
Timestamp: 2024-12-05T18:23:58.201Z
Learning: In the Avalonia UI project, within `src/Consolonia.Themes.TurboVision/Templates/Controls/MenuItem.axaml`, MenuItem controls are intentionally designed without a `:pointerover` state to maintain touch-friendliness and consistency across controls.
src/Consolonia.Themes/TurboVision/TurboVisionTheme.axaml (1)
Learnt from: tomlm
PR: jinek/Consolonia#122
File: src/Consolonia.Themes.TurboVision/Themes/TurboVisionDark/TurboVisionDarkColors.axaml:5-28
Timestamp: 2024-11-12T04:11:38.190Z
Learning: In the project, using `Mode="Shaded"` in `ConsoleBrush` within theme files is intentional and does not cause runtime issues.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (87)
src/Consolonia.Themes/Templates/Controls/TabItem.axaml (2)
3-3: LGTM! Namespace declaration is consistent with project-wide changes.The update from 'helpers' to 'console' namespace aligns with the broader effort to standardize control references across the codebase.
23-30: LGTM! CaretControl reference updated correctly.The CaretControl element has been properly updated to use the new 'console' namespace while maintaining all its original properties and child elements.
src/Consolonia.Themes/TurboVisionDark/Controls/DialogWindow.axaml (2)
3-3: LGTM! Namespace update aligns with standardization efforts.The transition to the new
consolenamespace aligns with the PR's objective of standardizing theme resources.
5-10: LGTM! Control theme definition is well-structured.The control theme is properly defined with:
- Correct namespace usage
- Appropriate base theme inheritance
- Consistent dynamic resource usage for styling
src/Consolonia.Themes/Templates/Controls/ListBoxItem.axaml (2)
3-3: LGTM! Namespace update aligns with standardization efforts.The change from the local assembly-based namespace to a standardized GitHub URL-based namespace is consistent with the broader namespace reorganization across theme files.
13-14: LGTM! CaretControl reference updated consistently.The CaretControl element has been updated to use the new 'console' namespace prefix while maintaining all existing properties and template bindings.
Also applies to: 24-24
src/Consolonia.Themes/Templates/Controls/ComboBoxItem.axaml (1)
3-3: LGTM! Namespace update aligns with standardization effort.The namespace change from 'helpers' to 'console' is consistent with the broader effort to standardize control references.
src/Consolonia.Themes/Templates/Controls/MenuItem.axaml (5)
3-3: LGTM! Namespace update is consistent with PR objectives.The namespace change from
helperstoconsolealigns with the standardization efforts across the codebase.
21-21: LGTM! CaretControl namespace update is correct.The CaretControl element has been properly updated to use the new console namespace while maintaining its functionality.
Also applies to: 89-89
38-44: LGTM! Check symbol now uses theme resource.The change from hardcoded symbol to theme resource improves customization capabilities and aligns with the PR's goal of standardizing symbols.
55-60: LGTM! Right arrow symbol now uses theme resource.The change from SymbolsControl to TextBlock with theme resource improves consistency and customization capabilities.
79-79: LGTM! MenuExtensions namespace update is correct.The MenuExtensions property has been properly updated to use the new console namespace.
src/Tests/Consolonia.Gallery.Tests/TooltipTests.cs (2)
1-6: LGTM!All necessary imports are present and well-organized.
9-11: LGTM!The test class is properly structured with appropriate test fixture attribute and inheritance.
src/Consolonia.Themes/Templates/Controls/DialogWindow.axaml (6)
3-3: LGTM! Namespace declaration follows standard conventions.The new 'console' namespace declaration aligns with the PR objectives of standardizing symbol resources.
7-7: LGTM! Style selector and template type are consistently updated.The namespace changes are applied consistently to both the style selector and control template target type.
Also applies to: 22-22
50-50: LGTM! SymbolsControl namespace update is correct.The namespace update maintains all existing bindings and properties.
63-63: LGTM! Close button now uses themed symbol resource.The change properly implements the PR objective of using theme resources for symbols.
71-71: LGTM! ButtonExtensions namespace update is correct.The namespace update maintains the existing shadow property functionality.
81-81: LGTM! LineBrush namespace update is correct.The namespace update maintains the existing brush configuration.
src/Tests/Consolonia.Gallery.Tests/NumericUpDownTests.cs (2)
1-8: LGTM! Imports and namespace are well-organized.All necessary dependencies are properly imported, and the namespace aligns with the project structure.
9-11: LGTM! Class declaration follows testing conventions.The class is properly decorated with TestFixture and inherits from the appropriate base class.
src/Consolonia.Themes/Templates/Controls/ScrollViewer.axaml (3)
4-4: LGTM: Namespace declaration follows standard conventions.The namespace declaration for
consolefollows XAML namespace conventions and aligns with the PR objective of standardizing symbol representation.
7-8: LGTM: Property setters consistently updated to use the new namespace.The ScrollViewerExtensions properties have been correctly migrated from the
helpersto theconsolenamespace while maintaining their original values.Also applies to: 13-14
126-129: LGTM: Style selector correctly updated.The style selector has been properly updated to target TextBlock elements while maintaining the same styling behavior.
src/Consolonia.Themes/Templates/Controls/DropDownButton.axaml (1)
47-47: LGTM! Good move to use theme resource instead of hardcoded symbol.The change from hardcoded "▼" to
{StaticResource ThemeDownArrowSymbol}aligns with the PR's objective of standardizing symbols in theme resources. This improves maintainability and theme consistency.Let's verify that the resource is defined in all themes:
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Check if ThemeDownArrowSymbol is defined in all theme files # Search for ThemeDownArrowSymbol in theme files echo "Checking theme files for ThemeDownArrowSymbol definition:" rg "ThemeDownArrowSymbol" "src/Consolonia.Themes/"Length of output: 1522
LGTM – ThemeDownArrowSymbol is consistently defined across theme files.
The verification confirms that the ThemeDownArrowSymbol resource is present in all relevant theme files (MaterialTheme.axaml, FluentTheme.axaml, and TurboVisionTheme.axaml) and is correctly referenced in DropDownButton.axaml.src/Tests/Consolonia.Gallery.Tests/TransitioningContentTests.cs (1)
1-10: LGTM! Class structure follows testing best practices.The class is properly structured with appropriate test attributes and access modifiers.
src/Consolonia.Themes/Templates/Controls/MenuFlyoutPresenter.axaml (2)
3-3: LGTM! Namespace declaration is consistent with the standardization effort.The namespace declaration follows the correct format and aligns with the broader effort to transition from "helpers" to "console" namespace.
39-39: Verify the MenuExtensions.FocusOnLoad property exists in the new namespace.The property reference has been updated to use the new "console" namespace. Please ensure that the MenuExtensions type and FocusOnLoad attached property are properly defined in the new namespace.
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Verify the MenuExtensions type and FocusOnLoad property exist in the new namespace. # Search for MenuExtensions type definition ast-grep --pattern $'class MenuExtensions { $$$ public static readonly AttachedProperty<bool> FocusOnLoad $$$ }' # Search for any references to ensure it's being used consistently rg -A 2 'MenuExtensions\.FocusOnLoad'Length of output: 1389
I'm not completely convinced by the initial ast-grep search results. While the XAML files clearly reference (console:MenuExtensions.FocusOnLoad) with the updated namespace—and the change appears to be consistently applied across multiple files—we haven't yet verified that the corresponding attached property is actually defined in the new namespace. To be sure the new definition exists, please run the following script to search for the MenuExtensions type and related attached property definitions in the codebase (including potential alternative naming such as FocusOnLoadProperty):
🏁 Script executed:
#!/bin/bash # Search for the definition of the MenuExtensions class in C# files. rg -n "class MenuExtensions" --type cs # Additionally, search for any potential definitions of the FocusOnLoad attached property # (sometimes defined with a "Property" suffix) in C# files. rg -n "FocusOnLoadProperty" --type csLength of output: 469
MenuExtensions and its FocusOnLoad attached property have been verified.
Our searches confirm that the MenuExtensions class now exists under the new namespace and defines the attached property as "FocusOnLoadProperty." The XAML files correctly reference this property as "FocusOnLoad" (the standard convention for attached properties), and its usage is consistent across files, ensuring the proper namespace transition.
src/Consolonia.Themes/Templates/Controls/Menu.axaml (4)
3-3: LGTM: Namespace update aligns with standardization goals.The change from local
helpersnamespace to the URL-basedconsolenamespace improves standardization across theme resources.
19-23: LGTM: CaretControl namespace updated correctly.The CaretControl element has been properly updated to use the new namespace while maintaining its existing functionality and bindings.
41-42: LGTM: MenuExtensions.FocusOnLoad namespace updated correctly.The attached property has been properly updated to use the new namespace while maintaining its focus behavior functionality.
70-70: Verify the TextBlock symbol binding.The selector has been changed from a custom SymbolsControl to a standard TextBlock. While this aligns with the PR's goal of moving symbols to theme resources, please verify that the TextBlock has the correct symbol resource binding.
Run this script to check the TextBlock's symbol binding:
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Check if the rightArrow TextBlock has a symbol resource binding # and verify its consistency across theme files # Search for rightArrow TextBlock definitions and their Text bindings rg -A 5 'TextBlock.*?#rightArrow' # Search for any remaining SymbolsControl usage that might need updating rg 'SymbolsControl'Length of output: 4535
Confirmed: The RightArrow Binding Is Set Through Theme Resources
The grep results show that in both Menu.axaml and MenuItem.axaml the custom SymbolsControl has been replaced by a TextBlock (with the selector "TextBlock#rightArrow")—and the empty state is handled by setting IsVisible to False as intended. There’s no evidence of an incorrect or missing binding; the symbol is now expected to derive its value from the theme’s resource definitions. The remaining SymbolsControl instances in other controls appear intentional.
src/Consolonia.Themes/Templates/Controls/SelectableTextBlock.axaml (1)
3-3: LGTM! Namespace update is consistent with theme resources refactoring.The namespace change from
helperstoconsoleand corresponding property setter update align with the PR objectives of pulling symbols into theme resources.Also applies to: 8-8
src/Consolonia.Themes/TurboVisionBlack/Controls/Button.axaml (1)
3-3: LGTM! Namespace update is consistent with theme resources refactoring.The namespace change from
helperstoconsoleand corresponding ButtonExtensions.Shadow property setter update align with the PR objectives.Also applies to: 11-11
src/Consolonia.Themes/TurboVisionBlack/TurboVisionBlackTheme.axaml (2)
12-13: LGTM! StyleInclude update aligns with theme resources refactoring.The update to use TurboVisionTheme.axaml is consistent with the PR objectives of pulling symbols into theme resources.
3-3: Verify if the System namespace is needed.The
systemnamespace is added but doesn't appear to be used in the visible code.✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Check if the System namespace is used in the file. rg -l "system:" src/Consolonia.Themes/TurboVisionBlack/TurboVisionBlackTheme.axamlLength of output: 82
I'm going to run a more detailed shell script to check both the namespace declaration and if any elements prefixed with "system:" are used in the file. This will help us determine whether the "System" namespace is actually used beyond its declaration.
🏁 Script executed:
#!/bin/bash # Check the axaml file for both the namespace declaration and any usage of elements with the "system:" prefix. echo "Checking for System namespace declaration:" rg -n "xmlns:system=" src/Consolonia.Themes/TurboVisionBlack/TurboVisionBlackTheme.axaml echo "Checking for usage of <system: tags:" rg -n "<system:" src/Consolonia.Themes/TurboVisionBlack/TurboVisionBlackTheme.axamlLength of output: 390
System namespace appears unused in TurboVisionBlackTheme.axaml
Our search confirms that the only occurrence of "system:" in this file is in the namespace declaration (line 3). No elements or bindings prefixed with "system:" were found in this file, which indicates that this namespace isn't actively used. If there are no indirect dependencies (e.g., usage in styles or templates referenced elsewhere that require it), the namespace declaration can safely be removed.
src/Consolonia.Themes/TurboVisionDark/TurboVisionDarkTheme.axaml (2)
3-3: LGTM! System namespace added for theme resources.The addition of the System namespace aligns with the PR objectives for symbol resources.
13-13: Verify the new TurboVisionTheme path.The StyleInclude source has been updated to use the new theme path.
Run this script to verify the theme file exists:
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Verify the TurboVisionTheme.axaml file exists fd -t f "TurboVisionTheme.axaml" "src/Consolonia.Themes/TurboVision"Length of output: 123
TurboVisionTheme file found at the new path
- The file exists in "src/Consolonia.Themes/TurboVision/TurboVisionTheme.axaml", confirming that the new theme path update is correct.
src/Tests/Consolonia.Gallery.Tests/RadioButtonTests.cs (1)
26-26: Investigate and fix Space key issue.The TODO comment indicates that the Space key doesn't work as expected. This should be investigated and fixed, as Space is a standard key for interacting with radio buttons.
Let me help investigate the Space key issue. I can:
- Generate a test to isolate and reproduce the issue
- Open a new issue to track this bug
Would you like me to proceed with either option?src/Consolonia.Themes/Templates/Controls/Calendar.axaml (1)
3-3: LGTM!The namespace change from
helperstoconsoleand the corresponding property setter update are consistent with the broader refactoring effort.Also applies to: 6-6
src/Consolonia.Themes/Templates/Controls/CaretControl.axaml (1)
3-3: LGTM!The namespace change and corresponding updates to ControlTheme key and TargetType are consistent with the broader refactoring effort.
Also applies to: 5-6
src/Consolonia.Themes/Templates/Controls/TreeView.axaml (1)
3-3: LGTM!The namespace change and corresponding update to the ScrollViewerExtensions property setter are consistent with the broader refactoring effort.
Also applies to: 33-33
src/Consolonia.Themes/Templates/Controls/ListBox.axaml (1)
3-3: LGTM! Namespace changes look good.The namespace update from
helperstoconsolealigns with the broader refactoring effort to standardize control references.Also applies to: 29-29
src/Consolonia.Themes/TurboVision/TurboVisionTheme.axaml (1)
10-13: Consider using consistent arrow symbols across themes.The arrow symbols (←, →, ↑, ↓) differ from those used in the Fluent and Material themes. This might affect visual consistency when switching themes.
✅ Verification successful
🌐 Web query:
Are the arrow symbols ←, →, ↑, ↓ widely supported across different terminal emulators and operating systems?💡 Result:
Arrow symbols (←, →, ↑, ↓) have become **widely supported** in modern terminal emulators and operating systems, but compatibility depends on several factors: ## Core Support Status - **Modern terminals** like iTerm2 (macOS), Windows Terminal, and GNOME Terminal fully support these symbols[2][6] - **Linux consoles** require Unicode-aware configurations and proper font support[5][7] - **Legacy systems** (e.g., basic Linux TTY) may display replacement characters or boxes[2][7] ## Key Compatibility Factors 1. **Font Requirements**: Terminals need fonts containing these Unicode glyphs (most modern fonts do)[2][7] 2. **Terminal Emulator Capabilities**: ```markdown | Emulator | Unicode Version | Arrow Support | |----------------|-----------------|---------------| | Windows Terminal | 15.0 | Full | | iTerm2 | 15.0 | Full | | xterm | 9.0 | Partial* | ``` *Requires `-u8` option for full Unicode[7] 3. **Environment Variables**: - `LANG`/`LC_ALL` must be set to UTF-8 locales - `TERM` should indicate Unicode-capable terminal type[3] ## Implementation Best Practices 1. Use **fallback mechanisms**: ```javascript const arrow = process.platform === 'win32' ? '->' : '→'; // Platform-specific fallback[2] ``` 2. Combine with ANSI escape codes for cursor movement where possible[1][4] 3. Test rendering with: ```bash echo -e "← → ↑ ↓" ``` While modern development environments (2025) largely handle these symbols well, mission-critical CLI tools should still implement: - ASCII fallbacks (`<-`, `->`, `^^`, `vv`) - Feature detection routines[2] - Font stack configurations specifying Unicode-aware typefaces[7] The exceptions occur primarily in constrained environments like: - Legacy embedded systems - Non-Unicode SSH sessions - Older Windows PowerShell configurations (pre-5.1)[2][3] Citations: - 1: https://www.youtube.com/watch?v=FmBi0FQbxMU - 2: https://app.studyraid.com/en/read/12628/409780/unicode-symbols-usage - 3: https://documentation.softwareag.com/natural/nat914win/spod/spod-terminal.htm - 4: https://github.com/WebAssembly/WASI/issues/163 - 5: https://www.linux.org/threads/arrow-keys-in-a-completely-text-mode-or-terminal.27843/ - 6: https://en.wikipedia.org/wiki/Arrow_keys - 7: https://github.com/powerline/powerline/issues/96 - 8: https://gnu.emacs.help.narkive.com/ps1gklKm/arrow-keys-in-the-console
Arrow Symbol Support Verified – Consider Design Consistency
The arrow symbols defined in TurboVisionTheme.axaml (←, →, ↑, ↓) are widely supported across modern terminal emulators and operating systems. The web query confirms that these symbols render correctly on modern setups (e.g., Windows Terminal, iTerm2, GNOME Terminal) provided the locale and font configurations are correctly set. That said, as the Fluent and Material themes use different arrow symbol styles, you might consider aligning them if visual consistency across themes is a priority.
- The symbols are technically safe for modern environments.
- If design consistency is desired, update the TurboVisionTheme to match the arrow symbols used in the Fluent and Material themes.
src/Consolonia.Themes/Fluent/FluentTheme.axaml (1)
27-31: Verify support for complex Unicode characters.The slider thumb symbols (🠷, 🠴) are using Unicode characters from the Supplemental Arrows-C block. These might not be widely supported across different terminal emulators.
✅ Verification successful
🌐 Web query:
Are the Unicode characters 🠷 (U+1F837) and 🠴 (U+1F834) widely supported in terminal emulators?💡 Result:
Support for Unicode characters 🠷 (U+1F837 RIGHTWARDS FINGER-POST ARROW) and 🠴 (U+1F834 LEFTWARDS FINGER-POST ARROW) in terminal emulators depends on several factors: --- ### **Unicode Version Support** - Both characters belong to **Supplemental Arrows-C** (Unicode block), introduced in **Unicode 7.0** (2014)[7]. Most modern terminals support at least Unicode 12–15[3], so these characters should theoretically be included. - However, terminals like **Visual Studio Code** (based on Electron) only support up to **Unicode 12.1** (2019)[3], which still includes these arrows. --- ### **Font Requirements** - Terminals require fonts containing glyphs for these specific code points: - U+1F834: Included in fonts like **Segoe UI Symbol** or **Noto Sans Symbols**[4]. - U+1F837: Less commonly supported; may require specialized symbol fonts. - Many Linux distributions prioritize readability over symbols in default fonts[9][10], leading to missing glyphs (displayed as rectangles or diamonds). --- ### **Terminal Emulator Behavior** - **GUI terminals** (e.g., Konsole, GNOME Terminal) generally handle these characters better than **text consoles**[6][9]. For example: - Raspberry Pi’s text console failed to display U+265E (chess knight) due to font limitations[9]. - GUI terminals like XFCE’s emulator resolved such issues[6]. - Testing revealed only **7 out of 23 terminals** fully supported advanced features like VS-16 variation sequences[3], though basic arrow rendering is more common. --- ### **Examples of Limitations** - Older systems or misconfigured locales (e.g., missing `en_US.UTF-8`) may fail to render these characters[6][9]. - Even modern tools like **Alacritty** or **Kitty** might omit rarely used symbols unless paired with updated fonts[3]. --- ### Recommendations 1. Use a **GUI terminal emulator** with a font like *Noto Sans Symbols* or *DejaVu Sans*. 2. Verify locale settings (`LANG=en_US.UTF-8`)[6][9]. 3. Test rendering directly using tools like `unicode -d 1F834`[4]. In summary, while these arrows are part of long-standing Unicode standards, consistent display requires updated terminals, fonts, and configurations[3][6][9]. Citations: - 1: https://learn.microsoft.com/en-us/globalization/encoding/unicode-standard - 2: https://regex101.com/r/oyp9Fk/3/codegen?language=autoit - 3: https://www.jeffquast.com/post/ucs-detect-test-results/ - 4: https://www.compart.com/en/unicode/U+1F834 - 5: https://regex101.com/r/oyp9Fk/3/codegen?language=swift5_2 - 6: https://www.linuxquestions.org/questions/linux-desktop-74/terminal-emulator-and-unicode-character-graphics-4175693473/ - 7: https://en.wikipedia.org/wiki/List_of_Unicode_characters - 8: https://www.sql-und-xml.de/unicode-database/so.html - 9: https://forums.raspberrypi.com/viewtopic.php?t=102373 - 10: https://www.emerson.com/documents/commercial-residential/professional-thermostat-comparison-chart-en-us-187204.pdf
Complex Unicode symbols are broadly supported, but font and configuration dependencies remain.
The web query indicates that most modern terminal emulators—especially those running Unicode 12+—will render these Supplemental Arrows-C characters (🠷 and 🠴) correctly when paired with appropriate fonts (e.g., Segoe UI Symbol or Noto Sans Symbols). However, environments using older terminal emulators or systems with default fonts lacking these glyphs may display fallback characters (like boxes or diamonds). It’s advisable to verify that the target deployment environments include the required Unicode support and fonts, or consider providing fallback symbols.
src/Consolonia.Themes/Templates/Controls/NumericUpDown.axaml (1)
7-13: Verify the visual appearance with the new brush resources.The changes to brush resources and border thickness might affect the visual appearance:
- Background changed to
ThemeSelectionBackgroundBrush- Foreground changed to
ThemeSelectionForegroundBrush- BorderBrush changed to
ThemeChooserBackgroundBrush- BorderThickness changed from dynamic resource to fixed
0✅ Verification successful
🏁 Script executed:
#!/bin/bash # Check if these brushes are defined in theme files rg "ThemeSelectionBackgroundBrush|ThemeSelectionForegroundBrush|ThemeChooserBackgroundBrush" -A 1Length of output: 16574
Verified the changes to brush resources and border thickness.
The brush keys ("ThemeSelectionBackgroundBrush", "ThemeSelectionForegroundBrush", and "ThemeChooserBackgroundBrush") are consistently defined across multiple theme files, and their usage in NumericUpDown.axaml aligns with similar patterns found in other controls. The fixed BorderThickness value (“0”) appears to be an intentional design decision for standardizing the control's visual appearance.
src/Consolonia.Themes/Templates/Controls/ToggleButton.axaml (1)
3-3: LGTM! Namespace update is consistent with the PR objectives.The namespace change from
helperstoconsolealigns with the standardization effort mentioned in the PR objectives.Also applies to: 22-22, 37-37
src/Consolonia.Themes/Templates/Controls/RadioButton.axaml (2)
48-63: LGTM! Style selectors updated correctly.The style selectors have been correctly updated to target the new TextBlock elements.
25-34: LGTM! Improved maintainability with theme resources.The replacement of hardcoded symbols with theme resources improves maintainability and consistency.
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Verify that the radio button symbol resources are defined rg "ThemeRadioButtonNotSelectedSymbol|ThemeRadioButtonSelectedSymbol|ThemeRadioButtonIndeterminateSymbol" -A 1Length of output: 2275
LGTM! The theme resource strings for the radio button symbols are correctly defined across multiple theme files (Material, TurboVision, and Fluent), ensuring consistency and maintainability.
- The symbols "ThemeRadioButtonNotSelectedSymbol", "ThemeRadioButtonSelectedSymbol", and "ThemeRadioButtonIndeterminateSymbol" are properly defined in the theme files.
- The RadioButton.axaml correctly references these static resources.
src/Consolonia.Themes/Templates/Controls/CheckBox.axaml (1)
3-3: LGTM! Clean implementation of themed symbols.The changes effectively implement themed symbols using static resources, making the checkbox states configurable and maintainable.
Also applies to: 24-35
src/Consolonia.Themes/Templates/Controls/CalendarDatePicker.axaml (2)
10-10: LGTM! Consistent namespace updates.The namespace changes and extension property updates maintain consistency with the new console namespace.
Also applies to: 14-14, 75-75
44-45: LGTM! Proper implementation of themed symbol.The down arrow symbol now uses a themed resource, aligning with the standardization effort.
src/Consolonia.Themes/Templates/Controls/ButtonSpinner.axaml (1)
52-52: LGTM! Consistent implementation of themed symbols.The up and down arrow symbols now use themed resources, maintaining consistency with other controls.
Also applies to: 57-57
src/Consolonia.Gallery/Gallery/GalleryViews/GalleryMenu.axaml (1)
53-53: LGTM! Simplified CheckBox styling.Removal of explicit margin allows for consistent spacing through the theme's default values.
src/Consolonia.Themes/Templates/Controls/TreeViewItem.axaml (2)
3-3: LGTM! Namespace changes are consistent.The namespace changes from
helperstoconsoleare consistent with the PR objectives and applied correctly across the file.Also applies to: 41-41, 76-76
26-26: Good improvement: Using theme resource for down arrow symbol.Replacing the hardcoded "▼" with
{StaticResource ThemeDownArrowSymbol}improves theme consistency and maintainability.src/Consolonia.Themes/Templates/Controls/ProgressBar.axaml (1)
3-3: LGTM! SymbolsControl implementation looks good.The changes correctly implement the theme resource for the progress bar background symbol and properly set the Fill and Foreground properties.
Also applies to: 17-19
src/Consolonia.Themes/Templates/Controls/ComboBox.axaml (2)
3-3: LGTM! Extension property references updated correctly.The namespace changes and ComboBoxExtensions property references are consistently updated.
Also applies to: 18-18, 87-87
57-64: Good improvement: Simplified down arrow implementation.Replacing SymbolsControl with a TextBlock using ThemeDownArrowSymbol resource simplifies the implementation while maintaining theme consistency.
src/Consolonia.Themes/Templates/Controls/CalendarItem.axaml (1)
3-3: LGTM! ButtonExtensions property reference updated correctly.The namespace change and ButtonExtensions.Shadow property reference are consistently updated.
Also applies to: 27-27
src/Consolonia.Themes/Templates/Controls/Slider.axaml (4)
31-36: LGTM! Horizontal slider background now uses themed symbols.The change from FastLineSeparator to SymbolsControl with theme-based symbols improves consistency with the theming system.
56-57: LGTM! Horizontal slider thumb now uses themed symbols.The thumb symbol is now properly bound to a theme resource instead of using a hardcoded character.
76-81: LGTM! Vertical slider background now uses themed symbols.Consistent with the horizontal slider changes, the vertical slider background now uses themed symbols.
101-102: LGTM! Vertical slider thumb now uses themed symbols.The vertical thumb symbol is now properly bound to a theme resource.
src/Consolonia.Themes/Templates/Controls/Button.axaml (4)
12-13: LGTM! ButtonExtensions properties updated to new namespace.The ButtonExtensions properties have been properly migrated to the new console namespace.
Also applies to: 20-21
36-52: LGTM! CaretControl updated to new namespace.The CaretControl element has been properly migrated to the new console namespace.
59-97: LGTM! SymbolsControl elements updated to new namespace.All SymbolsControl elements for shadows have been properly migrated to the new console namespace.
122-122: LGTM! Style selector updated to new namespace.The style selector has been properly updated to use the new console namespace.
src/Consolonia.Themes/Templates/Controls/TextBox.axaml (3)
5-7: LGTM! Converters updated to new namespace.BooleanAndConverter and AreIntegersEqualConverter have been properly migrated to the new console namespace.
62-74: LGTM! ConsoloniaTextPresenter updated to new namespace.The ConsoloniaTextPresenter control has been properly migrated to the new console namespace.
94-111: LGTM! TextBoxCaret and its properties updated to new namespace.The TextBoxCaret control and its Active/Position properties have been properly migrated to the new console namespace.
src/Consolonia.Themes/Templates/Controls/ScrollBar.axaml (7)
23-25: LGTM! Up button now uses themed symbol.The up button text now uses ThemeScrollBarUpSymbol resource instead of a hardcoded character.
31-34: LGTM! Vertical scrollbar background uses themed symbol.The vertical scrollbar background now uses ThemeScrollBarVerticalBackgroundSymbol resource.
58-62: LGTM! Down button now uses themed symbol.The down button text now uses ThemeScrollBarDownSymbol resource.
77-80: LGTM! Left button now uses themed symbol.The left button text now uses ThemeScrollBarLeftSymbol resource.
86-89: LGTM! Horizontal scrollbar background uses themed symbol.The horizontal scrollbar background now uses ThemeScrollBarHorizontalBackgroundSymbol resource.
112-115: LGTM! Right button now uses themed symbol.The right button text now uses ThemeScrollBarRightSymbol resource.
129-129: LGTM! Thumb symbols updated to use themed resources.The thumb symbols for both vertical and horizontal orientations now use themed resources.
Also applies to: 136-141
src/Consolonia.Themes/Templates/Controls/ToggleSwitch.axaml (3)
3-3: LGTM!The namespace declaration follows standard XAML conventions and aligns with the PR objectives.
14-18: LGTM!The CaretControl properties are well-structured with appropriate bindings.
120-120: LGTM!Replacing hardcoded symbols with theme resources improves customization and maintains consistency across themes.
Also applies to: 123-123
src/Consolonia.Themes/Templates/Controls/Expander.axaml (2)
3-3: LGTM!The namespace declaration and CaretControl properties are consistent with the project standards.
Also applies to: 14-18
60-60: LGTM!The arrow symbols have been consistently replaced with theme resources, following a clear naming pattern.
Also applies to: 68-68, 76-76, 84-84, 92-92, 100-100
src/Consolonia.Themes/Templates/Controls/DataGrid.axaml (3)
3-3: LGTM!The namespace declaration is consistent with other files in the project.
77-83: LGTM!The LineSeparator and CaretControl components are well-configured with appropriate bindings and templates.
Also applies to: 141-156
98-98: LGTM!The sort icons have been appropriately replaced with theme resources, maintaining consistency with the project's theming approach.
Also applies to: 105-105
src/Tests/Consolonia.Gallery.Tests/TransitioningContentTests.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
src/Tests/Consolonia.Gallery.Tests/ExpanderTests.cs (5)
12-22: Add XML documentation to describe test purpose and prerequisites.While the test is well-structured, adding XML documentation would improve maintainability by clearly stating:
- The purpose of this initialization test
- Any prerequisites or assumptions
- The expected initial state
Example documentation:
+/// <summary> +/// Verifies the initial state of all expanders: +/// - Validates presence of expander headers +/// - Ensures all content panels are initially collapsed +/// - Resets focus state +/// </summary> [Test, Order(1)] public async Task TestStart()
24-58: Extract common test logic into a helper method.The up and down expander tests share similar patterns. Consider extracting the common logic into a helper method to improve maintainability and reduce code duplication.
Example refactor:
+private async Task TestExpanderBehavior( + string direction, + string expandedSymbol, + string collapsedSymbol, + string contentText, + int tabCount) +{ + // Navigate to element + for (int i = 0; i < tabCount; i++) + await UITest.KeyInput(Key.Tab); + + // Test initial state + await UITest.AssertHasText($"{direction}.*{collapsedSymbol}"); + await UITest.AssertHasNoText(contentText); + + // Test expanded state + await UITest.KeyInput(Key.Space); + await UITest.AssertHasText($"{direction}.*{expandedSymbol}"); + await UITest.AssertHasText(contentText); + + // Test collapsed state + await UITest.KeyInput(Key.Space); + await UITest.AssertHasText($"{direction}.*{collapsedSymbol}"); + await UITest.AssertHasNoText(contentText); + + // Reset focus + for (int i = 0; i < tabCount; i++) + await UITest.KeyInput(1, Key.Tab, RawInputModifiers.Shift); +} [Test, Order(2)] public async Task TestUp() -{ - await UITest.KeyInput(Key.Tab); - await UITest.AssertHasText("Up.*▴"); - // ... -} + => await TestExpanderBehavior("Up", "▾", "▴", "Top content", 1); [Test, Order(3)] public async Task TestDown() -{ - await UITest.KeyInput(Key.Tab); - await UITest.KeyInput(Key.Tab); - await UITest.AssertHasText("Down.*▾"); - // ... -} + => await TestExpanderBehavior("Down", "▴", "▾", "Bottom content", 2);
60-102: Extend the helper method to support left/right expanders.The left and right expander tests can also use the proposed helper method. The symbols used align well with the PR's objective of standardizing symbols across the UI.
Example usage with the helper method:
[Test, Order(4)] public async Task TestLeft() -{ - await UITest.KeyInput(Key.Tab); - await UITest.KeyInput(Key.Tab); - await UITest.KeyInput(Key.Tab); - await UITest.AssertHasText("◂.*Left"); - // ... -} + => await TestExpanderBehavior("Left", "▸", "◂", "Left content", 3); [Test, Order(5)] public async Task TestRight() -{ - await UITest.KeyInput(Key.Tab); - await UITest.KeyInput(Key.Tab); - await UITest.KeyInput(Key.Tab); - await UITest.KeyInput(Key.Tab); - await UITest.AssertHasText("Right.*▸"); - // ... -} + => await TestExpanderBehavior("Right", "◂", "▸", "Right content", 4);
16-16: Define constants for expander symbols.The expander symbols are used across multiple test methods. Consider defining them as constants to improve maintainability and ensure consistency.
Example:
+private const string UP_COLLAPSED = "▴"; +private const string UP_EXPANDED = "▾"; +private const string DOWN_COLLAPSED = "▾"; +private const string DOWN_EXPANDED = "▴"; +private const string LEFT_COLLAPSED = "◂"; +private const string LEFT_EXPANDED = "▸"; +private const string RIGHT_COLLAPSED = "▸"; +private const string RIGHT_EXPANDED = "◂";Also applies to: 28-28, 32-32, 46-46, 50-50, 66-66, 70-70, 88-88, 92-92
12-12: Document the importance of test ordering.The tests use the Order attribute to ensure sequential execution. Add a class-level comment explaining why this ordering is crucial for UI testing.
Example:
+/// <summary> +/// Tests for the Expander UI component. +/// Test methods are ordered to match the tab navigation sequence, +/// ensuring consistent focus state and preventing test interference. +/// </summary> [TestFixture] internal class ExpanderTests : GalleryTestsBaseBaseAlso applies to: 24-24, 41-41, 60-60, 81-81
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/Tests/Consolonia.Gallery.Tests/CarouselTests.cs(1 hunks)src/Tests/Consolonia.Gallery.Tests/ExpanderTests.cs(1 hunks)src/Tests/Consolonia.Gallery.Tests/TransitioningContentTests.cs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/Tests/Consolonia.Gallery.Tests/CarouselTests.cs
- src/Tests/Consolonia.Gallery.Tests/TransitioningContentTests.cs
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (1)
src/Tests/Consolonia.Gallery.Tests/ExpanderTests.cs (1)
1-11: LGTM! Class structure and imports are well-organized.The test class follows proper testing conventions with appropriate imports and inheritance.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/Consolonia.Themes/Templates/Controls/CheckBox.axaml (1)
54-57: Consider removing commented-out styles.These commented-out styles appear to be obsolete after the migration to the new theme system.
Also applies to: 74-77
src/Consolonia.Themes/Templates/Controls/ScrollBar.axaml (1)
24-26: Consider adding AutomationProperties for accessibility.For better screen reader support, consider adding AutomationProperties.Name to the TextBlock elements containing navigation symbols:
<TextBlock HorizontalAlignment="Center" Foreground="{TemplateBinding Foreground}" + AutomationProperties.Name="Scroll Up" Text="{StaticResource ThemeScrollBarUpSymbol}" />Also applies to: 78-80, 112-114
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
src/Consolonia.Themes/Templates/Controls/CalendarDatePicker.axaml(3 hunks)src/Consolonia.Themes/Templates/Controls/CheckBox.axaml(3 hunks)src/Consolonia.Themes/Templates/Controls/ComboBox.axaml(4 hunks)src/Consolonia.Themes/Templates/Controls/ComboBoxItem.axaml(3 hunks)src/Consolonia.Themes/Templates/Controls/DataGrid.axaml(6 hunks)src/Consolonia.Themes/Templates/Controls/DialogWindow.axaml(5 hunks)src/Consolonia.Themes/Templates/Controls/Expander.axaml(3 hunks)src/Consolonia.Themes/Templates/Controls/MenuItem.axaml(7 hunks)src/Consolonia.Themes/Templates/Controls/ProgressBar.axaml(2 hunks)src/Consolonia.Themes/Templates/Controls/RadioButton.axaml(3 hunks)src/Consolonia.Themes/Templates/Controls/ScrollBar.axaml(7 hunks)src/Consolonia.Themes/Templates/Controls/Slider.axaml(5 hunks)src/Consolonia.Themes/Templates/Controls/ToggleSwitch.axaml(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- src/Consolonia.Themes/Templates/Controls/Slider.axaml
- src/Consolonia.Themes/Templates/Controls/ComboBoxItem.axaml
- src/Consolonia.Themes/Templates/Controls/ProgressBar.axaml
- src/Consolonia.Themes/Templates/Controls/CalendarDatePicker.axaml
- src/Consolonia.Themes/Templates/Controls/ComboBox.axaml
- src/Consolonia.Themes/Templates/Controls/RadioButton.axaml
- src/Consolonia.Themes/Templates/Controls/ToggleSwitch.axaml
- src/Consolonia.Themes/Templates/Controls/Expander.axaml
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (16)
src/Consolonia.Themes/Templates/Controls/CheckBox.axaml (1)
3-3: LGTM! The namespace and control updates look good.The changes correctly implement the new namespace and control structure:
- Updated to use the
consolenamespace- Replaced
SymbolsControlwithTextBlockelements using theme resources- Maintained the existing functionality while improving maintainability
Also applies to: 25-36
src/Consolonia.Themes/Templates/Controls/MenuItem.axaml (1)
3-3: LGTM! The namespace and symbol updates look good.The changes correctly implement:
- Updated namespace to
console- Replaced hardcoded symbols with theme resources
- Maintained consistent styling and functionality
Also applies to: 22-22, 39-43, 56-58, 80-81
src/Consolonia.Themes/Templates/Controls/DataGrid.axaml (3)
3-3: LGTM! The namespace and control updates look good.The changes correctly implement:
- Updated namespace to
console- Renamed FastLineSeparator to LineSeparator
- Updated DataGridExtensions references
Also applies to: 78-84, 142-157, 179-182
99-99: LGTM! Sort icons now use theme resources.The changes improve maintainability by using theme resources for sort icons instead of hardcoded symbols.
Also applies to: 106-106
78-78: Verify the impact of renaming FastLineSeparator.The control has been renamed from FastLineSeparator to LineSeparator. Let's verify this change doesn't break any existing references.
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for any remaining references to FastLineSeparator rg "FastLineSeparator"Length of output: 22
Verified the renaming change.
Our search for any references to "FastLineSeparator" produced no results, which confirms that all references have been updated to "LineSeparator" (as seen in the DataGrid.axaml file).
src/Consolonia.Themes/Templates/Controls/DialogWindow.axaml (7)
3-4: LGTM! Namespace declaration follows Avalonia UI conventions.The namespace declaration and ReSharper directive are correctly implemented.
8-8: LGTM! Style selector correctly uses the new namespace.The selector syntax follows XAML conventions and properly references the DialogWindow control.
23-23: LGTM! Control template target type is correctly updated.The ControlTemplate TargetType properly references the DialogWindow control using the new namespace.
51-53: LGTM! SymbolsControl reference is correctly updated.The control reference properly uses the new namespace while maintaining existing bindings and properties.
72-74: LGTM! ButtonExtensions attached property is correctly updated.The Shadow property is properly referenced using the new namespace.
82-85: LGTM! LineBrush reference is correctly updated.The brush is properly referenced using the new namespace while maintaining existing properties.
64-69: Verify ThemeCloseSymbol resource exists in theme files.The close button correctly uses a themed symbol resource, which aligns with the PR's objective of standardizing symbols.
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Verify ThemeCloseSymbol resource exists in theme files rg -l "ThemeCloseSymbol" "src/Consolonia.Themes/"Length of output: 262
Close Button Resource Verified
The resource "ThemeCloseSymbol" is present in multiple theme files (MaterialTheme.axaml, FluentTheme.axaml, DialogWindow.axaml, TurboVisionTheme.axaml), so the close button update is correctly implemented and aligns with the PR objective.
src/Consolonia.Themes/Templates/Controls/ScrollBar.axaml (4)
3-4: LGTM! Namespace update aligns with project objectives.The namespace change from 'helpers' to 'console' aligns with the PR's goal of standardizing symbol resources.
78-80: Verify the existence of horizontal scrollbar symbol resources.The changes appropriately move hardcoded symbols to theme resources. Let's verify that all referenced resources are properly defined:
Also applies to: 86-89, 112-114
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Check for the existence of horizontal scrollbar symbol resources in theme files # Search for the symbol resource definitions rg "ThemeScrollBarLeftSymbol|ThemeScrollBarRightSymbol|ThemeScrollBarHorizontalBackgroundSymbol" -A 1Length of output: 2844
Horizontal Scrollbar Symbol Resources Verified
The resource definitions referenced in the scrollbar template exist in all theme files (TurboVision, Material, and Fluent). The static resources "ThemeScrollBarLeftSymbol", "ThemeScrollBarRightSymbol", and "ThemeScrollBarHorizontalBackgroundSymbol" are properly defined, so no further changes are necessary.
24-26: Verify the existence of vertical scrollbar symbol resources.The changes appropriately move hardcoded symbols to theme resources. Let's verify that all referenced resources are properly defined:
Also applies to: 32-35, 59-63
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Check for the existence of vertical scrollbar symbol resources in theme files # Search for the symbol resource definitions rg "ThemeScrollBarUpSymbol|ThemeScrollBarDownSymbol|ThemeScrollBarVerticalBackgroundSymbol" -A 1Length of output: 2632
Vertical Scrollbar Symbol Resources Verified
The static resources for the vertical scrollbar symbols are defined as expected. They exist in the theme files (Material, Fluent, and TurboVision), ensuring the references in the scrollbar template are valid:
- "ThemeScrollBarUpSymbol" and "ThemeScrollBarDownSymbol" are defined in MaterialTheme.axaml, FluentTheme.axaml, and TurboVisionTheme.axaml.
- "ThemeScrollBarVerticalBackgroundSymbol" is also defined in these themes.
This confirms that the changes properly reference existing theme resources.
128-128: Verify the existence of thumb symbol resources.The changes appropriately move hardcoded symbols to theme resources. Let's verify that all referenced resources are properly defined:
Also applies to: 135-135, 149-149
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Check for the existence of thumb symbol resources in theme files # Search for the symbol resource definitions rg "ThemeScrollBarVerticalThumbSymbol|ThemeScrollBarHorizontalThumbSymbol" -A 1Length of output: 1833
Thumb symbol resources are defined.
The search confirms that each theme file (Material, Fluent, and TurboVision) includes definitions for both "ThemeScrollBarVerticalThumbSymbol" and "ThemeScrollBarHorizontalThumbSymbol". The references in ScrollBar.axaml correctly point to these resources.
Co-authored-by: <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (8)
src/Consolonia.Themes/Templates/Controls/ScrollBar.axaml (1)
59-63: Consider consolidating TextBlock properties.While the changes look good, the
FocusableandMinHeightproperties on this TextBlock are not present on other similar TextBlocks in the file.- <TextBlock HorizontalAlignment="Center" - Foreground="{TemplateBinding Foreground}" - Focusable="False" - MinHeight="1" - Text="{StaticResource ThemeScrollBarDownSymbol}" /> + <TextBlock HorizontalAlignment="Center" + Foreground="{TemplateBinding Foreground}" + Text="{StaticResource ThemeScrollBarDownSymbol}" />src/Tests/Consolonia.Gallery.Tests/ExpanderTests.cs (2)
12-23: Consider removing test ordering dependency.While the
[Order]attribute helps maintain a specific test sequence, it creates tight coupling between test methods. Consider making each test independent by setting up its own initial state.
25-41: Improve test readability with descriptive test cases.The test verifies multiple behaviors (initial state, expand, collapse). Consider splitting into separate test methods with descriptive names:
-[Test] -[Order(2)] -public async Task TestUp() +[TestCase(TestName = "Up Expander - Initially Collapsed")] +public async Task UpExpander_WhenInitiallyClosed_ShowsUpArrow() + +[TestCase(TestName = "Up Expander - Expands On Space")] +public async Task UpExpander_WhenSpacePressed_ExpandsAndShowsContent() + +[TestCase(TestName = "Up Expander - Collapses On Second Space")] +public async Task UpExpander_WhenExpandedAndSpacePressed_CollapsesAndHidesContent()src/Consolonia.Themes/Templates/Controls/DataGrid.axaml (1)
6-6: Remove ReSharper comment.The ReSharper comment about static resource resolution is not necessary and should be removed for cleaner code.
- <!-- ReSharper disable Xaml.StaticResourceNotResolved -->src/Consolonia.Themes/Material/MaterialTheme.axaml (1)
10-13: Consider using consistent arrow symbols.The arrow symbols (◂, ▸, ▴, ▾) are from different Unicode blocks which might render inconsistently across different terminals and fonts.
Consider using arrows from the same Unicode block for visual consistency. For example, the "Geometric Shapes" block (U+25xx) contains a complete set of triangular arrows.
src/Consolonia.Themes/Templates/Controls/CheckBox.axaml (1)
54-57: Consider removing commented-out styles.These commented-out style blocks appear to be obsolete after the migration from SymbolsControl to TextBlock.
Remove these commented-out sections to improve code maintainability.
Also applies to: 74-77
src/Consolonia.Themes/Templates/Controls/MenuItem.axaml (2)
39-45: Consider adjusting the check symbol margin.The check symbol TextBlock has a margin of "0,0,0,0" which is redundant.
Remove the redundant Margin property or adjust it if spacing is needed:
- Margin="0,0,0,0"
56-61: Consider adjusting the right arrow margin and alignment.Similar to the check symbol, the right arrow has a redundant margin. Also, consider using HorizontalAlignment for better control over the arrow position.
- Margin="0,0,0,0" + HorizontalAlignment="Center"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
src/Consolonia.Themes/Fluent/FluentTheme.axaml(1 hunks)src/Consolonia.Themes/Material/MaterialTheme.axaml(1 hunks)src/Consolonia.Themes/Templates/Controls/ButtonSpinner.axaml(1 hunks)src/Consolonia.Themes/Templates/Controls/CalendarDatePicker.axaml(3 hunks)src/Consolonia.Themes/Templates/Controls/CheckBox.axaml(3 hunks)src/Consolonia.Themes/Templates/Controls/ComboBox.axaml(4 hunks)src/Consolonia.Themes/Templates/Controls/DataGrid.axaml(6 hunks)src/Consolonia.Themes/Templates/Controls/Expander.axaml(2 hunks)src/Consolonia.Themes/Templates/Controls/MenuItem.axaml(6 hunks)src/Consolonia.Themes/Templates/Controls/RadioButton.axaml(3 hunks)src/Consolonia.Themes/Templates/Controls/ScrollBar.axaml(7 hunks)src/Consolonia.Themes/Templates/Controls/ScrollViewer.axaml(5 hunks)src/Consolonia.Themes/Templates/Controls/Slider.axaml(5 hunks)src/Consolonia.Themes/TurboVision/TurboVisionTheme.axaml(1 hunks)src/Consolonia.Themes/TurboVisionBlack/TurboVisionBlackTheme.axaml(1 hunks)src/Consolonia.Themes/TurboVisionDark/TurboVisionDarkTheme.axaml(1 hunks)src/Tests/Consolonia.Gallery.Tests/ExpanderTests.cs(1 hunks)src/Tests/Consolonia.Gallery.Tests/TreeViewTests.cs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (12)
- src/Consolonia.Themes/Templates/Controls/ButtonSpinner.axaml
- src/Tests/Consolonia.Gallery.Tests/TreeViewTests.cs
- src/Consolonia.Themes/TurboVisionBlack/TurboVisionBlackTheme.axaml
- src/Consolonia.Themes/Templates/Controls/CalendarDatePicker.axaml
- src/Consolonia.Themes/Templates/Controls/RadioButton.axaml
- src/Consolonia.Themes/Templates/Controls/Slider.axaml
- src/Consolonia.Themes/TurboVisionDark/TurboVisionDarkTheme.axaml
- src/Consolonia.Themes/Templates/Controls/ScrollViewer.axaml
- src/Consolonia.Themes/TurboVision/TurboVisionTheme.axaml
- src/Consolonia.Themes/Templates/Controls/Expander.axaml
- src/Consolonia.Themes/Templates/Controls/ComboBox.axaml
- src/Consolonia.Themes/Fluent/FluentTheme.axaml
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (22)
src/Consolonia.Themes/Templates/Controls/ScrollBar.axaml (9)
3-4: LGTM! Namespace update aligns with project objectives.The change from 'helpers' to 'console' namespace is consistent with the PR's goal of standardizing symbol resources.
24-26: LGTM! Up arrow symbol now uses theme resource.The change from hardcoded symbol to
ThemeScrollBarUpSymbolresource improves theme consistency.
32-35: LGTM! Vertical background symbol uses theme resource.The SymbolsControl now correctly uses
ThemeScrollBarVerticalBackgroundSymbolresource with proper namespace.
78-80: LGTM! Left arrow symbol uses theme resource.The change from hardcoded symbol to
ThemeScrollBarLeftSymbolresource improves theme consistency.
86-89: LGTM! Horizontal background symbol uses theme resource.The SymbolsControl now correctly uses
ThemeScrollBarHorizontalBackgroundSymbolresource with proper namespace.
112-114: LGTM! Right arrow symbol uses theme resource.The change from hardcoded symbol to
ThemeScrollBarRightSymbolresource improves theme consistency.
127-128: LGTM! Vertical thumb symbol uses theme resource.The change from hardcoded symbol to
ThemeScrollBarVerticalThumbSymbolresource improves theme consistency.
135-140: LGTM! SymbolsControl uses correct namespace.The SymbolsControl element correctly uses the 'console' namespace while maintaining all necessary properties.
148-149: LGTM! Horizontal thumb symbol uses theme resource.The change from hardcoded symbol to
ThemeScrollBarHorizontalThumbSymbolresource improves theme consistency.src/Tests/Consolonia.Gallery.Tests/ExpanderTests.cs (3)
1-11: LGTM! Clean test class structure.The class follows testing best practices with appropriate imports and inheritance.
43-61: Similar improvement opportunity as TestUp method.The same test readability improvements suggested for the TestUp method apply here.
85-107: Similar navigation improvement opportunity as TestLeft method.The same navigation robustness improvements suggested for the TestLeft method apply here.
src/Consolonia.Themes/Templates/Controls/DataGrid.axaml (5)
3-3: LGTM! Namespace update aligns with PR objectives.The namespace change from
helperstoconsolestandardizes the control references across the codebase.
78-84: LGTM! LineSeparator control updates are consistent.The replacement of
FastLineSeparatorwithLineSeparatoris applied consistently in both column header and grid header sections.Also applies to: 319-325
99-99: LGTM! Theme resources improve customization.The replacement of hardcoded arrow symbols (▼ and ▲) with theme resources (
ThemeDownArrowSymbolandThemeUpArrowSymbol) enhances theme customization capabilities.Also applies to: 106-106
149-149: Verify DataGridExtensions property usage.The DataGridExtensions.IsSelected property is used in both the CaretControl binding and row selection style. Ensure this attached property is properly defined and registered.
Also applies to: 180-180
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Check for DataGridExtensions class definition and property registration ast-grep --pattern 'class DataGridExtensions { $$$ public static readonly AttachedProperty<bool> IsSelectedProperty $$$ }'Length of output: 130
I'll generate a new shell script to search for the definition of the DataGridExtensions class and its usage of the IsSelectedProperty. This should help us verify that the attached property is actually registered in the codebase.
🏁 Script executed:
#!/bin/bash # Search for the definition of DataGridExtensions in the repository rg -n "class DataGridExtensions" # Search for any reference to 'IsSelectedProperty' rg -n "IsSelectedProperty"Length of output: 587
Review Comment Verified: DataGridExtensions Properly Defines IsSelectedProperty
The attached property IsSelectedProperty is located in src/Consolonia.Themes/Templates/Controls/Helpers/DataGridExtensions.cs (line 8) and is accompanied by getter and setter methods (lines 13 and 18). The property is properly defined and registered, as required for its usage in CaretControl binding and row selection style.
142-157: Verify CaretControl's binding behavior.The CaretControl's IsCaretShown binding uses a MultiBinding with BoolConverters.And. Ensure that:
- The DataGrid.IsFocused binding works as expected
- The DataGridExtensions.IsSelected property is properly propagated
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Check for other usages of CaretControl with similar bindings ast-grep --pattern 'CaretControl { $$$ IsCaretShown { $$$ MultiBinding { $$$ } $$$ } $$$ }'Length of output: 123
I'll run an additional set of shell scripts to examine the binding details in the file and across the repository. This will help us verify that the DataGrid.IsFocused binding and the DataGridExtensions.IsSelected propagation are declared as intended.
Below is the script that:
• Extracts the relevant snippet from "src/Consolonia.Themes/Templates/Controls/DataGrid.axaml" (lines 140–170)
• Searches for any usages of DataGridExtensions.IsSelected in the repository
• Searches for bindings referencing DataGrid and its IsFocused propertyPlease execute the following script:
Once we review these results, we’ll have a clearer picture of whether the bindings are set up as expected.
🏁 Script executed:
#!/bin/bash echo "Extracting lines 140-170 from DataGrid.axaml:" sed -n '140,170p' src/Consolonia.Themes/Templates/Controls/DataGrid.axaml echo "-------" echo "Searching for usage of DataGridExtensions.IsSelected in repository:" rg "DataGridExtensions\.IsSelected" -n . echo "-------" echo "Searching for DataGrid bindings with IsFocused:" rg 'Binding.*DataGrid.*IsFocused' -n .Length of output: 2887
CaretControl Binding Behavior Verified
The code shows that the MultiBinding in CaretControl correctly combines the DataGrid’s IsFocused property via RelativeSource and the DataGridExtensions.IsSelected property via TemplateBinding. Our searches confirm that these bindings are set as expected in the repository.
src/Consolonia.Themes/Material/MaterialTheme.axaml (3)
37-39: LGTM! Well-chosen checkbox symbols.The checkbox symbols (☐, ☒, ■) are from well-supported Unicode blocks and provide clear visual distinction between states.
15-23: Verify scrollbar symbols' visibility in different terminal color schemes.The scrollbar background symbol (░) might have poor visibility in certain terminal color schemes, especially with light backgrounds.
✅ Verification successful
🌐 Web query:
Are there any known issues with the visibility of Unicode character U+2591 (light shade) in terminal emulators with light backgrounds?💡 Result:
Yes, there are documented visibility and rendering issues with Unicode character U+2591 (LIGHT SHADE ░) in terminal emulators, particularly when using light backgrounds. These stem from differences in how terminals handle shading characters: ## Rendering Challenges - **Font-based vs. Custom Rendering**: Some terminals like **GNOME Terminal** render U+2591 as semi-transparent blocks blended with background colors[4], while others like **Alacritty** rely strictly on font glyphs[4]. This leads to inconsistent results: - Font-dependent terminals may show blocky patterns instead of smooth gradients[2][4]. - Kitty initially used fixed dot patterns for U+2591-U+2593 but later added a `shade_transparency` option to simulate transparency via alpha blending[2]. ## Specific Issues 1. **Visibility on Light Backgrounds**: The default 25% fill pattern of U+2591 can become nearly invisible against light backgrounds if rendered via font glyphs without transparency support[4]. For example: - Alacritty users reported poor contrast compared to GNOME Terminal's blended approach[4]. - Visual artifacts like pixel overlap were observed in VS Code's integrated terminal[9]. 2. **Terminal Workarounds**: - **Kitty**: Enable `shade_transparency yes` in `kitty.conf` to replace discrete dots with alpha-blended blocks[2]. - **Powerlevel10k**: Users adopted alternative characters (e.g., `▱`) to avoid relying on terminal-specific shading behavior[2]. ## Underlying Causes - Unicode defines U+2591 as a fixed 25% fill pattern (`░`), but its appearance depends on implementation[2][5]. - Terminals prioritizing font accuracy (Alacritty) may clash with those applying rendering optimizations (GNOME Terminal/Kitty)[2][4]. For best results, test shading characters in your target terminal and adjust settings or substitute glyphs if visibility issues arise[2][4][9]. Citations: - 1: https://www.fileformat.info/info/unicode/char/2591/index.htm - 2: https://github.com/kovidgoyal/kitty/issues/6108 - 3: http://www.unicode.org/reports/tr36/ - 4: https://github.com/alacritty/alacritty/issues/2854 - 5: https://www.compart.com/en/unicode/U+2591 - 6: https://sw.kovidgoyal.net/kitty/changelog/ - 7: https://forum.knime.com/t/how-to-deal-with-unicode-special-invisible-characters/48095 - 8: https://www.unicode.org/L2/L2011/11196-n4022-wingdings.pdf - 9: https://github.com/microsoft/vscode/issues/101651 - 10: https://www.unicode.org/Public/draft/UCD/ucd/NamesList.txt
Scrollbar Symbol Visibility Verified
The Unicode character U+2591 (░) is documented to have visibility issues in terminal emulators using light backgrounds, which confirms the concern in the review. In several terminals (e.g., Alacritty, VS Code's integrated terminal), the light shading can render with poor contrast. Consider testing in your target environments and, if necessary, providing an alternate fallback for improved readability.
33-35: Verify radio button indeterminate symbol compatibility.The indeterminate symbol (🟗) is from a newer Unicode block and might not be supported in all environments.
✅ Verification successful
🌐 Web query:
What is the compatibility of Unicode character U+1F5D7 (BLACK SQUARE CONTAINED IN WHITE SQUARE) across different terminal emulators and fonts?💡 Result:
The Unicode character U+1F5D7 (officially named "OVERLAP" rather than "BLACK SQUARE CONTAINED IN WHITE SQUARE") has limited compatibility across terminal emulators and fonts due to its niche usage and rendering requirements. Here's a detailed analysis: ### Terminal Compatibility 1. **Basic Support**: Most modern terminal emulators (like Kitty, WezTerm, or Windows Terminal) support Unicode 13.0+ and *may* display U+1F5D7 if: - The font includes the glyph - The terminal’s text layout engine supports Miscellaneous Symbols and Pictographs (block: U+1F300–U+1F5FF) [1][5] 2. **Legacy Terminals**: Older terminals (e.g., xterm without UTF-8 configuration) often fail to render this character, showing replacement symbols like `` or empty space [6]. 3. **Complex Script Challenges**: Terminals struggle with characters requiring contextual shaping or bidirectional text (e.g., Arabic), but U+1F5D7 is a static symbol unaffected by these issues [10]. ### Font Support - **Included Fonts**: Few system fonts natively include U+1F5D7. Notable exceptions: - *Segoe UI Emoji* (Windows) - *Noto Symbols* (Google) - *Apple Symbols* (macOS) [5][9] - **Missing Glyphs**: Fonts like DejaVu Sans Mono or Hack often omit this character, resulting in fallback rendering or blanks [3][6]. ### Workarounds and Testing To check compatibility: ```bash echo -e '\U0001F5D7' # Linux/macOS ``` If unsupported: - Use alternative symbols like ◼️ (U+25FC) or ▣ (U+25A3) - Configure terminal font fallback chains (e.g., prioritize Symbola/Noto) [2][6] ### Development Efforts The *Terminal Unicode Core Specification* aims to standardize Unicode support but remains in active development with limited adoption [2]. Projects like KiCad have encountered rendering issues with similar symbols due to font limitations [3]. In summary, U+1F5D7 works inconsistently across systems but is most reliable in modern terminals with emoji/symbol fonts installed. Developers should provide fallback options for critical use cases[1][3][5]. Citations: - 1: https://www.fileformat.info/info/unicode/char/1f5d7/index.htm - 2: https://github.com/contour-terminal/terminal-unicode-core - 3: https://forum.kicad.info/t/unicode-symbol-not-rendering/33042 - 4: https://en.wikipedia.org/wiki/Unicode_compatibility_characters - 5: https://www.compart.com/en/unicode/U+1F5D7 - 6: https://www.baeldung.com/linux/less-show-special-characters - 7: https://community.khronos.org/t/unicode-text-rendering/71362 - 8: https://www.unicode.org/L2/L2011/11196-n4022-wingdings.pdf - 9: https://www.unicodepedia.com/unicode/miscellaneous-symbols-and-pictographs/1f5d7/unassigned-1f5d7/ - 10: https://www.unicode.org/L2/L2023/23107-terminal-suppt.pdf
Action Required: Provide Fallback for Indeterminate Symbol
The Unicode character 🟗 (U+1F5D7, officially "OVERLAP") has been shown to suffer from limited compatibility in many legacy environments, especially when the terminal emulator’s font does not include this glyph. In modern setups using terminal emulators like Kitty, WezTerm, or Windows Terminal with fonts such as Segoe UI Emoji, Noto Symbols, or Apple Symbols, it might display correctly. However, older or less updated environments (e.g., xterm or those using fonts like DejaVu Sans Mono or Hack) are likely to render a fallback symbol or nothing at all.
• Consider implementing a fallback (such as ◼️ U+25FC or ▣ U+25A3) for environments lacking robust Unicode support.
• Verify the targeted deployment environments to decide if the fallback is necessary.src/Consolonia.Themes/Templates/Controls/CheckBox.axaml (1)
28-34: LGTM! Good use of theme resources for checkbox states.The TextBlock elements correctly bind to the theme resources for different checkbox states, maintaining consistency with the theme system.
src/Consolonia.Themes/Templates/Controls/MenuItem.axaml (1)
80-81: LGTM! Proper namespace update for MenuExtensions.The MenuExtensions attached property correctly uses the new console namespace.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
src/Consolonia.Themes/Templates/Controls/DropDownButton.axaml (1)
57-60: Remove commented-out style selector.Since the TextBlock has been replaced with SymbolsControl, this commented style selector is no longer needed and should be removed.
- <!--<Style Selector="^ /template/ TextBlock#DropDownGlyph"> - <Setter Property="Foreground" Value="{DynamicResource ThemeForegroundBrush}" /> - --><!--<Setter Property="Data" Value="M1939 486L2029 576L1024 1581L19 576L109 486L1024 1401L1939 486Z" />--><!-- - </Style>-->src/Consolonia.Themes/Templates/Controls/ScrollBar.axaml (2)
24-26: LGTM! Consider standardizing Focusable and MinHeight properties.The changes correctly implement theme resources for symbols. However, for consistency:
- The Focusable="False" property is only set on the down button's SymbolsControl
- The MinHeight="1" property is only set on the down button's SymbolsControl
Apply these properties consistently across all SymbolsControl elements:
<console:SymbolsControl HorizontalAlignment="Center" Foreground="{TemplateBinding Foreground}" + Focusable="False" + MinHeight="1" Text="{StaticResource ThemeScrollBarUpSymbol}" />Also applies to: 32-35, 59-63
78-80: Maintain consistency with vertical scrollbar properties.For consistency with the vertical scrollbar implementation, consider adding the same Focusable and MinHeight properties to the horizontal scrollbar's SymbolsControl elements.
Apply these properties to all horizontal SymbolsControl elements:
<console:SymbolsControl HorizontalAlignment="Center" Foreground="{TemplateBinding Foreground}" + Focusable="False" + MinHeight="1" Text="{StaticResource ThemeScrollBarLeftSymbol}" />Also applies to: 86-89, 112-114
src/Consolonia.Themes/Templates/Controls/MenuItem.axaml (2)
39-45: Consider removing redundant margin property.The
Margin="0,0,0,0"property on line 44 is redundant as it's equivalent to the default margin.- Margin="0,0,0,0"
56-61: Consider removing redundant margin property.The
Margin="0,0,0,0"property on line 59 is redundant as it's equivalent to the default margin.- Margin="0,0,0,0"src/Consolonia.Themes/Templates/Controls/CheckBox.axaml (1)
54-77: Clean up commented styles.Consider removing the commented-out TextBlock styles if they are no longer needed, or document why they are kept for reference.
src/Consolonia.Themes/Templates/Controls/Expander.axaml (1)
42-42: Remove unnecessary commented code.The commented-out closing tag for
CaretControlis redundant and should be removed.- <!--</console:CaretControl>-->
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
src/Consolonia.Themes/Templates/Controls/ButtonSpinner.axaml(2 hunks)src/Consolonia.Themes/Templates/Controls/CalendarDatePicker.axaml(3 hunks)src/Consolonia.Themes/Templates/Controls/CheckBox.axaml(3 hunks)src/Consolonia.Themes/Templates/Controls/ComboBox.axaml(4 hunks)src/Consolonia.Themes/Templates/Controls/DropDownButton.axaml(2 hunks)src/Consolonia.Themes/Templates/Controls/Expander.axaml(2 hunks)src/Consolonia.Themes/Templates/Controls/MenuItem.axaml(6 hunks)src/Consolonia.Themes/Templates/Controls/RadioButton.axaml(3 hunks)src/Consolonia.Themes/Templates/Controls/ScrollBar.axaml(7 hunks)src/Consolonia.Themes/Templates/Controls/Slider.axaml(5 hunks)src/Consolonia.Themes/Templates/Controls/ToggleSwitch.axaml(8 hunks)src/Consolonia.Themes/Templates/Controls/TreeViewItem.axaml(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- src/Consolonia.Themes/Templates/Controls/Slider.axaml
- src/Consolonia.Themes/Templates/Controls/ButtonSpinner.axaml
- src/Consolonia.Themes/Templates/Controls/TreeViewItem.axaml
- src/Consolonia.Themes/Templates/Controls/CalendarDatePicker.axaml
- src/Consolonia.Themes/Templates/Controls/RadioButton.axaml
- src/Consolonia.Themes/Templates/Controls/ComboBox.axaml
- src/Consolonia.Themes/Templates/Controls/ToggleSwitch.axaml
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (15)
src/Consolonia.Themes/Templates/Controls/DropDownButton.axaml (3)
3-3: LGTM: Namespace addition aligns with PR objectives.The console namespace addition enables the use of standardized symbol controls.
5-5: LGTM: Appropriate ReSharper directive.The directive correctly suppresses false positive warnings for static resource resolution.
44-49: LGTM: Standardized symbol implementation.The replacement of hardcoded symbol with a themeable SymbolsControl improves maintainability and consistency.
src/Consolonia.Themes/Templates/Controls/ScrollBar.axaml (2)
3-4: LGTM! Namespace update aligns with project objectives.The transition from helpers namespace to console namespace is consistent with the PR's goal of standardizing control references.
128-128: LGTM! Thumb style changes align with theme resource approach.The changes correctly implement theme resources for thumb symbols while maintaining the existing functionality.
Also applies to: 135-135, 149-149
src/Consolonia.Themes/Templates/Controls/MenuItem.axaml (4)
3-3: LGTM! Namespace changes align with project standardization.The namespace update from
helperstoconsoleis consistent with the PR objectives.Also applies to: 5-5
22-22: LGTM! CaretControl namespace updated correctly.The namespace change maintains the same functionality while aligning with the new convention.
Also applies to: 90-90
80-81: LGTM! MenuExtensions namespace updated correctly.The namespace change maintains the same functionality while aligning with the new convention.
117-117: LGTM! Style selector namespace updated correctly.The namespace change in the style selector maintains the same functionality while aligning with the new convention.
src/Consolonia.Themes/Templates/Controls/CheckBox.axaml (2)
25-36: Consider theme-specific caret visibility.Based on past review comments, terminals without thin caret support might display incorrectly. Consider disabling the caret for themes other than TurboVision.
Run the following script to check if other themes handle caret visibility differently:
#!/bin/bash # Description: Check caret visibility handling across different themes. # Search for caret-related settings in theme files rg -A 5 'CaretControl.*IsCaretShown'
3-4: LGTM! Verify static resource resolution.The namespace change aligns with the PR objectives. However, the ReSharper comment suggests potential static resource resolution warnings.
Run the following script to verify that all referenced static resources are properly defined:
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Verify that all ThemeCheckBox* resources are defined in theme files. # Search for ThemeCheckBox* resource definitions rg -A 1 'x:Key="ThemeCheckBox(Not)?Selected(Symbol)?"' rg -A 1 'x:Key="ThemeCheckBoxIndeterminate(Symbol)?"'Length of output: 1802
Static resource verification complete.
The static resource definitions for the CheckBox symbols are correctly defined in all theme files (Fluent, Material, and TurboVision). No missing resources were found.
src/Consolonia.Themes/Templates/Controls/Expander.axaml (4)
3-3: LGTM! Namespace change aligns with PR objectives.The change from
helperstoconsolenamespace standardizes the control references across the application.
61-61: LGTM! Consistent use of themed symbols.The replacement of hardcoded symbols with theme resources improves maintainability and customization options.
Also applies to: 69-69, 77-77, 85-85, 93-93, 101-101
13-41: LGTM! Well-structured layout with proper alignment.The Grid layout with proper column definitions and content alignment provides a clean and maintainable structure.
6-6: Verify static resource resolution.The ReSharper disable comment suggests potential static resource resolution issues. Please ensure all theme resources are properly defined and accessible.
Run this script to verify the static resources:
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Check if all referenced theme resources are defined # Search for all StaticResource references rg "StaticResource Theme.*Symbol" -A 1 # Search for the corresponding resource definitions rg "x:Key=\"Theme.*Symbol\""Length of output: 20082
Static resource resolution confirmed – all theme resources are correctly defined.
- The static resource keys referenced in
Expander.axaml(e.g.,ThemeDownArrowSymbol,ThemeUpArrowSymbol,ThemeRightArrowSymbol,ThemeLeftArrowSymbol) are all defined.- Definitions are present in multiple theme files (such as
FluentTheme.axaml,TurboVisionTheme.axaml, andMaterialTheme.axaml), ensuring they are accessible.
Co-authored-by: <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/Consolonia.Themes/Templates/Controls/Expander.axaml (1)
42-42: Remove unnecessary commented code.The commented out closing tag
<!--</console:CaretControl>-->appears to be obsolete and should be removed.- <!--</console:CaretControl>-->
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
src/Consolonia.Themes/Templates/Controls/ButtonSpinner.axaml(2 hunks)src/Consolonia.Themes/Templates/Controls/CalendarDatePicker.axaml(3 hunks)src/Consolonia.Themes/Templates/Controls/CheckBox.axaml(3 hunks)src/Consolonia.Themes/Templates/Controls/ComboBox.axaml(4 hunks)src/Consolonia.Themes/Templates/Controls/DropDownButton.axaml(2 hunks)src/Consolonia.Themes/Templates/Controls/Expander.axaml(2 hunks)src/Consolonia.Themes/Templates/Controls/MenuItem.axaml(7 hunks)src/Consolonia.Themes/Templates/Controls/RadioButton.axaml(3 hunks)src/Consolonia.Themes/Templates/Controls/ScrollBar.axaml(7 hunks)src/Consolonia.Themes/Templates/Controls/Slider.axaml(5 hunks)src/Consolonia.Themes/Templates/Controls/ToggleSwitch.axaml(8 hunks)src/Consolonia.Themes/Templates/Controls/TreeViewItem.axaml(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
- src/Consolonia.Themes/Templates/Controls/ButtonSpinner.axaml
- src/Consolonia.Themes/Templates/Controls/DropDownButton.axaml
- src/Consolonia.Themes/Templates/Controls/CalendarDatePicker.axaml
- src/Consolonia.Themes/Templates/Controls/TreeViewItem.axaml
- src/Consolonia.Themes/Templates/Controls/Slider.axaml
- src/Consolonia.Themes/Templates/Controls/ComboBox.axaml
- src/Consolonia.Themes/Templates/Controls/ScrollBar.axaml
- src/Consolonia.Themes/Templates/Controls/CheckBox.axaml
- src/Consolonia.Themes/Templates/Controls/RadioButton.axaml
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (13)
src/Consolonia.Themes/Templates/Controls/MenuItem.axaml (6)
3-3: LGTM! Namespace update aligns with PR objectives.The change from
helperstoconsolenamespace is consistent with the PR's goal of standardizing namespace usage.Also applies to: 5-5
22-22: LGTM! CaretControl namespace updated correctly.The CaretControl element has been properly updated to use the new
consolenamespace while maintaining its existing functionality.Also applies to: 90-90
39-45: LGTM! Check mark symbol now uses themed resource.The update properly replaces the hardcoded symbol with a themed resource, improving consistency across the UI.
56-61: LGTM! Right arrow symbol now uses themed resource.The update properly replaces the hardcoded arrow with a themed resource, maintaining UI consistency.
80-81: LGTM! MenuExtensions namespace updated correctly.The FocusOnLoad property has been properly updated to use the new
consolenamespace.
117-117: LGTM! Style selector updated to match new namespace.The selector syntax has been properly updated to use the new
consolenamespace.src/Consolonia.Themes/Templates/Controls/ToggleSwitch.axaml (2)
3-3: LGTM! Namespace update aligns with project objectives.The change from
helperstoconsolenamespace and the addition of the ReSharper suppression comment are appropriate.Also applies to: 5-5
104-128: Verify theme resources exist for toggle switch symbols.The replacement of hardcoded symbols with theme resources improves maintainability. However, we should verify that the required resources are defined.
Let's verify the theme resources are properly defined:
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Check if the toggle switch theme resources are defined in theme files # Search for toggle switch symbol definitions rg "ThemeToggleSwitchOnSymbol|ThemeToggleSwitchOffSymbol" --type xmlLength of output: 70
I'm not seeing any output from the initial search, so let's run a broader scan targeting XAML resource dictionaries to see if the keys are defined.
If these commands still return no hits, please manually verify that the theme resource definitions for these keys exist in your resource dictionaries (or add them if missing).
Please check that the ThemeToggleSwitchOnSymbol and ThemeToggleSwitchOffSymbol resources are properly defined in your theme resource files.
🏁 Script executed:
#!/bin/bash echo "Searching for ThemeToggleSwitchOnSymbol and ThemeToggleSwitchOffSymbol in .axaml files..." rg -g "*.axaml" 'x:Key\s*=\s*"(ThemeToggleSwitchOnSymbol|ThemeToggleSwitchOffSymbol)"' echo "Searching for ThemeToggleSwitchOnSymbol and ThemeToggleSwitchOffSymbol in .xaml files..." rg -g "*.xaml" 'x:Key\s*=\s*"(ThemeToggleSwitchOnSymbol|ThemeToggleSwitchOffSymbol)"'Length of output: 1428
Theme resources for toggle switch symbols are defined as expected.
ThemeToggleSwitchOnSymbolandThemeToggleSwitchOffSymbolare present in the TurboVision, Fluent, and Material theme resource dictionaries.src/Consolonia.Themes/Templates/Controls/Expander.axaml (5)
3-3: LGTM! Namespace update aligns with PR objectives.The change from
helperstoconsolenamespace standardizes the control references across the application.
13-41: LGTM! Template structure improvements.The template changes effectively:
- Use Grid layout for better control organization
- Replace hardcoded symbols with themed resources
- Standardize control references using the new namespace
59-102: LGTM! Consistent use of themed symbols.The changes effectively replace all hardcoded arrow symbols with themed resources, maintaining consistency across all directional variations:
- Up/Down arrows in vertical expansion
- Left/Right arrows in horizontal expansion
107-235: LGTM! Expander control template maintains consistency.The main expander template effectively:
- Uses the themed toggle buttons for all directions
- Maintains proper layout and visibility logic
- Integrates well with the updated symbol system
6-6: Verify if ReSharper warning suppression is necessary.The ReSharper disable comment suggests potential false positives in static resource resolution. Ensure that all referenced static resources are actually defined in the theme files.
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Check if all theme symbols used in this file are defined in theme files. # Search for all ThemeSymbol resource references symbols=$(rg -o 'ThemeSymbol\w+' src/Consolonia.Themes/Templates/Controls/Expander.axaml | sort -u) # Check if these symbols are defined in theme files for symbol in $symbols; do echo "Checking $symbol..." rg "x:Key=\"$symbol\"" src/Consolonia.Themes/ doneLength of output: 103
ReSharper Warning Suppression in Expander.axaml
The initial script searched for resource identifiers matching “ThemeSymbol…” and returned nothing. To further investigate the need for the suppression, we should verify whether the file uses any StaticResource markup that might trigger the “Xaml.StaticResourceNotResolved” warning. For example, we can search for any occurrences of “{StaticResource …}” in the file:
If this search also returns no results, it suggests that there are no direct static resource references in the file that would be unresolved—making the suppression likely unnecessary. However, please note the following:
- Indirect References: There might be resource usages resolved via merged dictionaries or in other parts of the theme that aren’t directly visible in this file.
- Design-Time Warnings: Sometimes ReSharper may show warnings due to design-time limitations even when the resources are correctly defined at runtime.
I recommend running the above script and then performing a manual check to confirm that all static resource references (if any) are properly defined in the theme files. If both automated and manual reviews confirm that no unresolved references exist, the suppression comment can be removed.
🏁 Script executed:
#!/bin/bash echo "Searching for StaticResource references in Expander.axaml..." rg "\{StaticResource\s+[^\}]+\}" src/Consolonia.Themes/Templates/Controls/Expander.axamlLength of output: 1871
Below is an additional script to search the theme files for definitions (via
x:Key) of all static resource keys referenced in the Expander file. This will help determine if all resources are defined and whether the ReSharper disable comment is masking a false positive. Please run the following script:Next steps:
- If all referenced keys are found in the theme files, then the warning suppression likely exists due to ReSharper’s limited design-time context (for example, when merge dictionaries are used), and you may consider removing it if no actual warnings occur.
- If any resource key is missing, it would justify the suppression or call for adding the missing resource definitions.
Please review the output to decide whether the ReSharper warning suppression is still necessary.
🏁 Script executed:
#!/bin/bash resources=( "ConsoleExpanderToggleButtonTheme" "ThemeDownArrowSymbol" "ThemeUpArrowSymbol" "ThemeRightArrowSymbol" "ThemeLeftArrowSymbol" "ConsoleExpanderToggleButtonUpTheme" "ConsoleExpanderToggleButtonDownTheme" "ConsoleExpanderToggleButtonLeftTheme" "ConsoleExpanderToggleButtonRightTheme" "ConsoleExpanderToggleButtonRightTheme2" "ConsoleExpanderToggleButtonLeftTheme2" ) for resource in "${resources[@]}"; do echo "Checking definition for $resource:" rg "x:Key\s*=\s*\"$resource\"" -n src/Consolonia.Themes/ || echo "No definition found for $resource." echo "---------------------------------" doneLength of output: 6054
Remove the unnecessary ReSharper suppression in Expander.axaml
Our investigation confirmed that all the static resource keys used in the file are defined as follows:
- ConsoleExpanderToggleButtonTheme is defined in Expander.axaml.
- ThemeDownArrowSymbol, ThemeUpArrowSymbol, ThemeRightArrowSymbol, and ThemeLeftArrowSymbol are defined in the theme files.
- The toggle button theme resources (ConsoleExpanderToggleButtonUpTheme, ConsoleExpanderToggleButtonDownTheme, ConsoleExpanderToggleButtonLeftTheme, ConsoleExpanderToggleButtonRightTheme, ConsoleExpanderToggleButtonRightTheme2, and ConsoleExpanderToggleButtonLeftTheme2) are all properly declared in Expander.axaml.
Since there are no unresolved static resource references, the ReSharper disable comment appears to be unnecessary—unless ReSharper still produces design-time warnings due to limitations in how it resolves merged dictionaries. Otherwise, you can safely remove the suppression comment.
| <helpers:SymbolsControl Text="[" | ||
| Foreground="{TemplateBinding BorderBrush}" /> | ||
| <helpers:CaretControl Grid.Column="1" | ||
| <console:CaretControl Grid.Column="1" |
There was a problem hiding this comment.
This gonna look weird. I'm proposing leave CaretControl in the middle for TurboVision theme, and for other hide CaretControl at all. For that we should create some additional property for CaretControl, lets say "IsHiddenOverride" and set it in a theme.
There was a problem hiding this comment.
This is caret with symbols
This is caret with turbo
normally controls indicate they have the focus with a psuedoStyle that highlights the control somehow. I do find the blinking cursor is useful.
Some thoughts:
- Could put the caret in front of the label text for Checkbox/Radio button, This is effectively where we put the caret for Button controls.
- Slider control doesn't have text, but it's useful to know that the control has the focus as left/right arrow change the value, currently I put it at the front of the slider.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/Consolonia.Themes/TurboVision/TurboVisionTheme.axaml (4)
5-9: Consider using platform-independent paths.While the resource dictionary merge works, consider using platform-independent paths with
avares://URI scheme for better cross-platform compatibility and to avoid potential path resolution issues.- <MergeResourceInclude Source="/TurboVision/TurboVisionDefaultColors.axaml" /> + <MergeResourceInclude Source="avares://Consolonia.Themes/TurboVision/TurboVisionDefaultColors.axaml" />
43-44: Consider using distinct symbols for different actions.Using the same symbol "X" for both
ThemeCheckedSymbolandThemeCloseSymbolmight be confusing for users. Consider using distinct symbols to better convey their different purposes.- <x:String x:Key="ThemeCheckedSymbol">X</x:String> - <x:String x:Key="ThemeCloseSymbol">X</x:String> + <x:String x:Key="ThemeCheckedSymbol">✓</x:String> + <x:String x:Key="ThemeCloseSymbol">✕</x:String>
46-47: Consider using distinct symbols for toggle states.Using the same symbol "■" for both on and off states of the toggle switch doesn't provide visual feedback about the current state.
- <x:String x:Key="ThemeToggleSwitchOffSymbol">■</x:String> - <x:String x:Key="ThemeToggleSwitchOnSymbol">■</x:String> + <x:String x:Key="ThemeToggleSwitchOffSymbol">□</x:String> + <x:String x:Key="ThemeToggleSwitchOnSymbol">■</x:String>
51-51: Consider using platform-independent paths for style includes.Similar to the resource dictionary merge, consider using the
avares://URI scheme for better cross-platform compatibility.- <StyleInclude Source="/Templates/Controls/AllControls.axaml" /> + <StyleInclude Source="avares://Consolonia.Themes/Templates/Controls/AllControls.axaml" />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Consolonia.Themes/Consolonia.Themes.csproj(0 hunks)src/Consolonia.Themes/TurboVision/TurboVisionTheme.axaml(1 hunks)
💤 Files with no reviewable changes (1)
- src/Consolonia.Themes/Consolonia.Themes.csproj
🧰 Additional context used
🧠 Learnings (1)
src/Consolonia.Themes/TurboVision/TurboVisionTheme.axaml (1)
Learnt from: tomlm
PR: jinek/Consolonia#122
File: src/Consolonia.Themes.TurboVision/Themes/TurboVisionDark/TurboVisionDarkColors.axaml:5-28
Timestamp: 2024-11-12T04:11:38.190Z
Learning: In the project, using `Mode="Shaded"` in `ConsoleBrush` within theme files is intentional and does not cause runtime issues.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (2)
src/Consolonia.Themes/TurboVision/TurboVisionTheme.axaml (2)
1-4: LGTM! Namespace declarations and class definition are well-structured.The namespace declarations follow standard conventions, and the class name appropriately reflects its purpose as a TurboVision theme.
53-59: LGTM! Consistent margin settings for CaretControl.The margin settings are consistently applied between CheckBox and RadioButton CaretControl, which helps maintain visual harmony.
Co-authored-by: <[email protected]>
Current symbols in theme files: