refactor: replace most GoogleMap() rememberUpdatedState() usages with single State object and encapsulate subcomposition functionality #522
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
… single State object and encapsulate subcomposition functionality Fixes googlemaps#492
|
@wangela can I get a review & merge for this PR, please? It is functionally equivalent to current code, but improves code organization/clarity and offers minor performance improvements due to fewer slots used and less work performed on Compose parameter equality testing. This being a refactor I'd like to avoid recurring merge conflicts by merging it soon. Thanks. |
|
Bump |
|
Hi @bubenheimer , thanks for the contribution. This looks good, gets the codebase a bit closer to the API design guidelines and improves overall the health of the repository. |
kikoso
approved these changes
Apr 18, 2024
kikoso
changed the title
philip-segerfast
added a commit
to philip-segerfast/android-maps-compose
that referenced
this pull request
May 9, 2024
…operty from MapApplier [see desc.] - Wrap MapUpdater parameters inside a MapUpdaterState data class - Remove MapClickListeners property from MapApplier - Pass MapClickListeners directly to MapClickListenerUpdater
dkhawk
pushed a commit
that referenced
this pull request
Jul 9, 2024
…rmance in lazy layouts (#436) * Modify GoogleMap to use AndroidView overload with onReset lambda This will make sure that the underlying MapView is re-used in a LazyColumn (and elsewhere) which will significantly improve scrolling performance in LazyColumn. If this overload isn't used the MapView will be destroyed every time it leaves composition. The MapView will still be destroyed when the parent node leaves the composition. * Update androidx.compose.ui:ui library to fix AndroidView bug Issue: https://issuetracker.google.com/issues/267642562 * Add MapsInLazyColumnActivity * Hoist list items state * Bump compose-ui dependency * Update libs.versions.toml * Show list of countries in MapsInLazyColumnActivity * Cleanup * Remove compose-ui dependency version override * Update libs.versions.toml * Add parameter to GoogleMap to allow user to re-use the underlying MapView * Update libs.versions.toml * Update MapsInLazyColumnActivity.kt * Fix merge issues * Refactor the GoogleMap composable to support reuse of underlying map+ remove LaunchedEffect block * Replace ComposeNode usages with ReusableComposeNode * Refactor GoogleMap to save Composition + MapClickListeners + other related things in MapView using setTag() This also includes some additional things for debugging/testing/demonstrations purposes which will be removed before merging * Add compose material library to build.gradle temporarily for demo purposes * Implement ComposeNodeLifecycleCallback for MapClickListenerNode + set/remove listener on reuse/deactivation * Update GoogleMap.kt * Update MapsInLazyColumnActivity.kt * Update ids.xml * Update GoogleMap.kt * Update MapClickListeners.kt * Update GoogleMap.kt * Make so that MapView lifecycle state "moves" through lifecycle states instead of setting the state directly. * Move MapViewLifecycleController into its own file * Update MapViewLifecycleController.kt * Remove composition reuse logic as it's moved to another PR * Simplify MapViewLifecycleController logic * Re-register componentCallbacks if context has changed * Store map tag data in default tag instead of in separate tags by resource ids * Use CoroutineStart.UNDISPATCHED for creating composition * Don't remove lifecycle from MapViewLifecycleController on detach to observe onDestroy event * Reformat code * Move disposingComposition function to StreetView.kt as it's no longer used in GoogleMap.kt * Use awaitCancellation instead of infinite delay * Fix lifecycle event error message * Use else branch instead of ON_ANY in nextLifecycleEvent * Change name of setCompositionAsync to launchComposition and make it more idiomatic * Undo import changes * Undo unrelated import changes * Undo unrelated refactoring * Clarify that dependency is temporary * Create IncrementalLifecycleApplier This class is simpler and uses better practices than MapViewLifecycleController. Instead of modifying a MapView directly it invokes events on an internface. Also it utilizes Lifecycle.State which is more clear and idiomatic than only managing `Lifecycle.Event`s. Also, instead of relying on custom lifecycle logic, standard Lifecycle.Event utils are used. * Delete MapViewLifecycleController and replace with IncrementalLifecycleApplier * Resolve requirements * Add some logs for IncrementalLifecycleApplier * Remove `currentLifecycleState` parameter for IncrementalLifecycleApplier * Make some functions in IncrementalLifecycleApplier pure and move to companion object * Add support to override lifecycle state Used to set lifecycle state to Created when mapView is detached + restore once reattached * Remove LifecycleApplier stuff from GoogleMap * Add show/hide button on MapsInLazyColumnActivity * Use LifecycleRegistry solution instead of custom solution * Delete IncrementalLifecycleApplier.kt * Update GoogleMap.kt * Move between lifecycle states using custom approach inspired by LifecycleRegistry#sync() * Move LifecycleEventObserver into its own class * Rename moveForward/Backward to moveUp/Down * Make moveToLifecycleState more clear * Make lifecycleOwner nullable and nullify on detach * Rename attachStateListener to onAttachStateListener for clarity * Add moveToBaseState method to avoid reaching Lifecycle.State.CREATED prematurely * Replace mapView.tag onRelease lambda with lifecycleObserver reference + add moveToDestroyedState method * Remove @synchronized annotation * Make tagData() method a bit more clear * Remove LocalContext.current + mapViewContext * Make MapTagData + MapLifecycleEventObserver private * Update GoogleMap.kt * Merge componentCallbacks into registerAndSaveNewComponentCallbacks * inline MapView.createComposition (move code to call site) * Inline unregisterLifecycleObserver * Replace `isCompositionSet` with nullable `subcompositionJob` * Rename `mapUpdaterScope` to `parentCompositionScope` * Make MapTagData fully immutable + remove all logs + some related refactoring * Remove debugging stuff * Add some comments in AndroidView->factory * Remove "Create Composition" comment * Remove compose material dependency for maps-compose module * Some improvements to MapsInLazyColumnActivity + add more debugging controls * Convert tagData() to extension property * Move `composition` inside `try` block * Formatting * Add comment on CoroutineStart.UNDISPATCHED * Add KDoc to MapTagData * Remove Suppress annotation * Rename launchComposition to launchSubcomposition * Remove obvious comments * Move `lifecycleOwner` inside `object : View.OnAttachStateChangeListener` * Fix comment * Convert registerComponentCallbacks to a top-level class * Inline componentCallbacks class * Store reference to `Lifecycle` instead of `LifecycleOwner` * Update comment * Fix typo * Remove unused TAG constant * Demo - Set buildingFocused state on initial composition * Make maps in MapsInLazyColumnActivity pannable * Incorporate changes from #522 + remove MapClickListeners property from MapApplier [see desc.] - Wrap MapUpdater parameters inside a MapUpdaterState data class - Remove MapClickListeners property from MapApplier - Pass MapClickListeners directly to MapClickListenerUpdater * Revert MapClickListeners change + fix parameter order convention * Use `mapUpdaterState.cameraPositionState` instead of `currentCameraPositionState` * Minor fixes * Update app/src/main/AndroidManifest.xml Co-authored-by: Enrique López Mañas <eenriquelopez@gmail.com> --------- Co-authored-by: Enrique López Mañas <eenriquelopez@gmail.com>
|
Incorporated in #436 . Thanks @bubenheimer ! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #492