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.

Sign up for GitHub

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

Jump to bottom

[New Sample] Add ENC exchange set #238

Merged
merged 15 commits into from
Oct 11, 2024
Merged

[New Sample] Add ENC exchange set #238

merged 15 commits into from
Oct 11, 2024

Conversation

01smito01
Copy link
Collaborator

@01smito01 01smito01 commented Oct 8, 2024 •
edited
Loading

Description

PR to add a new Kotlin sample "Add ENC exchange set" in Layers category.

Links and Data

Sample Epic: #4571

  • a vTest Job for this PR has been run

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 make sense

How to Test

Run the sample on the sample viewer or the repo.

To Discuss

  • Is this the right way to handle downloadable data & resources?
  • Is there a more elegant way to handle extent and dataset.extent both needing to be non-null for GeometryEngine?
  • Very broadly, what's the idiomatic way of handling "we need to do some async stuff before showing a map"?

@01smito01 01smito01 added the New sample New Kotlin sample using ArcGIS Maps SDK label Oct 9, 2024
@01smito01 01smito01 self-assigned this Oct 9, 2024
@01smito01 01smito01 requested a review from hud10837 October 9, 2024 13:01
hud10837
hud10837 reviewed Oct 9, 2024
View reviewed changes
Copy link
Collaborator

@hud10837 hud10837 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, just a couple questions

...ge-set/src/main/java/com/esri/arcgismaps/sample/addencexchangeset/components/MapViewModel.kt Outdated
private val encEnvironmentSettings: EncEnvironmentSettings = EncEnvironmentSettings

// Create a map with the oceans basemap style
val arcGISMap by mutableStateOf(ArcGISMap(BasemapStyle.ArcGISOceans))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if you use a delegated property with mutable state, it should be var, otherwise if it's not going to change you can just expose the value directly

...ge-set/src/main/java/com/esri/arcgismaps/sample/addencexchangeset/components/MapViewModel.kt Outdated
Comment on lines 68 to 69
arcGISMap.initialViewpoint = encExchangeSet.extentOrNull()?.let { extent ->
Viewpoint(extent)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible for this to run after the map has been displayed? What happens when the initial viewpoint is set after the map is displayed?

Copy link
Collaborator Author

@01smito01 01smito01 Oct 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes to the initial viewpoint are ignored after a map is added into a GeoView. Since this is in a launched block I'd expect that yes, this could be applied (and ignored) after MapViewModel initialisation has otherwise completed and the map gets added to the MapView.

In practice this works just fine, but I can revert to updating the viewpoint through a MapViewProxy if this is too flimsy.

hud10837
hud10837 approved these changes Oct 10, 2024
View reviewed changes
Copy link
Collaborator

@hud10837 hud10837 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think an empty map makes sense as an initial value, since it won't have any discernible locations for the user to be panning around 👍

@01smito01 01smito01 requested a review from shubham7109 October 10, 2024 15:24
@01smito01
Copy link
Collaborator Author

Thanks @hud10837. @shubham7109 can you give this a 2nd review?

shubham7109
shubham7109 requested changes Oct 11, 2024
View reviewed changes
Copy link
Collaborator

@shubham7109 shubham7109 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good @01smito01! Just a few general comments:

add-enc-exchange-set/README.md Outdated Show resolved Hide resolved
...-exchange-set/src/main/java/com/esri/arcgismaps/sample/addencexchangeset/DownloadActivity.kt Outdated Show resolved Hide resolved
...ge-set/src/main/java/com/esri/arcgismaps/sample/addencexchangeset/components/MapViewModel.kt Outdated Show resolved Hide resolved
...xchange-set/src/main/java/com/esri/arcgismaps/sample/addencexchangeset/screens/MainScreen.kt Show resolved Hide resolved
@01smito01
Copy link
Collaborator Author

Think next time I'll bother to run the style checker locally before pushing 🙃.

shubham7109
shubham7109 approved these changes Oct 11, 2024
View reviewed changes
Copy link
Collaborator

@shubham7109 shubham7109 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ran a new vTest build with the latest changes, and the sample runs as expected in the SV.

Thanks @01smito01 😄

@01smito01 01smito01 merged commit 65615d9 into v.next Oct 11, 2024
1 check passed
@01smito01 01smito01 deleted the oliver/add-enc-exchange-set branch October 11, 2024 16:27
@shubham7109 shubham7109 mentioned this pull request Oct 24, 2024
Merged
shubham7109 added a commit that referenced this pull request Nov 4, 2024
@shubham7109
Squashed commit of the following:
be609d9 
commit 65615d9
Merge: e33cffb d2d35d8
Author: Oliver Smith <67012037+01smito01@users.noreply.github.com>
Date:   Fri Oct 11 17:27:40 2024 +0100

    [New Sample] Add ENC exchange set (#238)

commit d2d35d8
Author: Oliver Smith <osmith@esri.com>
Date:   Fri Oct 11 16:38:18 2024 +0100

    Metadata fix (iii)

commit b237f01
Author: Oliver Smith <osmith@esri.com>
Date:   Fri Oct 11 16:36:33 2024 +0100

    Metadata fix (ii)

commit 1dd63c7
Author: Oliver Smith <osmith@esri.com>
Date:   Fri Oct 11 16:33:19 2024 +0100

    Metadata fix

commit 5399d01
Author: Oliver Smith <osmith@esri.com>
Date:   Fri Oct 11 16:31:18 2024 +0100

    Changes after review

commit eca935e
Author: Oliver Smith <osmith@esri.com>
Date:   Thu Oct 10 15:40:04 2024 +0100

    Revise comments

commit dd81e43
Author: Oliver Smith <osmith@esri.com>
Date:   Thu Oct 10 15:20:46 2024 +0100

    set map and viewpoint after ENC extent calculated

commit 9943441
Author: Oliver Smith <osmith@esri.com>
Date:   Wed Oct 9 16:13:14 2024 +0100

    Removed mutableStateOf from arcGISMap

commit c5c2edc
Author: Oliver Smith <osmith@esri.com>
Date:   Wed Oct 9 13:52:06 2024 +0100

    Rename things, change doc

commit f83bc4a
Author: Oliver Smith <osmith@esri.com>
Date:   Wed Oct 9 12:37:20 2024 +0100

    Replace lateinit w/ null checks, refactoring of extent calculation

commit bff9512
Author: Oliver Smith <osmith@esri.com>
Date:   Wed Oct 9 10:23:41 2024 +0100

    Fix manifest so sample works in viewer

commit 4d44cbf
Author: Oliver Smith <osmith@esri.com>
Date:   Tue Oct 8 16:45:48 2024 +0100

    Style checker appeasement II

commit 6ee456f
Author: Oliver Smith <osmith@esri.com>
Date:   Tue Oct 8 16:43:33 2024 +0100

    Style checker appeasement

commit 79561bd
Author: Oliver Smith <osmith@esri.com>
Date:   Tue Oct 8 16:12:10 2024 +0100

    Refactor, comments, metadata, etc.

commit cb93810
Author: Oliver Smith <osmith@esri.com>
Date:   Tue Oct 8 13:58:27 2024 +0100

    Sample working

commit 5e17c53
Author: Oliver Smith <osmith@esri.com>
Date:   Fri Oct 4 15:46:13 2024 +0100

    Create "Add ENC Exchange Set" sample
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shubham7109 shubham7109 shubham7109 approved these changes

@hud10837 hud10837 hud10837 approved these changes

Assignees

@shubham7109 shubham7109

@hud10837 hud10837

@01smito01 01smito01

Labels
New sample New Kotlin sample using ArcGIS Maps SDK
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@01smito01 @shubham7109 @hud10837