Skip to content
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.

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

Let OverflowBox be shrink-wrappable #129095

Merged
merged 39 commits into from
Oct 25, 2023
Merged

Conversation

fzyzcjy
Copy link
Contributor

@fzyzcjy fzyzcjy commented Jun 19, 2023

Close #129094

I have demonstrated how this PR fixes the problem using tests in #129094. I will further add tests in this PR if the PR looks roughly acceptable :)

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.

@github-actions github-actions bot added the framework flutter/packages/flutter repository. See also f: labels. label Jun 19, 2023
@goderbauer goderbauer self-requested a review June 20, 2023 22:12
@flutter-dashboard
Copy link

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.

Copy link
Member

@goderbauer goderbauer left a comment

Choose a reason for hiding this comment

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

Please ALWAYS add tests to your PRs. A PR is only ready for review if it has tests to prove that what is implemented actually works.

packages/flutter/lib/src/rendering/shifted_box.dart Outdated Show resolved Hide resolved
packages/flutter/lib/src/rendering/shifted_box.dart Outdated Show resolved Hide resolved
packages/flutter/lib/src/widgets/basic.dart Show resolved Hide resolved
packages/flutter/lib/src/widgets/basic.dart Outdated Show resolved Hide resolved
@fzyzcjy
Copy link
Contributor Author

fzyzcjy commented Jul 7, 2023

Thanks for the review! I will add tests soon. (I got rejected too many times for PRs with full tests and green CI, just because the main idea/target does not look good... So that's why I hope to have a brief review about whether the idea is acceptable)

@flutter-dashboard
Copy link

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.

@fzyzcjy
Copy link
Contributor Author

fzyzcjy commented Jul 27, 2023

@flutter-dashboard - I will update this PR as soon as I get back working on mobile (working on backend now)

@Piinks
Copy link
Contributor

Piinks commented Aug 15, 2023

(I got rejected too many times for PRs with full tests and green CI, just because the main idea/target does not look good... So that's why I hope to have a brief review about whether the idea is acceptable)

I would recommend discussing the proposed solution in the issue or on Discord first then, or writing a design doc and soliciting feedback there. I think this will make for a more efficient experience for both contributors and reviewers. :)

@fzyzcjy
Copy link
Contributor Author

fzyzcjy commented Aug 16, 2023

I would recommend discussing the proposed solution in the issue or on Discord first then, or writing a design doc and soliciting feedback there. I think this will make for a more efficient experience for both contributors and reviewers. :)

Thanks for the suggestion! I will try that next time.

PS. I did not forget this PR, just have no time available to finish it currently, but will definitely continue later!

@Hixie
Copy link
Contributor

Hixie commented Aug 22, 2023

If we do this we should probably do something like MainAxisSize or StackFit rather than calling this "shrink-wrap" (which I think we try to reserve for using intrinsic dimensions).

@fzyzcjy
Copy link
Contributor Author

fzyzcjy commented Aug 23, 2023

@Hixie Sure!

EDIT: Done

Copy link
Member

@goderbauer goderbauer left a comment

Choose a reason for hiding this comment

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

LGTM

packages/flutter/lib/src/rendering/shifted_box.dart Outdated Show resolved Hide resolved
packages/flutter/lib/src/rendering/shifted_box.dart Outdated Show resolved Hide resolved
packages/flutter/lib/src/rendering/shifted_box.dart Outdated Show resolved Hide resolved
packages/flutter/lib/src/rendering/shifted_box.dart Outdated Show resolved Hide resolved
packages/flutter/lib/src/rendering/shifted_box.dart Outdated Show resolved Hide resolved
fzyzcjy and others added 4 commits October 7, 2023 07:13
Co-authored-by: Michael Goderbauer <goderbauer@google.com>
Co-authored-by: Michael Goderbauer <goderbauer@google.com>
Copy link
Member

@cbracken cbracken left a comment

Choose a reason for hiding this comment

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

lgtm!

packages/flutter/lib/src/rendering/shifted_box.dart Outdated Show resolved Hide resolved
packages/flutter/lib/src/rendering/shifted_box.dart Outdated Show resolved Hide resolved
fzyzcjy and others added 2 commits October 10, 2023 07:07
Co-authored-by: Chris Bracken <chris@bracken.jp>
Co-authored-by: Chris Bracken <chris@bracken.jp>
@goderbauer
Copy link
Member

@cbracken The CLA bot seems to be unhappy about the commits you have co-authored in this PR?! Is that a config issue on your end?

@cbracken
Copy link
Member

cbracken commented Oct 11, 2023

I don't think so; I've never had the CLA bot shout about suggested commits in recent memory. Seems like potentially a bug in the CLA bot? The CLA is implicit for all Googlers, but I've also signed it manually (years ago, back in 2013) just in case.

There was a period years ago where the bot got upset when it saw commits from more than one contributor on a patch, but that hasn't been the case in years. Checked the corporate directory just in case the bot knows something I don't but seems I'm still employed by Google. 😅

@cbracken
Copy link
Member

cbracken commented Oct 11, 2023

I think I have a guess as to what happened. I just tried to manually re-sign the CLA and it wants you to log in with a GAIA account with that email address. Until recently I had my personal custom-domain email hosted via Google Workspaces but I migrated to another solution and deleted the dasher domain last week. I wonder if the CLA bot requires an email account managed by Google.

I can see about trying to restore the dasher domain this week, but given that we know I'm an employee, I think we're safe to merge regardless of CLA bot status.

Edit: I'm not entirely sure this is right actually; I looked myself up on the internal CLA tool and everything looks good. I think we should merge regardless.

@goderbauer
Copy link
Member

OK, thanks @cbracken. I am going to do a rebase to kick the google testing job and if the CLA complains again override it.

@goderbauer
Copy link
Member

@fzyzcjy Can you resolve the conflicts and then we should be able to get this in.

@fzyzcjy
Copy link
Contributor Author

fzyzcjy commented Oct 24, 2023

Sure!

@goderbauer goderbauer added the autosubmit Merge PR when tree becomes green via auto submit App label Oct 24, 2023
@auto-submit auto-submit bot merged commit 6df6c89 into flutter:master Oct 25, 2023
62 checks passed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Oct 25, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Oct 25, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Oct 25, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Oct 25, 2023
auto-submit bot pushed a commit to flutter/packages that referenced this pull request Oct 25, 2023
Roll Flutter from 5e8b5f4ea293 to 5dd2a4e0aaef (59 revisions)

flutter/flutter@5e8b5f4...5dd2a4e

2023-10-25 15619084+vashworth@users.noreply.github.com Ensure Xcode project is setup to start debugger (flutter/flutter#136977)
2023-10-25 fluttergithubbot@gmail.com Marks Windows build_tests_6_6 to be unflaky (flutter/flutter#137216)
2023-10-25 godofredoc@google.com Remove gem and docker files. (flutter/flutter#137200)
2023-10-25 98614782+auto-submit[bot]@users.noreply.github.com Reverts "[gallery] Reland roll gallery to  ecfb9e5352bd12032301b12b30d5853d83d89bda" (flutter/flutter#137264)
2023-10-25 jonahwilliams@google.com [gallery] Reland roll gallery to  ecfb9e5352bd12032301b12b30d5853d83d89bda (flutter/flutter#137199)
2023-10-25 engine-flutter-autoroll@skia.org Roll Packages from 2faf992 to f751ffb (11 revisions) (flutter/flutter#137254)
2023-10-25 fluttergithubbot@gmail.com Marks Windows build_tests_3_6 to be unflaky (flutter/flutter#137214)
2023-10-25 fluttergithubbot@gmail.com Marks Windows build_tests_5_6 to be unflaky (flutter/flutter#137215)
2023-10-25 fluttergithubbot@gmail.com Marks Windows build_tests_2_6 to be unflaky (flutter/flutter#137213)
2023-10-25 tessertaha@gmail.com Revert "Update `OutlinedButton` tests for Material 3 (#136809)" (flutter/flutter#137242)
2023-10-25 tessertaha@gmail.com Update `OutlinedButton` tests for Material 3 (flutter/flutter#136809)
2023-10-25 5236035+fzyzcjy@users.noreply.github.com Let `OverflowBox` be shrink-wrappable (flutter/flutter#129095)
2023-10-25 polinach@google.com Add dependency on leak_tracker to flutter_test. (flutter/flutter#137069)
2023-10-25 ybz975218925@gmail.com fix SliverReorderableLists item wrong offset (flutter/flutter#136828)
2023-10-25 jacksongardner@google.com Remove `bringup: true` from realm_checker and remove the redundant tool test. (flutter/flutter#137186)
2023-10-25 36861262+QuncCccccc@users.noreply.github.com Revert "Fix Gradle lockfiles." (flutter/flutter#137198)
2023-10-24 polinach@google.com Fix Gradle lockfiles. (flutter/flutter#137190)
2023-10-24 pateltirth454@gmail.com Fix Typos (flutter/flutter#137173)
2023-10-24 engine-flutter-autoroll@skia.org Roll Flutter Engine from d6a48e9963df to 6e09ee14e244 (1 revision) (flutter/flutter#137185)
2023-10-24 engine-flutter-autoroll@skia.org Roll Flutter Engine from 602b5131ee4d to d6a48e9963df (6 revisions) (flutter/flutter#137180)
2023-10-24 42715466+lirantzairi@users.noreply.github.com TextField - allow to customize cursor color in error state (flutter/flutter#136121)
2023-10-24 chris@bracken.jp [macOS] Refactor macOS build/codesize analysis (flutter/flutter#137164)
2023-10-24 engine-flutter-autoroll@skia.org Roll Flutter Engine from dc00de7dacf4 to 602b5131ee4d (1 revision) (flutter/flutter#137174)
2023-10-24 jacksongardner@google.com Check the realm file in its own shard. (flutter/flutter#137160)
2023-10-24 engine-flutter-autoroll@skia.org Roll Flutter Engine from 1890bd7dc412 to dc00de7dacf4 (1 revision) (flutter/flutter#137172)
2023-10-24 engine-flutter-autoroll@skia.org Roll Flutter Engine from ca7ef01c99c6 to 1890bd7dc412 (2 revisions) (flutter/flutter#137158)
2023-10-24 godofredoc@google.com Migrate mac builds to ruby dep. (flutter/flutter#136929)
2023-10-24 engine-flutter-autoroll@skia.org Roll Packages from 4bf5114 to 2faf992 (7 revisions) (flutter/flutter#137154)
2023-10-24 engine-flutter-autoroll@skia.org Roll Flutter Engine from 7224835bb65f to ca7ef01c99c6 (1 revision) (flutter/flutter#137153)
2023-10-24 imcusg@gmail.com fix some typos (flutter/flutter#137144)
2023-10-24 engine-flutter-autoroll@skia.org Roll Flutter Engine from 8f7488e0b4b5 to 7224835bb65f (1 revision) (flutter/flutter#137150)
2023-10-24 engine-flutter-autoroll@skia.org Roll Flutter Engine from d58895093d0a to 8f7488e0b4b5 (3 revisions) (flutter/flutter#137135)
2023-10-24 engine-flutter-autoroll@skia.org Roll Flutter Engine from 2a8471f188cd to d58895093d0a (4 revisions) (flutter/flutter#137130)
2023-10-24 engine-flutter-autoroll@skia.org Roll Flutter Engine from 71600d89aa3f to 2a8471f188cd (2 revisions) (flutter/flutter#137129)
2023-10-24 engine-flutter-autoroll@skia.org Roll Flutter Engine from 3dd197dcf9e5 to 71600d89aa3f (1 revision) (flutter/flutter#137127)
2023-10-24 engine-flutter-autoroll@skia.org Roll Flutter Engine from bdbd1058858b to 3dd197dcf9e5 (2 revisions) (flutter/flutter#137125)
2023-10-24 engine-flutter-autoroll@skia.org Roll Flutter Engine from 0ef5caa91c40 to bdbd1058858b (1 revision) (flutter/flutter#137119)
2023-10-24 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Use `coverage.collect`'s `coverableLineCache` param to speed up coverage" (flutter/flutter#137121)
2023-10-24 engine-flutter-autoroll@skia.org Roll Flutter Engine from dd6a6531e816 to 0ef5caa91c40 (1 revision) (flutter/flutter#137114)
2023-10-24 engine-flutter-autoroll@skia.org Roll Flutter Engine from 0bd703132cc1 to dd6a6531e816 (2 revisions) (flutter/flutter#137111)
2023-10-23 engine-flutter-autoroll@skia.org Roll Flutter Engine from abeab897a29c to 0bd703132cc1 (3 revisions) (flutter/flutter#137107)
2023-10-23 polinach@google.com Upgrade packages in flutter and flutter_test. (flutter/flutter#137106)
2023-10-23 49699333+dependabot[bot]@users.noreply.github.com Bump ossf/scorecard-action from 2.2.0 to 2.3.1 (flutter/flutter#137103)
2023-10-23 goderbauer@google.com Dartdoc warnings (flutter/flutter#137077)
2023-10-23 engine-flutter-autoroll@skia.org Roll Flutter Engine from e89f0421bcab to abeab897a29c (5 revisions) (flutter/flutter#137100)
2023-10-23 sokolovskyi.konstantin@gmail.com Cover last test/material tests with leak tracking. (flutter/flutter#137004)
...
HugoOlthof pushed a commit to moneybird/packages that referenced this pull request Dec 13, 2023
…r#5231)

Roll Flutter from 5e8b5f4ea293 to 5dd2a4e0aaef (59 revisions)

flutter/flutter@5e8b5f4...5dd2a4e

2023-10-25 15619084+vashworth@users.noreply.github.com Ensure Xcode project is setup to start debugger (flutter/flutter#136977)
2023-10-25 fluttergithubbot@gmail.com Marks Windows build_tests_6_6 to be unflaky (flutter/flutter#137216)
2023-10-25 godofredoc@google.com Remove gem and docker files. (flutter/flutter#137200)
2023-10-25 98614782+auto-submit[bot]@users.noreply.github.com Reverts "[gallery] Reland roll gallery to  ecfb9e5352bd12032301b12b30d5853d83d89bda" (flutter/flutter#137264)
2023-10-25 jonahwilliams@google.com [gallery] Reland roll gallery to  ecfb9e5352bd12032301b12b30d5853d83d89bda (flutter/flutter#137199)
2023-10-25 engine-flutter-autoroll@skia.org Roll Packages from 2faf992 to f751ffb (11 revisions) (flutter/flutter#137254)
2023-10-25 fluttergithubbot@gmail.com Marks Windows build_tests_3_6 to be unflaky (flutter/flutter#137214)
2023-10-25 fluttergithubbot@gmail.com Marks Windows build_tests_5_6 to be unflaky (flutter/flutter#137215)
2023-10-25 fluttergithubbot@gmail.com Marks Windows build_tests_2_6 to be unflaky (flutter/flutter#137213)
2023-10-25 tessertaha@gmail.com Revert "Update `OutlinedButton` tests for Material 3 (#136809)" (flutter/flutter#137242)
2023-10-25 tessertaha@gmail.com Update `OutlinedButton` tests for Material 3 (flutter/flutter#136809)
2023-10-25 5236035+fzyzcjy@users.noreply.github.com Let `OverflowBox` be shrink-wrappable (flutter/flutter#129095)
2023-10-25 polinach@google.com Add dependency on leak_tracker to flutter_test. (flutter/flutter#137069)
2023-10-25 ybz975218925@gmail.com fix SliverReorderableLists item wrong offset (flutter/flutter#136828)
2023-10-25 jacksongardner@google.com Remove `bringup: true` from realm_checker and remove the redundant tool test. (flutter/flutter#137186)
2023-10-25 36861262+QuncCccccc@users.noreply.github.com Revert "Fix Gradle lockfiles." (flutter/flutter#137198)
2023-10-24 polinach@google.com Fix Gradle lockfiles. (flutter/flutter#137190)
2023-10-24 pateltirth454@gmail.com Fix Typos (flutter/flutter#137173)
2023-10-24 engine-flutter-autoroll@skia.org Roll Flutter Engine from d6a48e9963df to 6e09ee14e244 (1 revision) (flutter/flutter#137185)
2023-10-24 engine-flutter-autoroll@skia.org Roll Flutter Engine from 602b5131ee4d to d6a48e9963df (6 revisions) (flutter/flutter#137180)
2023-10-24 42715466+lirantzairi@users.noreply.github.com TextField - allow to customize cursor color in error state (flutter/flutter#136121)
2023-10-24 chris@bracken.jp [macOS] Refactor macOS build/codesize analysis (flutter/flutter#137164)
2023-10-24 engine-flutter-autoroll@skia.org Roll Flutter Engine from dc00de7dacf4 to 602b5131ee4d (1 revision) (flutter/flutter#137174)
2023-10-24 jacksongardner@google.com Check the realm file in its own shard. (flutter/flutter#137160)
2023-10-24 engine-flutter-autoroll@skia.org Roll Flutter Engine from 1890bd7dc412 to dc00de7dacf4 (1 revision) (flutter/flutter#137172)
2023-10-24 engine-flutter-autoroll@skia.org Roll Flutter Engine from ca7ef01c99c6 to 1890bd7dc412 (2 revisions) (flutter/flutter#137158)
2023-10-24 godofredoc@google.com Migrate mac builds to ruby dep. (flutter/flutter#136929)
2023-10-24 engine-flutter-autoroll@skia.org Roll Packages from 4bf5114 to 2faf992 (7 revisions) (flutter/flutter#137154)
2023-10-24 engine-flutter-autoroll@skia.org Roll Flutter Engine from 7224835bb65f to ca7ef01c99c6 (1 revision) (flutter/flutter#137153)
2023-10-24 imcusg@gmail.com fix some typos (flutter/flutter#137144)
2023-10-24 engine-flutter-autoroll@skia.org Roll Flutter Engine from 8f7488e0b4b5 to 7224835bb65f (1 revision) (flutter/flutter#137150)
2023-10-24 engine-flutter-autoroll@skia.org Roll Flutter Engine from d58895093d0a to 8f7488e0b4b5 (3 revisions) (flutter/flutter#137135)
2023-10-24 engine-flutter-autoroll@skia.org Roll Flutter Engine from 2a8471f188cd to d58895093d0a (4 revisions) (flutter/flutter#137130)
2023-10-24 engine-flutter-autoroll@skia.org Roll Flutter Engine from 71600d89aa3f to 2a8471f188cd (2 revisions) (flutter/flutter#137129)
2023-10-24 engine-flutter-autoroll@skia.org Roll Flutter Engine from 3dd197dcf9e5 to 71600d89aa3f (1 revision) (flutter/flutter#137127)
2023-10-24 engine-flutter-autoroll@skia.org Roll Flutter Engine from bdbd1058858b to 3dd197dcf9e5 (2 revisions) (flutter/flutter#137125)
2023-10-24 engine-flutter-autoroll@skia.org Roll Flutter Engine from 0ef5caa91c40 to bdbd1058858b (1 revision) (flutter/flutter#137119)
2023-10-24 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Use `coverage.collect`'s `coverableLineCache` param to speed up coverage" (flutter/flutter#137121)
2023-10-24 engine-flutter-autoroll@skia.org Roll Flutter Engine from dd6a6531e816 to 0ef5caa91c40 (1 revision) (flutter/flutter#137114)
2023-10-24 engine-flutter-autoroll@skia.org Roll Flutter Engine from 0bd703132cc1 to dd6a6531e816 (2 revisions) (flutter/flutter#137111)
2023-10-23 engine-flutter-autoroll@skia.org Roll Flutter Engine from abeab897a29c to 0bd703132cc1 (3 revisions) (flutter/flutter#137107)
2023-10-23 polinach@google.com Upgrade packages in flutter and flutter_test. (flutter/flutter#137106)
2023-10-23 49699333+dependabot[bot]@users.noreply.github.com Bump ossf/scorecard-action from 2.2.0 to 2.3.1 (flutter/flutter#137103)
2023-10-23 goderbauer@google.com Dartdoc warnings (flutter/flutter#137077)
2023-10-23 engine-flutter-autoroll@skia.org Roll Flutter Engine from e89f0421bcab to abeab897a29c (5 revisions) (flutter/flutter#137100)
2023-10-23 sokolovskyi.konstantin@gmail.com Cover last test/material tests with leak tracking. (flutter/flutter#137004)
...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
autosubmit Merge PR when tree becomes green via auto submit App framework flutter/packages/flutter repository. See also f: labels.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

OverflowBox always sets its own size as constraints.biggest, while users frequently needs it to "shrink wrap"
5 participants