Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Support text align on new text rendering pipeline #3147

Merged
merged 1 commit into from
May 6, 2024

Conversation

luanpotter
Copy link
Member

@luanpotter luanpotter commented May 5, 2024

Description

Support text align on new text rendering pipeline.

Notes

I am re-using Flutter's TextAlign enum for simplicity; however, it has extra constants we don't quite need. It has start and end that for us are synonyms of left and right because the text rendering pipeline does not support rtl text directions. It also has justify which this PR does not support as it is much more involved (though we could support later).

The other options were to either make a new enum, use a double from 0 to 1, or use the anchor class (but only take the horizontal component - vertically it doesn't make sense as it is always exactly line height). Of those options, I believe re-using the enum is cleaner.

That being said, this does touch on a crucial point that I am noticing: it seems we are rebuilding a lot of what Flutter already gives us for free. There is a whole TextPainter based rendering pipeline available in Flutter is decomposed from flutter components and could be leverage more directly leverage by Flame. It includes layouting, line breaking, text metrics, handles rtl text, etc etc. I have some doubts about our current structure, even though it was vastly simplified with this series of PRs, we still have two minorly overlapping systems that both also have varying degrees of overlap with inherited Flutter abstractions, with often less powerful features.

Results

image

image

Checklist

  • I have followed the Contributor Guide when preparing my PR.
  • I have updated/added tests for ALL new/updated/fixed functionality.
  • I have updated/added relevant documentation in docs and added dartdoc comments with ///.
  • I have updated/added relevant examples in examples or docs.

Breaking Change?

  • Yes, this PR is a breaking change.
  • No, this PR is not a breaking change.

@luanpotter luanpotter changed the title feat: Support text align on new text rendering pipeline [WIP] feat: Support text align on new text rendering pipeline May 6, 2024
@luanpotter luanpotter marked this pull request as ready for review May 6, 2024 01:29
@luanpotter luanpotter requested a review from spydon May 6, 2024 01:31
Copy link
Member

@spydon spydon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lgtm! If we can use flutters text rendering system I think that would definitely be worth a breaking change.

@luanpotter
Copy link
Member Author

I need to do some digging before coming up with a concrete proposal, but I do see it as a necessary improvement.

@luanpotter luanpotter merged commit 194d553 into main May 6, 2024
11 checks passed
@luanpotter luanpotter deleted the luan.text-align branch May 6, 2024 12:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants