fix caret cursor #212
fix caret cursor #212
Conversation
I changed CaretControl template to output a Rectangle and everything works.
📝 Walkthrough📝 WalkthroughWalkthroughThis pull request introduces comprehensive changes to the Consolonia framework's caret and drawing functionality. The modifications span multiple files and focus on enhancing caret style management and rendering. A new The changes include updates to the Key modifications involve adding a Possibly related PRs
Suggested labels
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 (
|
Co-authored-by: <[email protected]>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/Consolonia.Core/Drawing/DrawingContextImpl.cs (1)
471-478: Consider refactoring duplicate caret positioning logic.The implementation for handling
MoveConsoleCaretToPositionBrushis duplicated betweenDrawRectangleandDrawLineInternal. Consider extracting this common logic into a private method.Apply this diff to reduce code duplication:
+private void DrawCaretPosition(Point position) +{ + Point head = position.Transform(Transform); + CurrentClip.ExecuteWithClipping(head, + () => { _pixelBuffer.Set((PixelBufferCoordinate)head, pixel => pixel.Blend(new Pixel(true))); }); +} case MoveConsoleCaretToPositionBrush: { - Point head = r.TopLeft.Transform(Transform); - CurrentClip.ExecuteWithClipping(head, - () => - { - _pixelBuffer.Set((PixelBufferCoordinate)head, pixel => pixel.Blend(new Pixel(true))); - }); + DrawCaretPosition(r.TopLeft); return; } if (pen.Brush is MoveConsoleCaretToPositionBrush) { - Point head = line.PStart.Transform(Transform); - CurrentClip.ExecuteWithClipping(head, - () => { _pixelBuffer.Set((PixelBufferCoordinate)head, pixel => pixel.Blend(new Pixel(true))); }); + DrawCaretPosition(line.PStart); return; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Consolonia.Core/Drawing/DrawingContextImpl.cs(2 hunks)
🔇 Additional comments (1)
src/Consolonia.Core/Drawing/DrawingContextImpl.cs (1)
264-273: Verify the intentional use of only TopLeft for caret positioning.
The implementation uses only the TopLeft point of the rectangle for caret positioning, ignoring the width and height. While this appears intentional since we only want to move the caret to a single position, please confirm this is the desired behavior.
Run the following script to verify similar implementations:
✅ Verification successful
Based on the search results, I can now generate the final response as I have sufficient evidence about caret positioning behavior across the codebase:
The single-point caret positioning is the correct implementation
The codebase consistently treats the caret as a single-point entity:
TextBoxCaretdraws the caret using a single position point (Position)IConsoleinterface and its implementations (DummyConsole,InputLessDefaultNetConsole) handle caret as a single coordinate- The caret rendering in
RenderTargetalso treats it as a single position - The
CaretControltemplate usesMoveConsoleCaretToPositionBrushwith a single point
Therefore, using only the TopLeft point of the rectangle for caret positioning is the correct approach, as the caret is consistently implemented as a single-point cursor throughout the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if other caret-related code also uses single-point positioning
# Look for caret-related patterns in the codebase
rg -A 5 "(?i)(caret|cursor).*(position|move|draw)"
Length of output: 21353
|
Caret does not show up in ComboBoxItem. Behavior observed when opening ComboBox. |
|
Hmmm....I can't seem to figure out how to get comboboxItem to show focus. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/Consolonia.Core/Drawing/MoveConsoleCaretToPositionBrush.cs (2)
13-15: Fix typo in documentation commentThe word "cursor" is misspelled as "curosr" in the summary.
- /// style of curosr + /// style of cursor
13-16: Consider enhancing XML documentationWhile the property addition looks good, consider adding more detailed XML documentation to better describe the property's purpose and default value.
/// <summary> - /// style of cursor + /// Gets or sets the cursor style for the caret. + /// Default value is CursorStyle.BlinkingBar. /// </summary>src/Consolonia.Core/Drawing/PixelBufferImplementation/PixelBuffer.cs (1)
35-35: Add XML documentation and consider input validationThe property addition looks good but could benefit from the following improvements:
- Add XML documentation to describe the property's purpose and default value
- Consider adding validation in the setter to ensure only valid cursor styles are assigned
+ /// <summary> + /// Gets or sets the cursor style for the pixel buffer. + /// Default value is CursorStyle.BlinkingBar. + /// </summary> public CursorStyle CursorStyle { get; set; } = CursorStyle.BlinkingBar;Consider adding validation:
- public CursorStyle CursorStyle { get; set; } = CursorStyle.BlinkingBar; + private CursorStyle _cursorStyle = CursorStyle.BlinkingBar; + public CursorStyle CursorStyle + { + get => _cursorStyle; + set + { + if (!Enum.IsDefined(typeof(CursorStyle), value)) + throw new ArgumentException($"Invalid cursor style: {value}", nameof(value)); + _cursorStyle = value; + } + }src/Consolonia.Core/Drawing/DrawingContextImpl.cs (1)
Line range hint
760-841: Enhanced text rendering with proper character handling.The implementation properly handles:
- Tab characters with consistent spacing
- Wide characters with proper pixel management
- Empty pixels and overlapping scenarios
However, consider extracting the tab size to a constant at the class level for better maintainability.
+ private const int TabSize = 8; private void DrawStringInternal(IBrush foreground, string text, IGlyphTypeface typeface, Point origin = new()) { // ... existing code ... switch (glyph) { case "\t": { - const int tabSize = 8; - var consolePixel = new Pixel(new SimpleSymbol(' '), foregroundColor); + var consolePixel = new Pixel(new SimpleSymbol(' '), foregroundColor); - for (int j = 0; j < tabSize; j++) + for (int j = 0; j < TabSize; j++) { // ... existing code ... } - currentXPosition += tabSize - 1; + currentXPosition += TabSize - 1; } break;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/Consolonia.Core/Drawing/CursorStyle.cs(1 hunks)src/Consolonia.Core/Drawing/DrawingContextImpl.cs(7 hunks)src/Consolonia.Core/Drawing/MoveConsoleCaretToPositionBrush.cs(1 hunks)src/Consolonia.Core/Drawing/PixelBufferImplementation/PixelBuffer.cs(1 hunks)src/Consolonia.Core/Drawing/RenderTarget.cs(2 hunks)src/Consolonia.Core/Infrastructure/InputLessDefaultNetConsole.cs(0 hunks)src/Consolonia.Core/Text/Esc.cs(1 hunks)
💤 Files with no reviewable changes (1)
- src/Consolonia.Core/Infrastructure/InputLessDefaultNetConsole.cs
✅ Files skipped from review due to trivial changes (1)
- src/Consolonia.Core/Drawing/CursorStyle.cs
🔇 Additional comments (7)
src/Consolonia.Core/Text/Esc.cs (1)
34-40: LGTM! Well-organized cursor shape constants
The new cursor shape constants are correctly implemented using standard VT100 escape sequences and follow the existing code organization pattern.
src/Consolonia.Core/Drawing/RenderTarget.cs (3)
13-14: LGTM: Required imports for text and JSON handling.
The added imports support the new cursor style functionality.
146-156: LGTM: Comprehensive cursor style handling.
The switch expression properly maps each cursor style to its corresponding escape sequence, ensuring proper cursor rendering. The implementation is exhaustive and handles all cursor style cases.
162-162: LGTM: Proper cursor visibility management.
Correctly hides the cursor when no caret position is detected.
src/Consolonia.Core/Drawing/DrawingContextImpl.cs (3)
99-129: Improved bitmap drawing with better quad pixel handling.
The bitmap drawing logic has been enhanced with:
- Clearer variable naming and comments
- Proper handling of quad pixels
- Improved color blending
264-274: LGTM: Proper caret positioning with cursor style support.
The implementation correctly:
- Transforms the position
- Sets the cursor style
- Handles clipping
- Updates the pixel buffer
472-479: LGTM: Consistent cursor style handling in line drawing.
The implementation properly handles the MoveConsoleCaretToPositionBrush in line drawing context, maintaining consistency with the rectangle drawing implementation.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/Consolonia.Core/Drawing/RenderTarget.cs (1)
145-154: Consider enhancing cursor style handling.The switch expression effectively maps cursor styles to escape sequences. However, consider these improvements:
- Extract the cursor style mapping to a separate method for better separation of concerns.
- Add XML documentation explaining the different cursor styles.
- Consider providing a default style instead of throwing for unknown values.
Here's a suggested refactor:
+ /// <summary> + /// Maps cursor style to the corresponding escape sequence. + /// </summary> + private string GetCursorStyleEscapeSequence(CursorStyle style) => + style switch + { + CursorStyle.BlinkingBar => Esc.BlinkingBarCursor, + CursorStyle.SteadyBar => Esc.SteadyBarCursor, + CursorStyle.BlinkingBlock => Esc.BlinkingBlockCursor, + CursorStyle.SteadyBlock => Esc.SteadyBlockCursor, + CursorStyle.BlinkingUnderline => Esc.BlinkingUnderlineCursor, + CursorStyle.SteadyUnderline => Esc.SteadyUnderlineCursor, + _ => Esc.BlinkingBarCursor // Default to blinking bar + }; - _console.WriteText(pixelBuffer.CursorStyle switch - { - CursorStyle.BlinkingBar => Esc.BlinkingBarCursor, - CursorStyle.SteadyBar => Esc.SteadyBarCursor, - CursorStyle.BlinkingBlock => Esc.BlinkingBlockCursor, - CursorStyle.SteadyBlock => Esc.SteadyBlockCursor, - CursorStyle.BlinkingUnderline => Esc.BlinkingUnderlineCursor, - CursorStyle.SteadyUnderline => Esc.SteadyUnderlineCursor, - _ => throw new ArgumentOutOfRangeException() - }); + _console.WriteText(GetCursorStyleEscapeSequence(pixelBuffer.CursorStyle));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Consolonia.Core/Drawing/RenderTarget.cs(2 hunks)src/Consolonia.Core/Infrastructure/InputLessDefaultNetConsole.cs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Consolonia.Core/Infrastructure/InputLessDefaultNetConsole.cs
🔇 Additional comments (2)
src/Consolonia.Core/Drawing/RenderTarget.cs (2)
13-13: LGTM!
The added import for Consolonia.Core.Text is necessary for accessing the Esc class containing cursor style escape sequences.
155-155: Verify cursor visibility behavior across different terminal emulators.
The cursor visibility commands are properly synchronized with the CaretVisible property. However, it's important to verify that these escape sequences work consistently across different terminal emulators.
Also applies to: 161-161
✅ Verification successful
Based on the search results, I can now generate the final response:
Cursor visibility handling is implemented consistently across the codebase
The codebase shows a well-structured and consistent implementation of cursor visibility:
- Cursor commands are properly defined as ANSI escape sequences in
Esc.cs - The visibility state is tracked via
CaretVisibleproperty in theIConsoleinterface - All console implementations (
DummyConsole,InputLessDefaultNetConsole,UnitTestConsole) handle the property - The
RenderTargetclass correctly pairsShowCursor/HideCursorcommands withCaretVisiblestate changes
No terminal-specific handling was found, which is good as these are standard ANSI escape sequences supported by modern terminal emulators.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for cursor visibility-related code to ensure consistent handling
# Test: Look for other places where cursor visibility is managed
rg -A 2 "CaretVisible|ShowCursor|HideCursor"
# Test: Look for terminal-specific handling
ast-grep --pattern 'if ($TERM) { $$$ }'
Length of output: 2861
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/Consolonia.Core/Drawing/DrawingContextImpl.cs (1)
264-274: Consider combining the transform and coordinate cast operations.The implementation correctly handles the caret positioning, but we can slightly optimize the coordinate transformation:
- Point head = r.TopLeft.Transform(Transform); - _pixelBuffer.CaretStyle = moveBrush.CaretStyle; - CurrentClip.ExecuteWithClipping(head, - () => - { - _pixelBuffer.Set((PixelBufferCoordinate)head, pixel => pixel.Blend(new Pixel(true))); - }); + var head = (PixelBufferCoordinate)r.TopLeft.Transform(Transform); + _pixelBuffer.CaretStyle = moveBrush.CaretStyle; + CurrentClip.ExecuteWithClipping(head, + () => _pixelBuffer.Set(head, pixel => pixel.Blend(new Pixel(true))));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
src/Consolonia.Core/Drawing/CaretStyle.cs(1 hunks)src/Consolonia.Core/Drawing/DrawingContextImpl.cs(2 hunks)src/Consolonia.Core/Drawing/MoveConsoleCaretToPositionBrush.cs(1 hunks)src/Consolonia.Core/Drawing/PixelBufferImplementation/PixelBuffer.cs(1 hunks)src/Consolonia.Core/Drawing/RenderTarget.cs(2 hunks)src/Consolonia.Core/Infrastructure/InputLessDefaultNetConsole.cs(1 hunks)src/Consolonia.Themes/Templates/Controls/CaretControl.axaml(1 hunks)src/Consolonia.Themes/Templates/Controls/ComboBoxItem.axaml(1 hunks)src/Consolonia.Themes/Templates/Controls/Helpers/CaretControl.cs(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- src/Consolonia.Core/Drawing/PixelBufferImplementation/PixelBuffer.cs
- src/Consolonia.Core/Infrastructure/InputLessDefaultNetConsole.cs
- src/Consolonia.Themes/Templates/Controls/CaretControl.axaml
- src/Consolonia.Core/Drawing/RenderTarget.cs
🔇 Additional comments (9)
src/Consolonia.Core/Drawing/MoveConsoleCaretToPositionBrush.cs (3)
6-6: Inheritance change enables use of Avalonia styled properties
By inheriting from AvaloniaObject, the class can now define and use StyledPropertys, which is necessary for CaretStyleProperty.
8-9: CaretStyleProperty is correctly registered
The CaretStyleProperty is properly registered using AvaloniaProperty.Register, which is the correct approach for defining styled properties in Avalonia.
14-18: CaretStyle property is properly implemented
The CaretStyle property uses the CaretStyleProperty as its backing store with appropriate getter and setter methods, following Avalonia's property pattern.
src/Consolonia.Core/Drawing/CaretStyle.cs (1)
3-11: CaretStyle enumeration is well-defined
The CaretStyle enum provides clear and descriptive values for different caret styles, facilitating easy extension and maintenance.
src/Consolonia.Themes/Templates/Controls/Helpers/CaretControl.cs (3)
4-4: Added necessary using directive for Consolonia.Core.Drawing
Including using Consolonia.Core.Drawing; allows access to the CaretStyle enum within CaretControl.
13-14: CaretStyleProperty is correctly registered
The CaretStyleProperty is properly registered using AvaloniaProperty.Register, enabling styled property functionality.
22-26: CaretStyle property is properly implemented
The CaretStyle property correctly wraps CaretStyleProperty with getter and setter methods, adhering to Avalonia's conventions.
src/Consolonia.Themes/Templates/Controls/ComboBoxItem.axaml (1)
16-17: Applied CaretStyle to CaretControl in ComboBoxItem template
Setting CaretStyle="BlinkingBlock" enhances the caret visibility within ComboBoxItem, addressing the issue where the caret did not appear when the ComboBox is opened.
src/Consolonia.Core/Drawing/DrawingContextImpl.cs (1)
264-274: Implementation successfully fixes caret rendering.
The changes effectively address the caret rendering issue by:
- Using a pixel-based approach instead of lines
- Properly handling coordinate transformations
- Ensuring consistent caret rendering across different drawing operations
Also applies to: 472-479
|
I figured it out. I need to draw the movebrush AFTER the content is drawn, not before, otherwise the content resets the MoveBrush pixel |
Co-authored-by: <[email protected]>
#179 Caret is not being drawn by caret control.
For some reason the Line template wasn't being drawn. It never called into DrawingContext. I think it may be because we were drawing a line which had Width and Height but no start point or end point. I verified that Avalonia GUI ignores Lines with just width/height, so this is avalonia behavior.
I changed CaretControl template to output a Rectangle of width/1,height/1 and now it calls, gets rendered correctly.