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

rememberCameraPositionState() should accept inputs parameter #198

Open
svenjacobs opened this issue Sep 8, 2022 · 9 comments
Open

rememberCameraPositionState() should accept inputs parameter #198

svenjacobs opened this issue Sep 8, 2022 · 9 comments
Labels
priority: p3 Desirable enhancement or fix. May not be included in next release. type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design.

Comments

@svenjacobs
Copy link

svenjacobs commented Sep 8, 2022

I noticed that in version 2.7.0 of maps-compose the function rememberCameraPositionState does not accept an inputs parameter although it just delegates to rememberSaveable, which has vararg inputs: Any?. Is this a deliberate design decision?

I have a situation where the value actually should be recreated because latitude and longitude change after the user has selected a different location.

I'm happy to provide a pull request if this is a desired change.

@svenjacobs svenjacobs added triage me I really want to be triaged. type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design. labels Sep 8, 2022
@svenjacobs
Copy link
Author

The same applies to rememberMarkerState(). Maybe there are more remember*() functions in the library that I don't use.

@bubenheimer
Copy link
Contributor

bubenheimer commented Sep 8, 2022

I think the idea, at least in the case of MarkerState, is to handle any changes via LaunchedEffect, so that the underlying GoogleMap model Marker will not be recreated, but updated:

    val markerState = rememberMarkerState()

    LaunchedEffect(coords) { markerState.position = coords }

I expect it will change at some point to make it less edgy: #149

@svenjacobs
Copy link
Author

Good point. In case of rememberCameraPositionState() I tried to work around it by using

val pos = remember(latitude, longitude) { LatLng(latitude, longitude) }
val cameraPositionState = rememberSaveable(pos, saver = CameraPositionState.Saver) {
    CameraPositionState(
        position = CameraPosition.fromLatLngZoom(pos, 17f),
    )
}

which works as expected but applying the same pattern to rememberMarkerState() did not work. The inner workings of Marker probably don't take state changes into account at the moment.

@arriolac
Copy link
Member

rememberCameraPositionState does accept inputs—both key, the key to use for rememberSaveable, and init, a function literal with receiver so you can provide the same init values CameraPositionState accepts. But perhaps I'm misunderstanding?

So you should be able to do:

val cameraPositionState = rememberCameraPositionState {
  position = CameraPosition.fromLatLngZoom(pos, 17f)
}

Same comment for MarkerState.

@svenjacobs
Copy link
Author

svenjacobs commented Oct 20, 2022

The functions have a key argument, that's correct, but not an inputs argument. Quoting documentation of rememberSaveable:

inputs: A set of inputs such that, when any of them have changed, will cause the state to reset and init to be rerun

key: An optional key to be used as a key for the saved value. If not provided we use the automatically generated by the Compose runtime which is unique for the every exact code location in the composition tree

I'm talking about inputs because I'd like to reset the state when for instance latitude or longitude change.

val position = rememberCameraPositionState(latitude, longitude) { ... }

@svenjacobs svenjacobs changed the title rememberCameraPositionState() should accept inputs rememberCameraPositionState() should accept inputs parameter Oct 20, 2022
@arriolac
Copy link
Member

Gotcha—yes I misunderstood your question. I don't see any reasons for inputs not to be exposed in Maps Compose remember*() functions. Feel free to send a PR for it @svenjacobs

@svenjacobs
Copy link
Author

I can create a PR for rememberCameraPositionState but for rememberMarkerState it is a bit more complicated. See comments here, here and this issue.

@arriolac
Copy link
Member

arriolac commented Oct 20, 2022

Adding inputs to rememberMarkerState will not recreate the Marker so in theory you can expose inputs to both remember functions. But, recreating CameraPositionState needs to set the associated GoogleMap and MarkerState needs to set the correct internal Marker. These side effects make implementing this feature not so straightforward actually.. I suggest holding off on the pull request for that reason.

@PeterAttardo
Copy link

I just ran into the same issue, and it took me about three hours of debugging to realize that key in remember and key in rememberSaveable aren't the same, and that despite rememberSaveable calling through to remember, it doesn't pass key to key but rather inputs to key. This is mostly hidden from the typical compose user because remember and rememberSaveable behave the same as long as you use positional arguments, which users will default to 99% of the time.

maps-compose's lack of a positional inputs means that the positional argument gets funneled into rememberSaveable's key argument, which most users have probably never used before, and aren't expecting. The above naming confusion means that even if they check the signature of rememberCameraPositionState, they won't notice anything amiss, because of the natural assumption that key means key universally throughout compose's remember paradigm; not realizing that in rememberSaveable key is not key, but rather inputs is key.

Providing a 1:1 match with rememberSaveable's signature will at least minimize the confusion, and keep the remaining fault with the underlying compose naming, where it belongs.

@kikoso kikoso added priority: p3 Desirable enhancement or fix. May not be included in next release. and removed triage me I really want to be triaged. labels Jul 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority: p3 Desirable enhancement or fix. May not be included in next release. type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design.
Projects
None yet
Development

No branches or pull requests

5 participants