Skip to content
This repository has been archived by the owner on Aug 8, 2023. It is now read-only.

[ios] Added "pending" completion blocks #15122

Merged
merged 5 commits into from
Aug 25, 2019

Conversation

julianrex
Copy link
Contributor

@julianrex julianrex commented Jul 15, 2019

Draft PR to experiment with pending completion blocks.

Without this several annotation selection integration tests were failing:

This is because the completion blocks introduced in #14381 are called before the annotation view's positions have been updated.

I think the user's expectation will be that when the completion block has been called, that the annotations are in their correct and updated position.

This same mechanism could be use for camera/region changes, but we need to investigate @pozdnyakov's #15086 first.

@friedbunny would love your thoughts here.

@julianrex julianrex added iOS Mapbox Maps SDK for iOS annotations Annotations on iOS and macOS or markers on Android labels Jul 15, 2019
@julianrex julianrex added needs changelog Indicates PR needs a changelog entry prior to merging. tests labels Jul 15, 2019
@julianrex julianrex added this to the release-q milestone Jul 16, 2019
@julianrex
Copy link
Contributor Author

Another FYI @astojilj

@astojilj
Copy link
Contributor

@julianrex

I think the user's expectation will be that when the completion block has been called, that the annotations are in their correct and updated position.

That would be convenient to user, but don't think we can guarantee it now in general case,... we would need to wait for all the tiles to get loaded and collision index updated.

Given the multithreading plans (placement, render, upload, split to threads), I don't think we can count that the approach with pending completion would work with the failing MGLAnnotationViewIntegrationTests.testSelectionMove*** tests.

How about using dispatch_group to wait in internalTestOffscreenSelectionTitle?

@julianrex
Copy link
Contributor Author

@astojilj is there a callback when the collision index is updated?

@julianrex
Copy link
Contributor Author

@friedbunny @astojilj Thinking about this further - I believe this is sufficient for view annotations.

@friedbunny would you mind checking my logic please?

@friedbunny
Copy link
Contributor

Conceptually this looks OK to me, but do we have a way of verifying this in tests?

@astojilj
Copy link
Contributor

@astojilj is there a callback when the collision index is updated?

We could add that - would it be preferred.

@friedbunny @astojilj Thinking about this further - I believe this is sufficient for view annotations.

@julianrex , I agree - and we'll have all integration tests passing to verify it.

@julianrex
Copy link
Contributor Author

julianrex commented Aug 22, 2019

@astojilj is there a callback when the collision index is updated?

We could add that - would it be preferred.

See #15440 for @pozdnyakov's POC

Conceptually this looks OK to me, but do we have a way of verifying this in tests?

Yep, this fixes the 3 failing integration tests (testSelectionMoveIntoView<XXX>)

EDIT: Also looking at adding some tests to ensure the completion block is called as expected.

@julianrex julianrex force-pushed the jrex/pending-completion-blocks branch from 89853dd to 878c402 Compare August 23, 2019 18:48
@julianrex julianrex removed the needs changelog Indicates PR needs a changelog entry prior to merging. label Aug 23, 2019
@julianrex julianrex marked this pull request as ready for review August 23, 2019 18:57
@julianrex julianrex requested a review from a team August 23, 2019 18:57
@julianrex
Copy link
Contributor Author

julianrex commented Aug 23, 2019

platform/ios/CHANGELOG.md Outdated Show resolved Hide resolved
@julianrex julianrex force-pushed the jrex/pending-completion-blocks branch from 6caf818 to 93fd2ed Compare August 25, 2019 04:30
@julianrex julianrex merged commit 4010b1d into master Aug 25, 2019
@julianrex julianrex deleted the jrex/pending-completion-blocks branch August 25, 2019 05:02
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
annotations Annotations on iOS and macOS or markers on Android iOS Mapbox Maps SDK for iOS tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants