Move caret/highlight painting to custom painters #72828
Conversation
flutter-dashboard
bot
added
f: cupertino
LongCatIsLooong
force-pushed
the
render-editable-mk3
branch
from
669fb50 to
30dc55b
Compare
| @@ -0,0 +1,12 @@ | |||
| // Copyright 2014 The Flutter Authors. All rights reserved. | |||
There was a problem hiding this comment.
This doesn't seem to be referenced anywhere not sure if I should keep it (same for fullscreen_textfield_perf__e2e_summary.dart)
There was a problem hiding this comment.
No opinion from me, I haven't done much with these devicelab perf tests before 🤷
| _floatingCursorAddedMargin = value; | ||
| markNeedsPaint(); | ||
| } | ||
| EdgeInsets floatingCursorAddedMargin; |
There was a problem hiding this comment.
This is currently only used in setFloatingCursor which is triggered by platform channel messages so triggering a repaint does not seem necessary.
There was a problem hiding this comment.
What if a user has implemented their own text field using RenderEditable and they directly set floatingCursorAddedMargin for some reason?
There was a problem hiding this comment.
Currently the floating cursor implementation does not handle some edge cases properly, changing floatingCursorAddedMargin is one of them. The floating cursor doesn't reposition itself when the text input resizes or moves. The visual update only happens when the user moves the touch point (and we get a text channel message).
| return; | ||
| _selection = value; | ||
| _selectionRects = null; | ||
| _selectionPainter.highlightedRange = value; |
There was a problem hiding this comment.
technically this is not needed, since the selection painter will repaint with the RenderEditable.
| canvas.drawRect(box.toRect().shift(effectiveOffset), _promptRectPaint); | ||
| _floatingCursorOn = state != FloatingCursorDragState.End; | ||
| _resetFloatingCursorAnimationValue = resetLerpValue; | ||
| if (_floatingCursorOn) { |
There was a problem hiding this comment.
This part is different. The caret calculation was originally partially deferred to paint and is now performed in setFloatingCursor.
| /// * [RenderEditable.setPainter], which takes a [RenderEditablePainter] | ||
| /// and sets it as the foreground painter of the [RenderEditable]. | ||
| /// * [CustomPainter] a similar class which paints within a [RenderCustomPaint]. | ||
| abstract class RenderEditablePainter extends ChangeNotifier { |
There was a problem hiding this comment.
The painter doesn't have hitTest or semanticsBuilder because these currently don't seem to have a use case.
There was a problem hiding this comment.
The RenderEditable property was removed because you can use the same painter instance in different render objects simultaneously when the tree finalizes (for example in a Hero widget we'd keep 2 copies of the subtree so in case there's a painter in the subtree it will be attached to 2 different RenderEditables).
|
|
||
| CaretChangedHandler caretPaintCallback; | ||
|
|
||
| bool showRegularCaret = false; |
There was a problem hiding this comment.
Does not trigger a repaint because this is floating cursor stuff. Might need a bit of audit when we move the caret painting stuff to a different place.
| bool showRegularCaret = false; | ||
|
|
||
| final Paint caretPaint = Paint(); | ||
| late final Paint floatingCursorPaint = Paint(); |
There was a problem hiding this comment.
Uses a different painter because of a gotcha in TestRecordingCanvas.
There was a problem hiding this comment.
How is this late but it still has an initial value? I think I just have never encountered this NNBD situation.
There was a problem hiding this comment.
I think this does lazy initialization. The instance variable won't be initialized until its first usage.
| assert(renderEditable != null); | ||
| final TextSelection? selection = renderEditable.selection; | ||
|
|
||
| if (selection == null || !selection.isCollapsed /* || !selection.isValid*/) |
There was a problem hiding this comment.
I thought we shouldn't paint the caret when selection is (-1, -1)?
There was a problem hiding this comment.
I remember this came up awhile back in flutter/engine#19785, was there somewhere else that we were talking about this?
There was a problem hiding this comment.
Not that I remember. Is the expected behavior of (-1, -1) documented anywhere? I couldn't seem to find anything about it.
The TextEditingValue constructor uses (-1, -1) as the default selection range for years. https://github.com/flutter/flutter/blame/d35a9db6fe439b1efaddffb19cea540b99917d16/packages/flutter/lib/src/services/text_input.dart#L82-L90
There was a problem hiding this comment.
No, I haven't seen any good docs about it. I think it makes sense to me uncomment that clause and make it not paint when the selection is (-1,-1). Is that not what was happening before this PR?
| expect(lastCharEndpoint.length, 1); | ||
| // The last character is now on screen. | ||
| expect(lastCharEndpoint[0].point.dx, moreOrLessEquals(786.73, epsilon: 0.01)); | ||
| expect(lastCharEndpoint[0].point.dx, moreOrLessEquals(786.73, epsilon: 2)); |
There was a problem hiding this comment.
Not sure why but I had to change the epsilon here so these tests pass.
There was a problem hiding this comment.
Weird, nothing jumps out at me as a cause. It is a big epsilon, but if no golden tests are failing then maybe it's not a concern...
There was a problem hiding this comment.
I reverted the epsilon changes. Tests are passing now.
LongCatIsLooong
force-pushed
the
render-editable-mk3
branch
from
38f5e3d to
7a76bac
Compare
LongCatIsLooong
force-pushed
the
render-editable-mk3
branch
from
911d560 to
45f97b8
Compare
justinmc
left a comment
justinmc
left a comment
There was a problem hiding this comment.
LGTM 👍
I re-read your design doc and refreshed myself on the migration plan at the end. So it seems that the plan is to move the RenderEditablePainters into their own files and then pass them into RenderEditable via the backgroundPainter and foregroundPainter params. I'm on board with that, especially that it will simplify rendering/editable.dart, which is overly long and complicated right now.
Also, this doesn't seem to interfere with anything from my text editing refactor plan. If should actually make it easier to pare down RenderEditable even further.
| '👨👩👦👨👩👦👨👩👦👨👩👦👨👩👦👨👩👦👨👩👦👨👩👦👨👩👦👨👩👦👨👩👦👨👩👦👨👩👦👨👩👦👨👩👦👨👩👦👨👩👦👨👩👦👨👩👦👨👩👦👨👩👦👨👩👦👨👩👦👨👩👦👨👩👦👨👩👦👨👩👦👨👩👦👨👩👦👨👩👦👨👩👦👨👩👦👨👩👦' | ||
| '👨👩👦👨👩👦👨👩👦👨👩👦👨👩👦👨👩👦👨👩👦👨👩👦👨👩👦👨👩👦👨👩👦👨👩👦👨👩👦👨👩👦👨👩👦👨👩👦👨👩👦👨👩👦👨👩👦👨👩👦👨👩👦👨👩👦👨👩👦👨👩👦👨👩👦👨👩👦👨👩👦👨👩👦👨👩👦👨👩👦👨👩👦👨👩👦👨👩👦' | ||
| '👨👩👦👨👩👦👨👩👦👨👩👦👨👩👦👨👩👦👨👩👦👨👩👦👨👩👦👨👩👦👨👩👦👨👩👦👨👩👦👨👩👦👨👩👦👨👩👦👨👩👦👨👩👦👨👩👦👨👩👦👨👩👦👨👩👦👨👩👦👨👩👦👨👩👦👨👩👦👨👩👦👨👩👦👨👩👦👨👩👦👨👩👦👨👩👦👨👩👦' | ||
| '👨👩👦👨👩👦👨👩👦👨👩👦👨👩👦👨👩👦👨👩👦👨👩👦👨👩👦👨👩👦👨👩👦👨👩👦👨👩👦👨👩👦👨👩👦👨👩👦👨👩👦👨👩👦👨👩👦👨👩👦👨👩👦👨👩👦👨👩👦👨👩👦👨👩👦👨👩👦👨👩👦👨👩👦👨👩👦👨👩👦👨👩👦👨👩👦👨👩👦' |
There was a problem hiding this comment.
Now we've got some extended grapheme clusters haha.
|
|
||
| /// Computes the offset to apply to the given [sourceOffset] so it perfectly | ||
| /// snaps to physical pixels. | ||
| Offset _snapToPhysicalPixel(Offset sourceOffset) { |
There was a problem hiding this comment.
Ah I missed both of those, you're right 👍
| || oldDelegate.highlightColor != highlightColor | ||
| || oldDelegate.highlightedRange != highlightedRange | ||
| || oldDelegate.selectionHeightStyle != selectionHeightStyle | ||
| || oldDelegate.selectionWidthStyle != selectionWidthStyle; |
There was a problem hiding this comment.
Ok that sounds good to me, it should always get repainted when needed.
LongCatIsLooong
force-pushed
the
render-editable-mk3
branch
from
bd6c6b0 to
64667a5
Compare
| RenderEditablePainter? painter, | ||
| RenderEditablePainter? foregroundPainter, |
There was a problem hiding this comment.
When we do that, would we also remove/deprecate some of the existing arguments to RenderEditable?
| /// The new [RenderEditablePainter] will replace the previously specified | ||
| /// foreground painter, and schedule a repaint if the new painter's | ||
| /// `shouldRepaint` method returns true. | ||
| void setForegroundPainter(RenderEditablePainter? newPainter) { |
There was a problem hiding this comment.
Why not a regular getter/setter?
| /// The new [RenderEditablePainter] will replace the previously specified | ||
| /// painter, and schedule a repaint if the new painter's `shouldRepaint` | ||
| /// method returns true. | ||
| void setPainter(RenderEditablePainter? newPainter) { |
| /// to repaint without triggering a repaint on the entire [RenderEditable] stack | ||
| /// when only auxiliary content changes (e.g. a blinking cursor) are present. It | ||
| /// will be scheduled to repaint when: | ||
| /// * It's assigned to a new [RenderEditable] and the [shouldRepaint] method |
There was a problem hiding this comment.
reindented to 1 space and added an empty line between the bullet points(I think you meant the latter?)
| /// painter's attributes change. | ||
| /// | ||
| /// See also: | ||
| /// * [RenderEditable.setForegroundPainter], which takes a [RenderEditablePainter] |
| if (promptRectColor != null) | ||
| _promptRectPaint.color = promptRectColor; | ||
|
|
||
| _selectionPainter.highlightColor = selectionColor; |
There was a problem hiding this comment.
For my understanding, the plan is to move all these painters out of this class? Or just some of them?
There was a problem hiding this comment.
Ideally, all of them.
| final _TextHighlightPainter _selectionPainter = _TextHighlightPainter(); | ||
| final _TextHighlightPainter _autocorrectHighlightPainter = _TextHighlightPainter(); | ||
|
|
||
| _CompositeRenderEditablePainter get _builtInForegounrdPainters => _cachedBuiltInForegroundPainters ??= _createBuiltInForegroundPainters(); |
There was a problem hiding this comment.
| _CompositeRenderEditablePainter get _builtInForegounrdPainters => _cachedBuiltInForegroundPainters ??= _createBuiltInForegroundPainters(); | |
| _CompositeRenderEditablePainter get _builtInForegroundPainters => _cachedBuiltInForegroundPainters ??= _createBuiltInForegroundPainters(); | |
| final _TextHighlightPainter _selectionPainter = _TextHighlightPainter(); | ||
| final _TextHighlightPainter _autocorrectHighlightPainter = _TextHighlightPainter(); | ||
|
|
||
| _CompositeRenderEditablePainter get _builtInForegounrdPainters => _cachedBuiltInForegroundPainters ??= _createBuiltInForegroundPainters(); |
There was a problem hiding this comment.
Also, you should be able to just defines this as:
late final _CompositeRenderEditablePainter _builtInForegounrdPainters = _createBuiltInForegroundPainters();With late it will only get instantiated on first access.
There was a problem hiding this comment.
Oh, never mind. I see below you need the option to clear this.
LongCatIsLooong
force-pushed
the
render-editable-mk3
branch
from
1320f6a to
ea074f2
Compare
4 participants
Description
This PR introduces 2 new fields
forgroundPainterandpaintertoRenderEditable, and moves caret/text highlight painting logic into these painters, without introducing actual behavior changes (hopefully).https://docs.google.com/document/d/10YiBZ_5oj6erdbgPSgpvEzNZUa7nEMYaa0X2Tw1GAWo/edit#
Tests
I added the following tests:
A few tests for custom painters.
Checklist
Before you create this PR, confirm that it meets all requirements listed below by checking the relevant checkboxes (
[x]). This will ensure a smooth and quick review process.///).flutter analyze --flutter-repo) does not report any problems on my PR.Breaking Change
Did any tests fail when you ran them? Please read Handling breaking changes.