这是indexloc提供的服务，不要输入任何密码
Skip to content

Fix RenderEditable.computeMaxIntrinsicWidth error return. #82286

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
May 13, 2021
Merged

Fix RenderEditable.computeMaxIntrinsicWidth error return. #82286

merged 2 commits into from
May 13, 2021

Conversation

YeungKC
Copy link
Member

@YeungKC YeungKC commented May 11, 2021

close #82277

Because _kCaretGap is not calculated in RenderEditable.computeMaxIntrinsicWidth, an incorrect with may be returned.

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 feature I am adding, or Hixie said the 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.

@flutter-dashboard flutter-dashboard bot added f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels. labels May 11, 2021
@google-cla google-cla bot added the cla: yes label May 11, 2021
@YeungKC YeungKC requested a review from justinmc May 11, 2021 19:27
LongCatIsLooong
LongCatIsLooong reviewed May 12, 2021
View reviewed changes
Copy link
Contributor

@LongCatIsLooong LongCatIsLooong left a comment

Choose a reason for hiding this comment

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

Thanks for the fix! Could you add a test to verify that if you layout a RenderEditable with the exact width getMaxIntrinsicWidth gave you, the render editable does not have a second line?

packages/flutter/lib/src/rendering/editable.dart
@@ -3123,7 +3123,7 @@ class RenderEditable extends RenderBox with RelayoutWhenSystemFontsChangeMixin {
@override
double computeMaxIntrinsicWidth(double height) {
_layoutText(maxWidth: double.infinity);
return _textPainter.maxIntrinsicWidth + cursorWidth;
return _textPainter.maxIntrinsicWidth + _caretMargin;
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we add this to computeMinIntrinsicWidth too?

Copy link
Member Author

Choose a reason for hiding this comment

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

About this I am not very clear, @justinmc how to think about it?

@YeungKC
Copy link
Member Author

YeungKC commented May 12, 2021

I don't think it's necessary, and in fact the real problem is not the wrong line break.

@LongCatIsLooong
Copy link
Contributor

LongCatIsLooong commented May 12, 2021 •
edited
Loading

getMaxIntrinsicWidth's documentation (https://master-api.flutter.dev/flutter/rendering/RenderBox/getMaxIntrinsicWidth.html) states:

Returns the smallest width beyond which increasing the width never decreases the preferred height. The preferred height is the value that would be returned by getMinIntrinsicHeight for that width.

That means if you layout the RenderEditable with the width given by getMaxIntrinsicWidth it should never soft break right? Otherwise the preferred height is going to decrease if a larger width is given because the number of text lines becomes smaller.

@YeungKC
Copy link
Member Author

YeungKC commented May 12, 2021 •
edited
Loading

It does look like it needs to be increased, but I found at 9161ec4 that justin doesn't have the computeMinIntrinsicWidth, so I'm a little unsure.

@LongCatIsLooong
Copy link
Contributor

Yeah looking at the implementation of TextPainter.minIntrinsicWidth, if the maxLines parameter is 1 it returns maxIntrinsicWidth. So it should probably need to take the width of the cursor into account as well. It's totally fine to leave it as-is for now, I can open a new issue for it.

@YeungKC
Copy link
Member Author

YeungKC commented May 13, 2021

Awesome, thanks for the explanation.

So we should be able to merge this pr now?

@LongCatIsLooong LongCatIsLooong mentioned this pull request May 13, 2021
Open
@fluttergithubbot fluttergithubbot merged commit 64765d8 into flutter:master May 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@LongCatIsLooong LongCatIsLooong LongCatIsLooong approved these changes

@justinmc justinmc Awaiting requested review from justinmc

Assignees
No one assigned
Labels
f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

when call RenderEditable.computeMaxIntrinsicWidth, which is one pixel less than the actual width.
3 participants
@YeungKC @LongCatIsLooong @fluttergithubbot