Merge master into current branch #1
Conversation
…ged callback to CarouselView
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces an onIndexChanged callback to the CarouselView widget, allowing developers to listen for changes to the leading item. It also adds a leadingItem getter to CarouselController for programmatic access. The changes are well-implemented and include comprehensive tests.
My review focuses on a few key areas:
- There is a discrepancy in the documentation for
onIndexChangedwhich incorrectly describes the "leading item" as the "primary item" for weighted carousels. I've suggested a clarification. - Several of the new tests contain incorrect assertions regarding the visibility of items in a weighted carousel, confusing the leading item with the primary (most visible) item. I've pointed these out with suggestions for how to correct them.
Overall, the feature is a great addition, and with the suggested fixes to the documentation and tests, it will be solid.
| /// - In a [CarouselView.weighted], the leading item is chosen by the weighted | ||
| /// layout algorithm (typically the one with the greatest effective weight; | ||
| /// ties are resolved using proximity to the leading edge). |
There was a problem hiding this comment.
The documentation for onIndexChanged in a CarouselView.weighted is misleading. It describes the "primary" item (the one with the greatest weight), but the implementation and the leadingItem property refer to the item at the leading edge of the viewport, which might not be the primary one. This can cause confusion for developers using this callback. The documentation should be updated to accurately reflect that onIndexChanged reports the index of the item at the leading edge of the viewport.
/// - In a [CarouselView.weighted], the leading item is also the one
/// positioned at the leading edge of the viewport. The primary item, which
/// is the one with the largest extent, may be a different item depending on
/// the `flexWeights`.| testWidgets('CarouselView shows correct item after animation with symmetric flexWeights', ( | ||
| WidgetTester tester, | ||
| ) async { | ||
| final controller = CarouselController(); | ||
| addTearDown(controller.dispose); | ||
| var leadingIndex = 0; | ||
|
|
||
| await tester.pumpWidget( | ||
| MaterialApp( | ||
| home: Scaffold( | ||
| body: CarouselView.weighted( | ||
| flexWeights: const <int>[2, 5, 2], | ||
| controller: controller, | ||
| itemSnapping: true, | ||
| onIndexChanged: (int index) { | ||
| leadingIndex = index; | ||
| }, | ||
| children: List<Widget>.generate(6, (int i) => Text('Item $i')), | ||
| ), | ||
| ), | ||
| ), | ||
| ); | ||
| await tester.pumpAndSettle(); | ||
|
|
||
| controller.animateToItem( | ||
| 4, | ||
| duration: const Duration(milliseconds: 200), | ||
| curve: Curves.linear, | ||
| ); | ||
| await tester.pumpAndSettle(); | ||
|
|
||
| expect(controller.leadingItem, equals(4)); | ||
| expect(leadingIndex, equals(4)); | ||
|
|
||
| final double visible4 = visiblePortionOf(tester, 'Item 4'); | ||
| final double visible3 = visiblePortionOf(tester, 'Item 3'); | ||
| final double visible5 = visiblePortionOf(tester, 'Item 5'); | ||
| expect(visible4, greaterThan(visible3)); | ||
| expect(visible4, greaterThan(visible5)); | ||
|
|
||
| controller.animateToItem( | ||
| 2, | ||
| duration: const Duration(milliseconds: 200), | ||
| curve: Curves.linear, | ||
| ); | ||
| await tester.pumpAndSettle(); | ||
|
|
||
| expect(controller.leadingItem, equals(2)); | ||
| expect(leadingIndex, equals(2)); | ||
|
|
||
| final double visible2 = visiblePortionOf(tester, 'Item 2'); | ||
| final double visible1 = visiblePortionOf(tester, 'Item 1'); | ||
| final double visible3After = visiblePortionOf(tester, 'Item 3'); | ||
| expect(visible2, greaterThan(visible1)); | ||
| expect(visible2, greaterThan(visible3After)); | ||
| }); |
There was a problem hiding this comment.
The visibility assertions in this test appear to be incorrect. They seem to confuse the "leading" item with the "primary" (most visible) item.
- After
animateToItem(4)withflexWeights: [2, 5, 2], item 4 is the leading item, but item 5 should be the primary and most visible one. - Similarly, after
animateToItem(2), item 2 is leading, but item 3 should be most visible.
The assertions should be updated to check for the correct most visible item. For example, after animateToItem(4), you should check that visible5 is greater than visible4 and visible6.
| testWidgets('CarouselView shows correct item after animation with asymmetric flexWeights', ( | ||
| WidgetTester tester, | ||
| ) async { | ||
| final controller = CarouselController(); | ||
| addTearDown(controller.dispose); | ||
| var leadingIndex = 0; | ||
|
|
||
| await tester.pumpWidget( | ||
| MaterialApp( | ||
| home: Scaffold( | ||
| body: CarouselView.weighted( | ||
| flexWeights: const <int>[1, 2, 3, 4], | ||
| controller: controller, | ||
| itemSnapping: true, | ||
| onIndexChanged: (int index) { | ||
| leadingIndex = index; | ||
| }, | ||
| children: List<Widget>.generate(6, (int i) => Text('Item $i')), | ||
| ), | ||
| ), | ||
| ), | ||
| ); | ||
| await tester.pumpAndSettle(); | ||
|
|
||
| controller.animateToItem( | ||
| 4, | ||
| duration: const Duration(milliseconds: 200), | ||
| curve: Curves.linear, | ||
| ); | ||
| await tester.pumpAndSettle(); | ||
|
|
||
| expect(controller.leadingItem, equals(4)); | ||
| expect(leadingIndex, equals(4)); | ||
| expect(find.text('Item 4'), findsOneWidget); | ||
|
|
||
| final double visible4 = visiblePortionOf(tester, 'Item 4'); | ||
| final double visible3 = visiblePortionOf(tester, 'Item 3'); | ||
| final double visible5 = visiblePortionOf(tester, 'Item 5'); | ||
| expect(visible4, greaterThan(visible3)); | ||
| expect(visible4, greaterThan(visible5)); | ||
|
|
||
| controller.animateToItem( | ||
| 2, | ||
| duration: const Duration(milliseconds: 200), | ||
| curve: Curves.linear, | ||
| ); | ||
| await tester.pumpAndSettle(); | ||
|
|
||
| expect(controller.leadingItem, equals(2)); | ||
| expect(leadingIndex, equals(2)); | ||
| expect(find.text('Item 2'), findsOneWidget); | ||
|
|
||
| final double visible2 = visiblePortionOf(tester, 'Item 2'); | ||
| final double visible1 = visiblePortionOf(tester, 'Item 1'); | ||
| final double visible3After = visiblePortionOf(tester, 'Item 3'); | ||
| expect(visible2, greaterThan(visible1)); | ||
| expect(visible2, greaterThan(visible3After)); | ||
| }); |
There was a problem hiding this comment.
The visibility assertions in this test are incorrect due to a confusion between the leading and primary items.
- After
animateToItem(4)withflexWeights: [1, 2, 3, 4], item 4 is leading, but item 7 should be the most visible. - After
animateToItem(2), item 2 is leading, but item 5 should be the most visible.
The tests should be corrected to assert the visibility of the correct primary item.
| final int mostVisibleIndexAfterLeftDrag = visibleAreasAfterLeftDrag.indexWhere( | ||
| (double area) => area == visibleAreasAfterLeftDrag.reduce(math.max), | ||
| ); | ||
| expect(mostVisibleIndexAfterLeftDrag, equals(controller.leadingItem)); |
There was a problem hiding this comment.
This assertion is incorrect for the given flexWeights: const <int>[2, 5, 2]. With this configuration, the most visible (primary) item is the one after the leading item (i.e., at leadingItem + 1), not at leadingItem.
| expect(mostVisibleIndexAfterLeftDrag, equals(controller.leadingItem)); | |
| expect(mostVisibleIndexAfterLeftDrag, equals(controller.leadingItem + 1)); |
| final int mostVisibleIndexAfterRightDrag = visibleAreasAfterRightDrag.indexWhere( | ||
| (double area) => area == visibleAreasAfterRightDrag.reduce(math.max), | ||
| ); | ||
| expect(mostVisibleIndexAfterRightDrag, equals(controller.leadingItem)); |
There was a problem hiding this comment.
Similar to the previous check, this assertion is incorrect. The most visible item's index should be controller.leadingItem + 1 for this flexWeights configuration.
| expect(mostVisibleIndexAfterRightDrag, equals(controller.leadingItem)); | |
| expect(mostVisibleIndexAfterRightDrag, equals(controller.leadingItem + 1)); |
1 participant
No description provided.