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

Rename source classes #11568

Merged
merged 6 commits into from
Apr 2, 2018
Merged

Rename source classes #11568

merged 6 commits into from
Apr 2, 2018

Conversation

1ec5
Copy link
Contributor

@1ec5 1ec5 commented Mar 30, 2018

This PR eliminates MGLAbstractShapeSource, elevating MGLShapeSource and MGLComputedShapeSource to direct subclasses of MGLSource. MGLRasterSource and MGLVectorSource have been renamed MGLRasterTileSource and MGLVectorTileSource, respectively. The class hierarchy now looks like this:

  • MGLSource
    • MGLImageSource
    • MGLComputedShapeSource (takes MGLShapeSourceOption)
    • MGLShapeSource (takes MGLShapeSourceOption)
    • MGLTileSource (takes MGLTileSourceOption)
      • MGLRasterTileSource
        • MGLRasterDEMSource
      • MGLVectorTileSource

This is the bare minimum change required to fix #10853. MGLComputedShapeSource’s name still suggests that it’s a subclass of MGLShapeSource, but it’s a minor issue overall. (A computed shape is different than a shape, or something like that.) Once we add an MGLComputedImageSource for #7471, with MGLComputedSource as an intermediate abstract class, I think this naming scheme will make a bit more sense. MGLComputedShapeSource and MGLShapeSource continue to use a Venn diagram of MGLShapeSourceOptions, but this PR comes with some documentation clarifications that should hopefully head off most of the confusion that would cause.

/cc @fabian-guerra @asheemmamoowala

1ec5 added 5 commits March 30, 2018 05:13
Also updated various source class listings to reflect the addition of image and raster DEM sources.
MGLComputedShapeSource is now a direct subclass of MGLSource and sibling of MGLShapeSource.
Also moved options to the primary classes that use them.
@1ec5 1ec5 added iOS Mapbox Maps SDK for iOS refactor macOS Mapbox Maps SDK for macOS beta blocker Blocks the next beta release runtime styling labels Mar 30, 2018
@1ec5 1ec5 added this to the ios-v4.0.0 milestone Mar 30, 2018
@1ec5 1ec5 self-assigned this Mar 30, 2018
@jfirebaugh
Copy link
Contributor

If we're going to rename MGLRasterSource and MGLVectorSource, why not follow the suggestion in #10853 (comment). In that hierarchy, there's a clear categorization via the suffixes RasterSource and VectorSource, and modifiers such as Computed and Tiled go before those suffixes.

Copy link
Contributor

@fabian-guerra fabian-guerra left a comment

Choose a reason for hiding this comment

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

I just commented about the blurb in the changelog, feel free to dismiss it. LGTM 👍🏼
This is what I reviewed:

  • Public class/methods/properties are renamed accordingly to MGLVectorTileSource and MGLRasterTileSource.
  • References to previous class names are removed.
  • Updated examples documentation.
  • Updated Jazzy documentation.
  • Updated spec overrides.
  • Xcode project updates.

@@ -14,6 +14,7 @@ Mapbox welcomes participation and contributions from everyone. Please read [CONT

* Added support for a new layer type: `MGLHeatmapStyleLayer`, a powerful way to visualize point data distributions using heatmaps, fully customizable through runtime styling. [#11046](https://github.com/mapbox/mapbox-gl-native/pull/11046)
* The layout and paint properties on subclasses of `MGLStyleLayer` are now of type `NSExpression` instead of `MGLStyleValue`. A new “Predicates and Expressions” guide provides an overview of the supported operators. ([#10726](https://github.com/mapbox/mapbox-gl-native/pull/10726))
* Renamed `MGLRasterSource` to `MGLRasterTileSource` and `MGLVectorSource` to `MGLVectorTileSource`. ([#11568](https://github.com/mapbox/mapbox-gl-native/pull/11568))
Copy link
Contributor

Choose a reason for hiding this comment

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

Will it be useful add the comment that we removed MGLAbstractShapeSource but all MGLShapeSource or MGLComputedShapeSource subclasses are unaffected?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

MGLAbstractShapeSource is new to this release, so the removal should be mentioned in the upcoming beta’s prerelease notes, but not in the final release notes.

@1ec5
Copy link
Contributor Author

1ec5 commented Mar 30, 2018

If we're going to rename MGLRasterSource and MGLVectorSource, why not follow the suggestion in #10853 (comment). In that hierarchy, there's a clear categorization via the suffixes RasterSource and VectorSource, and modifiers such as Computed and Tiled go before those suffixes.

First of all, I appreciate the feedback you gave in that issue. Seeing the options laid out that way helped me get past my excessive focus on class hierarchy.

I did try out your suggestion locally but wound up sticking with MGLComputedShapeSource. In my opinion, the main reason for avoiding “shape” in the name of MGLComputedShapeSource would’ve been to keep that class from looking like a subclass of MGLShapeSource, not necessarily to establish a stronger link with MGLVectorSource. (Conceptually and in terms of use cases, it’s a blend of MGLShapeSource and MGLVectorSource, and the new documentation makes that clear.)

The other consideration, I think, is that the relationships between the classes are clearer when each subclass name adds a prefix rather than an infix. MGLShapeVectorSource wouldn’t sound like something you’d say in a sentence: “add a shape vector source to the style”. This is why we renamed MGLStyleConstantValue to MGLConstantStyleValue in #8090. So as this PR is currently written, each class name indicates:

  • Whether it’s declarative (implied) or imperative (“computed”)
  • The data format (raster tile vs. Mapbox Vector Tiles vs. MGLShape vs. UIImage)

I considered renaming MGLRasterDEMSource to MGLRasterDEMTileSource, but that seemed too pedantic. I’m also biting my tongue to avoid carrying out the unification of MGLShapeSourceOption and MGLTileSourceOption, as proposed in #10853 (comment). It would give us more flexibility in the future, but at the cost of some code completion handholding now.

The nice thing about these changes, minimal as they are, is that we can revisit them during the v4.x timeframe without breaking backwards compatibility:

  • If we introduce MGLComputedImageSource, we can have both it and MGLComputedShapeSource inherit from an intermediate MGLComputedSource without breaking any Objective-C or Swift code.
  • If we do decide to rename MGLShapeSource and MGLImageSource, we can add compatibility aliases.
  • If we unify MGLShapeSourceOption and MGLTileSourceOption, then we can leave around some typedefs that avoid breaking Objective-C code. Some Swift code will require changes (with fix-its), but typically we don’t consider Swift-only changes to be breaking changes.

Copy link
Contributor

@asheemmamoowala asheemmamoowala left a comment

Choose a reason for hiding this comment

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

Overall looks good.

I noticed that the documentation specifically calls out shape and vector tile sources as the applicable types for vector layers and filters. DoesMGLComputedShapeSource need to be included in those references as well? ex: MGLCircleStyleLayer. This can be done separately too.

@@ -178,21 +178,22 @@ source object is a member of one of the following subclasses of `MGLSource`:

In style JSON | In the SDK
--------------|-----------
`vector` | `MGLVectorTileSource`
`raster` | `MGLRasterTileSource`
`raster-dem` | `MGLRasterDEMSource`
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be MGLRasterDemTileSource in case we decide to add a computed RasterDEM source in conjunction with computed raster/image source?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you think that’s at least as likely as a computed image source? If so, I think I’d push for some mbgl::style::Source classes to be consolidated, which would allow us to consolidate these classes at the SDK level.

Copy link
Contributor

Choose a reason for hiding this comment

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

I haven't seen any requests for it. And you make a good point, core sources will probably get refactored if we get that far.

@@ -401,7 +402,7 @@ For operators that have no corresponding `NSExpression` symbol, use the

## Filtering sources

You can filter a shape or vector source by setting the
You can filter a shape or vector tile source by setting the
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this include computed shape source, or is it implied with 'shape .. source' ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This document is meant as a correspondence from style JSON to the runtime styling API. Since computed shape sources can’t be represented in style JSON, I’ve omitted that type here.

`MGLShapeSourceOptionBuffer`, and `MGLShapeSourceOptionSimplificationTolerance.` This source does
not support clustering.
`MGLComputedShapeSource` is similar to `MGLShapeSource` but is optimized for
data sets that change dynamically but are too large to fit completely in
Copy link
Contributor

Choose a reason for hiding this comment

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

The optimization is for both large data sets and dynamic ones.

data sets that change dynamically but or are too large to fit completely in memory


`MGLShapeSource` is optimized for data sets that change dynamically and fit
completely in memory. For large data sets that do not fit completely in memory,
use the `MGLComputedShapeSource` or `MGLVectorTileSource` class.
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

Mentioned MGLComputedShapeSource in a few more places. Updated symbol style layer documentation to refer to aftermarket expression functions. Copyedited some heatmap style layer documentation.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
beta blocker Blocks the next beta release iOS Mapbox Maps SDK for iOS macOS Mapbox Maps SDK for macOS refactor runtime styling
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants