Fixes Wide Glyphs

[BDisp]

Fixes

• Fixes #4466
• Fixes #4480

Proposed Changes/Todos

• [ x] Reversing the hierarchy of runnables so that the current runnable is the last to be drawn.

Pull Request checklist:

• [x] I've named my PR in the form of "Fixes #issue. Terse description."
• [x] My code follows the style guidelines of Terminal.Gui - if you use Visual Studio, hit CTRL-K-D to automatically reformat your files before committing.
• [x] My code follows the Terminal.Gui library design guidelines
• [x] I ran dotnet test before commit
• [ ] I have made corresponding changes to the API documentation (using /// style comments)
• [x] My changes generate no new warnings
• [x] I have checked my code and corrected any poor grammar or misspellings
• [x] I conducted basic QA to assure all features are working

[codecov[bot]]

Codecov Report

:white_check_mark: All modified and coverable lines are covered by tests. :white_check_mark: Project coverage is 77.64%. Comparing base (c19ca48) to head (2c6df9a). :warning: Report is 1 commits behind head on v2_develop.

Additional details and impacted files

@@              Coverage Diff               @@
##           v2_develop    #4465      +/-   ##
==============================================
+ Coverage       77.61%   77.64%   +0.02%     
==============================================
  Files             386      386              
  Lines           44775    44775              
  Branches         6301     6301              
==============================================
+ Hits            34753    34765      +12     
+ Misses           8166     8155      -11     
+ Partials         1856     1855       -1     

Files with missing lines	Coverage Δ
Terminal.Gui/Drawing/Thickness.cs	98.46% <ø> (ø)
Terminal.Gui/ViewBase/Adornment/Margin.cs	93.54% <100.00%> (+4.03%)	:arrow_up:

... and 1 file with indirect coverage changes

---

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update c19ca48...2c6df9a. Read the comment docs.

:rocket: New features to boost your workflow:

• :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• :package: JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[BDisp]

This fix all the wide glyphs issue without breaks clipping.

[BDisp]

This was working perfectly locally but something with the merge broke it. I'll take a look.

[BDisp]

One of the advantages of this PR is that the margins are drawn during the superview and subview cycle, thus avoiding storing all the margin clipping regions and only drawing them at the end after all the runnables have been drawn.

[BDisp]

The commit https://github.com/gui-cs/Terminal.Gui/pull/4465/commits/52028d753fafd8b9748a634dc680a115502d3821 fixes an issue where the attribute of the next column of a wide glyph was not assigned, and moving a view over it displayed an incorrect attribute.

The issue:

After the fix:

[tig]

@BDisp this PR is impressive and a great refactoring of how Draw works to make it simpler. You also get all the credit for finding a fix for the actual bug!

Nice job!!

However, the bug reported in #4466 (which I've renamed now that we know the precise issue) had nothing to do with the View.Draw flow.

The unit tests that I added to my PR:

• #4486

clearly reproduce the bug without using any View (or even Application) logic. And the fix clearly fixes the problem without changing any code in anything other than OutputBufferImpl.cs and OutputBase.cs.

Thank you for finding the actual fix; I was able to use that to recreate the tests and then independently implement the same fix.

I suggest you rename this PR to "Simplifies View.Draw logic" and to update the first comment to be just about that.

Cool?

[BDisp]

Done. If you think I should change anything else, please let me know.

[tig]

Please code review

• #4486

Once we agree that fixes the issue (I've tested the heck out of it, but may have missed something) I'll merge it.

Then, this PR can be just about refactoring the View.Draw flow.

[BDisp]

I think it's done. What I don't understand is why, even though you don't find a PR as perfect as you'd like, but one that essentially solves many of the problems that are communicated and expressed, why don't you just merge it immediately so you can make your changes later? That way, you'd avoid having to resolve many merging conflicts, etc. Anyway, I think it's become a habit and sometimes discouraging.

[tig]

This breaks DrawIndicator for any IRunnable that IsModal:

Before this PR:

After:

[tig]

This PR is not done. I have not had a chance to review it. You changed some pretty fundamental logic here, that I spent a lot of time refining. I want to review it thoroughly to make sure I understand it and to ensure nothing is broken.

[BDisp]

If it's broken, it's not my fault. In my PR, it's perfect.

[BDisp]

I got confused with the Arrange button. The DrawIndicator isn't showing up. I'll take a look.

[tig]

Shadows are not being drawn transparent. IIRC, this is why I had the logic to reverse the views and the out-of-band Margin clipping regions.

Before:

After:

[BDisp]

I don't think this is actually a bug. If the view you're moving belongs to the same superview as the other view you're overlaying, it has to be added last if you want the desired effect. If that's the case, then you have to add that view to the end of the list to be the last one drawn. Otherwise, you'll be going against the logic of subviews belonging to the same superview. If you contradict this, then you'll be making patchwork solutions that could cause other issues.

[BDisp]

The way to do what you want, if you want the current view that is being overlaid on the others, is to change its position in the subview index when you click on it, so that it is the last one to be drawn, thus overlaying other windows.

[BDisp]

I just tested it with the v2_develop branch without merged your PR, and the DrawIndicator doesn't appear either. Therefore, this issue already existed and has nothing to do with this PR.

[tig]

Re: Transparent Shadows. This test succeeds on v2_develop but fails on this branch. Perhaps you can use it to track down how to fix them in this PR.

[Fact]
public void TransparentShadow_Draws_Transparent_At_Driver_Output ()
{
    // Arrange
    IApplication app = Application.Create ();
    app.Init ("fake");
    app.Driver!.SetScreenSize (5, 3);

    // Force 16-bit colors off to get predictable RGB output
    app.Driver.Force16Colors = false;

    var superView = new Runnable
    {
        Width = Dim.Fill (),
        Height = Dim.Fill (),
        Text = "ABC".Repeat (40)!
    };
    superView.SetScheme (new (new Attribute (Color.White, Color.Blue)));
    superView.TextFormatter.WordWrap = true;

    // Create an overlapped view with transparent shadow
    var overlappedView = new View
    {
        Width = 4,
        Height = 2,
        Text = "123",
        Arrangement = ViewArrangement.Overlapped,
        ShadowStyle = ShadowStyle.Transparent
    };
    overlappedView.SetScheme (new (new Attribute (Color.Black, Color.Green)));

    superView.Add (overlappedView);

    // Act
    SessionToken? token = app.Begin (superView);
    app.LayoutAndDraw ();
    app.Driver.Refresh ();

    // Assert
    _output.WriteLine ("Actual driver contents:");
    _output.WriteLine (app.Driver.ToString ());
    _output.WriteLine ("\nActual driver output:");
    string? output = app.Driver.GetOutput ().GetLastOutput ();
    _output.WriteLine (output);

    DriverAssert.AssertDriverOutputIs ("""
                                       \x1b[38;2;0;0;0m\x1b[48;2;0;128;0m123\x1b[38;2;0;0;0m\x1b[48;2;189;189;189mA\x1b[38;2;0;0;255m\x1b[48;2;255;255;255mBC\x1b[38;2;0;0;0m\x1b[48;2;189;189;189mABC\x1b[38;2;0;0;255m\x1b[48;2;255;255;255mABCABC
                                       """, _output, app.Driver);

    // The output should contain ANSI color codes for the transparent shadow
    // which will have dimmed colors compared to the original
    Assert.Contains ("\x1b[38;2;", output); // Should have RGB foreground color codes
    Assert.Contains ("\x1b[48;2;", output); // Should have RGB background color codes

    // Verify driver contents show the background text in shadow areas
    int shadowX = overlappedView.Frame.X + overlappedView.Frame.Width;
    int shadowY = overlappedView.Frame.Y + overlappedView.Frame.Height;

    Cell shadowCell = app.Driver.Contents! [shadowY, shadowX];
    _output.WriteLine ($"\nShadow cell at [{shadowY},{shadowX}]: Grapheme='{shadowCell.Grapheme}', Attr={shadowCell.Attribute}");

    // The grapheme should be from background text
    Assert.NotEqual (string.Empty, shadowCell.Grapheme);
    Assert.Contains (shadowCell.Grapheme, "ABC"); // Should be one of the background characters

    // Cleanup
    if (token is { })
    {
        app.End (token);
    }

    superView.Dispose ();
    app.Dispose ();
}

[tig]

I just tested it with the v2_develop branch without merged your PR, and the DrawIndicator doesn't appear either. Therefore, this issue already existed and has nothing to do with this PR.

Cool! Thanks for checking.

[BDisp]

@tig I renamed ShadowStyletests.cs file to ShadowTests.cs. If you look closely, it contains the tests that already existed plus the ones I added. If you don't mind, can I delete it?

[tig]

@tig I renamed ShadowStyletests.cs file to ShadowTests.cs. If you look closely, it contains the tests that already existed plus the ones I added. If you don't mind, can I delete it?

Yea, it needs to be renamed. Make sure you put the new transparent shadow test in there:

TransparentShadow_Draws_Transparent_At_Driver_Output

[BDisp]

@tig I renamed ShadowStyletests.cs file to ShadowTests.cs. If you look closely, it contains the tests that already existed plus the ones I added. If you don't mind, can I delete it?

Yea, it needs to be renamed. Make sure you put the new transparent shadow test in there:

TransparentShadow_Draws_Transparent_At_Driver_Output

Sure. Regarding testing, it's not easy to pinpoint the error exactly with ANSI. That's why I prefer using the AssertDriverAttributesAre method. Since you created this new method, you should be more familiar with how to troubleshoot the error.

[tig]

@tig I renamed ShadowStyletests.cs file to ShadowTests.cs. If you look closely, it contains the tests that already existed plus the ones I added. If you don't mind, can I delete it?

Yea, it needs to be renamed. Make sure you put the new transparent shadow test in there: TransparentShadow_Draws_Transparent_At_Driver_Output

Sure. Regarding testing, it's not easy to pinpoint the error exactly with ANSI. That's why I prefer using the AssertDriverAttributesAre method. Since you created this new method, you should be more familiar with how to troubleshoot the error.

AssertDriverAttributesAre only tests Contents. Because this use-case requires clipping to work propertly, we must test Output.

[BDisp]

Another problem that might be present in this unit test is that you're only using one schema. I think it's assuming Normal, but internally, depending on whether the view has focus, or has some HotNormal attribute, etc., the application changes the attributes. It's not easy to decipher ANSI colors without consulting a table, and frankly, without documenting the color value of each ANSI code, I won't be able to find the flaw.

[tig]

Another problem that might be present in this unit test is that you're only using one schema. I think it's assuming Normal, but internally, depending on whether the view has focus, or has some HotNormal attribute, etc., the application changes the attributes. It's not easy to decipher ANSI colors without consulting a table, and frankly, without documenting the color value of each ANSI code, I won't be able to find the flaw.

You can ignore attributes here. Just the Graphmeme's matter.

[Fact]
public void TransparentShadow_Draws_Transparent_At_Driver_Output ()
{
    // Arrange
    using IApplication app = Application.Create ();
    app.Init ("fake");
    app.Driver!.SetScreenSize (2, 1);
    app.Driver.Force16Colors = true;

    using Runnable superView = new ();
    superView.Width = Dim.Fill ();
    superView.Height = Dim.Fill ();
    superView.Text = "AB";
    superView.TextFormatter.WordWrap = true;
    superView.SetScheme (new (new Attribute (Color.Black, Color.White)));

    // Create view with transparent shadow
    View viewWithShadow = new ()
    {
        Width = Dim.Auto (),
        Height = Dim.Auto (),
        Text = "*",
        ShadowStyle = ShadowStyle.Transparent
    };
    // Make it so the margin is only on the right for simplicity
    viewWithShadow.Margin!.Thickness = new (0, 0, 1, 0);
    viewWithShadow.SetScheme (new (new Attribute (Color.Black, Color.White)));

    superView.Add (viewWithShadow);

    // Act
    app.Begin (superView);
    app.LayoutAndDraw ();
    app.Driver.Refresh ();

    // Assert
    _output.WriteLine ("Actual driver contents:");
    _output.WriteLine (app.Driver.ToString ());
    _output.WriteLine ("\nActual driver output:");
    string? output = app.Driver.GetOutput ().GetLastOutput ();
    _output.WriteLine (output);

    DriverAssert.AssertDriverOutputIs ("""
                                       \x1b[30m\x1b[107m*\x1b[90m\x1b[100mB
                                       """, _output, app.Driver);

}

Output:

Actual driver contents:
*B

Actual driver output:
\x1b[30m\x1b[107m*\x1b[90m\x1b[100mB

This makes it super obvious. If the B at the end is a , the transparent shadow isn't working.

[BDisp]

For subviews that are always on top and overlapping other views, the original code must be used at the end of each runnable. Similarly, with region clipping, only including the regions to be drawn and excluding the regions not to be drawn is irrelevant in the order in which they are drawn. For the same reason, it is irrelevant whether the content of the superview should be drawn before the subviews. So, now I recognize that with region clipping it works either way. I think I've introduced improvements and bug fixes. All the turbulence and instability observed was due to the bug with wide glyphs not rendering correctly, but I think I've helped improve it. That said, I think there's nothing else to fix and in that case it's ready to be reviewed. Thanks

[BDisp]

@tig I tried reverting to the original logic (drawing from the most exposed views) but the problem reported in the comment https://github.com/gui-cs/Terminal.Gui/pull/4465#issuecomment-3648431414 is occurring. If you can resolve this using the original logic, I don't mind if this PR is reverted.

[tig]

@tig I tried reverting to the original logic (drawing from the most exposed views) but the problem reported in the comment #4465 (comment) is occurring. If you can resolve this using the original logic, I don't mind if this PR is reverted.

I'd like to get a low-level repro of what that comment shows. There's a real bug there somewhere. I'll see if I can find some time today to do so. I did some quick hacking this morning but wasn't able to come up with anything super clear. I'll be using this PR to play:

• #4490

I think there's some really good refactoring you've done in this PR, as well as some really good new tests. I'd rather not lose that work!

[BDisp]

This is the unit test that test that issue. It's colors attribute that the only way to detect the issue. The 🦮 glyph is in a separated view with a different scheme to facilitate the test, which have the attribute 2 index.

https://github.com/BDisp/Terminal.Gui/blob/49cd6783024536e42cc82a2de2dbb74958ccd077/Tests/UnitTestsParallelizable/ViewBase/Adornment/BorderArrangementTests.cs#L95-L262

[BDisp]

the shadows with overlapped views is already fixed in this PR.