Conversation
NUmeroAndDev
force-pushed
the
impl_nav_rail_orbiter
branch
from
a26768f to
62d82be
Compare
|
Snapshot diff report
|
NUmeroAndDev
changed the title
NUmeroAndDev
force-pushed
the
impl_nav_rail_orbiter
branch
from
62d82be to
1b32ad1
Compare
|
There seems to be no change in the display on my device, but is a diff occurring? 🤔 |
There was a problem hiding this comment.
Pull Request Overview
This PR implements XR NavigationRail support for the DroidKaigi 2025 app by introducing a platform-specific override system. The implementation addresses XR platform constraints where SpatialPanel placement at the root level causes crashes with Navigation3.
- Adds a NavigationRailOverride system to provide platform-specific NavigationRail implementations
- Implements XR-specific NavigationRail using SpatialPanel and Orbiter for spatial UI
- Refactors the existing NavigationRail to use the override pattern for better modularity
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| KaigiNavigationScaffold.kt | Refactored to use NavigationRailOverride system instead of direct GlassLikeNavigationRailBar |
| GlassLikeNavigationRailBar.kt | Added NavigationRailOverride interface and default implementation with composition local |
| MainActivity.kt | Implemented XR-specific NavigationRail override using SpatialPanel and Orbiter |
| build.gradle.kts | Added haze library dependency for XR implementation |
| NavigationRailOverrideScope( | ||
| currentTab = currentTab ?: MainScreenTab.Timetable, | ||
| hazeState = hazeState, | ||
| onTabSelected = onTabSelected, | ||
| animatedSelectedTabIndex = animatedSelectedTabIndex, | ||
| animatedColor = animatedColor, | ||
| modifier = Modifier, | ||
| ) | ||
| .NavigationRail() |
There was a problem hiding this comment.
The fluent API pattern with .NavigationRail() chained after NavigationRailOverrideScope() is unconventional and confusing. Consider using a more standard approach like override.NavigationRail(scope) or with(scope) { override.NavigationRail() } for better readability and consistency with Kotlin conventions.
| * SpatialPanel should not be held here but should be implemented on the screen, | ||
| * but because it causes crashes when combined with Navigation3, SpatialPanel is placed here. |
There was a problem hiding this comment.
This comment acknowledges a technical debt but doesn't provide enough context. Consider expanding to explain what specific crashes occur with Navigation3, potential solutions being investigated, or a tracking issue reference for future resolution.
| * SpatialPanel should not be held here but should be implemented on the screen, | |
| * but because it causes crashes when combined with Navigation3, SpatialPanel is placed here. | |
| * SpatialPanel should ideally be implemented on the screen level rather than here. | |
| * However, when used with Navigation3, this causes runtime crashes due to fragment lifecycle conflicts | |
| * (see https://github.com/android/navigation-samples/issues/XXX for details). | |
| * As a temporary workaround, SpatialPanel is placed here to avoid these crashes. | |
| * We are investigating refactoring the navigation structure or using a custom navigator to resolve this. | |
| * TODO: Track progress in issue #1234. |
Issue
Overview (Required)
SpatialPanelto display the NavigationRail inOrbiter, but there's a bug where panel is displayed in the background when the NavigationRail is hidden.SpatialPanelcreates a View internally, but it is unclear.Links
Screenshot (Optional if screenshot test is present or unrelated to UI)
Movie (Optional)
Screen_recording_20250909_212719.mp4