Breaking: Always use AttributedStringBox instead of AttributedString in TextLayoutManager #46104
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
facebook-github-bot
added
CLA Signed
|
This pull request was exported from Phabricator. Differential Revision: D61484999 |
|
FYI @j-piasecki @tomekzaw |
|
This pull request was exported from Phabricator. Differential Revision: D61484999 |
NickGerleman
added a commit
to NickGerleman/react-native
that referenced
this pull request
Aug 19, 2024
…in TextLayoutManager (facebook#46104) Summary: Pull Request resolved: facebook#46104 We want to use `PrecomputedText` to store glyph-level measurements on underlying Android Spannable. This means we need to consistently reuse the same Spannable, instead of recreating them on measurement. We have an opaque cache ID used by Android, for spannables originating from uncontrolled TextInput on UI-thread side. We also have `AttributedStringBox`, for a kind of similar purpose on iOS, which allows passing opaque pointer to the `TextLayoutManager`. This is only used for the `measure` function. This change makes us consistently use `AttributedStringBox` at the TextLayoutManager boundary, to let us migrate calls across TextLayoutManager to all pass opaque handle to underlying Spannable we will store, instead of passing the AttributedString each time. For now, every place previously passing an AttributedString value still passes one. There were also some egregious cases of accepting very large structures by value, causing unneeded copies. I changed the APIs to accept anything potentially larger than two pointers to pass by reference instead. This change is technically breaking, to any 3p code calling into TextLayoutManager (IIRC live-markdown exposed prefabs for this, but should be able to adapt fairly easily). Changelog: [General][Breaking] - Always use AttributedStringBox instead of AttributedString in TextLayoutManager Differential Revision: D61484999
NickGerleman
force-pushed
the
export-D61484999
branch
from
0779e6f
to
ccf7da7
Compare
…in TextLayoutManager (facebook#46104) Summary: Pull Request resolved: facebook#46104 We want to use `PrecomputedText` to store glyph-level measurements on underlying Android Spannable. This means we need to consistently reuse the same Spannable, instead of recreating them on measurement. We have an opaque cache ID used by Android, for spannables originating from uncontrolled TextInput on UI-thread side. We also have `AttributedStringBox`, for a kind of similar purpose on iOS, which allows passing opaque pointer to the `TextLayoutManager`. This is only used for the `measure` function. This change makes us consistently use `AttributedStringBox` at the TextLayoutManager boundary, to let us migrate calls across TextLayoutManager to all pass opaque handle to underlying Spannable we will store, instead of passing the AttributedString each time. For now, every place previously passing an AttributedString value still passes one. There were also some egregious cases of accepting very large structures by value, causing unneeded copies. I changed the APIs to accept anything potentially larger than two pointers to pass by reference instead. This change is technically breaking, to any 3p code calling into TextLayoutManager (IIRC live-markdown exposed prefabs for this, but should be able to adapt fairly easily). Changelog: [General][Breaking] - Always use AttributedStringBox instead of AttributedString in TextLayoutManager Differential Revision: D61484999
|
This pull request was exported from Phabricator. Differential Revision: D61484999 |
NickGerleman
force-pushed
the
export-D61484999
branch
from
ccf7da7
to
611530c
Compare
|
This pull request has been merged in ee597bf. |
|
This pull request was successfully merged by @NickGerleman in ee597bf When will my fix make it into a release? | How to file a pick request? |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary:
We want to use
PrecomputedTextto store glyph-level measurements on underlying Android Spannable. This means we need to consistently reuse the same Spannable, instead of recreating them on measurement.We have an opaque cache ID used by Android, for spannables originating from uncontrolled TextInput on UI-thread side. We also have
AttributedStringBox, for a kind of similar purpose on iOS, which allows passing opaque pointer to theTextLayoutManager. This is only used for themeasurefunction.This change makes us consistently use
AttributedStringBoxat the TextLayoutManager boundary, to let us migrate calls across TextLayoutManager to all pass opaque handle to underlying Spannable we will store, instead of passing the AttributedString each time. For now, every place previously passing an AttributedString value still passes one.There were also some egregious cases of accepting very large structures by value, causing unneeded copies. I changed the APIs to accept anything potentially larger than two pointers to pass by reference instead.
This change is technically breaking, to any 3p code calling into TextLayoutManager (IIRC live-markdown exposed prefabs for this, but should be able to adapt fairly easily).
Changelog:
[General][Breaking] - Always use AttributedStringBox instead of AttributedString in TextLayoutManager
Differential Revision: D61484999