carplay-om-code-review #18

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>

Replace the hand-rolled, per-scene foreground/background coordination with a
single owner in MapsAppDelegate that observes the aggregate UIApplication
lifecycle notifications. This mirrors the approach Organic Maps converged on in
their CarPlay scene migration (organicmaps#12612, after review by biodranik) and
removes a class of ordering-dependent lifecycle bugs.
--- Why ---
The branch drove the framework's EnterForeground/EnterBackground and
rendering/location lifecycle per scene, split across the phone SceneDelegate,
the CarPlay app/dashboard scene delegates and a bespoke SceneLifecycle
coordinator (an isInForeground latch, shouldKeepForeground,
shouldKeepForeground(whenResigning:), leaveForegroundIfNoSceneActive, and
scattered enter*IfNeeded calls). The keep-foreground decision was reconstructed
by hand in several places and depended on the precise ordering of ~4 callbacks
across 3 delegates. It had at least one real soft spot:
leaveForegroundIfNoSceneActive() was only invoked from CarPlayService.destroy()
and the car scenes' sceneDidEnterBackground, so "phone already backgrounded +
dashboard hosting the map, dashboard disconnects" only backgrounded the engine
if the dashboard scene happened to fire sceneDidEnterBackground in the right
order.
UIKit posts UIApplicationDidBecomeActiveNotification when the *first* scene
becomes active and UIApplicationDidEnterBackgroundNotification when the *last*
scene backgrounds. Observing those gives "active while any scene (phone, CarPlay
app, or dashboard) is active; background only when all are" for free — no
bookkeeping, no ordering assumptions, no special-casing the map-on-car-screen
state. The notifications are still posted in a scene-based app (only the
UIApplicationDelegate lifecycle *methods* stop being auto-called), so the
observer is registered early in commonInit.
--- What changed ---
MapsAppDelegate (.h/.mm): becomes the single lifecycle owner.
 - observeApplicationLifecycle subscribes to the four UIApplication*
 notifications; thin onApplication*: wrappers forward to the real
 applicationDidBecomeActive:/WillResignActive:/WillEnterForeground:/
 DidEnterBackground: bodies.
 - Those bodies are re-homed here from SceneDelegate. The former
 handleDidBecomeActive (Spotlight categories, keyboard, TTS) and
 handleDidEnterBackground (background task, edits upload, saveRouteIfNeeded)
 are folded back in and dropped from the header.
SceneDelegate.mm: deleted all four sceneDid/Will* methods. Window setup,
 URL / user-activity / shortcut forwarding and the private mapViewController
 helper remain. Removed the now-unused AppInfo / MWMLocationManager /
 gps_tracker imports.
CarPlaySceneDelegate.swift: removed the per-scene foreground/background calls
 and deleted the entire SceneLifecycle class.
CarPlayDashboardSceneDelegate.swift: removed the SceneLifecycle calls but kept
 dashboardDidBecomeActive()/dashboardDidResignActive() — those drive map-host
 switching (which window shows the shared map view), which is orthogonal to the
 framework lifecycle.
CarPlayService.swift: destroy() no longer calls leaveForegroundIfNoSceneActive();
 backgrounding now happens when the last scene backgrounds.
MWMFrameworkHelper (.h/.mm): removed the enterForeground / enterBackground /
 enableRendering shims that were added in this branch solely for SceneLifecycle;
 MapsAppDelegate now calls the C++ framework directly.
--- Dead code dropped ---
The MWMOpenGLDriverMetalPre103 surface-destroy/recover branches that lived in the
moved methods are gone: AppInfo.openGLDriver only returns that case on iOS
10.0–10.3, but the app's minimum deployment target is iOS 15.6 (16.2 for
CarPlay), so the branch was unreachable. The only behavior that ever ran on
resign — SetRenderingDisabled(false) — is preserved. (The now-unused enum case
and its device lookup in AppInfo can be removed separately as a core cleanup.)
This supersedes the per-scene Fix #3 machinery
(shouldKeepForeground(whenResigning:) and friends), which is deleted along with
SceneLifecycle.
--- Verify ---
 - Phone-only: launch, lock, unlock — map pauses/resumes, location stops/starts,
 theme switching works; one applicationDidBecomeActive/DidEnterBackground pair
 per cycle.
 - Phone + CarPlay app, lock phone: rendering + location keep running (app stays
 active via the CarPlay scene; no resign fires).
 - Phone + CarPlay app, disconnect CarPlay while the phone is still locked:
 framework backgrounds (now driven by the aggregate notification).
 - Dashboard hosting, lock phone, then disconnect the dashboard: framework
 backgrounds cleanly (the previously order-dependent path).
 - CarPlay-first cold launch: map renders on the head unit; begin a route —
 voice guidance / Spotlight refresh run via applicationDidBecomeActive.
 - "Continue on phone" placeholder, lock phone: engine backgrounds and restores
 on unlock.
🤖 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>

Follow-up to the UIScene + CarPlay app/dashboard migration. Addresses the
findings from the code review of the branch (one shared EAGLView moved between
the phone, CarPlay app and dashboard windows). Finding #6 (dashboard stealing
the map from a co-visible app surface) was dropped: the dashboard and normal
CarPlay views are mutually exclusive, so re-parenting on toggle is necessary
work, not a blank-map bug.
Correctness fixes:
* Cold-launch regressions (#1, #2). After the UIScene migration only the warm
 paths re-handled Home-screen quick actions and Spotlight results, so a quick
 action or Spotlight tap that launched the app from a terminated state was
 silently dropped. scene:willConnectToSession: now reads
 connectionOptions.shortcutItem and runs connectionOptions.userActivities
 through a shared -handleUserActivity: helper that covers both
 CSSearchableItemActionType and universal links (also reused by the warm
 scene:continueUserActivity:).
* Nullable-window nil guards (#4). MapsAppDelegate.window became a nullable
 computed property (activeSceneDelegate.window), but a few Obj-C call sites
 still dereferenced it. MWMAlert.addControllerViewToWindow now early-returns
 when the window is nil instead of adding the alert to nothing and framing it
 CGRectZero; MWMNavigationInfoView.additionalStreetNameTopOffset no longer
 relies on nil-messaging returning a zeroed inset (and drops a stray ";;").
* CarPlay-only disconnect orphan (#3). When CarPlay disconnected with no phone
 scene connected, updateMapHost early-returned ("nowhere to host") leaving the
 shared map view parented in the tearing-down CarPlay window with car-screen
 mode still set. It now detaches to .none in that case: restores the phone
 representation, resets car-screen mode / arrow offset / visual scale, so a
 later phone scene re-hosts cleanly. CarPlayWindowScaleAdjuster.updateAppearance
 now takes an optional window for the no-phone-window deactivation path.
* Pending-search replay (#5). A dashboard "Search" tap while in "Continue on
 the phone" mode queued pendingShowSearch and replayed it unexpectedly the next
 time the app scene connected. showSearch() now only defers when the app scene
 is expected (!isPhoneModeRequested), and switchScreenToPhone() clears the flag.
Cleanup:
* Removed debug scaffolding shipping at LINFO (the default level): the
 log-only EAGLView didMoveToWindow override, the per-layout-pass EAGLView
 logs, the verbose updateMapHost state dump / no-op logs, and the two
 per-frame viewport-push logs (#7).
* Replaced the duplicated mapViewController lookup loop in SceneDelegate with a
 call to MapsAppDelegate.theApp.mapViewController (#8).
* Extracted the near-identical .carplay/.dashboard attach branches into a shared
 attachMapToCarScreen helper and lifted the arrow-offset magic number into a
 single kCarPositionArrowOffset constant (#9).
Testing / verification:
* Static: builds in Xcode with no new warnings on the nullable-window guards.
* Cold-launch (#1/#2): terminate the app, then (a) long-press the Home icon and
 tap a quick action, and (b) tap a CoMaps result in Spotlight; verify the
 route/search action runs on launch.
* CarPlay-only disconnect (#3): launch directly into CarPlay with no phone
 scene, disconnect CarPlay, then connect a phone scene; the map shows on the
 phone with phone scale and no car-screen mode (mapHost back to .phone).
* Pending search (#5): in the CarPlay app choose "Continue on the phone", tap
 the dashboard Search button, then "Continue in the car"; a search template
 must NOT auto-pop.
* Regression sweep: normal phone launch, deep link, universal link, CarPlay
 connect/disconnect, dashboard <-> app toggle, and phone lock during
 navigation all keep the map rendering and following position.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Signed-off-by: eisa01 <eisa01@gmail.com>

The previous commit's lifecycle-observation block reproduced the scaffolding from
Organic Maps PR organicmaps#12612 almost verbatim: the observeApplicationLifecycle
method name, the four addObserver:selector:name:object: blocks, and a layer of
onApplicationX: wrappers each forwarding to applicationX:UIApplication.sharedApplication.
That wrapper indirection only exists in OM to preserve the original
application...:(UIApplication *) UIApplicationDelegate signatures unchanged. UIKit no
longer calls those methods under the UIScene lifecycle, so we don't need to keep the
signatures — and keeping them meant carrying a forwarding layer for no benefit.
Collapse it: startObservingLifecycle registers the four aggregate UIApplication
notifications directly onto no-argument handlers (appDidActivate / appWillDeactivate /
appWillForeground / appDidBackground) that do the work themselves. The handler bodies
are unchanged (they are the project's own pre-scene code). Net -26 lines, no behavior
change, and the implementation is our own rather than a near-copy.
To be clear about provenance: the *strategy* — drive the framework's foreground/
background off the aggregate UIApplication notifications so it stays active while any
scene (phone, CarPlay app, or dashboard) is live — is derived from the approach Organic
Maps converged on in organicmaps#12612. The observer scaffolding here is implemented
independently; this supersedes the "mirrors the approach" wording in the previous
commit, which understated how closely that block tracked OM's code.
🤖 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>

UpdateDependentParameters only rebuilt the 3D projection matrices when the
perspective angle moved by more than kEps. At angle 0 SetRotationAngle builds a
singular projection whose inverse (m_3dtoP) is NaN/Inf; that is only safe while
m_isPerspective is false. If perspective turned on for a sub-kEps angle, the
delta check skipped the rebuild and left the NaN matrix active, corrupting
P3dtoP/PtoP3d and through them m_Org -> a NaN screen rect that trips the
m2::Rect min<=max assert (rect2d.hpp:45).
Rebuild whenever m_isPerspective flips, not just when the angle delta exceeds
kEps.
🤖 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>

Stress-testing CarPlay (rapid CarPlay<->phone switching, stop nav on the phone,
then re-open the CoMaps CarPlay icon) reproducibly aborted the debug build:
 m2::Rect<double>::Rect(...) ASSERT(minX<=maxX && minY<=maxY) rect2d.hpp:45
 +[MWMFrameworkHelper setVisibleViewport:scaleFactor:]
 CarPlayMapViewController.updateVisibleViewPortToNavigationState()
 CarPlayMapViewController.viewDidLayoutSubviews()
 -[CPWindow updateLayoutGuideWithInsets:] (layout-guide inset animation)
During the layout-guide animation / scene handoff the inset rect is transiently
degenerate or non-finite, so setVisibleViewport built an invalid m2::RectD whose
min<=max assert aborted the app (and would silently corrupt the viewport in
release).
setVisibleViewport now validates its input before constructing the rect: it
returns early unless scale is finite and > 0 and all four computed coordinates
are finite with x1 > x0 and y1 > y0. This guards every caller, not just CarPlay.
CarPlayMapViewController.updateVisibleViewPort additionally drops the frame up
front when the origin/size are not finite or the content scale factor is not
finite/positive; the previous width/height>0 check let a NaN origin through.
🤖 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>

The CarPlay position-mode map button was built with a default glyph and
relied on callers to sync its icon to the actual mode afterwards. Several
template paths never did, so the button showed a stale icon — most visibly
when changing follow/follow-and-rotate on the phone and returning to a car
screen, but also on trip cancel and when starting navigation.
Build the button from a single source of truth (current position mode) in
buildMyPositionModeButton(), used by both base and navigation UI, so every
template path is correct by construction. Re-sync the live button on
re-host (dashboard/app switch) where no rebuild happens.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Signed-off-by: eisa01 <eisa01@gmail.com>