Remove RenderEditable dependency from TextSelectionHandleOverlay #97967
Conversation
|
It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!). If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
flutter-dashboard
bot
added
a: text input
|
|
||
| late Offset _dragEndPosition; | ||
|
|
||
| void _handleSelectionEndHandleDragStart(DragStartDetails details) { |
There was a problem hiding this comment.
These handle methods were moved from the TextSelectionHandleOverlay
| endHandleRect?.height ?? widget.renderObject.preferredLineHeight, | ||
| widget.type, | ||
| widget.preferredLineHeight, | ||
| widget.glyphHeight, |
There was a problem hiding this comment.
I was confused at this API. It takes both start glyph and end glyph height, but it only use one of them based on the type. One parameter should be enough in this case, not sure if there will be case where one handle needs to depends on both start and end glyph height.
There was a problem hiding this comment.
@Renzo-Olivares Didn't you write a PR that messed with handles of different height back during your internship? Maybe this mattered before and now it doesn't.
This PR might not be the time to fix this with a potentially breaking change, but maybe it's worth opening an issue if it is something we should fix.
There was a problem hiding this comment.
I talked to Renzo and it seems like both start and end shouldn't be needed. His original work is still fine without both.
Do you want to change this here or maybe I should open an issue for it?
There was a problem hiding this comment.
I will have another pr I can introduce the breaking change there
| this.selectionControls, | ||
| bool handlesVisible = false, | ||
| this.selectionDelegate, | ||
| required this.selectionDelegate, |
There was a problem hiding this comment.
This is probably an error when converting to null safety. It will likely crash the app if this is null
There was a problem hiding this comment.
I agree, looks like it should have been required originally.
|
test-exempt: code refactor with no semantic change |
justinmc
left a comment
justinmc
left a comment
There was a problem hiding this comment.
LGTM 👍 . I think _SelectionHandleOverlay looks cleaner overall without the rendereditable dependency.
| this.selectionControls, | ||
| bool handlesVisible = false, | ||
| this.selectionDelegate, | ||
| required this.selectionDelegate, |
There was a problem hiding this comment.
I agree, looks like it should have been required originally.
| endHandleRect?.height ?? widget.renderObject.preferredLineHeight, | ||
| widget.type, | ||
| widget.preferredLineHeight, | ||
| widget.glyphHeight, |
There was a problem hiding this comment.
@Renzo-Olivares Didn't you write a PR that messed with handles of different height back during your internship? Maybe this mattered before and now it doesn't.
This PR might not be the time to fix this with a potentially breaking change, but maybe it's worth opening an issue if it is something we should fix.
4 participants
This is the first part of the refactoring pr. The ultimate goal is refactor renderEditable out of TextSelectionOverlay so that it can be reused in #95226
This PR remove rendereditable dependency from the TextSelectionHandleOverlay
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.