Sync chart cursor across route analysis charts - #5680
Conversation
| func tableView(_ tableView: UITableView, willDisplay cell: UITableViewCell, forRowAt indexPath: IndexPath) { | ||
| guard let cell = cell as? AnalyzeCardCell else { return } | ||
| if let chart = cell.cardView.subviews.first(where: { $0 is ElevationChart }) as? ElevationChart { | ||
| cell.layoutIfNeeded() |
| recognizer.state == .ended, | ||
| let chart = recognizer.view as? BarLineChartViewBase else { return } | ||
| refreshChartOnMap() | ||
| DispatchQueue.main.async { [weak self, weak chart] in |
There was a problem hiding this comment.
Do we really need [weak self, weak chart] here? There is no retain cycle with DispatchQueue.main.async
| if ([recognizer.view isKindOfClass:BarLineChartViewBase.class]) | ||
| { | ||
| __weak __typeof(self) weakSelf = self; | ||
| __weak BarLineChartViewBase *weakChart = (BarLineChartViewBase *)recognizer.view; |
There was a problem hiding this comment.
weakSelf and weakChart seem unnecessary here. The dispatched block is short-lived and doesn't create a retain cycle, so both self and chart can be captured strongly
| primaryXAxisType == .distance | ||
| } | ||
|
|
||
| @objc(setPrimaryChart:) func setPrimaryChart(_ chart: ElevationChart) { |
| barCharts.allObjects.forEach { clearHighlight(in: $0) } | ||
| } | ||
|
|
||
| @objc(reset) func reset() { |
|
Potential regression vs Android reference: in Android handles this during graph binding by forcing every bar chart axis min/max to the main chart range ( |
|
Leave a comment to test Route Details, as this functionality was affected by the changes |
|
Route Details is also affected because it uses the shared RouteChartSynchronizer |
|
Found one more regression risk after the range-alignment update. That can leave the route-attribute charts clipped or scaled against the old primary distance range after Distance -> Time/Time of day switching, and then normalized synchronization uses that stale bar axis range. Could we reset both left/right bar axes to their data range when |
|
Found one regression in the new synchronization path. When the user already has a selected cursor and then pans/zooms the chart,
That keeps the visual highlight and the other charts in sync, but it skips the existing Please either trigger the map refresh explicitly after viewport sync when a selection exists, or provide a safe callback/mode from the synchronizer so the primary chart update can still notify the owning controller. |
| if let primaryChart = chart as? ElevationChart { | ||
| chartSynchronizer.setPrimaryChart(primaryChart) | ||
| } else if let barChart = chart as? HorizontalBarChartView { | ||
| chartSynchronizer.registerBarChart(barChart) |
There was a problem hiding this comment.
Should we unregister the chart in didEndDisplaying? RouteChartSynchronizer keeps every registered bar chart in the weak hash table, so an off-screen/reused chart can still participate in viewport/highlight synchronization while it remains alive. It would be safer to explicitly unregister charts when their cells leave the screen
| applyVisibleRange(lowerValue...upperValue, to: chart) | ||
| } | ||
|
|
||
| private func applyVisibleRange(_ visibleRange: ClosedRange<Double>, to chart: BarLineChartViewBase) { |
There was a problem hiding this comment.
applyVisibleRange() rebuilds the touch matrix from fitScreen(), which also resets the target chart's Y scale/translation. Since this method can be applied to the primary ElevationChart as well, syncing only the horizontal viewport may unexpectedly reset its vertical viewport. Can we preserve the existing Y transform and update only scaleX/translationX?
| refreshChartOnMap() | ||
| DispatchQueue.main.async { | ||
| chart.layoutIfNeeded() | ||
| self.chartSynchronizer.syncViewPort(from: chart) |
There was a problem hiding this comment.
syncViewPort is already called from chartScaled / chartTranslated. Do we really need to call it again here after the gesture ends? This causes the same viewport/selection synchronization to run twice for pinch gestures and may also trigger an extra highlightValue(..., callDelegate: true) cycle
| private weak var chartView: ElevationChart? | ||
| private weak var yAxisButton: UIButton? | ||
| private weak var xAxisButton: UIButton? | ||
| private var chartView: ElevationChart? |
There was a problem hiding this comment.
Is a strong reference really needed here? It keeps the chart alive after its cell goes off-screen, while the synchronizer itself uses weak chart references
|
|
||
| let handler = chart.viewPortHandler | ||
| var matrix = handler.touchMatrix | ||
| matrix.a = scale |
There was a problem hiding this comment.
Сould we avoid modifying touchMatrix.a directly here? This relies on the matrix containing only scale/translation and makes the synchronization dependent on DGCharts' internal transform representation. Would it be safer to use the chart/viewPortHandler APIs to update only the X scale and translation
| } | ||
|
|
||
| func clearSynchronizedHighlights() { | ||
| selectedProgress = nil |
There was a problem hiding this comment.
Should clearing the synchronized selection also propagate the state change? Currently callers have to hide the map highlight separately, while selection updates are propagated through onChartStateChanged. It would be cleaner to keep this state transition inside RouteChartSynchronizer
There was a problem hiding this comment.
Following up on the distance-range alignment: pinning the bar-chart axis to primaryXAxisRange in updateHorizontalAxis fixes the axis, but the bar data is still scaled independently. buildStatisticChart derives divX and the stack values from analysis.totalDistance (GpxUIHelper.swift:851), while the primary chart uses calcWithoutGaps ? totalDistanceWithoutGaps : totalDistance (GpxUIHelper.swift:632).
On a route with gaps the bar data spans 0...totalDistance/divX against an axis pinned to 0...totalDistanceWithoutGaps/divX, so every attribute boundary is offset by the gap length and the far end of the bar is clipped. Plan a route passes overrideIsGeneralTrack: true and its tracks can contain gaps, so this is reachable.
There is also a sharper edge: if the two totals straddle a setupAxisDistance unit threshold (e.g. 950 m vs 1010 m with KILOMETERS_AND_METERS) the two charts pick different divX (1 vs 1000), so the bar values end up in kilometres against an axis forced to a metre range - the bar collapses to a sliver and the synchronized cursor is meaningless.
Android has the same construction (ChartUtils.buildStatisticChart uses analysis.getTotalDistance() and bindGraphAdapters forces the main chart range), so this is parity rather than an iOS-only regression - but it may be worth normalizing the bar data to the primary chart's distance basis instead of inheriting it.
| barCharts.allObjects.forEach { | ||
| updateHorizontalAxis(of: $0) | ||
| $0.notifyDataSetChanged() | ||
| } | ||
| } | ||
| applyStoredVisibleRange(to: chart) |
There was a problem hiding this comment.
applyStoredVisibleRange is called for the primary chart only. In Route Details the bar-chart cells persist in _data and keep their touch matrices, so when updateRouteStatisticsGraph -> setPrimaryChart switches the X-axis mode, updateHorizontalAxis swaps each bar chart's axis range while its existing scaleX/translationX still encode the previous range. The bar viewports - and the cursor pixel that applySelection derives from them - drift from the elevation chart until the user pans again.
Could we call applyStoredVisibleRange(to: $0) inside this loop as well?
| } | ||
|
|
||
| func tableView(_ tableView: UITableView, didEndDisplaying cell: UITableViewCell, forRowAt indexPath: IndexPath) { | ||
| guard let chart = (cell as? AnalyzeCardCell)?.chartView else { return } |
There was a problem hiding this comment.
All three card types share AnalyzeCardCell.reuseIdentifier, prepareForReuse nils chartView, and cellForRowAt assigns the new one. Plain scrolling is safe, but under reloads or animated updates a cell can be reconfigured before its didEndDisplaying fires - then this unregisters the chart that is currently on screen. That chart stops syncing permanently, because it loses both its weak-table entry and its gesture targets, and willDisplay will not fire for it again.
Capturing the chart in willDisplay, or checking identity against the currently registered chart before unregistering, would remove the dependency on UIKit's ordering.
| let proxy = AnalyzeChartDelegateProxy() | ||
| proxy.onNothingSelected = { [weak self] in | ||
| self?.hideChartLocation() | ||
| proxy.onNothingSelected = { [weak self] _ in |
There was a problem hiding this comment.
onValueSelected below filters on chart === chartView, but this one clears the whole synchronized selection for any chart. The bar charts share this proxy and run with highlightPerDragEnabled = true, so a DGCharts drag on a bar chart that resolves to a nil highlight would wipe the cursor everywhere.
It looks unlikely to fire in practice given maxHighlightDistance = 10_000 and a single bar entry, but the same guard here would make it symmetric.
| analysis:self.analysis | ||
| segment:self.segment]; | ||
| } | ||
| if ([recognizer.view isKindOfClass:BarLineChartViewBase.class]) |
There was a problem hiding this comment.
DGCharts already emits chartScaled: for both of these gestures (pinchGestureRecognized and doubleTapGestureRecognized in BarLineChartViewBase), and that is wired to syncViewPortFromChart: at line 968. This dispatch runs the same synchronization a second time, which re-enters applySelectionToPrimaryChart(callDelegate: true) -> chartValueSelected: -> refreshChart:fitTrack:YES, i.e. a second map fit per gesture.
Plan a route dropped its equivalent gesture hook entirely - should Route Details do the same, or is there a layout case here that chartScaled: misses?
| positions: positions, | ||
| offset: offset) | ||
|
|
||
| guard axis.drawAxisLineEnabled else { return } |
There was a problem hiding this comment.
This guard means the tick marks never draw in Route Details: setupHorizontalGPXChart sets rightAxis.drawAxisLineEnabled = false (GpxUIHelper.swift:452), and only PlanRouteAnalyzeViewController turns it back on (line 874). The PR summary says tick marks are added to route attribute charts - is Plan a route only the intended scope?
Unrelated nit while here: @objc(syncViewPortFromChart:) at line 173 is still present from the earlier "remove objc name" comment. Dropping it changes the selector to syncViewPortFrom:, so the two .mm call sites would need updating.
There was a problem hiding this comment.
Yes, the distance-axis tick marks are intended for the Plan a route Analyze tab only, following the Figma design. Route Details keeps its existing axis styling.
|
Re-checked at Verified fixed
Residual on the cell-reuse fix
Points raised by
|
# Conflicts: # Sources/Controllers/PlanRoute/Tabs/PlanRouteAnalyzeViewController.swift # Sources/Controllers/TargetMenu/Routing/OARouteDetailsViewController.mm
|
Plan a route → Analyze is the intended scope for the distance-axis tick marks. They were added there to match the Figma design and make the cursor position easier to read. Route Details keeps its existing chart styling, so drawAxisLineEnabled remains false there and RouteStatisticsYAxisRenderer intentionally does not draw the tick marks. Cursor synchronization itself is supported on both screens. |
|
Re-checked at Closed
That also resolves the three points from my previous comment:
Also worth noting the new Four smaller things from the new commit
As before this is read-only - not built, not run - so points 1 and 3 in particular would benefit from a pass on a real track with gaps. |
Summary