Update project geometry sample #292
Conversation
Optimize imports, use by for infotext
There was a problem hiding this comment.
@darryl-lynch Looks good! One very minor comment below. Also, the epic that this issue belongs to mentions a couple of extra things that are supposed to be done to each of the samples we're migrating:
- Add the line to the "Additional information section":
This sample uses the GeoCompose Toolkit module to be able to implement a Composable MapView. - Add the tags:
geoviewcompose, toolkitto readme and metadata.json file
| width = 30f | ||
| height = 30f | ||
| // offset in +y axis so the marker spawned | ||
| // is right on the touch point |
There was a problem hiding this comment.
Could put this comment all on one line
There was a problem hiding this comment.
I've done these, but as per this discussion it seems like there may be some question over the suitability of the of geoviewcompose tag
| @@ -5,8 +5,7 @@ | |||
|
|
|||
| <application><activity | |||
| android:exported="true" | |||
| android:name=".MainActivity" | |||
| android:label="@string/project_geometry_app_name"> | |||
There was a problem hiding this comment.
Just come upon this statement in the Kotlin Samples Contribution Guide:
The sample viewer will only correctly name each individual sample if
android:label="@string/app_name"is contained within the activity. So please make sure that in addition to any other lines, your activity contains the label. For example:
<activity
android:name=".MainActivity"
android:label="@string/app_name">
There was a problem hiding this comment.
That's interesting. The sample seems to be named correctly in the sample viewer without it, and I checked a few other compose based samples and they seem to lack it as well. I wonder if perhaps that document is outdated or if there is some other mechanism at play that I've missed?
There was a problem hiding this comment.
Good questions. I've posted a comment about it here: https://devtopia.esri.com/runtime/kotlin/issues/5111
Add keywords/tags
Appease the style checkers
darryl-lynch
force-pushed
the
project-geometry-sample
branch
from
4bbe9c6 to
33eeeba
Compare
There was a problem hiding this comment.
@darryl-lynch Looks good 👍 . Just one little typo to fix, but then ready for second review.
.../main/java/com/esri/arcgismaps/sample/projectgeometry/components/ProjectGeometryViewModel.kt
Outdated
Show resolved
Hide resolved
| "Failed to load map", | ||
| error.message.toString() |
There was a problem hiding this comment.
Comment from Shubham on one of my PRs at the moment - use named parameters for function calls with more than 1 argument. It helps people looking at samples via Github/the website understand what each parameter is.
| "Failed to load map", | |
| error.message.toString() | |
| title = "Failed to load map", | |
| description = error.message.toString() |
Android Studio has a handy context action to do it.
There was a problem hiding this comment.
There might be a few other places where you could do this, have a look.
| Text( | ||
| text = | ||
| stringResource(R.string.title_text) + "\n" + | ||
| infoText | ||
| ) |
There was a problem hiding this comment.
The "coordinates" text probably shouldn't be there until we have coordinates to display.
When the user taps on the map, this text goes from two lines to three, causing the MapView to change size which is a bit jarring. Use the minLines parameter on Text composables to avoid that.
| _infoText.value = app.resources.getString( | ||
| R.string.projection_info_text, | ||
| point.toDisplayFormat(), | ||
| projectedPoint?.toDisplayFormat() |
There was a problem hiding this comment.
This sort of string interpolation is essentially a UI task, so we should do it in the MainScreen.
| /** | ||
| * Extension function for the Point type that returns | ||
| * a float-precision formatted string suitable for display | ||
| */ | ||
| private fun Point.toDisplayFormat() = | ||
| "${String.format(Locale.getDefault(),"%.5f", x)}, ${String.format(Locale.getDefault(),"%.5f", y)}" |
There was a problem hiding this comment.
Move this extension function to the main screen.
| private val _infoText = MutableStateFlow(app.resources.getString(R.string.tap_to_begin)) | ||
| val infoText: StateFlow<String> = _infoText |
There was a problem hiding this comment.
I don't think we should be formatting the text in the viewmodel if we can do it in the main screen - and we can. We should move text formatting concerns to the screen, and instead expose the point and its projection from the view model.
| private val _infoText = MutableStateFlow(app.resources.getString(R.string.tap_to_begin)) | |
| val infoText: StateFlow<String> = _infoText | |
| // state flow of a point and its projection for presentation in UI | |
| private val _pointProjectionFlow = MutableStateFlow<PointProjection?>(null) | |
| val pointFlow = _pointProjectionFlow.asStateFlow() |
This uses a very simple data class:
data class PointProjection (val original: Point, val projection: Point)
.../main/java/com/esri/arcgismaps/sample/projectgeometry/components/ProjectGeometryViewModel.kt
Show resolved
Hide resolved
Description
Migrate the project geometry sample from XML to Compose
Links and Data
Issue: https://devtopia.esri.com/runtime/kotlin/issues/4978
vTest: https://runtime-kotlin.esri.com/view/all/job/vtest/job/sampleviewer/63/
How to Test
Run the sample on the sample viewer or the repo.