Feature/controller methods extention - #5
Conversation
|
I ran through the code and found a logic issue with how the autoplay override state is managed, along with the same page indicator issues from PR #4. Here is what needs to be fixed before I can merge:
To fix this, you should change final shouldAutoplay = _autoplayControllerOverride ?? widget.autoplay;
if (!shouldAutoplay) return;This cleanly defaults to the widget's parameter unless the controller has explicitly overridden it. Other than that it also has the page indicator issues from PR #4 . Also make sure you didnt update package version in pubspec.yaml Once these are updated, let me know and I'll take another look! |
GalaxyPhoenix716
left a comment
There was a problem hiding this comment.
autoplay has bug (check the comment)
There was a problem hiding this comment.
Pull request overview
This PR expands the CoverflowCarouselController API to provide more programmatic control over carousel behavior (autoplay and direct navigation), and introduces a reusable CoverflowPageIndicator widget (exported as part of the public package API) with example + tests.
Changes:
- Added controller APIs for
jumpTo(int),startAutoplay(),stopAutoplay(), and dynamic autoplay direction viasetAutoplayDirection(bool). - Updated carousel state to support controller-driven autoplay enable/disable overrides and backward autoplay ticking.
- Added
CoverflowPageIndicator(library export), with example usage and new widget tests.
Reviewed changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| lib/src/coverflow_carousel_controller.dart | Adds new controller APIs for autoplay control, direction, and instant navigation. |
| lib/src/coverflow_carousel.dart | Wires controller callbacks into carousel state; adds controller override for autoplay and backward direction ticking. |
| lib/src/coverflow_page_indicator.dart | Introduces the new page indicator widget driven by controller page metrics. |
| lib/coverflow_carousel.dart | Exports CoverflowPageIndicator as part of the public package surface. |
| test/coverflow_carousel_test.dart | Adds/extends tests for jumpTo, autoplay start/stop, and autoplay direction changes. |
| test/coverflow_page_indicator_test.dart | Adds widget tests validating dot count, updates, taps, and edge cases. |
| example/lib/main.dart | Updates the example app to use the new exported CoverflowPageIndicator and controller autoplay/jump controls. |
| example/pubspec.lock | Bumps the example’s path dependency version for the package. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
GalaxyPhoenix716
left a comment
There was a problem hiding this comment.
acknowledge the copilot reviews
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 8 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (4)
lib/src/coverflow_carousel_controller.dart:87
- The stopAutoplay() docstring claims autoplay can resume when the widget's
autoplayproperty istrue, but the carousel state uses_autoplayControllerOverride ?? widget.autoplay; once the controller calls stopAutoplay it sets the override tofalse, which continues to win overwidget.autoplayuntil startAutoplay is called. Please update the docstring (or change the override behavior) so the contract matches actual behavior.
/// Stops autoplay entirely. The carousel will not auto-advance until
/// [startAutoplay] is called again or the widget's `autoplay` property
/// is `true`.
void stopAutoplay() => _stopAutoplay?.call();
lib/src/coverflow_carousel_controller.dart:12
- Issue #3 (which this PR marks as fixed) explicitly requests
pauseAutoplay()/resumeAutoplay()controller APIs, but the controller only adds startAutoplay()/stopAutoplay(). Either add pause/resume (likely mapping to the widget's existing pause/resume behavior such as hover/drag pauses) or adjust the PR description / issue linkage so it doesn't claim to fully satisfy #3.
/// Also exposes autoplay controls — [startAutoplay], [stopAutoplay], and
/// [setAutoplayDirection] — plus instant [jumpTo] navigation without animation.
///
lib/src/coverflow_carousel.dart:535
- Autoplay enable/disable now depends on
_autoplayControllerOverride, but that override is not cleared when theCoverflowCarouselControllerinstance changes (didUpdateWidget detaches/attaches the controller without resetting the override). If one controller called stopAutoplay(), then a later rebuild with a new controller (or null controller) can unexpectedly keep autoplay disabled even whenwidget.autoplayis true. Consider resetting_autoplayControllerOverride(e.g., to null) whenoldWidget.controller != widget.controller.
if (oldWidget.autoplay != widget.autoplay ||
oldWidget.autoplayInterval != widget.autoplayInterval) {
if (_autoplayControllerOverride ?? widget.autoplay) {
_resumeAutoplay();
} else {
_pauseAutoplay();
}
}
lib/src/coverflow_page_indicator.dart:58
- The dot tap targets are positioned with a negative left offset, so part of the first dot’s enlarged tap area falls outside the Stack/SizedBox bounds and is not hittable. Also, the dots currently have no semantic labels/actions for screen readers. Adjust the layout so tap targets stay within bounds, and add Semantics so assistive tech can announce and activate each page dot.
left: i * step - (_tapTargetSize - dotSize) / 2,
top: 4 - (_tapTargetSize - dotSize) / 2,
width: _tapTargetSize,
height: _tapTargetSize,
child: GestureDetector(
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 8 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (2)
lib/src/coverflow_carousel_controller.dart:87
- The
stopAutoplaydoc says autoplay may resume when the widget'sautoplayproperty istrue, but the implementation uses a controller override that continues to disable autoplay even whenwidget.autoplayis true. Please align the documentation with the actual behavior (or adjust the logic if the doc is the intended contract).
/// Stops autoplay entirely. The carousel will not auto-advance until
/// [startAutoplay] is called again or the widget's `autoplay` property
/// is `true`.
void stopAutoplay() => _stopAutoplay?.call();
lib/src/coverflow_carousel_controller.dart:12
- Issue #3 explicitly asks for
pauseAutoplay()/resumeAutoplay()in addition to start/stop, but those APIs are not added in this PR (despite the PR description stating it fixes #3). Either add the missing controller methods or update the PR/issue linkage so it doesn't claim full completion of #3.
/// Also exposes autoplay controls — [startAutoplay], [stopAutoplay], and
/// [setAutoplayDirection] — plus instant [jumpTo] navigation without animation.
///
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 8 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (2)
lib/src/coverflow_carousel_controller.dart:86
- The stopAutoplay() docstring claims autoplay will resume if the widget’s
autoplayproperty istrue, but the implementation uses a controller override (_autoplayControllerOverride = false) that keeps autoplay disabled regardless of the widget property. The docs should match the actual override behavior (or the implementation should be changed to match the docs).
/// Stops autoplay entirely. The carousel will not auto-advance until
/// [startAutoplay] is called again or the widget's `autoplay` property
/// is `true`.
lib/src/coverflow_carousel_controller.dart:12
- The PR description says “Fixes #3”, but Issue #3 explicitly requests
pauseAutoplay()/resumeAutoplay()in addition to start/stop. Those APIs are not added here, so either implement pause/resume (and corresponding carousel wiring/tests) or adjust the PR/issue linkage so it doesn’t claim to fully fix #3.
/// Also exposes autoplay controls — [startAutoplay], [stopAutoplay], and
/// [setAutoplayDirection] — plus instant [jumpTo] navigation without animation.
///
|
Work on this after I merge the page indicator pr because you added that in this too |
|
I guess this should be good and ready for re-review. |
GalaxyPhoenix716
left a comment
There was a problem hiding this comment.
I did a deep dive into the safety of the new controller callbacks and gesture math. There are two potential crashes we should guard against:
- PageController Client Assertions (Possible Crash)
In_attachController(insidecoverflow_carousel.dart), all the programmatic controller methods (next,previous,animateTo,jumpTo) call the underlyingPageControllermethods directly.
If a developer triggers these methods before the carousel finishes its first build or when it is off-screen, it will crash with ahasClientsassertion error. We should add a guard check to each one:
jumpTo: (index) {
if (!_controller.hasClients) return;
// ...
}Division by Zero in Indicator Tap Calculation In coverflow_page_indicator.dart, we calculate final index = (dx / step).round(). If a developer passes dotSize: 0 and dotSpacing: 0, step becomes 0, resulting in a division-by-zero crash. We should add assertions to the CoverflowPageIndicator constructor to guarantee safe values:
dart
assert(dotSize > 0, 'dotSize must be greater than zero'),
assert(dotSpacing >= 0, 'dotSpacing must be non-negative'),Other than that, this looks excellent
Summary
This PR extends
CoverflowCarouselControllerwith additional programmatic controls for autoplay management and page navigation.Changes
startAutoplay()stopAutoplay()jumpTo(int page)for instant page navigation without animationFixes #3