Handle paragraph alignment and direction in newline rectangles

[DeMonkeyCoder]

Based on #17744 and fixes flutter/flutter#53057

[GaryQian]

Thanks for handling the RTL case, really appreciate the extra pair of eyes on this.

This will need a test before it goes in in order to stay in sync with the currently-under-development SkParagraph by Skia. Look at third_party/txt/tests/paragraph_unittests.cc where you should be able to adapt on of the existing tests to verify this behavior.

cc @jason-simmons This version handles the RTL case, we should go ahead with this instead.

[GaryQian]

Code looks good! Just missing a test :)

[DeMonkeyCoder]

I saw the test file but I don't how to run these tests locally.
Can you tell me, please? @GaryQian

[GaryQian]

I was just mid typing when you made the changes about tracking per line. Thanks.

The tests can be run by building a host debug engine build, and running the txt_unittests executable in the engine build directory. So ./txt_unittests

[GaryQian]

Note that the tests are meant to be run on linux, but should still be able to run on mac or windows. Some may be disabled on those platforms though due to differences in the values provided by coretext and opentype.

[DeMonkeyCoder]

This test fails. Do you know why? @GaryQian

[ RUN      ] ParagraphTest.GetRectsForRangeCenterParagraphNewlineCentered
../../flutter/third_party/txt/tests/paragraph_unittests.cc:4017: Failure
Expected equality of these values:
  boxes[0].rect.bottom()
    Which is: 73
  75
[  FAILED  ] ParagraphTest.GetRectsForRangeCenterParagraphNewlineCentered (169 ms)

[GaryQian]

What OS are you running this on? Things should pass on linux, but if it is windows or mac, we may have missed a disable here or there.

[DeMonkeyCoder]

I'm running tests on Debian 10 Linux and it failed.
Would it be fine if I replace that line with this one in the test?
EXPECT_FLOAT_EQ(boxes[0].rect.bottom(), paragraph->line_metrics_[1].height);

[GaryQian]

Hmm, can you try pushing your code and seeing if the presubmit tests pass? If it does, I would just ignore it. There can be minor differences based on the exact OS you are running, which we expect. We only require maintaining a passing state across our testing infra.

In particular, fonts are often interpreted slightly different across platforms. This usually manifests itself in places such as line height, which I suspect is what is happening here.

Although the latency is higher, feel free to push changes and look at the presubmit runs to see the results. build_and_test_linux_unopt_debug is the relevant one here, where the txt_unittests run under test host

[DeMonkeyCoder]

Tests are successful but it looks like the solution is still not reliable in mixed bidi runs in one line.
Still working on this.

[DeMonkeyCoder]

So I changed my code to get the newline position from the correct ending of the last bidi run.
Also added 6 tests to check for every direction (RLT, LTR) and alignment (left, right, center).
Please review again.
Thanks :D
@GaryQian

[jason-simmons]

Use newline_box_positions.find(line_number) here

[DeMonkeyCoder]

If newline is in the selected range, it's position is evaluated in one related bidi run in the previous loop.
So we are sure about the value.
I also don't know how to handle find() here or what to do if it's value becomes end()

[jason-simmons]

Try this:

        auto it = newline_x_positions.find(line_number);
        if (it != newline_x_positions.end()) {
          x = it->second;
        } else {
          x = GetLineXOffset(0, line_number, false);
        }

If there are glyphs on the line, then newline_x_positions will have a value for the line and that value should be used. If not, then use the offset for an empty line.

[DeMonkeyCoder]

You already handled that case in the if statement.

if (glyph_lines_[line_number].positions.empty()) {
    x = GetLineXOffset(0, line_number, false);
} else {
    x = newline_x_positions[line_number];
}

[jason-simmons]

The goal is to ensure that the code does not accidentally use a newline_x_positions value that was never set.

If something goes wrong and glyph_lines_[line_number].positions is nonempty but newline_x_positions[line_number] was never set, then newline_x_positions[line_number] is going to yield a default value. That will produce an unintended result.

Checking newline_x_positions.find(line_number) will ensure that this does not happen.

[DeMonkeyCoder]

Done.

[jason-simmons]

The newline_x_positions.find(line_number) check should replace the glyph_lines_[line_number].positions.empty() check. Both checks should always have the same value if everything is working as expected, but using newline_x_positions is safer as discussed above.

Remove the if glyph_lines_[line_number].positions.empty() block, and just keep the block inside the else below.

[DeMonkeyCoder]

I got it now.
Done.
Thanks for your review

[jason-simmons]

Thank you for your contribution!

Please rebase this onto the current flutter/engine master branch.

I removed the line_number parameter from GetLineXOffset, so that call will need to be updated.

[DeMonkeyCoder]

Done

[fluttergithubbot]

This pull request is not suitable for automatic merging in its current state.

• This pull request has changes requested by @GaryQian. Please resolve those before re-applying the label.