Update Harfbuzz implementation; upgrade to Unicode 17

[benoitkugler]

This PR ports the latest changes in the Harfbuzz C++ implementation (roughly v12.3.0).
It also bumps our Unicode version to 17, and upgrades our Unicode Line Breaking implementation (in the segmenter package).

There is no API change, expect for a change in the method Face.GlyphVOrigin whose signature is now GlyphVOrigin(glyph GID) (x, y float32). This method is used internally in Harfbuzz, and this change should not affect end users.

[andydotxyz]

Wow, this is huge - any tips on how to review?

[benoitkugler]

Wow, this is huge - any tips on how to review?

Yes.. I'm sorry about that. I've ported all Harfbuzz changes in a go, so the diff is very large. In the future, I'll try to update more often.

Anyway, you can start by the changes in segmenter, which are self contained.

[benoitkugler]

Friendly ping @andydotxyz and @whereswaldon. Is there any chance you could have a look at this work ?

The changeset is large but mostly internal to harfbuzz or segmenter, and pretty well covered by the harfbuzz test suite and the Unicode reference tests.

[andydotxyz]

There is no API change

I could be wrong but it looks like there are some public API changes - in the tables packages it looks like a number of exported functions were removed (GetDelta/Evaluate).

The API change you mention:

- func (f *Face) GlyphVOrigin(glyph GID) (x, y int32, found bool) {
+ func (f *Face) GlyphVOrigin(glyph GID) (x, y float32) {

is a public API and not internal only. Should much of this have been in an internal package, or are they APIs useful publicly as well?

[benoitkugler]

There is no API change

I could be wrong but it looks like there are some public API changes - in the tables packages it looks like a number of exported functions were removed (GetDelta/Evaluate).

The API change you mention:

- func (f *Face) GlyphVOrigin(glyph GID) (x, y int32, found bool) {
+ func (f *Face) GlyphVOrigin(glyph GID) (x, y float32) {

is a public API and not internal only. Should much of this have been in an internal package, or are they APIs useful publicly as well?

You're right.

I initially thought our font package could have been useful for users wanting to manipulate fonts for use cases outside text shaping, but I'm starting to realize that it would have been much easier to have these features in an internal package, just as you said.

The situation is a bit delicate because the font package currently exposes a mix of real public API (like GlyphData) and "internal only" exported types and method (typically AATStateTable).
A solution could be to move the internal methods to the harfbuzz package. That would increase the size and complexity of harfbuzz, but it may be worth. (Opened #230 to discuss this idea.)

[benoitkugler]

The API change you mention:

- func (f *Face) GlyphVOrigin(glyph GID) (x, y int32, found bool) {
+ func (f *Face) GlyphVOrigin(glyph GID) (x, y float32) {

Also note that this method is not used outside ouf our module, as indicated by https://github.com/search?q=language%3Ago+GlyphVOrigin&type=code

[whereswaldon]

Ultimately, I cannot intelligently review all of this. I have tried to review the non-generated code changes, and I see nothing that concerns me. However, the magnitude of these changes makes careful review impossible. I would really prefer to avoid PRs like this in the future.

[benoitkugler]

Ultimately, I cannot intelligently review all of this. I have tried to review the non-generated code changes, and I see nothing that concerns me. However, the magnitude of these changes makes careful review impossible. I would really prefer to avoid PRs like this in the future.

I understand your point. Mixing Unicode changes with other Harfbuzz stuff was a bad idea. I'm sorry for that.

If it is important to you, I can still try and split this PR in several smaller ones.

[whereswaldon]

While splitting the PR into several smaller ones would make reviewing it easier, it would still be several multi-thousand line PRs. I trust you and your tests for this section of the codebase, and I doubt that the cost/benefit ratio of splitting this PR will be worthwhile after the fact.

In the future, I'd love to pick up fixes from upstream harfbuzz one-per-PR. That would make it easy to understand the intent of each fix.

[benoitkugler]

@andydotxyz Considering GlyphVOrigin is not used outside our module, have you any other concern merging this PR ?

[andydotxyz]

@andydotxyz Considering GlyphVOrigin is not used outside our module, have you any other concern merging this PR ?

I guess it's fine and we can continue the discussion on #230