Fixes #4258 - Glyphs drawn at mid-point of wide glyphs don't get drawn with clipping #4462
Conversation
`DoDrawComplete` method for clip region updates, and clarified terminology. Added detailed remarks for the `OnDrawComplete` method and `DrawComplete` event. Refactored `ViewDrawingClippingTests` to simplify driver setup, use target-typed `new`, and add a new test for wide glyph clipping with bordered subviews. Improved handling of edge cases like empty viewports and nested clips. Added `WideGlyphs.DrawFlow.md` and `ViewDrawingClippingTests.DrawFlow.md` to document the draw flow, clipping behavior, and coordinate systems for both the scenario and the test. Commented out redundant `Driver.Clip` initialization in `ApplicationImpl`. Added a `BUGBUG` comment in `Border` to highlight missing redraw logic for `LineStyle` changes.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## v2_develop #4462 +/- ##
===============================================
+ Coverage 64.13% 77.44% +13.30%
===============================================
Files 386 386
Lines 44590 44584 -6
Branches 6274 6268 -6
===============================================
+ Hits 28598 34526 +5928
+ Misses 14079 8212 -5867
+ Partials 1913 1846 -67
... and 172 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
Refactored `OutputBufferImpl.AddStr` to improve handling of wide glyphs: - Wide glyphs now modify only the first column they occupy, leaving the second column untouched. - Removed redundant code that set replacement characters and marked cells as not dirty. - Synchronized cursor updates (`Col` and `Row`) with the buffer lock to prevent race conditions. - Modularized logic with helper methods for better readability and maintainability. Updated `WideGlyphs.cs`: - Removed dashed `BorderStyle` and added border thickness and subview for `arrangeableViewAtEven`. - Removed unused `superView` initialization. Enhanced tests: - Added unit tests to verify correct rendering of borders and content at odd columns overlapping wide glyphs. - Updated existing tests to reflect the new behavior of wide glyph handling. - Introduced `DriverAssert.AssertDriverOutputIs` to validate raw ANSI output. Improved documentation: - Expanded problem description and root cause analysis in `WideGlyphBorderBugFix.md`. - Detailed the fix and its impact, ensuring proper layering of content at any column position. General cleanup: - Removed unused imports and redundant code. - Improved code readability and maintainability.
There was a problem hiding this comment.
Pull request overview
This PR fixes issue #4258 where wide glyphs (characters spanning 2 columns) prevented content drawn at odd columns from rendering correctly. The fix removes code that was incorrectly setting the second column of wide glyphs to a replacement character and marking it as non-dirty, allowing content to properly render at odd columns.
Key Changes:
- Refactored
OutputBufferImpl.AddStr()into smaller methods (AddGrapheme,WriteGraphemeByWidth, etc.) for better maintainability - Removed code that wrote replacement characters to column N+1 when wide glyphs were written at column N
- Added comprehensive unit tests demonstrating wide glyph clipping at even and odd column positions
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 14 comments.
Show a summary per file
| File | Description |
|---|---|
| Tests/UnitTestsParallelizable/ViewBase/Draw/ViewDrawingClippingTests.cs | Added three new test methods demonstrating wide glyph rendering with borders at different column positions (even/odd); updated existing tests to use cleaner syntax |
| Tests/UnitTestsParallelizable/ViewBase/Draw/ViewDrawingClippingTests.DrawFlow.md | New comprehensive documentation explaining the draw flow from initial Draw() call through to DrawingContent event firing |
| Tests/UnitTestsParallelizable/Drivers/OutputBaseTests.cs | Updated test expectations to reflect correct wide glyph behavior where column N+1 retains its dirty state |
| Tests/UnitTests/DriverAssert.cs | Added AssertDriverOutputIs method to verify raw ANSI output and UnescapeString helper for parsing C# escape sequences |
| Terminal.Gui/ViewBase/View.Drawing.cs | Enhanced documentation comments throughout draw methods to clarify clipping behavior and coordinate systems |
| Terminal.Gui/ViewBase/Adornment/Border.cs | Added BUGBUG comment noting that LineStyle setter should call SetNeedsDraw |
| Terminal.Gui/Drivers/OutputBufferImpl.cs | Core fix: refactored AddStr into smaller methods and removed code that incorrectly modified column N+1 for wide glyphs |
| Terminal.Gui/Drivers/DriverImpl.cs | Added internal Output property to expose IOutput for testing purposes |
| Examples/UICatalog/Scenarios/WideGlyphs.cs | Added demonstration views showing wide glyph clipping with borders at various positions |
| Examples/UICatalog/Scenarios/WideGlyphs.DrawFlow.md | New documentation explaining the complete draw flow from Application.Run through to DrawingContent event |
Comments suppressed due to low confidence (2)
Tests/UnitTestsParallelizable/ViewBase/Draw/ViewDrawingClippingTests.cs:654
- This assignment to codepoint is useless, since its value is never read.
Rune codepoint = Glyphs.Apple;
Tests/UnitTestsParallelizable/ViewBase/Draw/ViewDrawingClippingTests.cs:718
- This assignment to codepoint is useless, since its value is never read.
Rune codepoint = Glyphs.Apple;
Tests/UnitTestsParallelizable/ViewBase/Draw/ViewDrawingClippingTests.cs
Outdated
Show resolved
Hide resolved
Tests/UnitTestsParallelizable/ViewBase/Draw/ViewDrawingClippingTests.cs
Outdated
Show resolved
Hide resolved
BDisp
left a comment
BDisp
left a comment
There was a problem hiding this comment.
Your solution is more or less, with some additional method, the situation where the next column is defined with IsDirty as false. Now you just move forward without making any changes to the next column, which has the same effect. However, I had said that this was not the correct solution. You only made a patch that will further complicate the resolution of this issue. See the GIF below and see the button text flashing. I have already said, and I repeat, even if you disagree with me, that this issue is a hierarchy problem. When you have a subview with a border that is in a superview full of wide glyphs, you have to add it to the end of the list of subviews to be the last one to be redrawn. In the same way, if you have a modal view with a border along with another runnable, you have to redraw the runnable first, in case the modal view causes changes, and finally redraw the modal view to overlap the runnable. Regarding keyboard and mouse manipulation, the hierarchy is not reversed, that is, the current modal view has exclusive access to its own events. This principle has been in place since version v1, and regardless of how you try to circumvent it, you're just patching things up. I apologize for my insistence, but I believe I'm correct on this matter, unless you prove me wrong, in which case I will have no problem apologizing and acknowledging my error.
|
I think the most correct solution is PR #4465. I'm not referring to code optimization, but rather to the most logical approach to handling wide glyphs. The implementation of region clipping is indeed one of the most important features, and it practically doesn't need to respect the hierarchy, but when wide glyphs are altered by overlapping other views, then it's necessary to respect the hierarchy. Furthermore, the margin doesn't need special treatment compared to other adornments. |
…gTests.cs Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
…gTests.cs Co-authored-by: Copilot <[email protected]>
…gTests.cs Co-authored-by: Copilot <[email protected]>
|
I appreciate you think you are completely correct in this matter and unwilling to consider the evidence. I believe:
Please look at the tests that I created that 1000000% reproduce the drawing problem, at the Driver output point, without using any View hierarchy at all. Those tests pass once the fix is in place. They fail when the fix is not in place. Note how in these tests, I'm actually comparing the contents of Your code in #4465 breaks clipping, Shadows, and a bunch of other things because it completely un-does how Clipping works. I do not think you actually understand how Clipping works. You say " This principle has been in place since version v1", but this is not true. I reversed this principle when I implemented the new Draw flow, with clipping in this PR: I have reproduced the bug you are seeing by playing with the ChineseUI Scenario as you show. I can reproduce it without moving the MessageBox near the buttons. Just by hovering the mouse over the Ok button in the Message box. This indicates the bug is related to If you can show me a simple unit test that reproduces the HighlightStates bug you seeing, without having to drag the mouse around in a Scenario, and the fix for it also fixes what my code fixes, I'll change my mind. We now have all the test infrastructure to test things like this without having to resort to user testing. Please see |
|
Another issue where glyphs disappear from the screen. The next column must be set with IsDirty as false and if any view want to draw on that cell it set IsDirty as true, otherwise you'll see that flickering.
|
tig
changed the title
Wide GlyphsRefactored `InputProcessor` and `Output` access by replacing direct property usage with `GetInputProcessor()` and `GetOutput()` methods to enhance encapsulation. Introduced `GetLastOutput()` and `GetLastBuffer()` methods for better debugging and testability. Centralized `StringBuilder` usage in `OutputBase` implementations to ensure consistency. Improved exception handling with clearer messages. Updated tests to align with the refactored structure and added a new test for wide glyph handling. Enhanced ANSI sequence handling and simplified cursor visibility logic to prevent flickering. Standardized method naming for consistency. Cleaned up redundant code and improved documentation for better developer clarity.
tig
changed the title
|
Here's the latest test that illustrates, at the lowest level possible the bug. Without the fix this test fails. With it, it passes. Note it does not use anything but // Tests fix for https://github.com/gui-cs/Terminal.Gui/issues/4258
[Theory]
[InlineData ("fake")]
[InlineData ("windows")]
[InlineData ("dotnet")]
[InlineData ("unix")]
public void All_Drivers_When_Clipped_AddStr_Glyph_On_Second_Cell_Of_Wide_Glyph_Outputs_Correctly (string driverName)
{
IApplication? app = Application.Create ();
app.Init (driverName);
IDriver driver = app.Driver!;
// Need to force "windows" driver to override legacy console mode for this test
driver.IsLegacyConsole = false;
driver.Force16Colors = false;
driver.SetScreenSize (6, 3);
driver!.Clip = new (driver.Screen);
driver.Move (1, 0);
driver.AddStr ("┌");
driver.Move (2, 0);
driver.AddStr ("─");
driver.Move (3, 0);
driver.AddStr ("┐");
driver.Clip.Exclude (new Region (new (1, 0, 3, 1)));
driver.Move (0, 0);
driver.AddStr ("🍎🍎🍎🍎");
DriverAssert.AssertDriverContentsAre (
"""
�┌─┐🍎
""",
output,
driver);
driver.Refresh ();
DriverAssert.AssertDriverOutputIs (@"\x1b[38;2;0;0;0m\x1b[48;2;0;0;0m�┌─┐🍎\x1b[38;2;255;255;255m\x1b[48;2;0;0;0m",
output, driver);
}
} |
…t` classes to support access to `Output` and added a `IDriver.GetOutput` to acess the `IOutput`. `IOutput` now has a `GetLastOutput` method. Simplified `DriverAssert` logic and enhanced `DriverTests` with a new test for wide glyph clipping across drivers. Performed general cleanup, including removal of unused code, improved formatting, and adoption of modern C# practices. Added `using System.Diagnostics` in `OutputBufferImpl` for debugging.
2 participants
Fixes
Problem
Content drawn at odd columns (1, 3, 5, etc.) that overlap with wide glyphs (e.g., 🍎 which spans 2 columns) would not render correctly. The content would either not appear at all or render with incorrect colors.
Example: Border at column 1, with 🍎 at column 0
�┌─┐🍎(replacement char, border, apple)�─┐🍎(missing left border┌)Affected content:
LineCanvasRoot Cause
In
OutputBufferImpl.AddStr(), when writing a wide character at column N:�) to column N+1 ❌IsDirty = false❌The bug: When content was later drawn at column N+1:
IsDirty = trueIsDirty = false, causingOutputBase.Write()to skip itFix
Three changes to
OutputBufferImpl.AddStr():Removed code writing replacement char to column N+1
Removed code setting column N+1 to
IsDirty = falseMoved
Col++inside the lockWhy it works:
Code Quality Improvements
Refactored
AddStr()for readability (no functional changes):AddGrapheme()- per-grapheme logicInvalidateOverlappedWideGlyph()- handles overlap detectionWriteGraphemeByWidth(),WriteSingleWidthGrapheme(),WriteWideGrapheme()- separates concerns by character widthNetOutput,FakeOutput,UnixOutput, andWindowsOutputclasses to support access toOutputand added aIDriver.GetOutputto acess theIOutput.IOutputnow has aGetLastOutputmethod.Testing
New tests in
ViewDrawingClippingTests.cs:Draw_WithBorderSubView_At_Col1_In_WideGlyph_DrawsCorrectlyDraw_WithBorderSubView_At_Col3_In_WideGlyph_DrawsCorrectlyAll_Drivers_When_Clipped_AddStr_Glyph_On_Second_Cell_Of_Wide_Glyph_Outputs_Correctlytests that this bug is fixed for all drivers.Updated test in
OutputBaseTests.cs:Write_Virtual_Or_NonVirtual_Uses_WriteToConsole_And_Clears_Dirty_Flags_Mixed_Graphemes- now expects correct behaviorNew test infrastructure:
DriverAssert.AssertDriverOutputIs()- verifies Actual Driver Output as ANSI output (not justContentsstate).