Replace Cairo Toy Text with Pango

[ForLoveOfCats]

Closes #222
These are initial changes to enable almost all of the Piet rich text API on Linux. It is not yet ready for merging. Rather than go the route of using skribo or even interfacing with HarfBuzz directly I decided to try to use Pango. For as long as we are pulling in Cairo then pulling in Pango as well seems reasonable.

At the time of submission two unit tests fail and the snapshots need to be updated.

• eol_hit_testing: This test is currently failing to get the expected value when hit testing a point at a soft line-break. I started to block in a workaround before I realized that I do not know enough about affinity to do so correctly.
• empty_layout_size: This test is failing on my system as oddly enough the non-empty layout is ~1.2 units shorter than the empty layout.
• Snapshots: I ran the snapshot comparison locally and of course it failed as this is an entirely different text stack and actually shapes the non-trivial text in test-7 ect. Once the eol_hit_testing test has been fixed someone with more expertise than me will need to look over test-11 before updating the snapshots

As I mentioned this is not the entire Piet rich text API. This does not (yet?) support Text::load_font() as Pango does not appear to support loading fonts from memory.

Unfortunately Pango makes the distinction between Left/Right alignment unlike our Start/End so that is not going to respect right-to-left scripts :(

There is an unsafe block in CairoTextLayout::update_width() that I'm not very happy about. I'm going to need to write up a PR to expose that upstream which we can use once released but this should suffice in the meantime.

That is all the notes I can think of at the moment. I fully expect to have overlooked some details and as mentioned there are a number of issues to resolve before merging so I don't expect to have this merged in overnight. However I am very excited at the prospect of having reasonable text support on Linux :)

[cmyr]

overall this looks great. it's nice to delete a bunch of old code, and it's nice to see that the implementation is relatively simple. Went over this with raph and we gave it a pretty thorough review. There are a few small logic errors which I've noted below; hopefully fixing these will help resolve some of the test failures and other questions; for instance sample #11 is drawing hits in the wrong position because we're incorrectly calculating the baseline position.

Other things you mention:

• eol_hit_testing test: if a line is wrapped, the offset of the linebreak corresponds to two different positions in the layout; it can either be the end of the first line, or the beginning of the second. Which position you want to use is generally a function of what user action moved the cursor to this offset. We don't keep track of that, and so for now we just always use 'downstream' affinity, and resolve the hit-test to the beginning of the second line. It is totally reasonable to special-case this.
• empty_layout_size: this is another special case; in the case that there is no text in the layout, we would still like to know how tall the layout would be if it did have text, so that we can do things like appropriately size an textbox based on the selected font. If necessary you should pick a default value based on either the reported extents of the default font, or else by just taking the default font size and multiplying by 1.2, which is a reasonable guess for the actual height of the font.
• Snapshots: totally natural that this would fail. When you have this patched up I would suggest just visually comparing the produced snapshots with the snapshots from the other backends, and looking for any major discrepancies; I'll do the same when you PR the updated snapshots.
• Unsafe: you might be able to avoid this by using the LayoutIter interface, which exposes a 'get_index` method for returning the current cursor position; I think your current solution is fine, though, and I think an upstream PR also makes sense.
• Font loading: raph thinks this is possible but might be a bit tricky. I think @derekdreery might have been looking into this as well (see zulip); we'll need to load the fonts ourselves, and then figure out how to get them into a font collection. This can definitely be future work, though.

In any case, I think this is pretty close to being ready to go, and that's great to see!

[richard-uk1]

Looks great, looking forward to playing with it :)

[richard-uk1]

It might be possible to load a font with freetype then hand off an FT_Face to pango. Not sure but possibly

[richard-uk1]

Looking some more, it seems pango really wants to push you down the path of using fontconfig. I found 1 example of manually creating a pango font description from a FcFace.

[ForLoveOfCats]

Apologies for the delays, I know they can make reviews much more difficult. I've been rather busy lately

I've fixed eol_hit_testing and improved the baseline situation. I wouldn't be surprised if they were still incorrect but snapshot 11 seems to look sane at this point. The test empty_layout_size remains broken as the values don't seem to me to logically connect with the font size value itself. Simply rounding the layout's size "fixes" the test by bringing the value within the assert_close! tolerance but that feels like a non-solution.

This has been rebased on current master as the update to Rust 1.50 caused some conflicts.

[richard-uk1]

Apologies for the delays

Don't apologise, it's awesome that you're doing this. I've got a few PRs open atm because I'm busy at college.

[cmyr]

@ForLoveOfCats fwiw the 'empty layout size' thing is always going to be a bit of a guess. I'm not sure exactly what you're doing here, but on macOS what we do is just store some placeholder values that we calculate based on the font, and then we use these values as a fallback when the layout is empty: see

piet/piet-coregraphics/src/text.rs

Line 169 in a0b7cd3

self.default_line_height = ascent + descent + leading;

In any case, the purpose of this is to let us give text boxes a reasonable initial height even if they don't contain any text; we mostly just want some sane value.

[ForLoveOfCats]

To be fair the value that it is currently producing is indeed sane (28.79 for empty layout height, 30 for non-empty layout height). So if you are happy with that then we can make the assertion slightly more tolerant.

If you could give snapshot 11 a glance over when you are able to. It looks pretty good to my untrained eye but there are some things at the bottom that make me thonk.

[cmyr]

@ForLoveOfCats how are you calculating the empty layout height? Can you point that out to me, just so I can double-check?

[ForLoveOfCats]

Pango itself is generating the value and currently that is used regardless of layout contents.

piet-cairo/src/text.rs lines 544-549

        let ink_extent = self.pango_layout.get_extents().0;
        let logical_extent = self.pango_layout.get_extents().1;
        self.size = Size::new(
            ink_extent.width as f64 / PANGO_SCALE,
            logical_extent.height as f64 / PANGO_SCALE,
        );

[cmyr]

hmm okay that looks slightly wrong; our size should be our logical size, and then the inking rect should use to calculate what we will return from image_bounds.

Not sure if this was explained well earlier, but there are two distinct ways of measuring the size of text; there are the typographic bounds (logical bounds in this case) and then there are the inking bounds. The typographic bounds are basically how big the text reports itself as being, but the text is allowed to draw outside of this area. A common example is certain combining accents, which might extent way above or below the reported size of the text they are part of; a more common example would be a fancy script font where, say, the stroke on the 'leg' of the Q might extend way off past the reported end of the Q glyph.

In any case I think this is mostly fine as written since we only care about height, and logical height should be good enough, here. We should also be using logical size as our base size, however.

[ForLoveOfCats]

I generally knew most of that from the documentation I was reading but at the time that I wrote those specific lines I was trying to be a bit too clever about trailing whitespace than I should have especially when Pango should handle that for us anyway. However a glyph painting outside logical bounds on the left and right is not something I had thought about but makes perfect sense.

Beyond that what are your thoughts on making the assertion slightly more tolerant? Additionally I would like to know how snapshot 11 looks to you once you are able to take a peek.

[ForLoveOfCats]

Ok no, using the logical width breaks trailing_whitespace_width test because Pango is including whitespace in the overall layout's logical width.

Edit: I have a local patch which uses the logical width but corrects for the whitespace width however it breaks the snapshot (I've also noticed a few other issues with the snapshot). Gonna look at it again tomorrow

[cmyr]

Okay so what I think you're going to have to do is to manually figure out what the non-whitespace width of each line is, probably by using pango_layout_line_index_to_x. So for the whole layout you will keep track of the max (logical) width both with and without whitespace; the width without whitespace we'll store in size, and then the width that includes whitespace will be stored separately, and only used when we want to calculate the return value for trailing_whitespace_width.

[cmyr]

I'm okay with making the assert more tolerant, but I'm curious why the value is different in the first place. If pango is reporting an actual height I would expect it to be correct? 😕

On snapshot 11:

You're right that something is wrong here. Looking at it visually, it appears that the points where are after the end of the layout are being resolved incorrectly; they should resolve to the end of the last line (the left edge) but they're resolving to the start of the last line.

I'm not sure exactly what's going on here. It could be an off-by-one, where we're accidentally resolving to the end of the second last line, or it could be some other logic error.

Another interesting thing is that in the bottom-left example, the text is being drawn outside of the layout. Given that the origin of these rects is fixed, this probably implies a miscalculation of the layout width (this would actually be explained reasonably well by the use of the inking-width, maybe?)

[ForLoveOfCats]

I have a local patch which does roughly that but I need to keep playing with it. On topic of the empty layout height I too do not like the situation, it's quite frustrating when it is so close to being correct but just isn't.

[cmyr]

Indeed. Maybe check the situation with various different fonts? One possibility is that there is a leading or lineGap value that is not being added to the height of the empty layout, but is being added to the height of other layouts? This value would likely be font-specific, so you might see some variation with different fonts? Something to experiment with at least.

[ForLoveOfCats]

I've fixed up the layout width to use the logical width but without the whitespace. I have also changed image_bounds to use the ink size of the layout.

Additionally I've tested all const-named fonts and checked their empty heights. Empty SYSTEM_UI was 1.2001953125 units smaller whereas the rest are all 1.0625 units smaller. I'll defer to you on how to best interpret those values.

I'm going to poke at snapshot 11 some more

[cmyr]

@ForLoveOfCats testing here, in a VM, I have identical values for those heights 🤔

[ForLoveOfCats]

As in you are getting the same heights for empty vs non-empty layouts (ie the test is passing) or you are getting the same values I got on my system?

[cmyr]

The test is passing; I'm getting identical values for all fonts I've tested, and they're all round numbers.

[ForLoveOfCats]

Very interesting, looking at the Ubuntu CI shows that many tests are failing whereas on my system empty_layout_size is the only test to fail but in the VM that you are checking in that specific test passes 🤔 Do all the tests pass for you?

[cmyr]

all of the piet-common tests pass. A number of tests in piet-cairo itself fail.

[cmyr]

This does feel to me like an issue related to the specific font that is being chosen on the different platforms.

[cmyr]

Ah: possibly related, possibly not, but we need to replace this line: https://github.com/linebender/piet/pull/389/files#diff-9dd8c7717a5acd0fefd853598bf7e7cf9484a5fee32069189304fee8abbb2f02L70

Here you should be checking the font collection for the existence of the font name provided, and only returning a FontFamily if one actually exists.

[ForLoveOfCats]

Could you point out the line in question via an inline comment? Github is refusing to resolve that link fully as the large diffs are collapsed by default

[cmyr]

I can't, because it didn't change in this diff, but it's the Text::font_family trait method, L167 or so.