Migrate "Add Features with Contingent Values" to Compose #283

01smito01 · 2024-12-02T17:34:13Z

Description

Migrates Add Features with Contingent Values sample from XML/views to 💫 Compose 💫

Links and Data

Sample Epic: #4961

a vTest Job for this PR has been run
- link: https://runtime-kotlin.esri.com/view/all/job/vtest/job/sampleviewer/55/

What To Review

Sample works, doesn't crash, etc. You shouldn't be able to select an invalid combination of values from the bottom sheet - you may still get failures if a value isn't assigned.
Is the code easy to follow? Is the level of function extraction good? Are things sensibly grouped together in code?
Is exposing flows from the view model a good design decision (vs. putting compose mutableStateOf's in the view model)
Slider implementation is a bit messy (needs to be responsive to user input, but also react to feature changes in the view model). Can this be simplified?
Does the Readme still make sense?

How to Test

Run the sample on the sample viewer.

samples/add-features-with-contingent-values/README.md

gunt0001 · 2024-12-06T15:58:49Z

samples/add-features-with-contingent-values/README.md

 Learn more about contingent values and how to utilize them on the [ArcGIS Pro documentation](https://pro.arcgis.com/en/pro-app/latest/help/data/geodatabases/overview/contingent-values.htm).

 ## Tags

-coded values, contingent values, feature table, geodatabase
+coded values, contingent values, feature table, geodatabase, geoviewcompose, toolkit


Suggested change

coded values, contingent values, feature table, geodatabase, geoviewcompose, toolkit

coded values, contingent values, feature table, geodatabase, geoview, mapview, compose, toolkit

geoviewcompose and toolkit are the tags the epic recommends to add. I don't think they need to be changed.

I find geoviewcompose odd as a tag but maybe there is a good reason for it. \cc @shubham7109

@gunt0001 The tag here was meant to match the geoviewcompose Toolkit package name. I see it as little odd too, and I recall tags must use lowercase. Alternatively, a valid option could be to use geoview-compose to match the Toolkit module name instead. (Quick test looks like the PR checker would be happy.)

\cc @TADraeseke Thoughts?

...c/main/java/com/esri/arcgismaps/sample/addfeatureswithcontingentvalues/screens/MainScreen.kt

...n/java/com/esri/arcgismaps/sample/addfeatureswithcontingentvalues/components/MapViewModel.kt

...c/main/java/com/esri/arcgismaps/sample/addfeatureswithcontingentvalues/screens/MainScreen.kt

gunt0001 · 2024-12-12T15:18:14Z

samples/add-features-with-contingent-values/README.md

 Learn more about contingent values and how to utilize them on the [ArcGIS Pro documentation](https://pro.arcgis.com/en/pro-app/latest/help/data/geodatabases/overview/contingent-values.htm).

 ## Tags

-coded values, contingent values, feature table, geodatabase
+coded values, contingent values, feature table, geodatabase, geoviewcompose, toolkit


I find geoviewcompose odd as a tag but maybe there is a good reason for it. \cc @shubham7109

...n/java/com/esri/arcgismaps/sample/addfeatureswithcontingentvalues/components/MapViewModel.kt

samples/add-features-with-contingent-values/README.md

...n/java/com/esri/arcgismaps/sample/addfeatureswithcontingentvalues/components/MapViewModel.kt

gunt0001 · 2024-12-13T10:32:51Z

...n/java/com/esri/arcgismaps/sample/addfeatureswithcontingentvalues/components/MapViewModel.kt

+            feature?.geometry = mapPoint
+
+            // create the graphic of the feature
+            val graphic = feature?.let { createGraphic(it) }


Is this the buffer graphic?

...n/java/com/esri/arcgismaps/sample/addfeatureswithcontingentvalues/components/MapViewModel.kt

gunt0001

@01smito01 - a few more comments.
Note, after I added a new feature, then rotate the screen, the buffer graphic for the added feature disappears, I only see the black dot. Is this expected?

...n/java/com/esri/arcgismaps/sample/addfeatureswithcontingentvalues/components/MapViewModel.kt

gunt0001 · 2024-12-17T12:10:48Z

...n/java/com/esri/arcgismaps/sample/addfeatureswithcontingentvalues/components/MapViewModel.kt

+            // add the feature to the feature table
+            viewModelScope.launch {
+                feature?.let { featureTable!!.addFeature(it) }
+                feature?.load()?.getOrElse {


Technically there is no need to load the feature explicitly, it will be loaded automatically when rendered in the MapView. Is this more for debugging purposes?

The add-then-load logic was copied over from the old version of the sample. Probably was for some debugging purpose - getOrElse makes a lot less sense to use than onFailure. Have removed it at any rate.

gunt0001 · 2024-12-17T12:12:35Z

...n/java/com/esri/arcgismaps/sample/addfeatureswithcontingentvalues/components/MapViewModel.kt

+     */
+    fun clearFeature() {
+        feature = null
+        _featureEditState.value = FeatureEditState(statusAttributes = featureTable!!.statusFieldCodedValues())


avoid !!. Couldldn't you just use default parameters here?

Suggested change

_featureEditState.value = FeatureEditState(statusAttributes = featureTable!!.statusFieldCodedValues())

_featureEditState.value = FeatureEditState()

Default parameters include an empty statusAttributes list, which is populated during init (after the contingentValuesDefinition is loaded from the feature table). So none of the UI elements would have any options available.

How about featureTable?.let{ ... } ?: return messageDialogVM.showMessage("Feature table not loaded")?

Could alternatively create a method to reload the statusAttributes, called in the mapView's onSingleTapConfirmed.

gunt0001 · 2024-12-17T13:33:24Z

...c/main/java/com/esri/arcgismaps/sample/addfeatureswithcontingentvalues/screens/MainScreen.kt

+                            mapViewModel.clearFeature()
+                        }
+                    ) {
+                        val onBottomSheetStateChanged = { state: SheetState ->


Is there a need for this local variable? This may actually have to be remembered. Alternatively, pass it inline to BottomSheetContents:

BottomSheetContents( ... onBottomSheetStateChange = { state: SheetState -> if (!state.isVisible) { showBottomSheet = false mapViewModel.clearFeature() } }, ... )

Doesn't need to be local rather than inline - I had it that way for clarity. Will change if you think that's better.

No strict need to remember it, as it's not getting assigned a new value anywhere else. Could remember it for performance, but I don't think it's complex enough to make any appreciable difference.

...c/main/java/com/esri/arcgismaps/sample/addfeatureswithcontingentvalues/screens/MainScreen.kt

...n/java/com/esri/arcgismaps/sample/addfeatureswithcontingentvalues/components/MapViewModel.kt

gunt0001

@01smito01 - looks great, one minor comment 👍

...n/java/com/esri/arcgismaps/sample/addfeatureswithcontingentvalues/components/MapViewModel.kt

01smito01 · 2024-12-18T17:11:32Z

Thanks @gunt0001. @shubham7109 can you give this a second review? This is a fairly involved sample.

shubham7109

Reviewed the Kotlin code files, didn't get through with the readme changes yet.

Hope @01smito01 you find these comments helpful, feel free to reach out if you would like me to chat about them more in detail. Sample looks and works great, my main goal is to help improve readability and reduce any complexity in the app.

Feel free to use these review notes to help you with this PR: oliver/contingent_values_migration...shubham/contingent-review

...n/java/com/esri/arcgismaps/sample/addfeatureswithcontingentvalues/components/MapViewModel.kt

...c/main/java/com/esri/arcgismaps/sample/addfeatureswithcontingentvalues/screens/MainScreen.kt

samples/add-features-with-contingent-values/src/main/res/values/strings.xml

shubham7109 · 2024-12-19T02:11:03Z

@01smito01 I also noticed that createGeodatabaseCacheDir is not working as expected, since I can see the Geodatabase changes between sample sessions. Ideally, they should be deleted and placed back in the cache on every launch. Didn't get the chance to look into this.

01smito01 · 2024-12-19T17:32:23Z

Thanks for the review @shubham7109 - have implemented most of your changes, so let me know what you think.
I've not had the time to deal with the database cache issue yet - can take a look tomoz.

Migrate "Add Features with Contingent Values" to Compose

88b1bf9

01smito01 force-pushed the oliver/contingent_values_migration branch from e5f1361 to 88b1bf9 Compare December 2, 2024 17:41

Visibility, names, and formatting

1210b08

01smito01 marked this pull request as ready for review December 3, 2024 12:23

gunt0001 assigned 01smito01 Dec 6, 2024

gunt0001 reviewed Dec 6, 2024

View reviewed changes

Oliver Smith added 2 commits December 11, 2024 10:03

Changes after PR review

b99f10b

Metadata

6e20e47

01smito01 requested a review from gunt0001 December 11, 2024 10:41

gunt0001 reviewed Dec 13, 2024

View reviewed changes

Changes after PR review

cab5643

01smito01 requested a review from gunt0001 December 16, 2024 16:22

gunt0001 reviewed Dec 17, 2024

View reviewed changes

Changes after PR review

98ebd87

01smito01 requested a review from gunt0001 December 18, 2024 12:05

gunt0001 approved these changes Dec 18, 2024

View reviewed changes

...n/java/com/esri/arcgismaps/sample/addfeatureswithcontingentvalues/components/MapViewModel.kt Outdated Show resolved Hide resolved

darryl-lynch mentioned this pull request Dec 18, 2024

Update project geometry sample #292

Open

01smito01 requested a review from shubham7109 December 18, 2024 17:10

shubham7109 requested changes Dec 19, 2024

View reviewed changes

Changes after PR review

2ccdd6d

shubham7109 mentioned this pull request Dec 23, 2024

Update sample to match contribution guidelines #296

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate "Add Features with Contingent Values" to Compose #283

Migrate "Add Features with Contingent Values" to Compose #283

01smito01 commented Dec 2, 2024 •

edited

Loading

gunt0001 Dec 6, 2024

01smito01 Dec 11, 2024 •

edited

Loading

gunt0001 Dec 12, 2024

shubham7109 Dec 19, 2024 •

edited

Loading

gunt0001 Dec 12, 2024

gunt0001 Dec 13, 2024

gunt0001 left a comment

gunt0001 Dec 17, 2024

01smito01 Dec 18, 2024

gunt0001 Dec 17, 2024

01smito01 Dec 17, 2024

gunt0001 Dec 17, 2024

01smito01 Dec 18, 2024

gunt0001 left a comment

01smito01 commented Dec 18, 2024

shubham7109 left a comment •

edited

Loading

shubham7109 commented Dec 19, 2024 •

edited

Loading

01smito01 commented Dec 19, 2024

	coded values, contingent values, feature table, geodatabase, geoviewcompose, toolkit
	coded values, contingent values, feature table, geodatabase, geoview, mapview, compose, toolkit

	_featureEditState.value = FeatureEditState(statusAttributes = featureTable!!.statusFieldCodedValues())
	_featureEditState.value = FeatureEditState()

Migrate "Add Features with Contingent Values" to Compose #283

Are you sure you want to change the base?

Migrate "Add Features with Contingent Values" to Compose #283

Conversation

01smito01 commented Dec 2, 2024 • edited Loading

Description

Links and Data

What To Review

How to Test

Choose a reason for hiding this comment

01smito01 Dec 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shubham7109 Dec 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gunt0001 left a 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

Choose a reason for hiding this comment

gunt0001 left a comment

Choose a reason for hiding this comment

01smito01 commented Dec 18, 2024

shubham7109 left a comment • edited Loading

Choose a reason for hiding this comment

shubham7109 commented Dec 19, 2024 • edited Loading

01smito01 commented Dec 19, 2024

01smito01 commented Dec 2, 2024 •

edited

Loading

01smito01 Dec 11, 2024 •

edited

Loading

shubham7109 Dec 19, 2024 •

edited

Loading

shubham7109 left a comment •

edited

Loading

shubham7109 commented Dec 19, 2024 •

edited

Loading