TextFormatter bugs

[tig]

This API is described as:

    /// <summary>
    ///     Returns the number of columns in the widest line in the list based on the <paramref name="startIndex"/> and
    ///     the <paramref name="length"/>.
    /// </summary>
    /// <remarks>
    ///     This API will return incorrect results if the text includes glyphs who's width is dependent on surrounding
    ///     glyphs (e.g. Arabic).
    /// </remarks>
    /// <param name="lines">The lines.</param>
    /// <param name="startIndex">The start index.</param>
    /// <param name="length">The length.</param>
    /// <param name="tabWidth">The number of columns used for a tab.</param>
    /// <returns>The maximum characters width.</returns>
    public static int GetWidestLineLength (
        List<string> lines,
        int startIndex = -1,
        int length = -1,
        int tabWidth = 0
    )

Based on this, this unit test should pass. It doesn't. The actual width returned is 1.

    [Theory]
    [InlineData (new [] { "0123456789" }, 10)]
    public void GetWidestLineLength_List_GetsWidth (IEnumerable<string> text, int expectedWidth
    )
    {
        Assert.Equal (expectedWidth, TextFormatter.GetWidestLineLength (text.ToList ()));
    }

I'm massively confused. Perhaps the API docs are completely wrong?

@BDisp can you advise?

[BDisp]

I understand your doubt. Maybe you can improve the documentation for this method. In fact, in this case it should return 1, because this method is used for the vertical direction of the text, to obtain the maximum size that a column will occupy in a set of characters. If in a line there is a glyph that occupies two columns then it will return 2. Therefore, it does not return the sum of the column sizes but only the maximum size occupied by a character.

[tig]

I understand your doubt. Maybe you can improve the documentation for this method. In fact, in this case it should return 1, because this method is used for the vertical direction of the text, to obtain the maximum size that a column will occupy in a set of characters. If in a line there is a glyph that occupies two columns then it will return 2. Therefore, it does not return the sum of the column sizes but only the maximum size occupied by a character.

Yeah, I think I see it now. On it...

[tig]

This helped me fix issues here:

    /// <summary>
    ///     Returns the number of columns required to render <paramref name="lines"/> oriented vertically.
    /// </summary>
    /// <remarks>
    ///     This API will return incorrect results if the text includes glyphs who's width is dependent on surrounding
    ///     glyphs (e.g. Arabic).
    /// </remarks>
    /// <param name="lines">The lines.</param>
    /// <param name="startLine">The line in the list to start with (any lines before will be ignored).</param>
    /// <param name="linesCount">The number of lines to process (if less than <c>lines.Count</c>, any lines after will be ignored).</param>
    /// <param name="tabWidth">The number of columns used for a tab.</param>
    /// <returns>The width required.</returns>
    public static int GetColumnsRequiredForVerticalText (
        List<string> lines,
        int startLine = -1,
        int linesCount = -1,
        int tabWidth = 0
    )

This was my bad. I renamed this method way back without really understanding what it did.

[BDisp]

Glad this helped fix the error. What matters is that now you know its purpose and it will certainly be well documented.

[tig]

Is it just me or is vertical text with multiple lines, except when centered horiz, completely broken?

Note, to test this, you have to use this PR which fixes a crash in the scenario and updates the API per above:

#3419

[tig]

While we're at it, perhaps you could help me understand why this test isn't working right

    [SetupFakeDriver]
    [Theory]
    [InlineData ("A", 0, "")]
    [InlineData ("A", 1, "A")]
    [InlineData ("A", 2, "A")]
    [InlineData ("A B", 3, "A B")]
    [InlineData ("A B", 1, "A")]
    [InlineData ("A B", 2, "A")]
    [InlineData ("A B", 3, "A B")]
    [InlineData ("A B", 4, "A B")] // BUGBUG: This should be "A  B"
    [InlineData ("A B", 5, "A B")]// BUGBUG: This should be "A   B"
    [InlineData ("A B", 6, "A B")]// BUGBUG: This should be "A   B"
    [InlineData ("A B", 10, "A B")]// BUGBUG: This should be "A      B"
    [InlineData ("ABC ABC", 10, "ABC ABC")] // BUGBUG: this is wrong
    [InlineData ("ABC ABC", 12, "ABC ABC")]  // BUGBUG: this is wrong
    public void Draw_Horizontal_Justified (string text, int width, string expectedText)
    {
        TextFormatter tf = new ()
        {
            Text = text,
            Alignment = TextAlignment.Justified,
        };
        tf.Draw (new Rectangle (0, 0, width, 1), Attribute.Default, Attribute.Default);

        TestHelpers.AssertDriverContentsWithFrameAre (expectedText, _output);
    }

[BDisp]

Only the ones in the middle vertically are correct. The ones on the sides are incorrect.

[tig]

Only the ones in the middle vertically are correct. The ones on the sides are incorrect.

I was not setting tf.Size. Nevermind.

[tig]

Now, I really did find a bug in TextAlignment.Right when tf.AutoSize = true:

    [SetupFakeDriver]
    [Theory]
    [InlineData ("A", 0, false, "")]
    [InlineData ("A", 1, false, "A")]
    [InlineData ("A", 2, false, " A")]
    [InlineData ("AB", 1, false, "A")]
    [InlineData ("AB", 2, false, "AB")]
    [InlineData ("ABC", 3, false, "ABC")]
    [InlineData ("ABC", 4, false, " ABC")]
    [InlineData ("ABC", 6, false, "   ABC")]

    [InlineData ("A", 0, true, "")]
    [InlineData ("A", 1, true, "A")]
    [InlineData ("A", 2, true, " A")]
    [InlineData ("AB", 1, true, "")] // BUGBUG: This is wrong, it should be "A"
    [InlineData ("AB", 2, true, "AB")]
    [InlineData ("ABC", 3, true, "ABC")]
    [InlineData ("ABC", 4, true, " ABC")]
    [InlineData ("ABC", 6, true, "   ABC")]
    public void Draw_Horizontal_Right (string text, int width, bool autoSize, string expectedText)
    {
        TextFormatter tf = new ()
        {
            Text = text,
            Alignment = TextAlignment.Right,
            AutoSize = autoSize,
        };

        if (!autoSize)
        {
            tf.Size = new Size (width, 1);
        }

        tf.Draw (new Rectangle (Point.Empty, new (width, 1)), Attribute.Default, Attribute.Default);
        TestHelpers.AssertDriverContentsWithFrameAre (expectedText, _output);
    }

[BDisp]

I've also seen that Button doesn't draw the right brackets in justified alignment in some situations. Have you ever noticed this?

[tig]

Now, I really did find a bug in TextAlignment.Right when tf.AutoSize = true:

(This is my bug. nevermind again... I can't repro it on v2_develop).

[tig]

Nevermind again and now mind...

It IS a bug in v2_develop. I just had the test disabled. This fails:

[InlineData ("AB", 1, true, "A")] // BUGBUG: This is wrong, it should be "A"

This works

[InlineData ("AB", 1, true, "")] // BUGBUG: This is wrong, it should be "A"

[tig]

Nevermind again and now mind...

It IS a bug in v2_develop. I just had the test disabled. This fails:

[InlineData ("AB", 1, true, "A")] // BUGBUG: This is wrong, it should be "A"

This works

[InlineData ("AB", 1, true, "")] // BUGBUG: This is wrong, it should be "A"

@BDisp since you wrote the TextFormatter.Draw code (?), could you look at this and see if you can see how to fix it?

[BDisp]

@BDisp since you wrote the TextFormatter.Draw code (?), could you look at this and see if you can see how to fix it?

If you don't mind, I'll look at it tomorrow. Thanks.

[dodexahedron]

A lot of this has already been fixed in code and documentation fixes I currently have outstanding.

I'd suggest not dealing with it right now, until that's done.

[dodexahedron]

There were a few test cases that were very clearly explicitly defined incorrectly to ignore what were probably deemed minor issues when it was done. Some even had comments explicitly saying that. Those now are defined correctly and pass correctly in what I've got in progress.

[BDisp]

If you don't mind, I'll look at it tomorrow. Thanks.

My laptop is broken and only on Tuesday the SSD will be ready. Sorry.

[tig]

If you don't mind, I'll look at it tomorrow. Thanks.

My laptop is broken and only on Tuesday the SSD will be ready. Sorry.

Bummer!

[BDisp]

Luckily it was ready early. Both are not correct. In horizontal alignment to the right, it is always the last letter of a word that is always placed to the right, in the case of left-to-right direction. In this case it will be "B". The following change works:

In the case of the vertical direction of the text, it still seems like you are confusing the function that returns the maximum width of a rune, whose value should be 1, 2 or the tab width and not the sum of all runes. Remember that the Draw method already separates new lines and therefore these functions do not have to deal with them.

[tig]

In horizontal alignment to the right, it is always the last letter of a word that is always placed to the right, in the case of left-to-right direction. In this case it will be "B".

I'm not following you here. Can you elaborate with a unit test? Or a visual?

[tig]

In horizontal alignment to the right, it is always the last letter of a word that is always placed to the right, in the case of left-to-right direction. In this case it will be "B".

I'm not following you here. Can you elaborate with a unit test? Or a visual?

Nevermind. I see now.

[BDisp]

Likewise, in the vertical alignment at the bottom, it is always the last letter of a word that is always placed at the bottom, in the case of the direction from top to bottom and it will also be “B”.

[tig]

• Fixes #3418. Vertical TextFormatter broken #3419

This PR fixes the vertical issue to some extent. However, wide runes are very broken and the way the current code is written will make it very challenging to fix.

Making the change suggested for the horizontal bug fixes that, but breaks other unit tests. I don't want to chase those down given how fragile TextFormatter.Draw is.

For

• Fixes #1785. Adds Pos.Align - Align sets of views horizontally or vertically #3415

I had the idea I could write a Justifier class that would handle the hard work of various justifications that could be used for Pos.Justify (replacing the fairly hacky code in Dialog now) AND could help really beef up how TextFormatter worked.

I finished the Justifier class last night along with what I think are a very robust set of unit tests. I encourage you to take a look at #3415 and see what you think.

I think the right fix for THIS PR is

a) Wait for @dodexahedron to get us the changes he made to TextFormatter. I'm assuming he made it far less fragile than it was.
b) Merge that into this PR.
c) Use the new Justifier class from #3415 to re-write TextFormatter.Draw to use it

[dodexahedron]

Yeah it is much more robust in its unicode handling.

I started trying to separate that out into a separate branch, but the changes I've had to make are extensive enough that it is a mess to do that. I got ambitious and tackled various problems that went deep/spidered out to other classes and/or that made it difficult to change it.

It actually is what started me on the whole source generator thing, because I found myself repeating some bits of code more than I liked and kept running into subtly different implementations of certain things. The change set touches dozens of files and goes clear down to some of the lower level stuff like the console drivers and Key. And all that needs to be rebased on changes that happened over the past month as well, so I've got a fair chunk of work to do on all that.

[BDisp]

The problem with this is not with the GetWidestLineLength method but with the GetLines method in which in the vertical text direction, it should return the linesFormatted exactly as it does for the horizontal text direction, but considering the height. The GetWidestLineLength method returns the maximum width based on the linesFormatted, the index where the linesFormatted should start, the size of the linesFormatted and the size of the TabWidth, passed in the parameters. These values must be passed correctly for the different directions and alignments of the text.

[dodexahedron]

Yes. The entire way all of that is handled has been redone.

[dodexahedron]

And I want to do more to it, but even when I was in the middle of working on it before I realized it would need to be a multi-stage process because it's just too much to dump all at once.

[BDisp]

But I think the method it works now is not the most correct. In the horizontal direction of the text it is the x coordinate (column) that is incremented and in the vertical direction of the text it is the y coordinate (row) that is incremented. Now practically the vertical text direction is totally broken, even in the v2_develop branch. I don't know how it got to this point.

[tig]

Yes. The entire way all of that is handled has been redone.

@dodexahedron would it be possible for you to share your WIP TextFormatter improvements?

I am sorta blocked on several PRs, including Dim.Auto because I want to fix-the-eff out of TextFormatter. I'd rather leverage your work than just go it alone.

[BDisp]

@tig initially I tried to fix the bugs using your branch, but you had already made a lot of changes, especially in the vertical direction of the text, which was confusing me and didn't help me fix them. As these bugs already existed in the v2_develop branch, I decided to fix them, as well as improve the centered and right alignment in the horizontal direction of the text and the middle and bottom alignment in the vertical direction of the text. I think these corrections and improvements can help you with your PR.

[tig]

@tig initially I tried to fix the bugs using your branch, but you had already made a lot of changes, especially in the vertical direction of the text, which was confusing me and didn't help me fix them. As these bugs already existed in the v2_develop branch, I decided to fix them, as well as improve the centered and right alignment in the horizontal direction of the text and the middle and bottom alignment in the vertical direction of the text. I think these corrections and improvements can help you with your PR.

I brought #3429 down locally and it's much better. Nice!

However, it's still broken for some vertical text scenarios with wide chars:

I will incorporate these changes into my Pos.Justify PR.

Now, I ask you to not work on this anymore, for now.. I do not think it is worth continuing to hack away at the old TextFormatter code to try to fix these vertical-text use cases. TextFormatter needs to be COMPLETELY RE-WRITTEN to not be so fragile. Let's see what @dodexahedron has come up with and how we can combine that with my new Justtification class.

Closing this issue.

[dodexahedron]

I totally missed the last few comments here.

I'll have to dig through my various stashes, though I do think at least some of it is already in one or more of the branches I had before.

But I agree pretty strongly with:

TextFormatter needs to be COMPLETELY RE-WRITTEN to not be so fragile.

And I got to writing more for this reply, but I need to go to bed and come back to it fresh.

I'll revise and finish up that clearly sleepy comment that it ultimately became tomorrow (Well...today, technicaly, in my time zone).

[tig]

I wanna be clear on my view:

• The current TextFormatter implementation works great for most scenarios.
• I made it even better in the dimauto PR.
• it is lacking great, primitive, unit tests. Most testing happens on Label and View unit tests which are also not great.
• I added more unit tests in the dimauto PR, but focused mostly on horizontal scenarios so tons more are needed.
• The Text Alignment Scenario does a good job of exposing bugs, but that's not a great of testing.

Rewriting TF is not a near term priority. We can ship v2 without rewriting it. This is because it DOES work great for almost all horizontal use cases and a lot of typical vertical cases.

Short term, any effort put into TextFormatter should be put into more unit tests. If they expose bugs, the tests should be annotated. Unless the bug is an obvious main-line use case we should not bother to fix it, but should ensure the eventual rewrite does.

When we DO rewrite it, I want us to start with a clear, complete, and concise design spec. Not code.

[tig]

One last point:

BDisp did an amazing job making TF support vertical text originally. He put a LOT of innovative work into it.

I, however, did a shitty job with the original implementation and as a result we have the fragility described above, which makes the work BDisp did even more impressive!

[dodexahedron]

All sounds great to me.

My experience while approaching it largely mirrored a lot of that, especially with regards to certain fixes exposing bugs that tests would have (and in some cases already did but were sidestepped) exposed.

It's an important piece of the puzzle because, after all, this is a text mode library. :)