search

go-text / typesetting

High quality text shaping in pure Go.
Other
100 stars 10 forks source link

Cleanup font packages structure #130

Closed benoitkugler closed 5 months ago

benoitkugler commented 9 months ago

Fixes #116

I'm afraid this PR will not be very pleasant to review, due to the numbers of files changed, but most changes are just renaming.

As proposed, it contains the following refactors :

Users should now be able to mostly use font as entry point.

As stated in #116 , this PR does not solve an actual issue, but makes our module a lot more coherent, so that new users should have an easier time discovering and understanding its logic.

It introduces backward incompatible changes, the more subtle one being that font.Font and font.Face are now struct types, not pointers.

andydotxyz commented 9 months ago

It introduces backward incompatible changes, the more subtle one being that font.Font and font.Face are now struct types, not pointers.

Can you comment on why this change was made? How is the new way better, for example? Just keen to understand as this is quite a wide hitting change.

benoitkugler commented 9 months ago

It introduces backward incompatible changes, the more subtle one being that font.Font and font.Face are now struct types, not pointers.

Can you comment on why this change was made? How is the new way better, for example? Just keen to understand as this is quite a wide hitting change.

It is better in the sense it removes indirection in the packages structure. Right now, font is just used as a shortcut to opentype/api/font, and, if we glance at the repository tree, we find both font and opentype, which is quite akward. Since we now are in control of the font library, it is simpler to remove this indirection.

Said otherwise, if we would write the font library from scratch, we would rather directly write the code in the font package, right ?

I'm not pretending this solves serious issues. It is more about overall coherence and cleaning up things. Let's say I would be a bit annoyed if I had to justify the current structure to, say, new users or developers (again, putting the historical reason apart).

andydotxyz commented 9 months ago

I see, I guess I didn't find the bit where "font.Face" was changed from a pointer, only the impact - can you remember which file it was?

benoitkugler commented 9 months ago

So, in our current version, font.Face is defined here : https://github.com/go-text/typesetting/blob/48cc05a56658160c485cbdbe274907ec430f8025/font/font.go#L16

With this PR, font.Face would now be the actual struct, defined here : https://github.com/go-text/typesetting/blob/52dad1cb99a5193721a91810d78c4be9e8dfdf4d/font/font.go#L487

(Same goes for font.Font)

andydotxyz commented 9 months ago

Thanks, that makes it quite plain. I guess in the future we should avoid aliasing to a different type to avoid this sort of need to break.

andydotxyz commented 9 months ago

I can see why this is desirable (and in many parts it was discussed before) but I think we need to discuss in the channel what to do regarding a break this big - as it will probably break any code using this API.