Show Callout Sample #137

prupani-7 · 2023-09-25T21:32:30Z

Description

PR to add a new Kotlin sample "Show Callout" in Maps category.

Links and Data

Sample Epic: runtime/kotlin/issues/1117

What To Review

Review the code to make sure it is easy to follow like other samples on Android
README.md and README.metadata.json files

How to Test

Run the sample on the sample viewer or the repo.

shubham7109

Nice sample @prupani-7, just a few minor comments mainly to use the MapViewModel to update and show the callout on the MapView

show-callout/src/main/java/com/esri/arcgismaps/sample/showcallout/components/ComposeMapView.kt

shubham7109 · 2023-09-26T18:31:25Z

show-callout/src/main/java/com/esri/arcgismaps/sample/showcallout/components/ComposeMapView.kt

+@Composable
+fun ComposeMapView(
+    modifier: Modifier = Modifier,
+    mapViewModel: MapViewModel,
+    application: Application
+) {


I think with this sample we should keep the business logic in the MapViewModel, where the mapPoint and the TextView would be set. The ComposeMapView should return a lambda on every tap event which is then consumed by the MainScreen similar to the Show coordinates in multiple formats sample. Would probably be helpful to have the Callout and viewpoint as properties of the MapViewState

arcgis-maps-sdk-kotlin-samples/show-coordinates-in-multiple-formats/src/main/java/com/esri/arcgismaps/sample/showcoordinatesinmultipleformats/screens/MainScreen.kt

Lines 51 to 53 in 5692022

onSingleTap = { singleTapConfirmedEvent ->

mapViewModel.onMapTapped(singleTapConfirmedEvent.mapPoint)

}

Suggested change

@Composable

fun ComposeMapView(

modifier: Modifier = Modifier,

mapViewModel: MapViewModel,

application: Application

) {

@Composable

fun ComposeMapView(

modifier: Modifier = Modifier,

mapViewModel: MapViewModel,

onSingleTap: (SingleTapConfirmedEvent) -> Unit = {}) {

Yeah, aware that the business logic goes in MapViewModel, and I tried that approach first, i.e. to add a new property callout to the MapViewState. But we cannot initialize Callout, as it has no constructors

Which is why I adapted the above implemented approach

If you have thoughts on the MapViewState callout property approach , we could discuss on a call

prupani-7 · 2023-09-26T23:16:20Z

@shubham7109 as Discussed in a call, the logic is now in mapViewModel with calloutContent and the latlonpoint properties of MapViewState and calling callout.show() in the update block of mapView. Thanks with the related implementation suggestions!
Please re-review.

shubham7109

Using the MapViewModel here looks nice! Just two minor comments, but good to approve 👍

shubham7109 · 2023-09-26T23:47:16Z

show-callout/src/main/java/com/esri/arcgismaps/sample/showcallout/components/ComposeMapView.kt

+    // launch coroutine functions in the composition's CoroutineContext
+    LaunchedEffect(Unit) {
+        launch {
+            mapView.onSingleTapConfirmed.collect {
+                onSingleTap(it)
+            }
+        }
+    }


Suggested change

// launch coroutine functions in the composition's CoroutineContext

LaunchedEffect(Unit) {

launch {

mapView.onSingleTapConfirmed.collect {

onSingleTap(it)

}

}

}

// launch a coroutine to detect a map tap

LaunchedEffect(Unit) {

mapView.onSingleTapConfirmed.collect {

onSingleTap(it)

}

}

I think it's creating two coroutine scopes here as the LaunchedEffect should launch a new coroutine. This seems to have been glossed over before since it is generated by the script. 😅 Maybe in the future I would like to show you the old-school Java project for the new module script and we could chat about how it works.

arcgis-maps-sdk-kotlin-samples/tools/NewModuleScript/ComposeMapViewTemplate.kt

Lines 62 to 69 in 5692022

// launch coroutine functions in the composition's CoroutineContext

LaunchedEffect(Unit) {

launch {

mapView.onSingleTapConfirmed.collect {

onSingleTap(it)

}

}

}

This seems to have been glossed over before since it is generated by the script.

Shouldn't launch be removed from the template then?

shubham7109 · 2023-09-26T23:54:51Z

show-callout/src/main/java/com/esri/arcgismaps/sample/showcallout/components/MapViewModel.kt

+            // create a textview for the callout
+            mapViewState.value.calloutContent.text = application.getString(


The TextView is created in the MapViewState

Suggested change

// create a textview for the callout

mapViewState.value.calloutContent.text = application.getString(

// set the callout text to display point coordinates

mapViewState.value.calloutContent.text = application.getString(

changanxian · 2023-09-27T17:17:19Z

show-callout/src/main/java/com/esri/arcgismaps/sample/showcallout/components/MapViewModel.kt

+
+    fun onMapTapped(mapPoint: Point?) {
+        // get map point from the Single tap event
+        mapPoint?.let { mapPoint ->


The argument mapPoint of Lamda closure shadows the mapPoint, it'd better change a different name, e.g 'point'

changanxian · 2023-09-27T17:19:28Z

show-callout/src/main/java/com/esri/arcgismaps/sample/showcallout/components/ComposeMapView.kt

+        modifier = modifier,
+        factory = { mapView },
+        // recomposes the MapView on changes in the MapViewState
+        update = { mapView ->


same here, The argument mapView of Lamda closure shadows the variable mapView, it'd better change a different name.

I am not sure if I understand this completely.

By shadows the mapView variable do you mean the one created above val mapView = createMapViewInstance() on line 48?
This is something we follow across all samples, and part of the sample template.

To me mapView name still holds valid in the update block.

If at all, we decide to make a change, I would vote for making change to the mapView var created using createMapViewInstance() to something like mapViewInstance or mapViewObject

CC: @shubham7109 since you spearheaded these changes in the initial sample design phase

I mean the line 60, the mapView overshadows the outside variable mapView

changanxian · 2023-09-27T17:20:54Z

show-callout/src/main/java/com/esri/arcgismaps/sample/showcallout/components/ComposeMapView.kt

+fun ComposeMapView(
+    modifier: Modifier = Modifier,
+    mapViewModel: MapViewModel,
+    onSingleTap: (SingleTapConfirmedEvent) -> Unit = {}


Could it be better for the callback be a suspend function, onSingleTap: suspend (SingleTapConfirmedEvent) -> Unit = {}

@changanxian I don't think I understand, what value will it bring in current samples, making the onSingleTap callback a suspend function?

It may not be necessary in this case, and keep the current implementation. But it will be an asynchronous call. Imagine if the onSingleTap is called in a loop, it will be different.

changanxian · 2023-09-27T17:25:32Z

show-callout/src/main/java/com/esri/arcgismaps/sample/showcallout/components/MapViewModel.kt

+data class MapViewState(val application: Application) {
+    var arcGISMap: ArcGISMap = ArcGISMap(BasemapStyle.ArcGISNavigationNight)
+    var viewpoint: Viewpoint = Viewpoint(34.056295, -117.195800, 1000000.0)
+    var calloutContent: TextView by mutableStateOf(TextView(application))


It is a little odd for me to have a UI TextView in the class mix in other non-UI classes, could it be refactored into a String? Keep the UI TextView outside of the class, we only need to keep track of the state of the contents, right?

Good point, this got me thinking that the 2 variables calloutContent and latLonPoint could be MapViewModel properties instead of MapViewState properties.

change to

TextView needs an applicationContext available in the viewmodel class

@shubham7109 Since we worked on this change together, tagging you for review.

changanxian · 2023-09-27T17:26:21Z

@prupani-7 a few comments.

changanxian · 2023-09-29T18:31:15Z

show-callout/src/main/java/com/esri/arcgismaps/sample/showcallout/components/ComposeMapView.kt

+    // launch coroutine functions in the composition's CoroutineContext
+    LaunchedEffect(Unit) {
+        mapView.onSingleTapConfirmed.collect {
+            onSingleTap(it)


After this Friday's coffee meeting(9/29), I think you need to change this line to

launch { onSingleTap(it) }

It is OK for this case, onSingleTap doesn't block anything

changanxian · 2023-09-29T18:47:11Z

show-callout/src/main/java/com/esri/arcgismaps/sample/showcallout/components/ComposeMapView.kt

+    )
+
+    // launch coroutine functions in the composition's CoroutineContext
+    LaunchedEffect(Unit) {


It is a little better to change this to LaunchedEffect(mapView)

it may be not necessary.

changanxian · 2023-10-03T17:36:12Z

@prupani-7 Looks good

prupani-7 · 2023-10-03T17:37:31Z

Thanks @changanxian @shubham7109 for your reviews. Merging...

prupani-7 added 3 commits September 25, 2023 13:16

Intial working app

1e67d7c

pass in application context

b1cfe8f

Readme and metadat and png

eae6aa9

shubham7109 assigned prupani-7 Sep 26, 2023

shubham7109 added the New sample New Kotlin sample using ArcGIS Maps SDK label Sep 26, 2023

shubham7109 self-requested a review September 26, 2023 18:07

shubham7109 requested changes Sep 26, 2023

View reviewed changes

add calloutView and tappedPoint as MapViewSTate property

62203e8

prupani-7 force-pushed the priy/show-callout branch from 83d5e1c to 62203e8 Compare September 26, 2023 22:26

PR feedback

aa66653

prupani-7 requested a review from shubham7109 September 26, 2023 23:14

shubham7109 approved these changes Sep 27, 2023

View reviewed changes

PR feedback

58f9972

changanxian assigned changanxian and unassigned changanxian Sep 27, 2023

changanxian self-requested a review September 27, 2023 15:39

changanxian reviewed Sep 27, 2023

View reviewed changes

changan PR feedback

625c73e

prupani-7 requested a review from changanxian September 27, 2023 20:05

changanxian reviewed Sep 29, 2023

View reviewed changes

changanxian approved these changes Oct 3, 2023

View reviewed changes

prupani-7 merged commit 5a9c517 into v.next Oct 3, 2023

prupani-7 deleted the priy/show-callout branch October 3, 2023 17:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Show Callout Sample #137

Show Callout Sample #137

prupani-7 commented Sep 25, 2023

shubham7109 left a comment

shubham7109 Sep 26, 2023

prupani-7 Sep 26, 2023 •

edited

Loading

prupani-7 commented Sep 26, 2023 •

edited

Loading

shubham7109 left a comment

shubham7109 Sep 26, 2023

prupani-7 Sep 27, 2023 •

edited

Loading

shubham7109 Sep 26, 2023

changanxian Sep 27, 2023

changanxian Sep 27, 2023

prupani-7 Sep 27, 2023 •

edited

Loading

changanxian Sep 27, 2023

changanxian Sep 27, 2023

prupani-7 Sep 27, 2023

changanxian Sep 27, 2023

prupani-7 Sep 28, 2023

changanxian Sep 27, 2023

prupani-7 Sep 27, 2023 •

edited

Loading

changanxian commented Sep 27, 2023

changanxian Sep 29, 2023

changanxian Oct 3, 2023

changanxian Sep 29, 2023

changanxian Oct 3, 2023

changanxian commented Oct 3, 2023

prupani-7 commented Oct 3, 2023

	onSingleTap = { singleTapConfirmedEvent ->
	mapViewModel.onMapTapped(singleTapConfirmedEvent.mapPoint)
	}

	// launch coroutine functions in the composition's CoroutineContext
	LaunchedEffect(Unit) {
	launch {
	mapView.onSingleTapConfirmed.collect {
	onSingleTap(it)
	}
	}
	}

		// create a textview for the callout
		mapViewState.value.calloutContent.text = application.getString(

Show Callout Sample #137

Show Callout Sample #137

Conversation

prupani-7 commented Sep 25, 2023

Description

Links and Data

What To Review

How to Test

shubham7109 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prupani-7 Sep 26, 2023 • edited Loading

Choose a reason for hiding this comment

prupani-7 commented Sep 26, 2023 • edited Loading

shubham7109 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prupani-7 Sep 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prupani-7 Sep 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prupani-7 Sep 27, 2023 • edited Loading

Choose a reason for hiding this comment

changanxian commented Sep 27, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

changanxian commented Oct 3, 2023

prupani-7 commented Oct 3, 2023

prupani-7 Sep 26, 2023 •

edited

Loading

prupani-7 commented Sep 26, 2023 •

edited

Loading

prupani-7 Sep 27, 2023 •

edited

Loading

prupani-7 Sep 27, 2023 •

edited

Loading

prupani-7 Sep 27, 2023 •

edited

Loading