Alan/generategeodatabasereplicafromfeatureservice migration #297

alan-edi · 2024-12-24T14:38:30Z

Description

Migrate the generate-geodatabase-replica-from-feature-service sample from XML to Compose.

Links and Data

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

What To Review

How to Test

Run the sample on the sample viewer or the repo.

…to Compose

…rting

…wnloadArea()

…ayouts

…database

…tadata

…service_migration

hud10837

Looks great, a few small comments

...java/com/esri/arcgismaps/sample/generategeodatabasereplicafromfeatureservice/MainActivity.kt

...eplicafromfeatureservice/components/GenerateGeodatabaseReplicaFromFeatureServiceViewModel.kt

hud10837 · 2024-12-30T11:40:37Z

...eplicafromfeatureservice/components/GenerateGeodatabaseReplicaFromFeatureServiceViewModel.kt

+     */
+    fun calculateDownloadArea() {
+        // upper left corner of the area to take offline
+        val minScreenPoint = ScreenCoordinate(200.0, 200.0)


Have we double checked if this is local or global device coordinates? If it is not local to the map view, we should make sure that it is

The doc for mapViewProxy.screenToLocationOrNull says it "Converts a screen coordinate (in pixels) to a coordinate within the mapview's spatial reference" so it seems OK to me, and it works fine.

I did some testing and also seems to work okay 👍

alan-edi

@hud10837 I've adopted all your suggestions apart from one. Please have another look.

alan-edi · 2024-12-30T16:33:39Z

...eplicafromfeatureservice/components/GenerateGeodatabaseReplicaFromFeatureServiceViewModel.kt

+     */
+    fun calculateDownloadArea() {
+        // upper left corner of the area to take offline
+        val minScreenPoint = ScreenCoordinate(200.0, 200.0)


The doc for mapViewProxy.screenToLocationOrNull says it "Converts a screen coordinate (in pixels) to a coordinate within the mapview's spatial reference" so it seems OK to me, and it works fine.

hud10837

LGTM 👍

01smito01

Have left some comments about handling UI state in a more idiomatic way, but all seems fine beyond that.

01smito01 · 2025-01-07T10:04:33Z

...odatabase-replica-from-feature-service/generate-geodatabase-replica-from-feature-service.png

Fiddled about with the bounding box so I could get a screenshot free of watermark/UI elements. Use it if you want:

Thanks, but actually I think it's better to show the UI elements, so I'm fairly happy with the screenshot I've got.

01smito01 · 2025-01-07T11:46:09Z

...eplicafromfeatureservice/components/GenerateGeodatabaseReplicaFromFeatureServiceViewModel.kt

+    // job progress dialog visibility state
+    var showJobProgressDialog by mutableStateOf(false)
+        private set
+
+    // job progress percentage
+    var jobProgress by mutableIntStateOf(0)
+        private set
+
+    // state variables indicating if the buttons are enabled
+    var resetButtonEnabled by mutableStateOf(false)
+        private set
+    var generateButtonEnabled by mutableStateOf(false)
+        private set


This all works and we can keep it the way it is, but I'm a bit sceptical of having so much Compose state in the ViewModel.

There's quite a few places in the VM where buttons or dialogues are enabled/disabled, which is a layer of implicit UI state that would be better to make explicit (EG as an enum class), and expose that to the UI through a StateFlow. But it could be a lot of work.

01smito01 · 2025-01-07T11:47:53Z

...eplicafromfeatureservice/components/GenerateGeodatabaseReplicaFromFeatureServiceViewModel.kt

+                geodatabaseSyncTask.load().onFailure { error ->
+                    messageDialogVM.showMessageDialog(
+                        title = "Failed to load GeodatabaseSyncTask",
+                        description = error.message.toString()
+                    )
+                }.onSuccess {
+                    generateButtonEnabled = true
+                }


Think this reads a bit cleaner if onSuccess and onFailure are swapped around.

01smito01 · 2025-01-07T11:48:56Z

...eplicafromfeatureservice/components/GenerateGeodatabaseReplicaFromFeatureServiceViewModel.kt

+     * Reset the map to its original state.
+     */
+    fun resetMap() {
+        // clear any layers  and symbols already on the map


Suggested change

// clear any layers and symbols already on the map

// clear any layers and symbols already on the map

01smito01 · 2025-01-07T12:06:22Z

...eplicafromfeatureservice/components/GenerateGeodatabaseReplicaFromFeatureServiceViewModel.kt

+        // create a flow-collection for the job's progress
+        viewModelScope.launch(Dispatchers.Main) {
+            job.progress.collect { progress ->
+                jobProgress = progress
+            }
+        }


Could expose job.progress to the UI directly as well - although there isn't always a generateGeodatabaseJob, so it may not be so simple.

01smito01 · 2025-01-07T12:18:38Z

...abasereplicafromfeatureservice/screens/GenerateGeodatabaseReplicaFromFeatureServiceScreen.kt

+                        .padding(16.dp),
+                    horizontalArrangement = Arrangement.SpaceEvenly
+                ) {
+                    // the Reset Map button


Think this comment (and the one for the other button) are a bit superfluous.

01smito01 · 2025-01-07T12:19:14Z

...abasereplicafromfeatureservice/screens/GenerateGeodatabaseReplicaFromFeatureServiceScreen.kt

+                // display progress dialog while generating a geodatabase replica
+                if (mapViewModel.showJobProgressDialog) {
+                    JobLoadingDialog(
+                        title = "Generating geodatabase replica...",


Could extract this string into strings.xml since all the other ones are.

…re onFailure

…& move a string to the resources file

…when ViewModel is destroyed

…riables by one UiState flow

alan-edi · 2025-01-13T14:00:03Z

Thanks @01smito01, good feedback. I've made changes as you suggested, including reworking the UI state handling. Please have another look.

01smito01

Looks good - a few comments on naming which I think could be clearer. The only issue is that I haven't figured out how.

01smito01 · 2025-01-13T16:58:53Z

...eplicafromfeatureservice/components/GenerateGeodatabaseReplicaFromFeatureServiceViewModel.kt

+    READY_FOR_GENERATE,
+    GENERATING,
+    REPLICA_DISPLAYED
+}


Add newline

01smito01 · 2025-01-14T10:07:01Z

...eplicafromfeatureservice/components/GenerateGeodatabaseReplicaFromFeatureServiceViewModel.kt

+
+enum class UiStatus {
+    STARTING,
+    READY_FOR_GENERATE,


Reads slightly odd to me, how about READY_TO_GENERATE or READY_FOR_GENERATION?

01smito01 · 2025-01-14T10:54:58Z

...eplicafromfeatureservice/components/GenerateGeodatabaseReplicaFromFeatureServiceViewModel.kt

+    // state flow to expose current UI state to UI
+    private val _uiStateFlow = MutableStateFlow(UiState(status = UiStatus.STARTING))
+    val uiStateFlow = _uiStateFlow.asStateFlow()


There are a lot of things called uiState/status here - wonder if some of them could be renamed for clarity.

Not 100% sure what to do: maybe UiState → GenerateGeodatabaseJobStatus or UiStatus → GeodatabaseGenerationStatus?

Think the comment could be rewritten too - surely we're exposing application state to the UI, not UI state? UI state is the product of whatever data we pass to the UI.

01smito01 · 2025-01-14T11:08:13Z

...abasereplicafromfeatureservice/screens/GenerateGeodatabaseReplicaFromFeatureServiceScreen.kt

                    Button(
                        onClick = {
                            mapViewModel.resetMap()
                        },
-                        enabled = mapViewModel.resetButtonEnabled
+                        enabled = uiState.status == UiStatus.REPLICA_DISPLAYED


On naming: uiState.status feels particularly redundant/confusing.

… AppStatus

alan-edi · 2025-01-14T12:55:37Z

@01smito01 I've renamed READY_FOR_GENERATE to READY_TO_GENERATE, and UiStatus to AppStatus.

As for UiState, that seems to me to be valid usage. The Compose docs show several examples where a ViewModel holds a UiState, for example in:

and the UI layer guide doc, when talking about State Holders, includes several diagrams showing the ViewModel holding the UI state.

Please have another look.

01smito01

LGTM 👍

alan-edi · 2025-01-14T15:02:24Z

Thanks @01smito01

alan-edi added 9 commits December 18, 2024 16:03

generate-geodatabase-replica-from-feature-service: initial migration …

e690b96

…to Compose

generate-geodatabase-replica-from-feature-service: improve error repo…

9ba05f7

…rting

generate-geodatabase-replica-from-feature-service: delete TODOs

0b08389

generate-geodatabase-replica-from-feature-service: rename calculateDo…

ebd27dc

…wnloadArea()

generate-geodatabase-replica-from-feature-service: delete redundant l…

237df47

…ayouts

generate-geodatabase-replica-from-feature-service: unregister the geo…

b3cd64a

…database

generate-geodatabase-replica-from-feature-service: update README & me…

6aff845

…tadata

generate-geodatabase-replica-from-feature-service: update screenshot

a5111cc

Merge branch 'v.next' into alan/generategeodatabasereplicafromfeature…

a3747c3

…service_migration

hud10837 reviewed Dec 30, 2024

View reviewed changes

alan-edi commented Dec 30, 2024

View reviewed changes

alan-edi requested a review from hud10837 December 30, 2024 16:58

generate-geodatabase-replica-from-feature-service: code review changes

f430f2d

hud10837 approved these changes Dec 31, 2024

View reviewed changes

generate-geodatabase-replica-from-feature-service: update screenshot

f2f9bcb

01smito01 approved these changes Jan 7, 2025

View reviewed changes

alan-edi added 4 commits January 7, 2025 14:25

generate-geodatabase-replica-from-feature-service: put onSuccess befo…

619ed28

…re onFailure

generate-geodatabase-replica-from-feature-service: fix some comments …

0c7ec96

…& move a string to the resources file

generate-geodatabase-replica-from-feature-service: close geodatabase …

bfa10e8

…when ViewModel is destroyed

generate-geodatabase-replica-from-feature-service: replace 4 state va…

7f87a0e

…riables by one UiState flow

alan-edi requested a review from 01smito01 January 13, 2025 14:00

01smito01 approved these changes Jan 14, 2025

View reviewed changes

generate-geodatabase-replica-from-feature-service: rename UiStatus to…

53d4eda

… AppStatus

alan-edi requested a review from 01smito01 January 14, 2025 12:55

01smito01 approved these changes Jan 14, 2025

View reviewed changes

alan-edi merged commit d4610d2 into v.next Jan 14, 2025
1 check passed

alan-edi deleted the alan/generategeodatabasereplicafromfeatureservice_migration branch January 14, 2025 15:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Alan/generategeodatabasereplicafromfeatureservice migration #297

Alan/generategeodatabasereplicafromfeatureservice migration #297

alan-edi commented Dec 24, 2024 •

edited

Loading

hud10837 left a comment

hud10837 Dec 30, 2024

alan-edi Dec 30, 2024

hud10837 Dec 31, 2024

alan-edi left a comment

alan-edi Dec 30, 2024

hud10837 left a comment

01smito01 left a comment

01smito01 Jan 7, 2025

alan-edi Jan 7, 2025

01smito01 Jan 7, 2025

01smito01 Jan 7, 2025

01smito01 Jan 7, 2025

01smito01 Jan 7, 2025

01smito01 Jan 7, 2025

01smito01 Jan 7, 2025

alan-edi commented Jan 13, 2025

01smito01 left a comment

01smito01 Jan 13, 2025

01smito01 Jan 14, 2025

01smito01 Jan 14, 2025

01smito01 Jan 14, 2025

alan-edi commented Jan 14, 2025

01smito01 left a comment

alan-edi commented Jan 14, 2025

	// clear any layers and symbols already on the map
	// clear any layers and symbols already on the map

Alan/generategeodatabasereplicafromfeatureservice migration #297

Alan/generategeodatabasereplicafromfeatureservice migration #297

Conversation

alan-edi commented Dec 24, 2024 • edited Loading

Description

Links and Data

What To Review

How to Test

hud10837 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

alan-edi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hud10837 left a comment

Choose a reason for hiding this comment

01smito01 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alan-edi commented Jan 13, 2025

01smito01 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

alan-edi commented Jan 14, 2025

01smito01 left a comment

Choose a reason for hiding this comment

alan-edi commented Jan 14, 2025

alan-edi commented Dec 24, 2024 •

edited

Loading