Skip to content

Fix CarouselView rebuild #152791

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 4 commits into from
Aug 5, 2024
Merged

Fix CarouselView rebuild #152791

merged 4 commits into from
Aug 5, 2024

Conversation

@QuncCccccc
Copy link
Contributor

@QuncCccccc QuncCccccc commented Aug 3, 2024 •
edited
Loading

Fixes #152787

Originally, when we want to update itemExtent, we didn't check if the old itemExtent is null but getItemFromPixels() needs that information.
https://github.com/flutter/flutter/blob/master/packages/flutter/lib/src/material/carousel.dart#L1343-L1347
Then in getItemFromPixels(), it goes to the else statement which assert flexWeights is not null, then the exception happens.

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.
  • [x ] 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].
  • I followed the [breaking change policy] and added [Data Driven Fixes] where supported.
  • All existing and new tests are passing.

@github-actions github-actions bot added framework flutter/packages/flutter repository. See also f: labels. f: material design flutter/packages/flutter/material repository. labels Aug 3, 2024
navaronbracke
navaronbracke reviewed Aug 3, 2024
View reviewed changes
packages/flutter/test/material/carousel_test.dart
await tester.tap(find.byType(Switch));
await tester.pumpAndSettle();

// No exception.
Copy link
Contributor

@navaronbracke navaronbracke Aug 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

You can use an expect to verify this :)

Suggested change
// No exception.
expect(tester.takeException(), isNull);

@QuncCccccc QuncCccccc marked this pull request as ready for review August 5, 2024 16:32
@QuncCccccc QuncCccccc requested a review from Piinks August 5, 2024 16:32
Piinks
Piinks approved these changes Aug 5, 2024
View reviewed changes
Copy link
Contributor

@Piinks Piinks left a comment

Choose a reason for hiding this comment

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

LGTM

@QuncCccccc QuncCccccc added the autosubmit Merge PR when tree becomes green via auto submit App label Aug 5, 2024
@auto-submit auto-submit bot merged commit 173bf86 into flutter:master Aug 5, 2024
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Aug 6, 2024
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 6, 2024
@engine-flutter-autoroll
173bf86 Fix CarouselView rebuild (flutter/flutter#152791)
edfc3bb
auto-submit bot pushed a commit to flutter/packages that referenced this pull request Aug 6, 2024
@engine-flutter-autoroll
Manual roll Flutter from 3832823 to 1dd7141 (21 revisions) (#7317)
551bde5 
Manual roll requested by [email protected]

flutter/flutter@3832823...1dd7141

2024-08-06 [email protected] Roll Flutter Engine from 376a853846ce to f5f6e966e7e7 (1 revision) (flutter/flutter#152911)
2024-08-06 [email protected] Pass --no-dds to some integration tests driven by flutter drive (flutter/flutter#152898)
2024-08-06 [email protected] Manual dependency bump (flutter/flutter#152881)
2024-08-06 [email protected] Add tests for ordered_traversal_group.0.dart (flutter/flutter#152849)
2024-08-06 [email protected] Implement `on` clauses (flutter/flutter#152706)
2024-08-06 [email protected] Roll Flutter Engine from bc140a0124b7 to 376a853846ce (2 revisions) (flutter/flutter#152905)
2024-08-06 [email protected] Roll Flutter Engine from e073ad2e3ad4 to bc140a0124b7 (1 revision) (flutter/flutter#152895)
2024-08-05 [email protected] added functionality to where SR will communicate button clicked (flutter/flutter#152185)
2024-08-05 [email protected] more docImports (flutter/flutter#151951)
2024-08-05 [email protected] Bump dartdoc to 8.0.13 (flutter/flutter#152896)
2024-08-05 [email protected] [Reland] Introduce `double` `Flex.spacing` parameter for `Row`/`Column` spacing (flutter/flutter#152890)
2024-08-05 [email protected] Fix CarouselView rebuild (flutter/flutter#152791)
2024-08-05 [email protected] Roll Flutter Engine from 1bfd06cda313 to e073ad2e3ad4 (4 revisions) (flutter/flutter#152889)
2024-08-05 [email protected] Add migration to git ignore SwiftPM build directories (flutter/flutter#152766)
2024-08-05 [email protected] Roll pub packages (flutter/flutter#152127)
2024-08-05 [email protected] Roll Flutter Engine from 0aac60342005 to 1bfd06cda313 (2 revisions) (flutter/flutter#152868)
2024-08-05 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Introduce `double` `Flex.spacing` parameter for `Row`/`Column` spacing (#152472)" (flutter/flutter#152885)
2024-08-05 [email protected] [Docs] DeviceOrientation Enum Correction (flutter/flutter#152876)
2024-08-05 [email protected] Move Linux_build_test tests from staging to prod (flutter/flutter#152877)
2024-08-05 [email protected] Introduce `double` `Flex.spacing` parameter for `Row`/`Column` spacing (flutter/flutter#152472)
2024-08-05 [email protected] Mark Linux_android_emu tests bringup: true (flutter/flutter#152867)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-packages
Please CC [email protected],[email protected],[email protected] on the revert to ensure that a human
is aware of the problem.

To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://issues.skia.org/issues/new?component=1389291&template=1850622

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
TytaniumDev pushed a commit to TytaniumDev/flutter that referenced this pull request Aug 7, 2024
@QuncCccccc @TytaniumDev
Fix CarouselView rebuild (flutter#152791)
3e6912a 
Fixes flutter#152787

Originally, when we want to update `itemExtent`, we didn't check if the old itemExtent is null but `getItemFromPixels()` needs that information.
https://github.com/flutter/flutter/blob/master/packages/flutter/lib/src/material/carousel.dart#L1343-L1347
Then in `getItemFromPixels()`, it goes to the else statement which assert `flexWeights` is not null, then the exception happens.

- [x ] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement].
Buchimi pushed a commit to Buchimi/flutter that referenced this pull request Sep 2, 2024
@QuncCccccc @Buchimi
Fix CarouselView rebuild (flutter#152791)
b88806b 
Fixes flutter#152787

Originally, when we want to update `itemExtent`, we didn't check if the old itemExtent is null but `getItemFromPixels()` needs that information.
https://github.com/flutter/flutter/blob/master/packages/flutter/lib/src/material/carousel.dart#L1343-L1347
Then in `getItemFromPixels()`, it goes to the else statement which assert `flexWeights` is not null, then the exception happens.

- [x ] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement].
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Dec 11, 2024
@engine-flutter-autoroll
173bf86 Fix CarouselView rebuild (flutter/flutter#152791)
eb38d30
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Dec 12, 2024
@engine-flutter-autoroll
173bf86 Fix CarouselView rebuild (flutter/flutter#152791)
a906ecf
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Piinks Piinks Piinks approved these changes

+1 more reviewer

@navaronbracke navaronbracke navaronbracke left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

autosubmit Merge PR when tree becomes green via auto submit App 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.

CarouselView throws assert exception when rebuild

3 participants

@QuncCccccc @Piinks @navaronbracke