Skip to content

Reland #63639 #64615

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
Closed
LongCatIsLooong wants to merge 8 commits into from
Closed

Reland #63639 #64615

LongCatIsLooong wants to merge 8 commits into from
+200 −72

Conversation

@LongCatIsLooong
Copy link
Contributor

@LongCatIsLooong LongCatIsLooong commented Aug 26, 2020 •
edited
Loading

This reverts commit 6536f65.

Prerequisite: #48679

Tests

I added the following tests:

Replace this with a list of the tests that you added as part of this PR. A change in behavior with no test covering it
will likely get reverted accidentally sooner or later. PRs must include tests for all changed/updated/fixed behaviors. See Test Coverage.

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.

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I signed the CLA.
  • I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I updated/added relevant documentation (doc comments with ///).
  • All existing and new tests are passing.
  • The analyzer (flutter analyze --flutter-repo) does not report any problems on my PR.
  • I am willing to follow-up on review comments in a timely manner.

Breaking Change

Did any tests fail when you ran them? Please read Handling breaking changes.

@LongCatIsLooong
Reland flutter#63639
20ee371 
This reverts commit 6536f65.
@flutter-dashboard flutter-dashboard bot added a: tests "flutter test", flutter_test, or one of our tests framework flutter/packages/flutter repository. See also f: labels. work in progress; do not review labels Aug 26, 2020
@fluttergithubbot
Copy link
Contributor

fluttergithubbot commented Sep 1, 2020

This pull request is not suitable for automatic merging in its current state.

  • Please get at least one approved review before re-applying this label. Reviewers: If you left a comment approving, please use the "approve" review action instead.

@Piinks Piinks added a: typography Text rendering, possibly libtxt work in progress; do not review labels Sep 2, 2020
@goderbauer
Copy link
Member

goderbauer commented Sep 9, 2020

(PR triage): What's the status of this PR? Is it waiting for #48679 to be fixed?

@LongCatIsLooong
Copy link
Contributor Author

LongCatIsLooong commented Sep 9, 2020

@goderbauer yes. Breaks internal tests otherwise.

@fluttergithubbot
Copy link
Contributor

fluttergithubbot commented Oct 9, 2020

This pull request is not suitable for automatic merging in its current state.

  • Please get at least one approved review before re-applying this label. Reviewers: If you left a comment approving, please use the "approve" review action instead.

@goderbauer
Copy link
Member

goderbauer commented Dec 9, 2020

#48679 has been fixed.

@goderbauer
Copy link
Member

goderbauer commented Jan 20, 2021

@LongCatIsLooong Any updates on this PR?

@LongCatIsLooong
Copy link
Contributor Author

LongCatIsLooong commented Mar 4, 2021 •
edited
Loading

I'll see if I can make RenderConstraintsTransformBox an abstract class since it's difficult to implement computeDryLayout for it.

nvm it's super straightforward. I might have had the wrong method in mind.

@skia-gold
Copy link

skia-gold commented Mar 11, 2021

Gold has detected about 1 untriaged digest(s) on patchset 2.
View them at https://flutter-gold.skia.org/cl/github/64615

@skia-gold
Copy link

skia-gold commented Mar 11, 2021

Gold has detected about 1 untriaged digest(s) on patchset 3.
View them at https://flutter-gold.skia.org/cl/github/64615

@skia-gold
Copy link

skia-gold commented Mar 11, 2021

Gold has detected about 1 untriaged digest(s) on patchset 4.
View them at https://flutter-gold.skia.org/cl/github/64615

@flutter-dashboard flutter-dashboard bot added the f: material design flutter/packages/flutter/material repository. label Mar 11, 2021
@skia-gold
Copy link

skia-gold commented Mar 11, 2021

Gold has detected about 1 untriaged digest(s) on patchset 5.
View them at https://flutter-gold.skia.org/cl/github/64615

@LongCatIsLooong
Copy link
Contributor Author

LongCatIsLooong commented Mar 12, 2021 •
edited
Loading

Ran TGP and there're 31 test failures. I'll split the PR into 2 parts and try to merge the safe part first.
Then TextField, CupertinoTextField etc all need to have a clipBehavior property to opt-out of the debug mode warning.

@LongCatIsLooong LongCatIsLooong mentioned this pull request Mar 12, 2021
Merged
8 tasks
justinmc
justinmc approved these changes Mar 16, 2021
View reviewed changes
Copy link
Contributor

@justinmc justinmc left a comment

Choose a reason for hiding this comment

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

LGTM 👍

I left some comments that didn't end up as part of this review for some reason:
#77994 (comment)
#77994 (comment)

Just some stuff to think about. It definitely took me awhile to understand exactly how this widget will behave in comparison to other widgets like ConstrainedBox. It seems like that's due to the complexity of those widgets, not with this new one, though.

packages/flutter/lib/src/rendering/shifted_box.dart
/// * [RenderSizedOverflowBox], a render object that is a specific size but
/// passes its original constraints through to its child, which it allows to
/// overflow.
class RenderUnconstrainedBox extends RenderConstraintsTransformBox {
Copy link
Contributor

@justinmc justinmc Mar 16, 2021

Choose a reason for hiding this comment

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

I wanted something like this when I was working on InteractiveViewer, and this is how I did it:

flutter/packages/flutter/lib/src/widgets/interactive_viewer.dart

Lines 1073 to 1082 in c30fdfb

if (!widget.constrained) {
child = OverflowBox(
alignment: Alignment.topLeft,
minWidth: 0.0,
minHeight: 0.0,
maxWidth: double.infinity,
maxHeight: double.infinity,
child: child,
);
}

If I remember correctly it was actually really hard to do for sizes bigger than the screen. Using a ConstrainedBox with infinite constraints didn't work, strangely.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@justinmc justinmc justinmc approved these changes

Assignees

No one assigned

Labels

a: tests "flutter test", flutter_test, or one of our tests a: typography Text rendering, possibly libtxt 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.

7 participants

@LongCatIsLooong @fluttergithubbot @goderbauer @skia-gold @justinmc @googlebot @Piinks