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.

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

Migrate "Query features with Arcade expression" to compose #294

Open
wants to merge 4 commits into
base: v.next
Choose a base branch
from

Conversation

01smito01
Copy link
Collaborator

Description

Query features with Arcade expression, but compose

Links and Data

Old version: here
Sample design: here
Sample Epic: #4961

What To Review

  • Review the code to make sure it is easy to follow like other samples on Android
  • README.md makes sense
  • Compose used sensibly

How to Test

Run the sample on the sample viewer.

@01smito01 01smito01 force-pushed the oliver/arcade_query_compose branch from 801e4ec to 268b660 Compare December 17, 2024 15:41
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.

@01smito01 looks good, but I've run into a bug with the layer not being set

}

_queryStateFlow.value = QueryState(arcadeEvaluationResult.result as Double, LoadState.LOADED)
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

if policeBeatsLayer is null, then the load state never gets set back to LOADED

Comment on lines 101 to 103
policeBeatsLayer = arcGISMap.operationalLayers.firstOrNull { layer ->
layer.id == "RPD_Reorg_9254"
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this needs to go in the coroutine scope above, because if the map is not loaded then the layer remains null

@01smito01 01smito01 requested a review from hud10837 December 20, 2024 11:34
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.

LGTM, app is now working on my device

}

// update query state, map is ready for user interaction
_queryStateFlow.value = QueryState()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Little Nitpick: I think it's a bit clearer to explicitly set this to READY_TO_START here rather than leave it as a default value

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I swithered a bit on passing READY_TO_START explicitly, but it does make things clearer. I'll add it back in.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants