Skip to content

Conversation

@tobrun
Copy link
Member

@tobrun tobrun commented May 12, 2021

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></changelog>.

Summary of changes

We currently maintain style loaded state in the SDK to workaround an upstream issue.
This PR validates if we can remove that state checking from our end.

@tobrun tobrun force-pushed the tvn-style-load branch 2 times, most recently from 2e099db to 5d24690 Compare May 12, 2021 13:27
@tobrun tobrun self-assigned this May 19, 2021
@tobrun tobrun requested a review from a team May 19, 2021 14:05
@tobrun tobrun marked this pull request as ready for review May 19, 2021 14:05
handlerMain.post {
nativeMapWeakRef.call { (this as StyleManagerInterface).styleURI = styleUri }
}
nativeMapWeakRef.call { (this as StyleManagerInterface).styleURI = styleUri }
Copy link
Contributor

Choose a reason for hiding this comment

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

A bit concerned here.
We're removing handler logic here but leaving it in sdk/src/main/java/com/mapbox/maps/NativeObserver.kt. If I remember correctly some some metrics benchmarks were having a deadlock because of not posting here.

Copy link
Contributor

Choose a reason for hiding this comment

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

If tests pass - we can follow up later.

Copy link
Member Author

Choose a reason for hiding this comment

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

agreed, let me dig up why we have that construct

Copy link
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.

LGTM but better check metrics before merging.

@tobrun tobrun force-pushed the tvn-style-load branch 6 times, most recently from fb46aa8 to 87f98ee Compare May 20, 2021 11:54
@CLAassistant
Copy link

CLAassistant commented Jun 2, 2021

CLA assistant check
All committers have signed the CLA.

@tobrun tobrun force-pushed the tvn-style-load branch 2 times, most recently from c5819e8 to d8fdd31 Compare June 24, 2021 14:57
@tobrun
Copy link
Member Author

tobrun commented Jun 30, 2021

RC.3 tag landed, going to merge to be included in RC.4

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.

3 participants