Skip to content

Change the logic of handling geojson update#655

Closed
Chaoba wants to merge 3 commits intomainfrom
kl-geojson-update
Closed

Change the logic of handling geojson update#655
Chaoba wants to merge 3 commits intomainfrom
kl-geojson-update

Conversation

@Chaoba
Copy link
Copy Markdown
Contributor

@Chaoba Chaoba commented Sep 22, 2021

PRs must be submitted under the terms of our Contributor License Agreement CLA.
Fixes: #381

Pull request checklist:

  • Briefly describe the changes in this PR.
  • Include before/after visuals or gifs if this PR includes visual changes.
  • Write tests for all new functionality. If tests were not written, please explain why.
  • Add example if relevant.
  • Document any changes to public APIs.
  • Apply changelog label ('breaking change', 'bug 🪲', 'build', 'docs', 'feature 🍏', 'performance ⚡', 'testing 💯') or use the label 'skip changelog'
  • Add an entry inside this element for inclusion in the mapbox-maps-android changelog: <changelog>Change the logic of handling geojson update</changelog>.

Summary of changes

Introduced a HashSet to record the data that need to update for each GeojsonSource to avoid the issue that aync data update reset ignoreParsedGeoJsonRegistry tag.

User impact (optional)

@Chaoba Chaoba self-assigned this Sep 22, 2021
@Chaoba Chaoba requested a review from a team September 22, 2021 07:26
@Chaoba Chaoba added the bug 🪲 Something isn't working label Sep 22, 2021
@kiryldz
Copy link
Copy Markdown
Contributor

kiryldz commented Sep 22, 2021

@Chaoba please add a test for this scenario

@Chaoba
Copy link
Copy Markdown
Contributor Author

Chaoba commented Sep 28, 2021

@kiryldz Add a test case for this scenario. We can't verify the sync set data in this test case, however, we have other cases to verify sync set data, so IMO we can skip this part.

) : this(builder) {
rawGeoJson?.let {
ignoreParsedGeoJsonRegistry[sourceId] = false
val dataKey = sourceId + it.hashCode()
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

why can't it be simply hash code?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

If set the same data to two different sources, they will affect each other.

fun data(value: String) = apply {
ignoreParsedGeoJsonRegistry[sourceId] = true
// Clear all asynchronous data
updateParsedGeoJsonRegistry.clear()
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

shouldn't actually it be the whole fix?
why do we need a set instead of the hash map?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The set contains those data that need to be updated to source, no need additional true or false values for them. In other words, those data that are not in the set will not be updated to the source

Copy link
Copy Markdown
Contributor

@kiryldz kiryldz left a comment

Choose a reason for hiding this comment

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

@Chaoba this PR will become redundant when upcoming gl-native release will land as parsing geojson synchronously will be removed.
Let's leave it open for now though.

@kiryldz
Copy link
Copy Markdown
Contributor

kiryldz commented Oct 5, 2021

not relevant as #699 landed.

@kiryldz kiryldz closed this Oct 5, 2021
@kiryldz kiryldz deleted the kl-geojson-update branch October 5, 2021 13:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug 🪲 Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Do not apply geojson async setting data in specific scenario

3 participants