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

Style JSON configuration on map snapshotter #11976

Merged
merged 3 commits into from
May 23, 2018
Merged

Conversation

tobrun
Copy link
Member

@tobrun tobrun commented May 22, 2018

Closes #11974 , this PR adds the ability to use a local style.json file as style for a map snapshotter. This option is needed to port our node render tests to Android in #10047.

@LukasPaczos would you be able to review the java code?
@ChrisLoer would you be able to review the core code? (I'm not satisfied with current optional setup, is there a way to make this cleaner? variant or other c++ construct?)

@tobrun tobrun added Android Mapbox Maps SDK for Android Core The cross-platform C++ core, aka mbgl labels May 22, 2018
@tobrun tobrun added this to the android-v6.2.0 milestone May 22, 2018
@tobrun tobrun self-assigned this May 22, 2018
@tobrun tobrun requested review from ChrisLoer and LukasPaczos May 22, 2018 13:22
Copy link
Contributor

@ChrisLoer ChrisLoer left a comment

Choose a reason for hiding this comment

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

Hmm yeah, that use of the optional is awkward, although I can see why you did it. I think ideally the arguments wouldn't be string typed so the code could know what to do (something like "variant<URL, JSON>"), but maybe a cheap way to approximate that is just to make a single style argument that's a std::pair with a bool indicating whether it's JSON or a URL? It'd still be a bit awkward, but it would avoid the unused parameters (e.g. having to provide a "styleURL" that will be ignored).

@tobrun
Copy link
Member Author

tobrun commented May 22, 2018

@ChrisLoer in 73d6dcf, I went with the std::pair<bool, std::string> approach. Lmk what you think, if 👍 I will update the other platforms.

@ChrisLoer
Copy link
Contributor

@tobrun yeah, I think that's an improvement. Everything else on the core side looks good. 👍

@tobrun tobrun force-pushed the tvn-style-json-snapshotter branch from 73d6dcf to 978c7b5 Compare May 22, 2018 18:24
@tobrun tobrun merged commit 671fe37 into master May 23, 2018
@tobrun tobrun deleted the tvn-style-json-snapshotter branch May 23, 2018 08:52
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Android Mapbox Maps SDK for Android Core The cross-platform C++ core, aka mbgl
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants