Fix Attach to roads flows after Plan Route refactor - #5740
Fix Attach to roads flows after Plan Route refactor#5740aleksandr-tata wants to merge 17 commits into
Conversation
|
Reviewed this against 1. The
|
|
Remaining #5484 regressions this branch does not cover yet, continuing the numbering from my previous comment. The first one below is the only item in this set with irreversible consequences; the other two are repository cleanup. 3. Data already lost to the implicit GPX saveThe 5.4 implementation wrote
Dropping the write in Those leftovers need handling here too: either register them in the GPX database so they become visible and removable in My Places, or delete them on migration. 4. Orphaned
|
PR #5740 — Changes after reviewRelated issue / taskFixes #5735 PR: #5740 SummaryThis update addresses the findings raised after the previous review of PR #5740. Review range:
The follow-up changes:
Review feedback addressedSource GPX identityThe GPX created by Route Info remains an in-memory object with an empty The source path is used only where the workflow needs the identity of the original track:
The source path is not copied into the editable in-memory GPX, and the source GPX is not implicitly overwritten. Related commits:
Attach Apply and generic Navigation
The Attach Apply path keeps the The generic Navigation action does not use the Attach-only guard. An unchanged plain track can enter Route Planning again. Related commits:
Restored Navigation decisionsThe pre-refactor branches required by generic Navigation were restored:
Before generic Navigation enters Route Planning, Related commits:
Waypoints and Save folder
The Save dialog now preselects the original track folder. A source track in the GPX root continues to use the root folder. Related commits:
Refactor cleanupRemoved the unreferenced Related commits:
TestingTested on
Automated tests
Coverage includes:
Coverage includes:
Both suites were run on a physical iPhone. The existing duplicate-runtime-class and QThread host warnings appeared outside the test assertions, after the suites had reported success. In total, the two focused suites executed 17 tests with 0 failures. Manual scenarios
Relevant checks
AI disclaimerImplementation:
Prompts used (summarised):
Decided by the agent, not requested explicitly:
Final review:
Significant findings:
|
|
Re-reviewed the eight commits since Items 1, 2, 4 and 5 are confirmed fixed. Splitting Attach Apply from generic Navigation is the right shape: the Attach path keeps the Item 3 is half done: the write is gone, but the files 5.4.0/5.4.1 already created on devices are still unreferenced by the GPX database and unreachable from inside the app. Nothing in the branch adopts or removes them. One more #5484 regression surfaced while comparing the restored generic Navigation against 5.3. 6. Multi-point route with no calculated route follows the straight line instead of routing between points
if (_editingContext.hasRoute) {
OASGpxFile *gpx = [_editingContext exportGpx:trackName]; // trk segments + rte points
...
} else if (_editingContext.isApproximationNeeded) {
[self enterApproximationMode];
} else {
OASGpxFile *gpx = [[OASGpxFile alloc] initWithAuthor:...];
[gpx addRoutePointsPoints:points addRoute:NO]; // rte points only, no track
[targetPointsHelper clearAllPoints:NO];
...
}The current branch routes all non-single-point cases through if ([doc hasRtePt] && ![doc hasTrkPt])
[_settings.gpxCalculateRtept set:YES];
else
[_settings.gpxCalculateRtept set:NO];
So a freshly drawn multi-point route with no calculated route and no approximation required now navigates along the straight segments rather than being routed between the points. The 5.3 branch also called Restoring the third branch — rte-only GPX plus MinorWith zero points, |
tigrim
left a comment
tigrim
left a comment
There was a problem hiding this comment.
Static review: two inline comments added.
| @@ -0,0 +1,265 @@ | |||
| #import <XCTest/XCTest.h> | |||
There was a problem hiding this comment.
Why isn't this test written in Swift?
| import XCTest | ||
|
|
||
| final class PlanRouteTrackSourceTests: XCTestCase { | ||
|
|
There was a problem hiding this comment.
private let gpxDirectory = "/Documents/GPX"
| OASGpxFile *gpx = _gpx; | ||
| NSString *fileName = gpx.path.lastPathComponent.stringByDeletingPathExtension; | ||
| [self dismissViewControllerAnimated:NO completion:^{ | ||
| [[OARootViewController instance].mapPanel closeRouteInfo]; |
There was a problem hiding this comment.
[P1] closeRouteInfo animates for 0.2 seconds and only then runs OARouteInfoView.onDismiss, but Plan Route is presented immediately after this call. During active navigation, that delayed onDismiss can still call switchToRouteFollowingLayout over the newly presented Plan Route screen—the same lifecycle race fixed in the direct Route Info entry point. Please present Plan Route from closeRouteInfo:YES onComplete: here as well.
| dataProvider.fitTrackOnMap(bottomInset: isLeftSidePresentation() ? 0 : getViewHeight(), | ||
| leftInset: isLeftSidePresentation() ? getLandscapeViewWidth() : 0) | ||
| } | ||
| if !hasPresentedInitialSnapWarning |
There was a problem hiding this comment.
[P2] The edit button and the gpx_route row both call openPlanRoute(false), but the object-based entry point always creates this controller with followTrackMode == true. Consequently, every unattached track forces this approximation popup even when showSnapWarning is false; applying it then calls applyAttachedTrackToNavigation and closes the editor. This changes the ordinary Edit Track flow into Attach. Please gate the forced initial popup on showSnapWarning, or pass a separate flag that distinguishes editing from Attach.
| dataProvider.onPointEditModeRequested = { [weak self] mode in | ||
| self?.showPointEditingView(mode: mode) | ||
| } | ||
| dataProvider.onApproximationApplied = { [weak self] in |
There was a problem hiding this comment.
// Inside DataProvider
DispatchQueue.main.async {
self.onApproximationApplied?()
}
| self?.handleApproximationApplied() | ||
| } | ||
| } | ||
| dataProvider.onApproximationPopupDismissed = { [weak self] in |
3 participants
Description
Fixes #5735.
This PR restores the
Attach to the roadsflows that regressed after the Plan Route refactor in #5484.Manual Attach from Route Info could open an empty Plan Route screen with
0 ftandNo points added yet. The same migration also lost part of the editing and navigation state previously passed by the 5.3 implementation.Root cause
Route Info uses a synthesized in-memory GPX created from the calculated route. This GPX intentionally has no file path. After the refactor, the new Plan Route controller received only
gpx.path, so it could not load the track.The refactor also stopped forwarding the selected segment, navigation profile, follow-track mode, explicit approximation-warning request, viewport policy, and post-Apply navigation behavior.
Two active-navigation lifecycle cases also made the Attach button appear unresponsive:
Changes
Behavior intentionally unchanged
Detailed track guidance = Alwayscontinues to use the existing automatic attachment and recalculation flow.Testing
Automated and static checks
OAPlanRouteEditingBridgeTest/testInMemoryGpxWithoutPathLoadsSelectedSegmentWithNavigationModeOAPlanRouteEditingBridgeTest/testRoutePointProfileOverridesNavigationModeplutil -lint OsmAnd.xcodeproj/project.pbxprojpassed.git diff --checkpassed.The hosted test application can terminate after XCTest reports success: the physical-device run stopped in background map-style parsing, and the simulator reported
QThread: Destroyed while thread is still running. Both test methods had already completed with 0 failures. The test-host shutdown behavior is separate from the assertions covered by this PR and still needs comparison with an unrelated focused test.Manual verification
Ask every time: Plan Route containstwisty-route.gpx, has a non-zero distance, and shows the approximation warning despite missing timestamps.Navigation -> Settings -> Advanced -> Follow track -> Attach to the roads: the selected track opens and Apply returns to navigation.Detailed track guidance = Always: automatic attachment finishes without opening an empty Plan Route screen.twisty-route.gpx: Bicycle remains the navigation-profile fallback.My_35_Km (1).gpx: the Car route-point profile takes precedence.AI disclaimer
Implementation:
Prompts used, summarized:
Decisions made by the agent:
Final review: