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

text: Improve accuracy and performance of caret placement in text #18446

Merged
merged 8 commits into from
Nov 4, 2024

Conversation

kjarosh
Copy link
Member

@kjarosh kjarosh commented Nov 2, 2024

This PR fixes screen_position_to_index and makes some refactors:

  • the gutter is now subtracted instead of added when translating local position to layout coordinates,
  • text bounds are now taken into account in screen_position_to_index (it was broken when the bounds did not start at (0,0)),
  • binary search is now used for finding the line in screen_position_to_index.

This makes screen_position_to_index a lot more accurate.

Four tests are added:

  • text_caret_placement_leading – it checks how the caret is being placed in text depending on text leading,
  • text_caret_placement_align – it checks how the caret is being placed in text depending on text align,
  • text_caret_placement_translated_bounds – it checks how the caret is being placed when bounds do not start at (0,0),
  • text_caret_placement_scroll – it checks how the caret is being placed with non-zero horizontal and vertical scroll.

@kjarosh kjarosh added text Issues relating to text rendering/input A-core Area: Core player, where no other category fits T-fix Type: Bug fix (in something that's supposed to work already) labels Nov 2, 2024
Copy link
Collaborator

@adrian17 adrian17 left a comment

Choose a reason for hiding this comment

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

Just to be sure, this also corrects for scrolling? (I’m not familiar what the abstractions cover there)

@kjarosh kjarosh force-pushed the screen-position-to-index-fix branch from b5eaad6 to 89f11cc Compare November 2, 2024 10:20
@kjarosh
Copy link
Member Author

kjarosh commented Nov 2, 2024

Yes, scrolling worked okay, nonetheless I've added the test text_caret_placement_scroll while I was at it.

When transforming TextField local coordinates to layout coordinates,
we want to subtract the gutter, not add it.
This fixes the weird 4px constant translation when selecting text.
This method allows finding lines by their y coordinates with binary search.
This simplifies code a lot and makes it more performant.
This test verifies how the caret is placed in text with different leadings.
This test verifies how caret placement works depending on the align.
This test verifies how the caret placement works when
bounds do not start with 0,0.
This test verifies how the caret placement works with
non-zero vertical and horizontal scroll.
@torokati44 torokati44 enabled auto-merge (rebase) November 4, 2024 11:04
@torokati44 torokati44 merged commit 31a8101 into ruffle-rs:master Nov 4, 2024
22 checks passed
@kjarosh kjarosh deleted the screen-position-to-index-fix branch November 4, 2024 11:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-core Area: Core player, where no other category fits T-fix Type: Bug fix (in something that's supposed to work already) text Issues relating to text rendering/input
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants