Caret location improvements by LongCatIsLooong · Pull Request #113268 · flutter/flutter

LongCatIsLooong · 2022-10-11T06:48:39Z

Fixes The cursor height becomes shorter when I delete all characters from TextField #92080
Removes some usages of toPlainText, as it could be expensive.
getLocalRectForCaret now reports the real rect that will be painted.
move caret x-coordinate clamping to RenderEditable since TextPainter doesn't know about clipping.

Pre-launch Checklist

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
I signed the CLA.
I listed at least one issue that this PR fixes in the description above.
I updated/added relevant documentation (doc comments with ///).
I added new tests to check the change I am making, or this PR is test-exempt.
All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

LongCatIsLooong · 2022-10-11T06:58:58Z

packages/flutter/lib/src/painting/text_painter.dart

+  const _CaretMetrics({required this.offset, required this.fullHeight});
+  /// The offset of the top start corner of the caret from the top left
  /// corner of the paragraph.
  final Offset offset;


We're currently always painting to the right of this offset regardless of the writing direction. That's probably not right.

flutter-dashboard · 2022-10-11T22:35:14Z

Golden file changes have been found for this pull request. Click here to view and triage (e.g. because this is an intentional change).

If you are still iterating on this change and are not ready to resolve the images on the Flutter Gold dashboard, consider marking this PR as a draft pull request above. You will still be able to view image results on the dashboard, commenting will be silenced, and the check will not try to resolve itself until marked ready for review.

For more guidance, visit Writing a golden file test for package:flutter.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

Changes reported for pull request #113268 at sha 67a99e9a35e11c279db4d901b59c485ec129af16

LongCatIsLooong · 2022-10-12T04:47:33Z

4.1K scuba changes but nothing stood out.

justinmc

Do you know what's going on with the scuba changes? I see that you changed a few pixel values in the tests. Should the caret height be the only thing that changed visually?

justinmc · 2022-10-24T19:58:23Z

packages/flutter/lib/src/painting/text_painter.dart

  }

-  // Returns true iff the given value is a valid UTF-16 surrogate. The value
+  // Returns true iff the given value is a valid UTF-16 high surrogate. The value


Good catch.

justinmc · 2022-10-24T20:02:48Z

packages/flutter/lib/src/painting/text_painter.dart

+  // Returns null when the range [start, end) does not contain a full grapheme
+  // in the paragrah.
+  static _CaretMetrics? _caretMetricsFromTextBox(int start, int end, TextAffinity affinity, ui.Paragraph paragraph) {
+    assert(end > start, '$end > $start');


Nit: Maybe '$end must be greater than $start'.

justinmc · 2022-10-24T20:10:16Z

packages/flutter/lib/src/painting/text_painter.dart

+  //
+  // Returns null when the range [start, end) does not contain a full grapheme
+  // in the paragrah.
+  static _CaretMetrics? _caretMetricsFromTextBox(int start, int end, TextAffinity affinity, ui.Paragraph paragraph) {


What exactly does the affinity mean here?

I see it's used to indicate direction elsewhere in this PR. Maybe it's a standard thing or maybe it requires a little bit more description in these comments.

justinmc · 2022-10-24T20:11:26Z

packages/flutter/lib/src/painting/text_painter.dart

-    final int? nextCodeUnit = _text!.codeUnitAt(min(offset, flattenedText.length - 1));
-    if (nextCodeUnit == null) {
+  // Search for graphemes in the paragraph in the given range, and exponentially
+  // expands the search range if there are no graphemes in the current range


"expands" => "expand"

justinmc · 2022-10-24T20:13:59Z

packages/flutter/lib/src/painting/text_painter.dart

+  // Search for graphemes in the paragraph in the given range, and exponentially
+  // expands the search range if there are no graphemes in the current range
+  // until a grapheme is found. Return the _CaretMetrics based on the TextBox
+  // of that grapheme.


Nit: Maybe mention that this can also be expensive if that's true. I see it calls _caretMetricsFromTextBox.

justinmc · 2022-10-24T20:16:48Z

packages/flutter/lib/src/painting/text_painter.dart

+  ///
+  /// Valid only after [layout] has been called.
+  Offset getOffsetForCaret(TextPosition position, Rect caretPrototype) {
+    return (_computeCaretMetrics(position) ?? _fallbakCaretMetrics).offset;


"_fallbak" => "_fallback"

justinmc · 2022-10-24T20:19:14Z

packages/flutter/lib/src/painting/text_painter.dart

-  // Checks if the [position] and [caretPrototype] have changed from the cached
-  // version and recomputes the metrics required to position the caret.
-  void _computeCaretMetrics(TextPosition position, Rect caretPrototype) {
+  // the metrics required to position the caret.


"the" => "The"

Or maybe I'm reading this wrong with all of the deletions.

justinmc · 2022-10-24T21:47:32Z

packages/flutter/lib/src/painting/text_painter.dart

+    // TODO(LongCatIsLooong): introduce ui.Paragraph-level api for this. The
+    // exponential search using getBoxesForRange isn't necessary and does not
+    // work well for certian edge cases (e.g., when the position is inside a
+    // glyph, and the previous character is a newline character).


Nit: Consider linking/creating a GitHub issue here.

This will be taken care of in the new paragraph api. But I don't think there's a github issue.

justinmc · 2022-10-24T21:48:22Z

packages/flutter/lib/src/painting/text_painter.dart

+      // TODO(LongCatIsLooong): ui.Paragraph.width always reports the input
+      // width. The double layout keeps the two width in sync so TextPainter
+      // does not have to work with a non-zero paint offset. SkParagraph
+      // invalidates the layout entirely in this case so this is expensive.


Should we create an issue for this? Or maybe it's not possible or easy to reproduce a userland bug caused by this.

This is one important use case that has to be covered in the new paragraph API. I'll describe that in the design doc instead.

justinmc · 2022-10-24T21:49:58Z

packages/flutter/lib/src/painting/text_painter.dart

-    // Cache the input parameters to prevent repeat work later.
-    _previousCaretPosition = position;
-    _previousCaretPrototype = caretPrototype;
+    return _cachedCaretMetrics = caretMetrics;


Why did you change this to both return and assign?

caretPrototype doesn't affect the anchoring of the caret and it's not expensive at all to compute, there's no need to invalidate the cache over that.

Renzo-Olivares · 2022-10-27T18:48:36Z

packages/flutter/lib/src/painting/text_painter.dart

+    final bool done = (searchStep > 0 || start <= 0)
+                   && (searchStep < 0 || text.codeUnitAt(end) == null);
+
+    // Exapnds the search region exponentially if we need to keep searching.


Exapnds -> Expands

Renzo-Olivares · 2022-10-27T18:55:51Z

packages/flutter/lib/src/painting/text_painter.dart

+      return _cachedCaretMetrics;
+    }
+    final InlineSpan text = _text!;
+    // Special case: the text is emtpy. ui.Paragraph returns a TextBox located


emtpy -> empty

flutter-dashboard · 2022-12-28T03:32:50Z

This pull request executed golden file tests, but it has not been updated in a while (20+ days). Test results from Gold expire after as many days, so this pull request will need to be updated with a fresh commit in order to get results from Gold.

For more guidance, visit Writing a golden file test for package:flutter.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

guidezpl · 2023-01-13T14:54:36Z

I think this also fixes #64072

justinmc · 2023-02-01T18:03:21Z

@LongCatIsLooong Are you still planning to merge this? Sorry I never gave a final review. Looks like it needs to be merged with master now.

LongCatIsLooong · 2023-02-01T22:23:55Z

I'll get back to this once needed lower-level APIs are exposed in ui.Paragraph.

LongCatIsLooong added 3 commits October 10, 2022 22:15

fix caret painting

f68468d

Merge remote-tracking branch 'upstream/master' into caret-painting-fix

3ca5e5e

add tests

440d06b

LongCatIsLooong requested review from Renzo-Olivares, goderbauer, jason-simmons and justinmc October 11, 2022 06:48

LongCatIsLooong commented Oct 11, 2022

View reviewed changes

flutter-dashboard bot added the will affect goldens Changes to golden files label Oct 11, 2022

Special case for the HTML renderer

d03f6fc

LongCatIsLooong force-pushed the caret-painting-fix branch from 67a99e9 to d03f6fc Compare October 14, 2022 18:51

justinmc reviewed Oct 24, 2022

View reviewed changes

Renzo-Olivares reviewed Oct 27, 2022

View reviewed changes

Merge remote-tracking branch 'upstream/master' into caret-painting-fix

2d0100b

LongCatIsLooong force-pushed the caret-painting-fix branch from d28cd63 to de812d0 Compare November 1, 2022 23:33

review

c815d6e

LongCatIsLooong force-pushed the caret-painting-fix branch from de812d0 to c815d6e Compare November 2, 2022 04:45

LongCatIsLooong requested review from Renzo-Olivares and justinmc November 23, 2022 17:34

LongCatIsLooong added 2 commits December 2, 2022 16:28

Merge remote-tracking branch 'upstream/master' into caret-painting-fix

56f9123

Merge remote-tracking branch 'upstream/master' into caret-painting-fix

355d15b

LongCatIsLooong closed this Feb 1, 2023

LongCatIsLooong mentioned this pull request Feb 15, 2023

Fix Caret Height On Empty Lines #120834

Merged

8 tasks

Conversation

LongCatIsLooong commented Oct 11, 2022

Pre-launch Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

flutter-dashboard bot commented Oct 11, 2022

Uh oh!

LongCatIsLooong commented Oct 12, 2022

Uh oh!

justinmc left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

flutter-dashboard bot commented Dec 28, 2022

Uh oh!

guidezpl commented Jan 13, 2023

Uh oh!

justinmc commented Feb 1, 2023

Uh oh!

LongCatIsLooong commented Feb 1, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants