0
0
Fork
You've already forked comaps
0
forked from comaps/comaps

[carplay] Fix scene-lifecycle review findings #17

Closed
eisa01 wants to merge 1 commit from carplay-code-review-jun28 into carplay-dashboard-mvp2
pull from: carplay-code-review-jun28
merge into: eisa01:carplay-dashboard-mvp2
eisa01:main
eisa01:carplay-dashboard-support
eisa01:modes-ui
eisa01:eisa01/carplay-road-shields
eisa01:multi-surface-map-rendering
eisa01:carplay-0708
eisa01:forgejo-macos-runner
eisa01:create-venv
eisa01:carplay-0706
eisa01:carplay-0704-findings
eisa01:fable-review
eisa01:testflight/2026.06.30
eisa01:carplay-dashboard
eisa01:carplay-om-code-review
eisa01:carplay-dashboard-mvp2
eisa01:car-follow-on-reacquire
eisa01:carplay-location-keepalive
eisa01:testflight/2026.06.20
eisa01:carplay-roundabout-exit-number
eisa01:carplay-destination
eisa01:carplay-dashboard-mvp
eisa01:name-cherry-picking
eisa01:om-pr-11243-2
eisa01:icu-cjk-fix
eisa01:simple-cjk-fix
eisa01:fix-crash-search
eisa01:scene-support
eisa01:om-carplay-button
eisa01:om-pr-10545
eisa01:submodule-main-branch
eisa01:improve-unit-testing-docs

Addresses four issues found while reviewing the squashed CarPlay + iOS
scene-support work on this branch. Each stems from the new multi-scene
lifecycle being split across the phone SceneDelegate, the CarPlay app/
dashboard scene delegates and CarPlayService.updateMapHost, with a few
decisions not centralized so they drifted.

--- #1 Per-session car defaults now reset when leaving all car screens ---

hasAppliedDefaultCarZoom / hasEngagedInitialCarFollow were reset only in
CarPlayService.destroy(), which runs on CarPlay app-scene disconnect. A
dashboard that connected, applied the defaults, disconnected and reconnected
within the same app launch skipped the driving-friendly overview zoom and the
initial heading-up follow on its second session. Reset is now routed through a
single resetCarSessionDefaults() helper, called both from destroy() and from
updateMapHost() whenever the map settles back on the phone (i.e. leaves all car
screens).

Verify:

  • Connect CarPlay, then in the dashboard tile confirm the map zooms out to
    the overview level and starts heading-up follow.
  • Disconnect and reconnect the dashboard without quitting the app.
  • Before: second dashboard session keeps the previous (close) zoom and does
    not re-engage follow.
  • After: second session re-applies the overview zoom and heading-up follow.

--- #2 Car-first launch runs the app-active service refresh ---

The phone path ran MWMSearch.addCategoriesToSpotlight,
MWMKeyboard.applicationDidBecomeActive and MWMTextToSpeech.applicationDidBecomeActive
on activation; the CarPlay path (SceneLifecycle.carSceneDidBecomeActive) did
not, so on a launch made directly into CarPlay (no phone scene) those never
ran until a phone scene later activated. The three calls are centralized in a
new -[MapsAppDelegate handleDidBecomeActive] (mirroring handleDidEnterBackground)
and invoked from both the phone sceneDidBecomeActive and the CarPlay path, so
they can't drift again.

Verify:

  • With the phone locked/not launched, start the app directly into CarPlay and
    begin a route.
  • Before: TTS voice state / Spotlight categories are not refreshed on the
    car-first activation (only after a phone scene becomes active).
  • After: handleDidBecomeActive runs on the CarPlay activation; voice guidance
    and Spotlight indexing are refreshed without needing the phone scene.

--- #3 Phone resign uses a shared, resign-aware keep-foreground check ---

SceneDelegate.sceneWillResignActive guarded only on isHostingMapOnCarScreen and
then called enterBackgroundIfNeeded directly, bypassing the shouldKeepForeground
predicate the CarPlay path funnels through. It could not reuse
leaveForegroundIfNoSceneActive because at willResignActive the resigning scene
is still .foregroundActive, so isPhoneSceneForegroundActive would wrongly report
the engine as needed. Added SceneLifecycle.shouldKeepForeground(whenResigning:)
which excludes the resigning scene, and routed the phone path through it.
Behavior on iPhone is unchanged (single phone scene, so locking with a
non-hosting CarPlay placeholder still backgrounds the engine — correct for
battery); the asymmetric hand-rolled decision is gone and it is now
multi-window safe.

Verify:

  • Connect CarPlay with the map hosted on the car screen, lock the phone:
    rendering and location updates keep running (unchanged).
  • Choose "Continue on phone" so CarPlay shows the placeholder, lock the phone:
    the engine backgrounds and restores on unlock (unchanged).
  • Watch the [CarPlayHost] / sceneWillResignActive logs to confirm the decision
    now comes from shouldKeepForegroundWhenResigning: rather than the ad-hoc
    isHostingMapOnCarScreen check.

--- #4 MapsAppDelegate.window no longer rescans connectedScenes ---

-[MapsAppDelegate window] looped UIApplication.connectedScenes on every access,
and window is read on hot layout paths (TabBarArea.areaFrame, the theme
renderers). There is only ever one phone SceneDelegate, so the scan was waste.
The connected phone scene delegate is now cached in a weak activeSceneDelegate
property that SceneDelegate sets on connect and clears on disconnect; the getter
reads it directly. Weak so it auto-nils if a disconnect is ever missed; the
value still resolves to nil during a car-first launch.

Verify:

  • Normal phone launch: map UI lays out and light/dark theme switching still
    works (these read theApp.window).
  • Car-first launch: theApp.window is nil and the map attaches to the CarPlay
    window as before (see the phoneWindow=... field in the updateMapHost log).
  • Before: every window access walks connectedScenes.
  • After: window access is a single weak-pointer read.

🤖 Generated with Claude Code

Signed-off-by: eisa01 eisa01@gmail.com
Co-Authored-By: Claude Opus 4.8 noreply@anthropic.com

Addresses four issues found while reviewing the squashed CarPlay + iOS scene-support work on this branch. Each stems from the new multi-scene lifecycle being split across the phone SceneDelegate, the CarPlay app/ dashboard scene delegates and CarPlayService.updateMapHost, with a few decisions not centralized so they drifted. --- #1 Per-session car defaults now reset when leaving all car screens --- hasAppliedDefaultCarZoom / hasEngagedInitialCarFollow were reset only in CarPlayService.destroy(), which runs on CarPlay *app*-scene disconnect. A dashboard that connected, applied the defaults, disconnected and reconnected within the same app launch skipped the driving-friendly overview zoom and the initial heading-up follow on its second session. Reset is now routed through a single resetCarSessionDefaults() helper, called both from destroy() and from updateMapHost() whenever the map settles back on the phone (i.e. leaves all car screens). Verify: - Connect CarPlay, then in the dashboard tile confirm the map zooms out to the overview level and starts heading-up follow. - Disconnect and reconnect the dashboard without quitting the app. - Before: second dashboard session keeps the previous (close) zoom and does not re-engage follow. - After: second session re-applies the overview zoom and heading-up follow. --- #2 Car-first launch runs the app-active service refresh --- The phone path ran MWMSearch.addCategoriesToSpotlight, MWMKeyboard.applicationDidBecomeActive and MWMTextToSpeech.applicationDidBecomeActive on activation; the CarPlay path (SceneLifecycle.carSceneDidBecomeActive) did not, so on a launch made directly into CarPlay (no phone scene) those never ran until a phone scene later activated. The three calls are centralized in a new -[MapsAppDelegate handleDidBecomeActive] (mirroring handleDidEnterBackground) and invoked from both the phone sceneDidBecomeActive and the CarPlay path, so they can't drift again. Verify: - With the phone locked/not launched, start the app directly into CarPlay and begin a route. - Before: TTS voice state / Spotlight categories are not refreshed on the car-first activation (only after a phone scene becomes active). - After: handleDidBecomeActive runs on the CarPlay activation; voice guidance and Spotlight indexing are refreshed without needing the phone scene. --- #3 Phone resign uses a shared, resign-aware keep-foreground check --- SceneDelegate.sceneWillResignActive guarded only on isHostingMapOnCarScreen and then called enterBackgroundIfNeeded directly, bypassing the shouldKeepForeground predicate the CarPlay path funnels through. It could not reuse leaveForegroundIfNoSceneActive because at willResignActive the resigning scene is still .foregroundActive, so isPhoneSceneForegroundActive would wrongly report the engine as needed. Added SceneLifecycle.shouldKeepForeground(whenResigning:) which excludes the resigning scene, and routed the phone path through it. Behavior on iPhone is unchanged (single phone scene, so locking with a non-hosting CarPlay placeholder still backgrounds the engine — correct for battery); the asymmetric hand-rolled decision is gone and it is now multi-window safe. Verify: - Connect CarPlay with the map hosted on the car screen, lock the phone: rendering and location updates keep running (unchanged). - Choose "Continue on phone" so CarPlay shows the placeholder, lock the phone: the engine backgrounds and restores on unlock (unchanged). - Watch the [CarPlayHost] / sceneWillResignActive logs to confirm the decision now comes from shouldKeepForegroundWhenResigning: rather than the ad-hoc isHostingMapOnCarScreen check. --- #4 MapsAppDelegate.window no longer rescans connectedScenes --- -[MapsAppDelegate window] looped UIApplication.connectedScenes on every access, and window is read on hot layout paths (TabBarArea.areaFrame, the theme renderers). There is only ever one phone SceneDelegate, so the scan was waste. The connected phone scene delegate is now cached in a weak activeSceneDelegate property that SceneDelegate sets on connect and clears on disconnect; the getter reads it directly. Weak so it auto-nils if a disconnect is ever missed; the value still resolves to nil during a car-first launch. Verify: - Normal phone launch: map UI lays out and light/dark theme switching still works (these read theApp.window). - Car-first launch: theApp.window is nil and the map attaches to the CarPlay window as before (see the phoneWindow=... field in the updateMapHost log). - Before: every window access walks connectedScenes. - After: window access is a single weak-pointer read. 🤖 Generated with [Claude Code](https://claude.ai/code) Signed-off-by: eisa01 <eisa01@gmail.com> Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Addresses four issues found while reviewing the squashed CarPlay + iOS
scene-support work on this branch. Each stems from the new multi-scene
lifecycle being split across the phone SceneDelegate, the CarPlay app/
dashboard scene delegates and CarPlayService.updateMapHost, with a few
decisions not centralized so they drifted.
--- #1 Per-session car defaults now reset when leaving all car screens ---
hasAppliedDefaultCarZoom / hasEngagedInitialCarFollow were reset only in
CarPlayService.destroy(), which runs on CarPlay *app*-scene disconnect. A
dashboard that connected, applied the defaults, disconnected and reconnected
within the same app launch skipped the driving-friendly overview zoom and the
initial heading-up follow on its second session. Reset is now routed through a
single resetCarSessionDefaults() helper, called both from destroy() and from
updateMapHost() whenever the map settles back on the phone (i.e. leaves all car
screens).
Verify:
 - Connect CarPlay, then in the dashboard tile confirm the map zooms out to
 the overview level and starts heading-up follow.
 - Disconnect and reconnect the dashboard without quitting the app.
 - Before: second dashboard session keeps the previous (close) zoom and does
 not re-engage follow.
 - After: second session re-applies the overview zoom and heading-up follow.
--- #2 Car-first launch runs the app-active service refresh ---
The phone path ran MWMSearch.addCategoriesToSpotlight,
MWMKeyboard.applicationDidBecomeActive and MWMTextToSpeech.applicationDidBecomeActive
on activation; the CarPlay path (SceneLifecycle.carSceneDidBecomeActive) did
not, so on a launch made directly into CarPlay (no phone scene) those never
ran until a phone scene later activated. The three calls are centralized in a
new -[MapsAppDelegate handleDidBecomeActive] (mirroring handleDidEnterBackground)
and invoked from both the phone sceneDidBecomeActive and the CarPlay path, so
they can't drift again.
Verify:
 - With the phone locked/not launched, start the app directly into CarPlay and
 begin a route.
 - Before: TTS voice state / Spotlight categories are not refreshed on the
 car-first activation (only after a phone scene becomes active).
 - After: handleDidBecomeActive runs on the CarPlay activation; voice guidance
 and Spotlight indexing are refreshed without needing the phone scene.
--- #3 Phone resign uses a shared, resign-aware keep-foreground check ---
SceneDelegate.sceneWillResignActive guarded only on isHostingMapOnCarScreen and
then called enterBackgroundIfNeeded directly, bypassing the shouldKeepForeground
predicate the CarPlay path funnels through. It could not reuse
leaveForegroundIfNoSceneActive because at willResignActive the resigning scene
is still .foregroundActive, so isPhoneSceneForegroundActive would wrongly report
the engine as needed. Added SceneLifecycle.shouldKeepForeground(whenResigning:)
which excludes the resigning scene, and routed the phone path through it.
Behavior on iPhone is unchanged (single phone scene, so locking with a
non-hosting CarPlay placeholder still backgrounds the engine — correct for
battery); the asymmetric hand-rolled decision is gone and it is now
multi-window safe.
Verify:
 - Connect CarPlay with the map hosted on the car screen, lock the phone:
 rendering and location updates keep running (unchanged).
 - Choose "Continue on phone" so CarPlay shows the placeholder, lock the phone:
 the engine backgrounds and restores on unlock (unchanged).
 - Watch the [CarPlayHost] / sceneWillResignActive logs to confirm the decision
 now comes from shouldKeepForegroundWhenResigning: rather than the ad-hoc
 isHostingMapOnCarScreen check.
--- #4 MapsAppDelegate.window no longer rescans connectedScenes ---
-[MapsAppDelegate window] looped UIApplication.connectedScenes on every access,
and window is read on hot layout paths (TabBarArea.areaFrame, the theme
renderers). There is only ever one phone SceneDelegate, so the scan was waste.
The connected phone scene delegate is now cached in a weak activeSceneDelegate
property that SceneDelegate sets on connect and clears on disconnect; the getter
reads it directly. Weak so it auto-nils if a disconnect is ever missed; the
value still resolves to nil during a car-first launch.
Verify:
 - Normal phone launch: map UI lays out and light/dark theme switching still
 works (these read theApp.window).
 - Car-first launch: theApp.window is nil and the map attaches to the CarPlay
 window as before (see the phoneWindow=... field in the updateMapHost log).
 - Before: every window access walks connectedScenes.
 - After: window access is a single weak-pointer read.
🤖 Generated with [Claude Code](https://claude.ai/code)
Signed-off-by: eisa01 <eisa01@gmail.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Author
Owner
Copy link

On #1:
Is on zoom level 15 regardless?
Heading up north do not work

Still makes sense

On #2:
Works regardless???

On #1: Is on zoom level 15 regardless? Heading up north do not work Still makes sense On #2: Works regardless???
Author
Owner
Copy link

No longer relevant

No longer relevant
eisa01 closed this pull request 2026年06月30日 23:03:09 +02:00

Pull request closed

Please reopen this pull request to perform a merge.
Sign in to join this conversation.
No reviewers
Labels
Clear labels
No items
No labels
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
eisa01/comaps!17
Reference in a new issue
eisa01/comaps
No description provided.
Delete branch "carplay-code-review-jun28"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?