feat: Jim/DTRA-702/improve chart auto-scrolling for inconsistent feeds #418
+133
−12
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- Update Flutter version constraint from >=3.10.1 to >=3.24.1 - Update CI/CD workflow to use Flutter 3.24.1 - Upgrade all dependencies to latest compatible versions - Fix Android build configuration: - Update Gradle from 7.6 to 8.0 - Update Android Gradle Plugin from 7.3.0 to 8.1.4 - Update Kotlin from 1.7.21 to 1.9.22 - Update compileSdkVersion and targetSdkVersion to 35 - Add namespace declaration for new AGP requirements - Add JVM compatibility settings - Add android:exported=true to MainActivity - Fix breaking changes: - Replace MaterialState with WidgetState APIs - Update showBottomSheet generic type usage - Add required landingCompany parameter to ActiveSymbolsRequest - Remove unnecessary dart:ui imports - All tests passing (104/104) - Android build verified successful
- Add example/.metadata to track Flutter version and migration info - Contains Flutter revision 5874a72aa4c779a02553007c47dacbefba2374dc - Tracks project type and platform migration history
- Change [email protected]:regentmarkets/flutter-deriv-api.git to HTTPS - Resolves CI permission issues with SSH key access - Enables public repository access without authentication
- Replace deriv_lint with inline analysis_options configuration - Update minimum iOS version from 11.0 to 12.0 - Upgrade dependencies: connectivity_plus, device_info_plus, package_info_plus - Remove market change reminder functionality and tests - Update iOS project configuration and Podfile - Add keystore security entries to Android .gitignore
- Add continuous frame updates via Ticker in XAxisWeb for proper auto-scrolling - Enhance _followCurrentTick logic to handle inconsistent feeds - Add _shouldFollowLastTickForInconsistentFeed for better feed handling - Improve onNewFrame method to scroll to last tick position when needed - Add proper ticker disposal in XAxisWeb dispose method
…jim/dtra-2536/upgrade-flutter-version
Reviewer's GuideEnhance XAxisModel and XAxisWeb to better handle irregular data feeds by following the last data tick, pause/resume auto-pan during manual interactions, optimize state notifications to reduce redundant updates, and add controlled ticker lifecycle management for web auto-scrolling. Sequence diagram for auto-pan pause/resume during user interactionsequenceDiagram
participant User as actor User
participant XAxisModel
participant Chart
User->>XAxisModel: scrollBy(pxShift)
XAxisModel->>XAxisModel: set _userInteracting = true
XAxisModel->>XAxisModel: disableAutoPan()
User->>XAxisModel: scrollBy(pxShift) (near latest tick)
XAxisModel->>XAxisModel: _checkIfUserScrolledToLatestTick()
XAxisModel->>XAxisModel: set _userInteracting = false (if near latest)
XAxisModel->>XAxisModel: enableAutoPan()
XAxisModel->>Chart: notifyListeners()
Sequence diagram for web XAxis ticker lifecycle managementsequenceDiagram
participant XAxisWeb
participant _XAxisStateWeb
participant Ticker
participant XAxisModel
XAxisWeb->>_XAxisStateWeb: initState()
_XAxisStateWeb->>XAxisModel: addListener(_onModelChanged)
_XAxisStateWeb->>_XAxisStateWeb: _startAutoScrollTicker() (if needed)
_XAxisStateWeb->>Ticker: createTicker(model.onNewFrame)
_XAxisModel->>_XAxisStateWeb: notifyListeners() (state change)
_XAxisStateWeb->>_XAxisStateWeb: _onModelChanged()
alt shouldRunTicker
_XAxisStateWeb->>_XAxisStateWeb: _startAutoScrollTicker()
else !shouldRunTicker
_XAxisStateWeb->>_XAxisStateWeb: _stopAutoScrollTicker()
_XAxisStateWeb->>Ticker: stop(), dispose()
end
_XAxisWeb->>_XAxisStateWeb: dispose()
_XAxisStateWeb->>XAxisModel: removeListener(_onModelChanged)
_XAxisStateWeb->>_XAxisStateWeb: _stopAutoScrollTicker()
Class diagram for updated XAxisModel auto-scroll and user interaction logicclassDiagram
class XAxisModel {
- AnimationController _scrollAnimationController
- double? _prevScrollAnimationValue
- bool _autoPanEnabled
- bool _userInteracting
- bool _dataFitMode
- double _msPerPx
- double? _prevMsPerPx
+ bool get isAutoPanEnabled
+ void enableAutoPan()
+ void disableAutoPan()
+ void resetUserInteraction()
+ void enableDataFit()
+ void disableDataFit()
+ void scrollBy(double pxShift)
+ void scrollToLastTick({bool animate = true})
- void _checkIfUserScrolledToLatestTick()
+ bool get _shouldFollowLastTickForInconsistentFeed
}
XAxisModel <|-- ChangeNotifier
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
jim-deriv
changed the title
jim-deriv
changed the title
|
@sourcery-ai review |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `lib/src/deriv_chart/chart/x_axis/widgets/x_axis_web.dart:43` </location>
<code_context>
super.initState();
+ // Add continuous frame updates for proper auto-scrolling
+ _ticker = createTicker(model.onNewFrame)..start();
+
_scrollAnimationController = AnimationController(
</code_context>
<issue_to_address>
**suggestion (performance):** Starting the ticker in initState may lead to resource usage even when auto-scrolling is not needed.
Start the ticker only when auto-scrolling is enabled to prevent unnecessary resource usage.
Suggested implementation:
```
@override
void initState() {
super.initState();
_scrollAnimationController = AnimationController(
vsync: this,
duration: widget.scrollAnimationDuration,
```
```
void _startAutoScrollTicker() {
if (_ticker == null) {
_ticker = createTicker(model.onNewFrame)..start();
}
}
void _stopAutoScrollTicker() {
_ticker?.stop();
_ticker?.dispose();
_ticker = null;
}
@override
void dispose() {
_stopAutoScrollTicker();
_scrollAnimationController?.dispose();
super.dispose();
```
You will need to:
1. Call `_startAutoScrollTicker()` when auto-scrolling is enabled (e.g., in a listener or callback when `model.isAutoScrolling` becomes true).
2. Call `_stopAutoScrollTicker()` when auto-scrolling is disabled (e.g., when `model.isAutoScrolling` becomes false).
3. Ensure that the ticker is only running when needed, and stopped otherwise.
4. If you have a listener for auto-scrolling state changes, add the calls there.
</issue_to_address>
### Comment 2
<location> `lib/src/deriv_chart/chart/x_axis/widgets/x_axis_web.dart:91-94` </location>
<code_context>
@override
void dispose() {
+ _ticker?.dispose();
_scrollAnimationController?.dispose();
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Disposing the ticker in dispose is correct, but ensure it is stopped before disposal.
Explicitly stop the ticker before disposing to avoid callbacks after widget disposal.
```suggestion
@override
void dispose() {
if (_ticker != null) {
_ticker!.stop();
_ticker!.dispose();
}
_scrollAnimationController?.dispose();
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Hey there - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `lib/src/deriv_chart/chart/x_axis/widgets/x_axis_web.dart:43` </location>
<code_context>
super.initState();
+ // Add continuous frame updates for proper auto-scrolling
+ _ticker = createTicker(model.onNewFrame)..start();
+
_scrollAnimationController = AnimationController(
</code_context>
<issue_to_address>
**suggestion (performance):** Starting the ticker in initState may have performance implications.
Pausing the ticker when the widget is hidden or auto-pan is off can help reduce unnecessary CPU usage.
Suggested implementation:
```
Ticker? _ticker;
bool _isTickerActive = false;
@override
void initState() {
super.initState();
_ticker = createTicker(model.onNewFrame);
_scrollAnimationController = AnimationController(
vsync: this,
duration: widget.scrollAnimationDuration,
```
```
void _updateTickerState() {
final shouldRunTicker = model.isAutoPanEnabled && ModalRoute.of(context)?.isCurrent == true;
if (shouldRunTicker && !_isTickerActive) {
_ticker?.start();
_isTickerActive = true;
} else if (!shouldRunTicker && _isTickerActive) {
_ticker?.stop();
_isTickerActive = false;
}
}
@override
void didChangeDependencies() {
super.didChangeDependencies();
_updateTickerState();
}
@override
void didUpdateWidget(covariant XAxisWeb oldWidget) {
super.didUpdateWidget(oldWidget);
_updateTickerState();
}
@override
void dispose() {
_ticker?.dispose();
_scrollAnimationController?.dispose();
super.dispose();
```
- You must ensure that `model.isAutoPanEnabled` is a property that reflects whether auto-pan is enabled.
- If auto-pan can change dynamically, you should listen to its changes (e.g., via a ValueNotifier, ChangeNotifier, or Stream) and call `_updateTickerState()` when it changes.
- If you have a custom visibility logic (other than ModalRoute), adjust the visibility check accordingly.
- Replace `XAxisWeb` in `didUpdateWidget` with the actual widget class name if different.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
- Add user interaction tracking to prevent auto-scroll interference - Modify _shouldFollowLastTickForInconsistentFeed to respect manual scrolling - Auto-scroll only re-enables when user scrolls back to latest tick area - Preserve existing auto-follow behavior for live data feeds - Fix issue where users couldn't scroll left to view tick history Resolves scrolling limitation that prevented viewing historical data
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey there - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `lib/src/deriv_chart/chart/x_axis/x_axis_model.dart:432-436` </location>
<code_context>
+
+ /// Resets the user interaction flag to allow auto-pan to resume.
+ /// This can be called when you want to programmatically re-enable auto-following.
+ void resetUserInteraction() {
+ _userInteracting = false;
+ }
</code_context>
<issue_to_address>
**suggestion:** resetUserInteraction does not notify listeners.
Consider adding notifyListeners() to resetUserInteraction so dependent UI elements receive state updates.
```suggestion
/// Resets the user interaction flag to allow auto-pan to resume.
/// This can be called when you want to programmatically re-enable auto-following.
void resetUserInteraction() {
_userInteracting = false;
notifyListeners();
}
```
</issue_to_address>
### Comment 2
<location> `lib/src/deriv_chart/chart/x_axis/x_axis_model.dart:543` </location>
<code_context>
void scrollBy(double pxShift) {
+ // If this is a significant manual scroll (not just tiny adjustments),
+ // mark as user interaction to prevent auto-scroll interference
+ if (pxShift.abs() > 1.0) {
+ _userInteracting = true;
+ }
</code_context>
<issue_to_address>
**nitpick:** Magic number for pxShift threshold could be extracted.
Extract the 1.0 threshold into a named constant to improve readability and maintainability.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
- Add significantScrollThreshold constant (value: 1) to improve readability - Replace magic number 1.0 in scrollBy() method with named constant - Improves code maintainability and makes threshold purpose explicit
- Ensures UI elements are notified when user interaction state changes - Fixes inconsistency where other methods setting _userInteracting call notifyListeners() - Improves responsiveness when programmatically resetting auto-pan behavior
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- In resetUserInteraction you always call notifyListeners even when _userInteracting hasn’t changed—wrap that in a conditional to avoid redundant rebuilds.
- The _checkIfUserScrolledToLatestTick method clears _userInteracting but doesn’t call notifyListeners or explicitly restart the auto-pan ticker—consider adding notification or resume logic there so the UI updates immediately.
- You repeat the "if (newValue != oldValue) { setValue; notifyListeners(); }" pattern in several setters—extract that into a helper to reduce boilerplate and ensure consistency.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In resetUserInteraction you always call notifyListeners even when _userInteracting hasn’t changed—wrap that in a conditional to avoid redundant rebuilds.
- The _checkIfUserScrolledToLatestTick method clears _userInteracting but doesn’t call notifyListeners or explicitly restart the auto-pan ticker—consider adding notification or resume logic there so the UI updates immediately.
- You repeat the "if (newValue != oldValue) { setValue; notifyListeners(); }" pattern in several setters—extract that into a helper to reduce boilerplate and ensure consistency.
## Individual Comments
### Comment 1
<location> `lib/src/deriv_chart/chart/x_axis/x_axis_model.dart:548` </location>
<code_context>
void scrollBy(double pxShift) {
+ // If this is a significant manual scroll (not just tiny adjustments),
+ // mark as user interaction to prevent auto-scroll interference
+ if (pxShift.abs() > significantScrollThreshold) {
+ _userInteracting = true;
+ }
</code_context>
<issue_to_address>
**suggestion:** Consider making significantScrollThreshold configurable.
Making the threshold configurable would allow adaptation to different requirements and improve flexibility.
Suggested implementation:
```
// If this is a significant manual scroll (not just tiny adjustments),
// mark as user interaction to prevent auto-scroll interference
if (pxShift.abs() > significantScrollThreshold) {
_userInteracting = true;
}
```
```
double significantScrollThreshold;
/// Called to scroll the chart
void scrollBy(double pxShift) {
// If this is a significant manual scroll (not just tiny adjustments),
// mark as user interaction to prevent auto-scroll interference
```
```
break;
case ViewingMode.fitData:
fitAvailableData();
```
```
/// Enables data fit viewing mode.
void enableDataFit() {
if (_dataFitMode != true) {
```
1. Add `significantScrollThreshold` as a parameter to the class constructor, with a default value (e.g., `significantScrollThreshold = 5.0`).
2. Replace all usages of `significantScrollThreshold` in the class with `this.significantScrollThreshold`.
3. If `significantScrollThreshold` is currently a global or static variable, remove or refactor it accordingly.
4. Update any places where the class is instantiated to optionally pass a custom threshold value.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Clickup link:
Fixes issue: #
This PR contains the following changes:
Developers Note (Optional)
Pre-launch Checklist (For PR creator)
As a creator of this PR:
Reviewers
Pre-launch Checklist (For Reviewers)
As a reviewer I ensure that:
Pre-launch Checklist (For QA)
Pre-launch Checklist (For Maintainer)
Summary by Sourcery
Improve chart auto-scrolling by adding support for inconsistent data feeds, pausing and resuming auto-pan around user interactions, and optimizing the web ticker lifecycle
New Features:
Bug Fixes:
Enhancements: