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

Add and remove layers dynamically #4839

Merged
merged 8 commits into from
Jun 2, 2016
Merged

Add and remove layers dynamically #4839

merged 8 commits into from
Jun 2, 2016

Conversation

jfirebaugh
Copy link
Contributor

@jfirebaugh jfirebaugh commented Apr 25, 2016

This PR implements the first bit of the core runtime styling API: the ability to dynamically add and remove layers.

As appropriate for a C++ API, the layers API is strongly typed: there are subclasses for each layer type, with correctly-typed accessors for each style property. This results in a large API surface area. Fortunately, this is automated, by generating the API – and the regular portion of the implementation – from the style specification.

The layers API makes a distinction between public API and internal implementation using the Impl idiom seen elsewhere in the codebase. Here, it takes the form of two parallel class hierarchies:

  • Layer and its subclasses form the public API.
  • Layer::Impl and its subclasses form the internal API.

As well as forming the boundary between public and internal, these two class hierarchies form the boundary between generated code and handwritten code. Except for CustomLayer and CustomLayer::Impl:

  • Layer subclasses are entirely generated. (Layer itself is handwritten.)
  • Layer::Impl and its subclasses are entirely handwritten.

There is still work to be done before this is ready to be exposed via SDK bindings:

  • The types relating to property values need to be cleaned up. Conceptually, a property value is a variant with three possibilities: not set (equivalent to a default value), constant value, or function value. The property API needs to reflect this. Currently every value is treated as a function value.
  • The namespacing needs to be rationalized. Everything style-related should live in the mbgl::style namespace and mbgl/style directory. (Beneath that directory there can be a layer directory for organization.)
  • Accessors for the layer filter and zoom range need to be added.
  • There needs to be a layer getter. I'm leaning towards map.getStyle().getLayer() as the API. Map::{add,remove}Layer would also move to Style.


// Private implementation

std::unique_ptr<Layer> clone() const final;
Copy link
Contributor

Choose a reason for hiding this comment

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

Any way we can move this to Impl?

@kkaefer
Copy link
Contributor

kkaefer commented Apr 26, 2016

Looks good! As for the generated files, it's probably useful to have them pre-generated to be able to look at them. We could still add the generation script to the build system, and check whether files were modified and fail if that is the case.

@jfirebaugh jfirebaugh force-pushed the layer-api branch 2 times, most recently from 976397a to 67ce67c Compare April 27, 2016 22:30
@1ec5
Copy link
Contributor

1ec5 commented Apr 27, 2016

We could still add the generation script to the build system, and check whether files were modified and fail if that is the case.

How about running the generation script as part of the build, but having the script output to the checkout, as it does now?

@jfirebaugh
Copy link
Contributor Author

For now I'm going to go with running the generator manually when you make a change. I looked at including it as part of the build step but outputting to the src directory (which I want to do) didn't fit in cleanly with our use of gyp.

@jfirebaugh jfirebaugh force-pushed the layer-api branch 4 times, most recently from d298ab0 to 00c9500 Compare April 29, 2016 19:33
@jfirebaugh
Copy link
Contributor Author

The last commit here involves a lot of renaming and re-namespacing. Would be great to get a second opinion on my approach:

  • Everything "style-related" (roughly: implementing a concept from the style specification) lives in the style subdirectory and mbgl::style namespace.
  • Inside the mbgl::style namespace, don't prefix class names with Style (except for the Style itself).
  • Outside the mbgl::style, use forward declarations in headers where available.
  • Use using namespace style in cpp files (not in headers).

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

Successfully merging this pull request may close these issues.

3 participants