-
Notifications
You must be signed in to change notification settings - Fork 118
Sync chart cursor across route analysis charts #5680
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from 12 commits
54503b9
dfea27a
b3ec09d
896d765
95340fc
0b1bbae
0923f9c
979be64
2357bfc
eb7572a
57f70dc
2abd298
54b7d61
be13ac0
a7995a3
02add7c
82079a0
789dfdf
e2ae519
a8a2bd5
0485f92
b71d286
ce941b3
14699f6
3f4804c
89cd1eb
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -39,20 +39,25 @@ private struct AnalyzeStatItem { | |
| } | ||
|
|
||
| private final class AnalyzeChartDelegateProxy: NSObject, ChartViewDelegate { | ||
| var onNothingSelected: (() -> Void)? | ||
| var onValueSelected: (() -> Void)? | ||
| var onTranslated: (() -> Void)? | ||
| var onNothingSelected: ((ChartViewBase) -> Void)? | ||
| var onValueSelected: ((ChartViewBase, Highlight) -> Void)? | ||
| var onTranslated: ((ChartViewBase) -> Void)? | ||
| var onScaled: ((ChartViewBase) -> Void)? | ||
|
|
||
| func chartValueNothingSelected(_ chartView: ChartViewBase) { | ||
| onNothingSelected?() | ||
| onNothingSelected?(chartView) | ||
| } | ||
|
|
||
| func chartValueSelected(_ chartView: ChartViewBase, entry: ChartDataEntry, highlight: Highlight) { | ||
| onValueSelected?() | ||
| onValueSelected?(chartView, highlight) | ||
| } | ||
|
|
||
| func chartTranslated(_ chartView: ChartViewBase, dX: CGFloat, dY: CGFloat) { | ||
| onTranslated?() | ||
| onTranslated?(chartView) | ||
| } | ||
|
|
||
| func chartScaled(_ chartView: ChartViewBase, scaleX: CGFloat, scaleY: CGFloat) { | ||
| onScaled?(chartView) | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -63,6 +68,7 @@ final class PlanRouteAnalyzeViewController: UIViewController, PlanRouteTabConten | |
| var onAttachToRoadsRequested: (() -> Void)? | ||
|
|
||
| private let tableView = CancelableTableView(frame: .zero, style: .plain) | ||
| private let chartSynchronizer = RouteChartSynchronizer() | ||
|
|
||
| private var selectedYAxisTypes: [NSNumber] = [ | ||
| NSNumber(value: GPXDataSetType.altitude.rawValue), | ||
|
|
@@ -82,27 +88,32 @@ final class PlanRouteAnalyzeViewController: UIViewController, PlanRouteTabConten | |
| private var cachedSyntheticSteepnessSignature: Double = -1 | ||
| private var pendingSteepnessSignature: Double = -1 | ||
| private var lastRenderState: AnalyzeRenderState? | ||
| private var currentChartDataSignature: String? | ||
| private var trackChartFilePath: String? | ||
| private var trackChartHelper: TrackChartHelper? | ||
| private var highlightDrawX: CGFloat = -1 | ||
| private var lastTranslation: CGPoint = .zero | ||
| private weak var dataSource: PlanRouteAnalyzeDataSource? | ||
| private weak var chartView: ElevationChart? | ||
| private weak var yAxisButton: UIButton? | ||
| private weak var xAxisButton: UIButton? | ||
| private var chartView: ElevationChart? | ||
| private lazy var chartDelegateProxy: AnalyzeChartDelegateProxy = { | ||
| let proxy = AnalyzeChartDelegateProxy() | ||
| proxy.onNothingSelected = { [weak self] in | ||
| proxy.onNothingSelected = { [weak self] _ in | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
It looks unlikely to fire in practice given |
||
| self?.chartSynchronizer.clearSynchronizedHighlights() | ||
| self?.hideChartLocation() | ||
| } | ||
| proxy.onValueSelected = { [weak self] in | ||
| self?.refreshChartOnMap() | ||
| proxy.onValueSelected = { [weak self] chart, highlight in | ||
| guard let self, let chartView, chart === chartView else { return } | ||
| chartSynchronizer.syncHighlight(highlight, sourceChart: chartView) | ||
| refreshChartOnMap() | ||
| } | ||
| proxy.onTranslated = { [weak self] chart in | ||
| self?.handleChartViewPortChanged(chart) | ||
| } | ||
| proxy.onTranslated = { [weak self] in | ||
| self?.handleChartTranslated() | ||
| proxy.onScaled = { [weak self] chart in | ||
| self?.handleChartViewPortChanged(chart) | ||
| } | ||
| return proxy | ||
| }() | ||
| private weak var dataSource: PlanRouteAnalyzeDataSource? | ||
| private weak var yAxisButton: UIButton? | ||
| private weak var xAxisButton: UIButton? | ||
|
|
||
| private var currentState: AnalyzeState { | ||
| cachedState | ||
|
|
@@ -165,6 +176,13 @@ final class PlanRouteAnalyzeViewController: UIViewController, PlanRouteTabConten | |
| } | ||
| wasCalculatingElevation = isElevationCalculating | ||
| let analysisData = dataSource?.analysisData | ||
| let nextChartDataSignature = chartDataSignature(for: analysisData) | ||
| if currentChartDataSignature != nextChartDataSignature { | ||
| chartSynchronizer.reset() | ||
| chartView = nil | ||
| currentChartDataSignature = nextChartDataSignature | ||
| hideChartLocation() | ||
| } | ||
| cachedHasElevationData = analysisData?.hasElevationData == true | ||
| cachedHasSpeedData = analysisData?.hasSpeedData == true | ||
| cachedHasOverviewData = cachedHasElevationData || cachedHasSpeedData | ||
|
|
@@ -177,6 +195,8 @@ final class PlanRouteAnalyzeViewController: UIViewController, PlanRouteTabConten | |
| roadAttributeStatistics: cachedRoadAttributeStatistics) | ||
| let renderState = makeRenderState(analysisData: analysisData) | ||
| if renderState.graphSignature == nil { | ||
| chartSynchronizer.reset() | ||
| chartView = nil | ||
| hideChartLocation() | ||
| } | ||
| applyRenderState(renderState) | ||
|
|
@@ -249,6 +269,7 @@ final class PlanRouteAnalyzeViewController: UIViewController, PlanRouteTabConten | |
| secondType: secondType, | ||
| axisType: selectedXAxisType, | ||
| calcWithoutGaps: GpxUtils.calcWithoutGaps(gpxFile, gpxDataItem: gpxItem, overrideIsGeneralTrack: true)) | ||
| chartSynchronizer.setPrimaryChart(chart) | ||
| if !chart.highlighted.isEmpty { | ||
| refreshChartOnMap() | ||
| } | ||
|
|
@@ -275,12 +296,9 @@ final class PlanRouteAnalyzeViewController: UIViewController, PlanRouteTabConten | |
| segment: segment) | ||
| } | ||
|
|
||
| private func bindChartGestures(_ chart: ElevationChart) { | ||
| private func bindChartGestures(_ chart: BarLineChartViewBase) { | ||
| chart.delegate = chartDelegateProxy | ||
| chart.gestureRecognizers?.forEach { recognizer in | ||
| if recognizer is UIPanGestureRecognizer { | ||
| recognizer.addTarget(self, action: #selector(onChartScrolled(_:))) | ||
| } | ||
| recognizer.addTarget(self, action: #selector(onChartGesture(_:))) | ||
| } | ||
| } | ||
|
|
@@ -315,57 +333,9 @@ final class PlanRouteAnalyzeViewController: UIViewController, PlanRouteTabConten | |
| dataSource?.hideChartHighlight() | ||
| } | ||
|
|
||
| @objc private func onChartScrolled(_ recognizer: UIPanGestureRecognizer) { | ||
| guard let chart = recognizer.view as? ElevationChart else { return } | ||
|
|
||
| if recognizer.state == .changed { | ||
| if chart.lowestVisibleX > 0.1, | ||
| roundedChartValue(chart.highestVisibleX) != roundedChartValue(chart.chartXMax) { | ||
| lastTranslation = recognizer.translation(in: chart) | ||
| return | ||
| } | ||
|
|
||
| let touchPoint = recognizer.location(in: chart) | ||
| let translation = recognizer.translation(in: chart) | ||
| let highlightX = chart.isFullyZoomedOut | ||
| ? touchPoint.x | ||
| : highlightDrawX + (lastTranslation.x - translation.x) | ||
| guard let highlight = chart.getHighlightByTouchPoint(CGPoint(x: highlightX, y: 0)) else { return } | ||
| chart.lastHighlighted = highlight | ||
| chart.highlightValue(highlight, callDelegate: true) | ||
| } else if recognizer.state == .ended { | ||
| lastTranslation = .zero | ||
| if let highlight = chart.highlighted.first { | ||
| highlightDrawX = highlight.drawX | ||
| } | ||
| } | ||
| } | ||
|
|
||
| @objc private func onChartGesture(_ recognizer: UIGestureRecognizer) { | ||
| guard let chart = recognizer.view as? ElevationChart else { return } | ||
|
|
||
| if recognizer.state == .began { | ||
| if let highlight = chart.highlighted.first { | ||
| highlightDrawX = highlight.drawX | ||
| } else { | ||
| highlightDrawX = -1 | ||
| } | ||
| } else if (recognizer is UIPinchGestureRecognizer | ||
| || (recognizer is UITapGestureRecognizer | ||
| && (recognizer as? UITapGestureRecognizer)?.numberOfTapsRequired == 2)) | ||
| && recognizer.state == .ended { | ||
| refreshChartOnMap() | ||
| } | ||
| } | ||
|
|
||
| private func roundedChartValue(_ value: Double) -> Double { | ||
| (value * 10).rounded() / 10 | ||
| } | ||
|
|
||
| private func handleChartTranslated() { | ||
| guard let chart = self.chartView, highlightDrawX != -1 else { return } | ||
| guard let highlight = chart.getHighlightByTouchPoint(CGPoint(x: highlightDrawX, y: 0)) else { return } | ||
| chart.highlightValue(highlight, callDelegate: true) | ||
| private func handleChartViewPortChanged(_ chart: ChartViewBase) { | ||
| guard let chart = chart as? BarLineChartViewBase else { return } | ||
| chartSynchronizer.syncViewPort(from: chart) | ||
| } | ||
|
|
||
| private func resolvedYAxisTypes() -> (GPXDataSetType, GPXDataSetType) { | ||
|
|
@@ -448,6 +418,17 @@ final class PlanRouteAnalyzeViewController: UIViewController, PlanRouteTabConten | |
| ].joined(separator: "|") | ||
| } | ||
|
|
||
| private func chartDataSignature(for analysisData: PlanRouteAnalysisData?) -> String? { | ||
| guard let analysis = analysisData?.gpxAnalysis, | ||
| let gpxFile = analysisData?.gpxFile else { return nil } | ||
| return [ | ||
| gpxFile.path, | ||
| String(analysis.totalDistance), | ||
| String(analysis.timeSpan), | ||
| String(analysis.startTime) | ||
| ].joined(separator: "|") | ||
| } | ||
|
|
||
| private func statsSignature(for analysisData: PlanRouteAnalysisData?) -> String? { | ||
| guard cachedHasOverviewData, let analysisData else { return nil } | ||
| return [ | ||
|
|
@@ -644,8 +625,6 @@ extension PlanRouteAnalyzeViewController: UITableViewDataSource { | |
|
|
||
| let chart = ElevationChart(frame: .zero) | ||
| chart.translatesAutoresizingMaskIntoConstraints = false | ||
| chartView = chart | ||
| bindChartGestures(chart) | ||
|
|
||
| let gpxItem = dataItem(for: gpxFile) | ||
| let useHours = (analysis.timeSpan / Self.millisecondsPerHour) > 0 | ||
|
|
@@ -664,6 +643,8 @@ extension PlanRouteAnalyzeViewController: UITableViewDataSource { | |
| axisType: selectedXAxisType, | ||
| calcWithoutGaps: GpxUtils.calcWithoutGaps(gpxFile, gpxDataItem: gpxItem, overrideIsGeneralTrack: true)) | ||
| chart.dragYEnabled = false | ||
| chartView = chart | ||
| bindChartGestures(chart) | ||
|
|
||
| let recalcSeparator = SeparatorView() | ||
| recalcSeparator.translatesAutoresizingMaskIntoConstraints = false | ||
|
|
@@ -701,6 +682,7 @@ extension PlanRouteAnalyzeViewController: UITableViewDataSource { | |
| recalcBtn.heightAnchor.constraint(equalToConstant: 50) | ||
| ]) | ||
|
|
||
| cell.chartView = chart | ||
| return cell | ||
| } | ||
|
|
||
|
|
@@ -880,7 +862,6 @@ extension PlanRouteAnalyzeViewController: UITableViewDataSource { | |
| let isExpanded = expandedStatIndexes.contains(statIndex) | ||
|
|
||
| let barChart = HorizontalBarChartView(frame: .zero) | ||
| barChart.isUserInteractionEnabled = false | ||
| barChart.translatesAutoresizingMaskIntoConstraints = false | ||
| GpxUIHelper.refreshBarChart(chartView: barChart, | ||
| statistics: stat, | ||
|
|
@@ -897,6 +878,7 @@ extension PlanRouteAnalyzeViewController: UITableViewDataSource { | |
| rightAxis.drawGridLinesEnabled = true | ||
| rightAxis.gridColor = .chartAxisGridLine | ||
| rightAxis.labelTextColor = .textColorSecondary | ||
| bindChartGestures(barChart) | ||
|
|
||
| let legendView = isExpanded ? makeExpandedRoadAttrLegend(stat: stat) : makeCompactRoadAttrLegend(stat: stat) | ||
| card.addSubview(barChart) | ||
|
|
@@ -909,6 +891,7 @@ extension PlanRouteAnalyzeViewController: UITableViewDataSource { | |
| barChart.heightAnchor.constraint(equalToConstant: 54) | ||
| ]) | ||
|
|
||
| cell.chartView = barChart | ||
| return cell | ||
| } | ||
|
|
||
|
|
@@ -1223,6 +1206,16 @@ extension PlanRouteAnalyzeViewController: UITableViewDataSource { | |
|
|
||
| extension PlanRouteAnalyzeViewController: UITableViewDelegate { | ||
|
|
||
| func tableView(_ tableView: UITableView, willDisplay cell: UITableViewCell, forRowAt indexPath: IndexPath) { | ||
| guard let chart = (cell as? AnalyzeCardCell)?.chartView else { return } | ||
| cell.layoutIfNeeded() | ||
| if let primaryChart = chart as? ElevationChart { | ||
| chartSynchronizer.setPrimaryChart(primaryChart) | ||
| } else if let barChart = chart as? HorizontalBarChartView { | ||
| chartSynchronizer.registerBarChart(barChart) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed |
||
| } | ||
| } | ||
|
|
||
| func tableView(_ tableView: UITableView, didSelectRowAt indexPath: IndexPath) { | ||
| tableView.deselectRow(at: indexPath, animated: false) | ||
| guard case .hasData = currentState, | ||
|
|
@@ -1780,6 +1773,18 @@ private extension PlanRouteAnalyzeViewController { | |
|
|
||
| private extension PlanRouteAnalyzeViewController { | ||
|
|
||
| @objc private func onChartGesture(_ recognizer: UIGestureRecognizer) { | ||
| let isDoubleTap = (recognizer as? UITapGestureRecognizer)?.numberOfTapsRequired == 2 | ||
| guard recognizer is UIPinchGestureRecognizer || isDoubleTap, | ||
| recognizer.state == .ended, | ||
| let chart = recognizer.view as? BarLineChartViewBase else { return } | ||
| refreshChartOnMap() | ||
| DispatchQueue.main.async { | ||
| chart.layoutIfNeeded() | ||
| self.chartSynchronizer.syncViewPort(from: chart) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
| } | ||
| } | ||
|
|
||
| @objc private func onRecalculateTapped() { | ||
| showGetElevationSheet() | ||
| } | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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