Skip to content
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

Navigation compose #670

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from
Draft

Navigation compose #670

wants to merge 4 commits into from

Conversation

dkhawk
Copy link
Contributor

@dkhawk dkhawk commented Jan 9, 2025

Thank you for opening a Pull Request!


Before submitting your PR, there are a few things you can do to make sure it goes smoothly:

  • Make sure to open a GitHub issue as a bug/feature request before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
  • Ensure the tests and linter pass
  • Code coverage does not decrease (if any source code was changed)
  • Appropriate docs were updated (if necessary)

Fixes #<issue_number_goes_here> 🦕

dkhawk added 4 commits January 6, 2025 16:47
Adds support for the Navigation SDK to Maps Compose. It just allows the user to replace a standard MapView with a NavigationView within a GoogleMap composable.

The following changes were made:

Added a new NavigationViewDelegate class to handle the integration between NavigationView and Maps Compose.
Added a new NavigationScreen composable function to display the navigation view.
Added a new MovableMarker composable function to display a draggable marker on the map.
Updated the GoogleMap composable function to support the use of NavigationView.
Added a new NavigationApplication class to initialize the Places SDK.
Added a new ApiKeyProvider class to provide API keys for the Maps and Places SDKs.
Added a new LocationProvider class to provide location data.
Added a new PermissionChecker class to check for location permissions.
Updated the build.gradle.kts files to include the necessary dependencies.
Updated the local.defaults.properties file to include the Places API key.
Adds NavigationViewDelegate to support NavigationView in the navigation sample app.

This change allows the navigation sample app to use NavigationView, which provides turn-by-turn navigation functionality.

The NavigationViewDelegate handles the lifecycle and rendering of the NavigationView, ensuring that it is properly integrated into the Jetpack Compose UI.

Uses a MarkerComposable to demonstrate the map really is a composable
@dkhawk dkhawk marked this pull request as draft January 9, 2025 00:51
activitycompose = "1.9.3"
agp = "8.7.2"
androidxtest = "1.6.2"
agpVersion = "8.7.2"

Check warning

Code scanning / Android Lint

Obsolete Android Gradle Plugin Version Warning

A newer version of com.android.application than 8.7.2 is available: 8.8.0. (There is also a newer version of 8.7.𝑥 available, if upgrading to 8.8.0 is difficult: 8.7.3)
activitycompose = "1.9.3"
agp = "8.7.2"
androidxtest = "1.6.2"
agpVersion = "8.7.2"

Check warning

Code scanning / Android Lint

Obsolete Android Gradle Plugin Version Warning

A newer version of com.android.application than 8.7.2 is available: 8.8.0. (There is also a newer version of 8.7.𝑥 available, if upgrading to 8.8.0 is difficult: 8.7.3)
activitycompose = "1.9.3"
agp = "8.7.2"
androidxtest = "1.6.2"
agpVersion = "8.7.2"

Check warning

Code scanning / Android Lint

Obsolete Android Gradle Plugin Version Warning

A newer version of com.android.application than 8.7.2 is available: 8.8.0. (There is also a newer version of 8.7.𝑥 available, if upgrading to 8.8.0 is difficult: 8.7.3)
@googlemaps-bot
Copy link
Contributor

Code Coverage

Overall Project 17.36%

There is no coverage information present for the Files changed

@@ -69,7 +69,7 @@ private fun MapView.ensureContainerView(): NoDrawContainerView {
@RestrictTo(RestrictTo.Scope.LIBRARY_GROUP)
@Composable
public fun rememberComposeUiViewRenderer(): ComposeUiViewRenderer {
val mapView = (currentComposer.applier as MapApplier).mapView
val mapView = (currentComposer.applier as MapApplier).mapViewDelegate
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't we need to pass the mapView here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem is that the first common ancestor of MapView and NavigationView is a View (technically, a ViewGroup). So there really isn't a shared "MapView" that we can use here (which is why I created the delegate).


override fun onLowMemory() {
// TODO(): what is the correct value here?
mapView.onTrimMemory(ComponentCallbacks2.TRIM_MEMORY_COMPLETE)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There does not seem to be any reference to which value is the correct one, given that all of them are deprecated. The method does not really seem to be documented.

@@ -145,19 +150,31 @@ public fun GoogleMap(
var subcompositionJob by remember { mutableStateOf<Job?>(null) }
val parentCompositionScope = rememberCoroutineScope()

var delegate by remember {
// TODO: this could leak the view?
Copy link
Collaborator

@kikoso kikoso Jan 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My two cents: if the AbstractMapViewDelegate is holding a reference to the Context it could leak. But I think we are directly not doing it, if we take a look at the mapViewDelegate, right? Not sure if using the java.lang.ref.WeakReference will make sense.

EDIT: Yeah, I think we are holding a reference in this file.

override fun onStart(): Unit = mapView.onStart()
override fun onResume(): Unit = mapView.onResume()
override fun onPause(): Unit = mapView.onPause()
override fun onStop(): Unit = mapView.onStop()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have a few other common methods, like some of the method listeners, saving/restoring instances, etc. Are they covered by the AbstractMapViewDelegate?

@kikoso kikoso force-pushed the navigation-compose branch from d3c6162 to 934fee6 Compare January 13, 2025 17:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants