diff --git a/example/pubspec.yaml b/example/pubspec.yaml index 659ce6701..7690e1a9e 100644 --- a/example/pubspec.yaml +++ b/example/pubspec.yaml @@ -27,6 +27,9 @@ dev_dependencies: sdk: flutter intl_utils: ^2.8.2 +dependency_overrides: + intl: 0.20.2 + flutter: uses-material-design: true diff --git a/lib/src/add_ons/add_ons_repository.dart b/lib/src/add_ons/add_ons_repository.dart index 03844d6e5..2cd9ccb1e 100644 --- a/lib/src/add_ons/add_ons_repository.dart +++ b/lib/src/add_ons/add_ons_repository.dart @@ -3,6 +3,7 @@ import 'package:collection/collection.dart'; import 'package:flutter/material.dart'; import 'package:deriv_chart/src/add_ons/add_on_config.dart'; import 'package:deriv_chart/src/add_ons/repository.dart'; +import 'package:deriv_chart/src/misc/chart_diagnostics.dart'; import 'package:shared_preferences/shared_preferences.dart'; /// Called to create an AddOnConfig object from a map. @@ -73,6 +74,7 @@ class AddOnsRepository extends ChangeNotifier if (!prefs.containsKey(addOnsKey)) { // No saved indicators or drawing tools. + chartDiag('repo#$hashCode loadFromPrefs($addOnsKey): no saved items'); notifyListeners(); return; } @@ -90,6 +92,9 @@ class AddOnsRepository extends ChangeNotifier _hiddenStatus.add(false); } + chartDiag('repo#$hashCode loadFromPrefs($addOnsKey): loaded ' + '${items.map((T c) => c.configId).toList()}'); + notifyListeners(); } @@ -98,6 +103,8 @@ class AddOnsRepository extends ChangeNotifier void add(T addOnConfig) { items.add(addOnConfig); _hiddenStatus.add(false); + chartDiag('repo#$hashCode add(${addOnConfig.configId}) -> $addOnsKey, ' + 'items: ${items.map((T c) => c.configId).toList()}'); _writeToPrefs(); notifyListeners(); } @@ -128,6 +135,9 @@ class AddOnsRepository extends ChangeNotifier } final removedItem = items.removeAt(index); _hiddenStatus.removeAt(index); + chartDiag('repo#$hashCode removeAt($index) ' + 'removed ${removedItem.configId} -> $addOnsKey, ' + 'items: ${items.map((T c) => c.configId).toList()}'); _writeToPrefs(); // Notify about the deletion onDeleteCallback?.call(removedItem, index); @@ -148,6 +158,7 @@ class AddOnsRepository extends ChangeNotifier void clear() { items.clear(); _hiddenStatus.clear(); + chartDiag('repo#$hashCode clear() -> $addOnsKey'); _writeToPrefs(); notifyListeners(); } diff --git a/lib/src/deriv_chart/chart/main_chart.dart b/lib/src/deriv_chart/chart/main_chart.dart index 54c050867..e729aef57 100644 --- a/lib/src/deriv_chart/chart/main_chart.dart +++ b/lib/src/deriv_chart/chart/main_chart.dart @@ -399,6 +399,11 @@ class _ChartImplementationState extends BasicChartState { @override Widget build(BuildContext context) => LayoutBuilder( + // Force remount when markerSeries changes. + // LayoutBuilder only re-invokes its builder on constraint changes; + // without a key that tracks markerSeries identity, the old + // MarkerArea subtree persists after trade-type switches. + key: ObjectKey(widget.markerSeries), builder: (BuildContext context, BoxConstraints constraints) { final XAxisModel xAxis = context.watch(); @@ -579,17 +584,20 @@ class _ChartImplementationState extends BasicChartState { ), ); - Widget _buildMarkerArea() => MultipleAnimatedBuilder( - animations: [ - currentTickAnimation, - topBoundQuoteAnimationController, - bottomBoundQuoteAnimationController - ], - builder: (BuildContext context, _) => MarkerArea( - markerSeries: widget.markerSeries!, - quoteToCanvasY: chartQuoteToCanvasY, - animationInfo: AnimationInfo( - currentTickPercent: currentTickAnimation.value, + Widget _buildMarkerArea() => KeyedSubtree( + key: ObjectKey(widget.markerSeries), + child: MultipleAnimatedBuilder( + animations: [ + currentTickAnimation, + topBoundQuoteAnimationController, + bottomBoundQuoteAnimationController + ], + builder: (BuildContext context, _) => MarkerArea( + markerSeries: widget.markerSeries!, + quoteToCanvasY: chartQuoteToCanvasY, + animationInfo: AnimationInfo( + currentTickPercent: currentTickAnimation.value, + ), ), ), ); diff --git a/lib/src/deriv_chart/interactive_layer/interactive_layer.dart b/lib/src/deriv_chart/interactive_layer/interactive_layer.dart index 05c496d3b..d061b4a67 100644 --- a/lib/src/deriv_chart/interactive_layer/interactive_layer.dart +++ b/lib/src/deriv_chart/interactive_layer/interactive_layer.dart @@ -17,8 +17,11 @@ import 'package:deriv_chart/src/deriv_chart/interactive_layer/interactive_layer_ import 'package:deriv_chart/src/models/axis_range.dart'; import 'package:deriv_chart/src/models/chart_config.dart'; import 'package:deriv_chart/src/theme/chart_theme.dart'; +import 'package:deriv_chart/src/misc/chart_diagnostics.dart'; +import 'package:flutter/foundation.dart' show ValueGetter, listEquals; import 'package:flutter/gestures.dart'; import 'package:flutter/material.dart'; +import 'package:flutter/scheduler.dart' show SchedulerPhase; import 'package:provider/provider.dart'; import '../chart/data_visualization/chart_data.dart'; @@ -34,6 +37,7 @@ import 'interactive_layer_base.dart'; import 'enums/state_change_direction.dart'; import 'interactive_layer_behaviours/interactive_layer_behaviour.dart'; import 'interactive_layer_states/interactive_normal_state.dart'; +import 'interactive_layer_states/interactive_state.dart'; /// Defines the different interaction modes for the interactive layer. /// @@ -140,20 +144,103 @@ class _InteractiveLayerState extends State { final Map _interactableDrawings = {}; + /// Fired whenever [_interactableDrawings] changes, so the drawings paint + /// layer repaints from the live map even if the widget rebuild carrying the + /// new list does not propagate (observed in release builds on the + /// bottom-sheet "clear all" flow). + final InteractionNotifier _drawingsChanged = InteractionNotifier(); + + bool _stateResetScheduled = false; + @override void initState() { super.initState(); + chartDiag('layer#$hashCode initState, ' + 'repo#${widget.drawingToolsRepo.hashCode} ' + 'items: ${widget.drawingToolsRepo.items.map((c) => c.configId).toList()}'); + widget.drawingToolsRepo.addListener(syncDrawingsWithConfigs); + + // Sync with the repo content on mount. The repo may already contain items + // whose notifications were fired before this layer was mounted (e.g. + // drawings loaded from SharedPreferences while the chart subtree was + // being (re)mounted, such as after a symbol switch). Without this initial + // sync, those drawings would never appear until the next repo mutation. + // + // Deferred to post-frame because the behaviour's `interactiveLayer` + // reference is bound by the child gesture handler's initState, which runs + // after this initState. + WidgetsBinding.instance.addPostFrameCallback((_) { + if (mounted) { + syncDrawingsWithConfigs(); + } + }); } + @override + void didUpdateWidget(covariant InteractiveLayer oldWidget) { + super.didUpdateWidget(oldWidget); + + // If the repository instance changed, move the listener to the new + // instance, otherwise mutations on the new repo would never reach this + // layer. The build that follows this didUpdateWidget reconciles the + // drawings against the new repo. + if (!identical(oldWidget.drawingToolsRepo, widget.drawingToolsRepo)) { + chartDiag('layer#$hashCode repo changed ' + '#${oldWidget.drawingToolsRepo.hashCode} -> ' + '#${widget.drawingToolsRepo.hashCode}'); + oldWidget.drawingToolsRepo.removeListener(syncDrawingsWithConfigs); + widget.drawingToolsRepo.addListener(syncDrawingsWithConfigs); + } + } + + /// Repository listener: reconciles and triggers a rebuild. void syncDrawingsWithConfigs() { + if (!mounted) { + return; + } + + try { + _reconcileDrawings(); + } on Object catch (error, stackTrace) { + // Diagnostics: a throw between the map mutation and setState would + // leave the painted drawings stale with no visible error in release. + // Log it and still rebuild from whatever state the map is in. + chartDiag('layer#$hashCode reconcile threw: $error\n$stackTrace'); + } + setState(() {}); + } + + /// Reconciles [_interactableDrawings] with the repository contents. + /// + /// This is called from [build] (as well as from the repository listener) so + /// that the painted drawings always reflect the current repository state on + /// every rebuild. Correctness must not depend on ChangeNotifier listener + /// timing alone: in release builds the chart subtree can be remounted + /// (e.g. keyed remounts on symbol/marker changes) in timings where a + /// repository notification fires while no listener is attached, which + /// leaves stale drawings painted (and hit-testable) or loaded drawings + /// never shown. + void _reconcileDrawings() { final interactiveLayerBehaviour = widget.interactiveLayerBehaviour; + + if (!interactiveLayerBehaviour.isInitialized) { + // Very first build: the gesture handler hasn't bound the behaviour to + // this layer yet. The post-frame initial sync from initState will + // reconcile right after. + chartDiag('layer#$hashCode reconcile skipped: behaviour not initialized'); + return; + } + final interactiveLayer = interactiveLayerBehaviour.interactiveLayer; final drawingContext = interactiveLayer.drawingContext; // Ensure drawing context is available before creating drawings if (drawingContext.fullSize == Size.zero) { + chartDiag('layer#$hashCode reconcile skipped: drawing context has zero ' + 'size (bound layer mounted: ${interactiveLayer.isStillMounted}), ' + 'retrying post-frame'); // Drawing context not ready yet, schedule retry after next frame WidgetsBinding.instance.addPostFrameCallback((_) { if (mounted) { @@ -166,38 +253,147 @@ class _InteractiveLayerState extends State { final configListIds = widget.drawingToolsRepo.items.map((c) => c.configId).toSet(); + // The behaviour and its state machine outlive this State (the behaviour + // is owned by the chart's client and survives layer remounts). Gestures + // mutate the selected InteractableDrawing instance held by the state + // machine, so when (re)building the map, reuse that instance instead of + // creating a duplicate for the same configId; otherwise drags would + // mutate an instance that is no longer the one being painted. + final InteractiveState currentState = + widget.interactiveLayerBehaviour.currentState; + final InteractableDrawing? selectedDrawing = + currentState is InteractiveSelectedToolState + ? currentState.selected + : null; + + final List addedIds = []; + for (final config in widget.drawingToolsRepo.items) { if (!_interactableDrawings.containsKey(config.configId)) { // Add new drawing if it doesn't exist - final drawing = config.getInteractableDrawing( - drawingContext, - interactiveLayerBehaviour.getToolState, - ); + final drawing = + selectedDrawing != null && selectedDrawing.id == config.configId + ? selectedDrawing + : config.getInteractableDrawing( + drawingContext, + interactiveLayerBehaviour.getToolState, + ); _interactableDrawings[config.configId!] = drawing; + addedIds.add(config.configId!); } } - bool anyToolRemoved = false; + bool selectedInstanceReplaced = false; + + // The map and the state machine can hold DIFFERENT instances for the same + // configId (e.g. during the add flow the repo listener creates a map + // instance before the selection is established with the preview's + // instance). Gestures mutate the selected instance, so it must also be + // the painted/hit-tested one. + if (selectedDrawing != null && + _interactableDrawings.containsKey(selectedDrawing.id) && + !identical( + _interactableDrawings[selectedDrawing.id], selectedDrawing)) { + chartDiag('layer#$hashCode reconcile: replacing map instance for ' + '${selectedDrawing.id} with the selected instance'); + _interactableDrawings[selectedDrawing.id] = selectedDrawing; + selectedInstanceReplaced = true; + } + + if (addedIds.isNotEmpty) { + chartDiag('layer#$hashCode reconcile: added $addedIds, ' + 'map: ${_interactableDrawings.keys.toList()}'); + } + + final List removedIds = []; // Remove drawings that are not in the config list _interactableDrawings.removeWhere((id, _) { if (!configListIds.contains(id)) { - anyToolRemoved = true; + removedIds.add(id); return true; } return false; }); - if (anyToolRemoved) { + if (removedIds.isNotEmpty) { + chartDiag('layer#$hashCode reconcile: removed $removedIds, ' + 'repo: ${configListIds.toList()}, ' + 'map: ${_interactableDrawings.keys.toList()}'); + } + + // If the state machine still references a drawing whose config no longer + // exists in the repo (deleted tool, or a selection that survived a keyed + // remount into another symbol), reset to normal state. Otherwise the + // stale selection keeps showing its floating menu (via the state's + // preview widgets) for a drawing that can no longer be interacted with. + final bool selectedToolIsGone = + selectedDrawing != null && !configListIds.contains(selectedDrawing.id); + + if (selectedToolIsGone) { + chartDiag('reconcile: selected tool ${selectedDrawing.id} is gone, ' + 'scheduling state reset'); + _scheduleStateReset(); + } + + if (addedIds.isNotEmpty || + removedIds.isNotEmpty || + selectedInstanceReplaced) { + _notifyDrawingsChanged(); + } + } + + /// Signals the drawings paint layer that [_interactableDrawings] changed. + /// + /// Deferred to post-frame when reconciliation runs during build (notifying + /// mid-build would trigger setState-during-build in the listening + /// AnimatedBuilder); fired immediately otherwise so deletes repaint within + /// the same frame. + void _notifyDrawingsChanged() { + if (WidgetsBinding.instance.schedulerPhase == + SchedulerPhase.persistentCallbacks) { + WidgetsBinding.instance.addPostFrameCallback((_) { + if (mounted) { + _drawingsChanged.notify(); + } + }); + } else { + _drawingsChanged.notify(); + } + } + + /// Resets the behaviour state machine to normal state after the current + /// frame. + /// + /// Deferred to post-frame because reconciliation can run during build, and + /// the state transition triggers `onUpdate` callbacks that must not run + /// mid-build. + /// + /// The reset is applied synchronously (no animation wait): it is a cleanup + /// for a state that references a deleted drawing, and waiting on an + /// animation would make the reset itself lose the race against the next + /// keyed remount. + void _scheduleStateReset() { + if (_stateResetScheduled) { + return; + } + _stateResetScheduled = true; + + WidgetsBinding.instance.addPostFrameCallback((_) { + _stateResetScheduled = false; + if (!mounted) { + return; + } + widget.interactiveLayerBehaviour.updateStateTo( InteractiveNormalState( interactiveLayerBehaviour: widget.interactiveLayerBehaviour, ), StateChangeAnimationDirection.forward, + waitForAnimation: false, + animate: false, ); - } - - setState(() {}); + }); } /// Updates the config in the repository with debouncing @@ -239,7 +435,10 @@ class _InteractiveLayerState extends State { @override void dispose() { + chartDiag('layer#$hashCode dispose, ' + 'map: ${_interactableDrawings.keys.toList()}'); widget.drawingToolsRepo.removeListener(syncDrawingsWithConfigs); + _drawingsChanged.dispose(); super.dispose(); } @@ -247,8 +446,19 @@ class _InteractiveLayerState extends State { @override Widget build(BuildContext context) { + // Reconcile on every build so the painted/hit-tested drawings always + // match the repository, regardless of listener timing (MainChart watches + // the repository through Provider, so every repo mutation rebuilds this + // widget). + _reconcileDrawings(); + return _InteractiveLayerGestureHandler( - drawings: _interactableDrawings.values.toList(), + // A live getter (not a snapshot list): paint and hit-testing must always + // reflect the reconciled map, even if a widget rebuild carrying a new + // snapshot fails to propagate (observed in release builds after + // repo.clear() from the bottom sheet). + getDrawings: () => _interactableDrawings.values.toList(growable: false), + drawingsChanged: _drawingsChanged, epochFromX: widget.epochFromCanvasX, quoteFromY: widget.quoteFromCanvasY, epochToX: widget.epochToCanvasX, @@ -275,7 +485,8 @@ class _InteractiveLayerState extends State { class _InteractiveLayerGestureHandler extends StatefulWidget { const _InteractiveLayerGestureHandler({ - required this.drawings, + required this.getDrawings, + required this.drawingsChanged, required this.epochFromX, required this.quoteFromY, required this.epochToX, @@ -298,7 +509,12 @@ class _InteractiveLayerGestureHandler extends StatefulWidget { this.onCrosshairDisappeared, }); - final List drawings; + /// Live access to the layer's reconciled drawings. + final ValueGetter> getDrawings; + + /// Fires whenever the reconciled drawings change, to trigger a repaint + /// independently of widget rebuild propagation. + final Listenable drawingsChanged; final InteractiveLayerBehaviour interactiveLayerBehaviour; @@ -392,9 +608,20 @@ class _InteractiveLayerGestureHandlerState duration: const Duration(milliseconds: 240), ); + chartDiag('handler#$hashCode init, binding to ' + 'behaviour#${widget.interactiveLayerBehaviour.hashCode} ' + '(state: ${widget.interactiveLayerBehaviour.currentState.runtimeType})'); + widget.interactiveLayerBehaviour.init( interactiveLayer: this, - onUpdate: () => setState(() {}), + onUpdate: () { + // The shared behaviour can outlive this State across keyed remounts; + // a transition finishing late must not call setState on a disposed + // State. + if (mounted) { + setState(() {}); + } + }, stateChangeController: _stateChangeController, ); // Initialize the drawing tool gesture recognizer once @@ -409,12 +636,21 @@ class _InteractiveLayerGestureHandlerState onCrosshairCancel: _cancelCrosshair, debugOwner: this, ); + + // The add-flow handoff must not depend on a widget rebuild reaching this + // element: react to reconciled-map changes directly. + widget.drawingsChanged.addListener(_checkIsAToolAdded); } @override void didUpdateWidget(covariant _InteractiveLayerGestureHandler oldWidget) { super.didUpdateWidget(oldWidget); + if (!identical(oldWidget.drawingsChanged, widget.drawingsChanged)) { + oldWidget.drawingsChanged.removeListener(_checkIsAToolAdded); + widget.drawingsChanged.addListener(_checkIsAToolAdded); + } + _checkAddingToolToLayer(oldWidget); } @@ -435,15 +671,22 @@ class _InteractiveLayerGestureHandlerState /// Checks if a tool has been added to the layer and updates the state to /// [InteractiveSelectedToolState] if it has. void _checkIsAToolAdded() { - for (final drawing in widget.drawings) { + if (_addedDrawing == null) { + return; + } + + for (final drawing in widget.getDrawings()) { if (drawing.id == _addedDrawing) { + // Consume the marker only on a successful match. A widget update can + // arrive BEFORE the rebuild that carries the newly added drawing + // (release-mode timing); clearing unconditionally would drop the + // selection handoff to the repo-backed instance. + _addedDrawing = null; WidgetsFlutterBinding.ensureInitialized().addPostFrameCallback( (_) => widget.interactiveLayerBehaviour.aNewToolsIsAdded(drawing)); break; } } - - _addedDrawing = null; } @override @@ -451,6 +694,12 @@ class _InteractiveLayerGestureHandlerState StateChangeAnimationDirection direction, { bool animate = true, }) async { + if (!mounted) { + // This layer was disposed (e.g. a keyed remount on symbol/marker + // change). There is nothing to animate; return so that awaiting state + // transitions complete immediately instead of hanging forever. + return; + } await _runAnimation(direction, animate); } @@ -458,19 +707,30 @@ class _InteractiveLayerGestureHandlerState StateChangeAnimationDirection direction, bool animate, ) async { - if (direction == StateChangeAnimationDirection.forward) { - _stateChangeController.reset(); - if (animate) { - await _stateChangeController.forward(); - } else { - _stateChangeController.value = 1.0; - } - } else { - if (animate) { - await _stateChangeController.reverse(from: 1); + try { + if (direction == StateChangeAnimationDirection.forward) { + _stateChangeController.reset(); + if (animate) { + // `.orCancel` is essential: a plain TickerFuture NEVER completes if + // the animation is interrupted (controller reset by a concurrent + // transition, or disposed by a keyed remount). Without it, the + // `await` in InteractiveLayerBehaviour.updateStateTo hangs forever + // and the state assignment after it is silently lost, leaving the + // shared state machine stuck (release-mode ghost tools). + await _stateChangeController.forward().orCancel; + } else { + _stateChangeController.value = 1.0; + } } else { - _stateChangeController.value = 0.0; + if (animate) { + await _stateChangeController.reverse(from: 1).orCancel; + } else { + _stateChangeController.value = 0.0; + } } + } on TickerCanceled { + // Animation interrupted: completing normally is the desired behaviour, + // the awaiting state transition decides whether it still applies. } } @@ -545,24 +805,50 @@ class _InteractiveLayerGestureHandlerState }); } + /// Last painted drawing ids, used only for diagnostics. + List _lastPaintedIds = const []; + Widget _buildDrawingsLayer(BuildContext context, XAxisModel xAxis) => RepaintBoundary( child: MultipleAnimatedBuilder( - animations: [ + animations: [ _stateChangeController, _interactionNotifier, - widget.interactiveLayerBehaviour.controller + widget.interactiveLayerBehaviour.controller, + widget.drawingsChanged, + // Chart scroll (pan / zoom / autoscroll after a symbol switch) + // changes the epochToX / quoteToY closures captured by the + // CustomPainter. Without this listener the painter retained + // stale closures, causing drawings to drift off their anchored + // candle / price after an autoscroll — a release-only defect + // whose sibling (clear-all flow) was already patched via + // drawingsChanged. + xAxis, ], builder: (_, __) { final double animationValue = _stateChangeCurve.transform(_stateChangeController.value); + final List currentDrawings = + widget.getDrawings(); + + if (kChartDiagnosticsEnabled) { + final List ids = currentDrawings + .map((InteractableDrawing d) => d.id) + .toList(); + if (!listEquals(ids, _lastPaintedIds)) { + chartDiag( + 'handler#$hashCode painting: $_lastPaintedIds -> $ids'); + _lastPaintedIds = ids; + } + } + return Stack( fit: StackFit.expand, children: widget.series.input.isEmpty ? [] : [ - ...widget.drawings + ...currentDrawings .where( (e) => widget.interactiveLayerBehaviour @@ -576,7 +862,7 @@ class _InteractiveLayerGestureHandlerState animationValue, )) .toList(), - ...widget.drawings + ...currentDrawings .where( (e) => widget.interactiveLayerBehaviour @@ -646,7 +932,8 @@ class _InteractiveLayerGestureHandlerState ); @override - List> get drawings => widget.drawings; + List> get drawings => + widget.getDrawings(); @override EpochFromX get epochFromX => widget.epochFromX; @@ -680,6 +967,9 @@ class _InteractiveLayerGestureHandlerState @override void dispose() { + chartDiag('handler#$hashCode dispose (behaviour still bound to this: ' + '${identical(widget.interactiveLayerBehaviour.interactiveLayer, this)})'); + widget.drawingsChanged.removeListener(_checkIsAToolAdded); _interactionNotifier.dispose(); _stateChangeController.dispose(); _drawingToolGestureRecognizer.dispose(); diff --git a/lib/src/deriv_chart/interactive_layer/interactive_layer_behaviours/interactive_layer_behaviour.dart b/lib/src/deriv_chart/interactive_layer/interactive_layer_behaviours/interactive_layer_behaviour.dart index 2a8162756..e02b6d2e7 100644 --- a/lib/src/deriv_chart/interactive_layer/interactive_layer_behaviours/interactive_layer_behaviour.dart +++ b/lib/src/deriv_chart/interactive_layer/interactive_layer_behaviours/interactive_layer_behaviour.dart @@ -2,6 +2,7 @@ import 'dart:async'; import 'package:deriv_chart/src/add_ons/drawing_tools_ui/drawing_tool_config.dart'; import 'package:deriv_chart/src/deriv_chart/interactive_layer/helpers/types.dart'; +import 'package:deriv_chart/src/misc/chart_diagnostics.dart'; import 'package:deriv_chart/src/deriv_chart/interactive_layer/interactive_layer_behaviours/interactive_layer_desktop_behaviour.dart'; import 'package:deriv_chart/src/deriv_chart/interactive_layer/interactive_layer_behaviours/interactive_layer_mobile_behaviour.dart'; import 'package:flutter/gestures.dart'; @@ -65,6 +66,10 @@ abstract class InteractiveLayerBehaviour { bool _initialized = false; + /// Whether [init] has been called at least once, i.e. whether + /// [interactiveLayer], [onUpdate] and [stateChangeController] are bound. + bool get isInitialized => _initialized; + /// The interactive layer that this manager is managing. /// /// Note: This is not final to allow reassignment when the widget rebuilds. @@ -75,6 +80,11 @@ abstract class InteractiveLayerBehaviour { /// Note: This is not final to allow reassignment when the widget rebuilds. late VoidCallback onUpdate; + /// Monotonically increasing id of state transitions. Used to detect when a + /// transition that was suspended on its animation has been superseded by a + /// newer one (see [updateStateTo]). + int _stateTransitionSeq = 0; + /// Initializes the [InteractiveLayerBehaviour]. /// /// This method can be called multiple times (e.g., when the widget rebuilds @@ -123,17 +133,43 @@ abstract class InteractiveLayerBehaviour { bool waitForAnimation = true, bool animate = true, }) async { + final int transitionSeq = ++_stateTransitionSeq; + + chartDiag('updateStateTo #$transitionSeq ' + '${_describeState(currentState)} -> ${_describeState(newState)} ' + '(wait: $waitForAnimation, animate: $animate)'); + if (waitForAnimation) { await interactiveLayer.animateStateChange(direction, animate: animate); + + // While this transition was suspended on its animation, a newer + // transition may have started (the await can also resume late after the + // animation was cancelled by a keyed remount disposing the layer). + // The newer transition owns the state machine now; applying this one + // would overwrite it with stale state. + if (transitionSeq != _stateTransitionSeq) { + chartDiag('updateStateTo #$transitionSeq superseded by ' + '#$_stateTransitionSeq, dropping ${_describeState(newState)}'); + return; + } } else { unawaited( interactiveLayer.animateStateChange(direction, animate: animate)); } _controller.currentState = newState; + chartDiag('updateStateTo #$transitionSeq applied ' + '${_describeState(newState)}'); onUpdate(); } + /// Describes a state for diagnostics, including the selected drawing id + /// when applicable. + static String _describeState(InteractiveState state) => + state is InteractiveSelectedToolState + ? '${state.runtimeType}(${state.selected.id})' + : '${state.runtimeType}'; + /// Handles the addition of a drawing tool. /// Will be called when we want to add [drawingTool] to the layer. void startAddingTool(DrawingToolConfig drawingTool) { diff --git a/lib/src/deriv_chart/interactive_layer/interactive_layer_states/interactive_adding_tool_state.dart b/lib/src/deriv_chart/interactive_layer/interactive_layer_states/interactive_adding_tool_state.dart index 3113971a3..7127df585 100644 --- a/lib/src/deriv_chart/interactive_layer/interactive_layer_states/interactive_adding_tool_state.dart +++ b/lib/src/deriv_chart/interactive_layer/interactive_layer_states/interactive_adding_tool_state.dart @@ -71,21 +71,31 @@ class InteractiveAddingToolState extends InteractiveState ); if (addingStateInfo.isFinished) { - interactiveLayer - ..clearAddingDrawing() - ..addDrawing(_drawingPreview!.interactableDrawing.getUpdatedConfig()); + interactiveLayer.clearAddingDrawing(); - // Update the state to selected tool state with the newly added drawing. + final DrawingToolConfig addedConfig = interactiveLayer + .addDrawing(_drawingPreview!.interactableDrawing.getUpdatedConfig()); + + // Bind the persisted config (which now carries the generated configId) + // to the drawing instance BEFORE selecting it. // - // Once we have saved the drawing config in [AddOnsRepository] we should - // update to selected state with the interactable drawing that comes from - // that configs and not the preview one. + // Without this the selected instance keeps a configId-less config: + // - dragging it is never persisted (config updates with a null id are + // dropped by the layer), + // - the floating-menu delete is a silent no-op (the repository cannot + // find a config with a null id), + // - the painted (repo-backed) instance is a separate object, so the + // selection acts on an invisible "ghost" copy of the tool. + _drawingPreview!.interactableDrawing.config = addedConfig; + + // Update the state to selected tool state with the newly added drawing. interactiveLayerBehaviour.updateStateTo( InteractiveSelectedToolState( selected: _drawingPreview!.interactableDrawing, interactiveLayerBehaviour: interactiveLayerBehaviour, ), StateChangeAnimationDirection.forward, + waitForAnimation: false, ); _drawingPreview = null; diff --git a/lib/src/misc/chart_diagnostics.dart b/lib/src/misc/chart_diagnostics.dart new file mode 100644 index 000000000..0de32d0c9 --- /dev/null +++ b/lib/src/misc/chart_diagnostics.dart @@ -0,0 +1,15 @@ +import 'package:flutter/foundation.dart'; + +/// Diagnostics for the interactive layer / drawing tools sync. +/// +/// Enabled only in debug builds. To trace a release-mode issue with +/// `flutter run --release` (where `debugPrint` output is still visible), +/// temporarily set this to `true`. +const bool kChartDiagnosticsEnabled = kDebugMode; + +/// Logs [message] with a `[chart-diag]` prefix when diagnostics are enabled. +void chartDiag(String message) { + if (kChartDiagnosticsEnabled) { + debugPrint('[chart-diag] $message'); + } +} diff --git a/test/deriv_chart/interactive_layer/deriv_chart_drawing_tools_integration_test.dart b/test/deriv_chart/interactive_layer/deriv_chart_drawing_tools_integration_test.dart new file mode 100644 index 000000000..e64861fef --- /dev/null +++ b/test/deriv_chart/interactive_layer/deriv_chart_drawing_tools_integration_test.dart @@ -0,0 +1,187 @@ +import 'dart:convert'; + +import 'package:deriv_chart/deriv_chart.dart'; +import 'package:deriv_chart/src/deriv_chart/chart/data_visualization/drawing_tools/data_model/edge_point.dart'; +import 'package:deriv_chart/src/deriv_chart/interactive_layer/interactive_layer_behaviours/interactive_layer_behaviour.dart'; +import 'package:deriv_chart/src/deriv_chart/interactive_layer/interactive_layer_behaviours/interactive_layer_mobile_behaviour.dart'; +import 'package:deriv_chart/src/deriv_chart/interactive_layer/interactive_layer_controller.dart'; +import 'package:flutter/material.dart'; +import 'package:flutter_test/flutter_test.dart'; +import 'package:shared_preferences/shared_preferences.dart'; + +/// Integration-style tests that mirror how deriv_trader's TradeChart wires +/// drawing tools into the full [DerivChart] widget: a single external +/// [AddOnsRepository] instance reused across symbol switches (reloaded from +/// SharedPreferences), a persistent [InteractiveLayerMobileBehaviour], and +/// `useDrawingToolsV2: true`. +void main() { + final List ticks = [ + Tick(epoch: 1000, quote: 10), + Tick(epoch: 2000, quote: 20), + Tick(epoch: 3000, quote: 30), + ]; + + String prefsKey(String symbol) => 'addOns_DrawingToolConfig_$symbol'; + + String encodedHorizontal(String id) => jsonEncode( + HorizontalDrawingToolConfig( + configId: id, + edgePoints: const [EdgePoint(epoch: 2000, quote: 20)], + ).toJson(), + ); + + Widget app({ + required String symbol, + required AddOnsRepository repo, + required DrawingTools drawingTools, + required InteractiveLayerBehaviour behaviour, + }) => + MaterialApp( + home: Scaffold( + body: DerivChart( + // Mirrors TradeChart: full remount of the chart subtree per + // symbol (same pattern as the marker rebuild fix). + key: ValueKey('trade_chart_$symbol'), + mainSeries: LineSeries(ticks), + granularity: 1000, + activeSymbol: symbol, + drawingTools: drawingTools, + drawingToolsRepo: repo, + interactiveLayerBehaviour: behaviour, + useDrawingToolsV2: true, + ), + ), + ); + + testWidgets( + 'drawings reload across symbol switches and deletes repaint ' + '(full DerivChart wiring)', (WidgetTester tester) async { + SharedPreferences.setMockInitialValues({ + prefsKey('R_100'): [encodedHorizontal('r100_drawing')], + prefsKey('R_50'): [encodedHorizontal('r50_drawing')], + }); + + final AddOnsRepository repo = + AddOnsRepository( + createAddOn: (Map map) => + DrawingToolConfig.fromJson(map), + sharedPrefKey: 'R_100', + ); + final DrawingTools drawingTools = DrawingTools(); + final InteractiveLayerBehaviour behaviour = InteractiveLayerMobileBehaviour( + controller: InteractiveLayerController(), + ); + + // Mount for symbol R_100; the async prefs load completes after mount, + // exactly like TradeChart's initState flow. + await tester.pumpWidget(app( + symbol: 'R_100', + repo: repo, + drawingTools: drawingTools, + behaviour: behaviour, + )); + final SharedPreferences prefs = await SharedPreferences.getInstance(); + repo.loadFromPrefs(prefs, 'R_100'); + await tester.pump(); + await tester.pump(); + + expect( + find.byKey(const ValueKey('r100_drawing')), + findsOneWidget, + reason: 'R_100 drawings should load on first mount', + ); + + // Switch symbol -> R_50: DerivChart rebuilds with new activeSymbol, the + // consumer reloads the same repo instance from prefs. + await tester.pumpWidget(app( + symbol: 'R_50', + repo: repo, + drawingTools: drawingTools, + behaviour: behaviour, + )); + repo.loadFromPrefs(prefs, 'R_50'); + await tester.pump(); + await tester.pump(); + + expect( + find.byKey(const ValueKey('r50_drawing')), + findsOneWidget, + reason: 'R_50 drawings should appear after switching symbol', + ); + expect( + find.byKey(const ValueKey('r100_drawing')), + findsNothing, + reason: 'R_100 drawings must not leak onto R_50', + ); + + // Switch back -> R_100: drawings must reload. + await tester.pumpWidget(app( + symbol: 'R_100', + repo: repo, + drawingTools: drawingTools, + behaviour: behaviour, + )); + repo.loadFromPrefs(prefs, 'R_100'); + await tester.pump(); + await tester.pump(); + + expect( + find.byKey(const ValueKey('r100_drawing')), + findsOneWidget, + reason: 'R_100 drawings should reload when switching back', + ); + + // Delete from the repo (what the bottom sheet does) and verify the chart + // stops painting it. + repo.removeAt(0); + await tester.pump(); + await tester.pump(); + + expect( + find.byKey(const ValueKey('r100_drawing')), + findsNothing, + reason: 'deleted drawing must disappear from the chart', + ); + + // Let pending state-change animations run before teardown (the chart has + // continuous animations, so pumpAndSettle would never settle). + await tester.pump(const Duration(milliseconds: 300)); + }); + + testWidgets( + 'drawings loaded before the chart subtree mounts still appear ' + '(release-mode race)', (WidgetTester tester) async { + SharedPreferences.setMockInitialValues({ + prefsKey('R_100'): [encodedHorizontal('early_drawing')], + }); + + final AddOnsRepository repo = + AddOnsRepository( + createAddOn: (Map map) => + DrawingToolConfig.fromJson(map), + sharedPrefKey: 'R_100', + ); + + // Load completes BEFORE the chart is mounted: its notification has no + // chart listener yet. + final SharedPreferences prefs = await SharedPreferences.getInstance(); + repo.loadFromPrefs(prefs, 'R_100'); + + await tester.pumpWidget(app( + symbol: 'R_100', + repo: repo, + drawingTools: DrawingTools(), + behaviour: InteractiveLayerMobileBehaviour( + controller: InteractiveLayerController(), + ), + )); + await tester.pump(); + await tester.pump(); + + expect( + find.byKey(const ValueKey('early_drawing')), + findsOneWidget, + reason: 'drawings loaded before mount must appear via initial sync', + ); + }); +} diff --git a/test/deriv_chart/interactive_layer/interactive_layer_sync_test.dart b/test/deriv_chart/interactive_layer/interactive_layer_sync_test.dart new file mode 100644 index 000000000..8dbdac2cc --- /dev/null +++ b/test/deriv_chart/interactive_layer/interactive_layer_sync_test.dart @@ -0,0 +1,394 @@ +import 'package:deriv_chart/deriv_chart.dart'; +import 'package:deriv_chart/src/deriv_chart/chart/data_visualization/drawing_tools/data_model/edge_point.dart'; +import 'package:deriv_chart/src/deriv_chart/chart/x_axis/x_axis_model.dart'; +import 'package:deriv_chart/src/deriv_chart/chart/y_axis/y_axis_config.dart'; +import 'package:deriv_chart/src/deriv_chart/interactive_layer/crosshair/crosshair_controller.dart'; +import 'package:deriv_chart/src/deriv_chart/interactive_layer/crosshair/crosshair_variant.dart'; +import 'package:deriv_chart/src/deriv_chart/interactive_layer/interactable_drawings/interactable_drawing.dart'; +import 'package:deriv_chart/src/deriv_chart/interactive_layer/interactive_layer.dart'; +import 'package:deriv_chart/src/deriv_chart/interactive_layer/interactive_layer_behaviours/interactive_layer_desktop_behaviour.dart'; +import 'package:deriv_chart/src/deriv_chart/interactive_layer/enums/state_change_direction.dart'; +import 'package:deriv_chart/src/deriv_chart/interactive_layer/interactive_layer_states/interactive_normal_state.dart'; +import 'package:deriv_chart/src/deriv_chart/interactive_layer/interactive_layer_states/interactive_selected_tool_state.dart'; +import 'package:deriv_chart/src/models/axis_range.dart'; +import 'package:flutter/material.dart'; +import 'package:flutter_test/flutter_test.dart'; +import 'package:provider/provider.dart'; + +void main() { + final List ticks = [ + Tick(epoch: 1000, quote: 10), + Tick(epoch: 2000, quote: 20), + Tick(epoch: 3000, quote: 30), + ]; + + HorizontalDrawingToolConfig horizontalConfig(String id) => + HorizontalDrawingToolConfig( + configId: id, + edgePoints: const [EdgePoint(epoch: 2000, quote: 20)], + ); + + AddOnsRepository createRepo() => + AddOnsRepository( + createAddOn: (Map map) => + DrawingToolConfig.fromJson(map), + sharedPrefKey: 'test_symbol', + ); + + late AnimationController xAxisAnimationController; + + Widget buildLayer({ + required Repository repo, + required InteractiveLayerBehaviour behaviour, + }) { + YAxisConfig.instance.setLabelWidth(60); + + final DataSeries series = LineSeries(ticks); + final XAxisModel xAxisModel = XAxisModel( + entries: ticks, + granularity: 1000, + animationController: xAxisAnimationController, + isLive: false, + snapMarkersToIntervals: false, + maxCurrentTickOffset: 150, + ) + // Normally set by the XAxis widget during layout. + ..width = 800 + ..graphAreaWidth = 740; + + return MaterialApp( + home: MultiProvider( + providers: [ + ChangeNotifierProvider.value(value: xAxisModel), + Provider.value(value: ChartDefaultDarkTheme()), + ], + child: InteractiveLayer( + drawingTools: DrawingTools(), + series: series, + chartConfig: const ChartConfig(granularity: 1000), + quoteToCanvasY: (double quote) => quote, + quoteFromCanvasY: (double y) => y, + epochToCanvasX: (int epoch) => epoch.toDouble() / 100, + epochFromCanvasX: (double x) => (x * 100).toInt(), + drawingToolsRepo: repo, + quoteRange: QuoteRange(topQuote: 40, bottomQuote: 0), + interactiveLayerBehaviour: behaviour, + crosshairZoomOutAnimation: const AlwaysStoppedAnimation(0), + crosshairController: CrosshairController( + xAxisModel: xAxisModel, + series: series, + showCrosshair: false, + crosshairVariant: CrosshairVariant.smallScreen, + ), + crosshairVariant: CrosshairVariant.smallScreen, + showCrosshair: false, + ), + ), + ); + } + + setUp(() { + xAxisAnimationController = AnimationController( + vsync: const TestVSync(), + duration: const Duration(milliseconds: 100), + ); + }); + + tearDown(() { + xAxisAnimationController.dispose(); + }); + + group('InteractiveLayer repo sync', () { + testWidgets( + 'renders drawings already present in the repo before the layer mounts', + (WidgetTester tester) async { + final AddOnsRepository repo = createRepo() + // Populated BEFORE the layer is mounted: its notification fires when + // no listener exists yet (the release-mode symbol-switch scenario). + ..add(horizontalConfig('pre_mount_drawing')); + + await tester.pumpWidget(buildLayer( + repo: repo, + behaviour: InteractiveLayerDesktopBehaviour(), + )); + + // Allow the post-frame initial sync to run and rebuild. + await tester.pump(); + + expect( + find.byKey(const ValueKey('pre_mount_drawing')), + findsOneWidget, + ); + }); + + testWidgets('adds and removes drawings on repo mutations', + (WidgetTester tester) async { + final AddOnsRepository repo = createRepo(); + + await tester.pumpWidget(buildLayer( + repo: repo, + behaviour: InteractiveLayerDesktopBehaviour(), + )); + await tester.pump(); + + repo.add(horizontalConfig('drawing_1')); + await tester.pump(); + expect(find.byKey(const ValueKey('drawing_1')), findsOneWidget); + + repo.removeAt(0); + await tester.pump(); + expect(find.byKey(const ValueKey('drawing_1')), findsNothing); + }); + + testWidgets( + 're-subscribes and re-syncs when the repository instance changes', + (WidgetTester tester) async { + final AddOnsRepository repoA = createRepo() + ..add(horizontalConfig('repo_a_drawing')); + final AddOnsRepository repoB = createRepo() + ..add(horizontalConfig('repo_b_drawing')); + + final InteractiveLayerBehaviour behaviour = + InteractiveLayerDesktopBehaviour(); + + await tester.pumpWidget(buildLayer(repo: repoA, behaviour: behaviour)); + await tester.pump(); + expect( + find.byKey(const ValueKey('repo_a_drawing')), + findsOneWidget, + ); + + // Swap the repository instance without remounting the layer. + await tester.pumpWidget(buildLayer(repo: repoB, behaviour: behaviour)); + await tester.pump(); + + expect( + find.byKey(const ValueKey('repo_b_drawing')), + findsOneWidget, + ); + expect( + find.byKey(const ValueKey('repo_a_drawing')), + findsNothing, + ); + + // Mutations on the new repo instance must reach the layer. + repoB.removeAt(0); + await tester.pump(); + expect( + find.byKey(const ValueKey('repo_b_drawing')), + findsNothing, + ); + }); + + testWidgets( + 'reuses the selected drawing instance when rebuilding after a remount', + (WidgetTester tester) async { + final AddOnsRepository repo = createRepo() + ..add(horizontalConfig('selected_drawing')); + + final InteractiveLayerBehaviour behaviour = + InteractiveLayerDesktopBehaviour(); + + await tester.pumpWidget(buildLayer(repo: repo, behaviour: behaviour)); + await tester.pump(); + + // Simulate a user selection: the state machine holds the drawing + // instance that gestures mutate. + final InteractableDrawing selectedInstance = + behaviour.interactiveLayer.drawings.single; + behaviour.controller.currentState = InteractiveSelectedToolState( + selected: selectedInstance, + interactiveLayerBehaviour: behaviour, + ); + + // Force a full remount of the layer (what happens on symbol/marker + // changes in MainChart) while behaviour and repo survive. + await tester.pumpWidget(const SizedBox.shrink()); + await tester.pumpWidget(buildLayer(repo: repo, behaviour: behaviour)); + await tester.pump(); + + // Selection must survive and the rebuilt map must reference the very + // same instance the state machine mutates on drag. + expect( + behaviour.currentState, + isA(), + ); + expect( + identical( + behaviour.interactiveLayer.drawings.single, + selectedInstance, + ), + isTrue, + ); + + // Deleting the tool while it is selected must reset the state machine + // (the reset happens after the state-change animation completes). + repo.removeAt(0); + await tester.pumpAndSettle(); + expect(behaviour.currentState, isA()); + expect( + find.byKey(const ValueKey('selected_drawing')), + findsNothing, + ); + }); + + testWidgets( + 'state transition completes even when a remount cancels its animation', + (WidgetTester tester) async { + final AddOnsRepository repo = createRepo() + ..add(horizontalConfig('drawing_1')); + + final InteractiveLayerBehaviour behaviour = + InteractiveLayerDesktopBehaviour(); + + await tester.pumpWidget(buildLayer(repo: repo, behaviour: behaviour)); + await tester.pump(); + + final InteractableDrawing drawing = + behaviour.interactiveLayer.drawings.single; + + // Start a transition that waits for its 240ms animation. A plain + // TickerFuture would never complete once the layer (and its + // AnimationController) is disposed mid-animation, silently dropping the + // state assignment — the release-mode "stuck state machine" bug. + final Future transition = behaviour.updateStateTo( + InteractiveSelectedToolState( + selected: drawing, + interactiveLayerBehaviour: behaviour, + ), + StateChangeAnimationDirection.forward, + ); + + // Animation in flight. + await tester.pump(const Duration(milliseconds: 50)); + + // Keyed remount (symbol/marker change) disposes the gesture handler and + // its AnimationController while the transition is still awaiting. + await tester.pumpWidget(const SizedBox.shrink()); + await tester.pumpWidget(buildLayer(repo: repo, behaviour: behaviour)); + await tester.pump(); + + // Must complete (would hang/time out before the fix) and must have + // applied the state. + await transition; + expect(behaviour.currentState, isA()); + }); + + testWidgets( + 'map adopts the selected instance when it differs for the same id', + (WidgetTester tester) async { + final AddOnsRepository repo = createRepo(); + + final InteractiveLayerBehaviour behaviour = + InteractiveLayerDesktopBehaviour(); + + await tester.pumpWidget(buildLayer(repo: repo, behaviour: behaviour)); + await tester.pump(); + + // Add flow ordering: the repo listener creates a map instance for the + // new config BEFORE the adding flow selects its own (preview) instance + // for the same configId. + repo.add(horizontalConfig('added_drawing')); + await tester.pump(); + + final InteractableDrawing mapInstance = + behaviour.interactiveLayer.drawings.single; + + // A different instance built from the same config gets selected (what + // the adding flow does with the preview's drawing). + final InteractableDrawing selectedInstance = + horizontalConfig('added_drawing').getInteractableDrawing( + behaviour.interactiveLayer.drawingContext, + behaviour.getToolState, + ); + expect(identical(mapInstance, selectedInstance), isFalse); + + behaviour.controller.currentState = InteractiveSelectedToolState( + selected: selectedInstance, + interactiveLayerBehaviour: behaviour, + ); + + // Any rebuild must unify them: the painted/hit-tested instance has to + // be the one gestures mutate. + repo.update(); + await tester.pump(); + + expect( + identical( + behaviour.interactiveLayer.drawings.single, + selectedInstance, + ), + isTrue, + ); + }); + + testWidgets( + 'clear() empties paint/hit-test data even before a rebuild arrives', + (WidgetTester tester) async { + final AddOnsRepository repo = createRepo() + ..add(horizontalConfig('drawing_1')) + ..add(horizontalConfig('drawing_2')) + ..add(horizontalConfig('drawing_3')); + + final InteractiveLayerBehaviour behaviour = + InteractiveLayerDesktopBehaviour(); + + await tester.pumpWidget(buildLayer(repo: repo, behaviour: behaviour)); + await tester.pump(); + expect(behaviour.interactiveLayer.drawings.length, 3); + + // "Clear all" from the bottom sheet. In release builds the widget + // rebuild carrying the new (empty) snapshot was observed not to reach + // the gesture handler, leaving ghost lines painted and hit-testable. + // The handler must therefore read the reconciled drawings live: they + // have to be empty immediately after clear(), before any pump delivers + // a rebuilt widget. + repo.clear(); + expect(behaviour.interactiveLayer.drawings, isEmpty); + + await tester.pump(); + expect(find.byKey(const ValueKey('drawing_1')), findsNothing); + expect(find.byKey(const ValueKey('drawing_2')), findsNothing); + expect(find.byKey(const ValueKey('drawing_3')), findsNothing); + }); + + testWidgets('a superseded transition does not overwrite a newer state', + (WidgetTester tester) async { + final AddOnsRepository repo = createRepo() + ..add(horizontalConfig('drawing_1')); + + final InteractiveLayerBehaviour behaviour = + InteractiveLayerDesktopBehaviour(); + + await tester.pumpWidget(buildLayer(repo: repo, behaviour: behaviour)); + await tester.pump(); + + final InteractableDrawing drawing = + behaviour.interactiveLayer.drawings.single; + + // Slow transition: suspends on its animation. + final Future slowTransition = behaviour.updateStateTo( + InteractiveSelectedToolState( + selected: drawing, + interactiveLayerBehaviour: behaviour, + ), + StateChangeAnimationDirection.forward, + ); + + // Newer transition applies immediately, cancelling the slow one's + // animation. When the slow transition resumes it must detect it has + // been superseded and not roll the state back. + await behaviour.updateStateTo( + InteractiveNormalState(interactiveLayerBehaviour: behaviour), + StateChangeAnimationDirection.forward, + waitForAnimation: false, + animate: false, + ); + + await tester.pumpAndSettle(); + await slowTransition; + + expect(behaviour.currentState, isA()); + }); + }); +}