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

Initializers for MGLStyle #6386

Open
1ec5 opened this issue Sep 20, 2016 · 12 comments
Open

Initializers for MGLStyle #6386

1ec5 opened this issue Sep 20, 2016 · 12 comments
Assignees
Labels
Core The cross-platform C++ core, aka mbgl gl-ios iOS Mapbox Maps SDK for iOS macOS Mapbox Maps SDK for macOS refactor
Milestone

Comments

@1ec5
Copy link
Contributor

1ec5 commented Sep 20, 2016

Now that MGLStyle behaves similar to a model object (actually a window into MGLMapView), we should add initializers to MGLStyle and replace MGLMapView's styleURL property with a style property.

We'd want some subset of the following initializers:

  • -initWithURL: for parity with the current API on MGLMapView
  • The oft-requested -initWithJSONData:
  • A more object-oriented -initWithSources:layers:

This enhancement would shift some of the complexity from MGLMapView to MGLStyle and reunite the default style API with the style URL code path. MGLOfflineRegion would continue to use style URLs. The existing "Style URL" IB inspectable will remain, because IB3 plugins still haven't made a comeback. 😉

The upshot is that styles will be fully object oriented, instead of the current situation where the developer initially deals with an NSURL only to switch to MGLStyle as soon as they want to make modifications. I'm told this approach is even a bit more Reactive, although that isn't a design goal.

This has the potential to be a fairly disruptive change, so we may want to deprecate the existing NSURL-based API instead of removing it outright.

/cf mapbox/mapbox-gl-js#1989
/cc @jfirebaugh @incanus @bsudekum @frederoni

@1ec5
Copy link
Contributor Author

1ec5 commented Sep 20, 2016

-initWithJSONData: would restore the functionality I took out as part of an MGLMapView spring cleaning last year. I remain convinced that there are usually cleaner, more performant, less circuitous ways to load a style, including referring to a local file URL, but I've included the JSON initializer in this proposal for completeness' sake. I think the added complexity in MGLStyle would be manageable.

Previous discussion: #900, #1147 (comment), #1299, #1367, #1997, #4769.

@1ec5 1ec5 added iOS Mapbox Maps SDK for iOS refactor macOS Mapbox Maps SDK for macOS labels Sep 20, 2016
@1ec5
Copy link
Contributor Author

1ec5 commented Sep 22, 2016

Combined with #6383, we could also expose an -[MGLMapView setStyle:completionHandler:] in addition to the MGLMapView.style property envisioned by this proposal. I think every developer would prefer such an API over the delegate method proposed in #6412.

@1ec5
Copy link
Contributor Author

1ec5 commented Oct 7, 2016

@incanus points out in #6636 (review) that, once we expose any API that makes use of setStyleJSON(), we’ll need to make any style URL API nullable.

@1ec5 1ec5 added this to the ios-3.4.1 milestone Dec 21, 2016
@1ec5
Copy link
Contributor Author

1ec5 commented Dec 21, 2016

Putting on the v3.4.1 milestone for the initializers themselves. The completion block–based API proposed in #6386 (comment) would be a somewhat longer-term goal.

@1ec5
Copy link
Contributor Author

1ec5 commented Dec 31, 2016

#7563 would add a method to get the current style JSON, which would complement the -initWithJSONData: initializer proposed here.

@1ec5
Copy link
Contributor Author

1ec5 commented Jan 10, 2017

One common task that currently requires some hand-editing of JSON is creating a raster style, whether for compatibility with TileMill or to display naturally rasterized tiles like aerial imagery. We can add a convenience initializer, -initWithTileSetIdentifier:, similar to what MapboxStatic.swift currently provides.

@1ec5
Copy link
Contributor Author

1ec5 commented Jun 19, 2017

With a full implementation of this feature, the developer may expect to be able to use a single MGLStyle object with multiple instances of MGLMapView, either concurrently or in series. For example, one view controller with a map view may need to push another view controller onto the stack that has a similar-looking map view. Or perhaps an iPad application implements a split-screen view that must synchronize the two maps across runtime styling changes.

I can see how sharing a style across multiple map views could become a rabbit hole. For one thing, mbgl would need to support sharing a single mbgl::style::Style among multiple mbgl::Maps, so that we could remove the backpointer from MGLStyle to MGLMapView. Various parts of the SDK’s runtime styling code depend on this backpointer, including -[MGLVectorSource featuresInSourceLayersWithIdentifiers:predicate:].

MGLOpenGLStyleLayer’s public methods such as -drawInMapView:withContext: may also complicate matters. In these methods, the map view parameter is used for disambiguating between multiple map views and potentially for obtaining state that isn’t part of MGLStyleLayerDrawingContext. Maybe we can pass in an MGLStyle instead of an MGLMapView for disambiguation purposes.

Beyond that, I haven’t thought very deeply about the implementations of style sharing. It would be an improvement over the basic feature proposed here, which would already make the API a lot more pleasant to work with.

@stale
Copy link

stale bot commented Nov 30, 2018

This issue has been automatically detected as stale because it has not had recent activity and will be archived. Thank you for your contributions.

@stale stale bot closed this as completed Nov 30, 2018
@1ec5 1ec5 reopened this Mar 26, 2019
@stale stale bot removed the archived Archived because of inactivity label Mar 26, 2019
@1ec5
Copy link
Contributor Author

1ec5 commented Mar 26, 2019

Reopening per #14137.

@1ec5
Copy link
Contributor Author

1ec5 commented Mar 27, 2019

#9280 introduced a Style constructor as part of mbgl’s public API. It takes a Scheduler, FileSource, and pixel ratio, all of which can be obtained from +[MGLRendererConfiguration currentConfiguration] and mbgl::sharedThreadPool(). getURL() and getJSON() would correspond to the proposed -initWithURL: and -initWithJSONData: methods.

So here’s a sketch of what the proposed implementation would look like in MGLStyle:

- (instancetype)initWithURL:(NSURL *)url {
    if (self = [super init]) {
        [self commonInit];
        _rawStyle->loadURL(url.absoluteString.UTF8String);
    }
}

- (instancetype)initWithData:(NSData *)data encoding:(NSStringEncoding)encoding error:(NSError * _Nullable *)outError {
    if (self = [super init]) {
        [self commonInit];
        NSString *string = [[NSString alloc] initWithData:data encoding:encoding];
        if (!string) {
            if (outError) {
                *outError = [NSError errorWithDomain:MGLErrorDomain code:MGLErrorCodeUnknown userInfo:nil];
            }
            return nil;
        }
        
        _rawStyle->loadJSON(string.UTF8String);
    }
}

- (void)commonInit {
    MGLRendererConfiguration *config = [MGLRendererConfiguration currentConfiguration];
    _rawStyle = std::make_unique<mbgl::style::Style>(mbgl::sharedThreadPool(), config.fileSource, config.scaleFactor);
}

and in MGLMapView:

- (void)setStyle:(MGLStyle *)style {
    _mbglMap->setStyle(style);
}

@1ec5
Copy link
Contributor Author

1ec5 commented Mar 27, 2019

In the meantime, if all you have is the JSON and no file on disk, a workaround for the lack of -[MGLStyle initWithData:encoding:error:] would be to save the JSON to a temporary file and set the style URL to that file’s URL.

/cc @tobrun

@captainbarbosa
Copy link
Contributor

captainbarbosa commented May 1, 2019

👋 For those following along, I wanted to circle back and note where the team is currently at regarding this issue. For iOS, we're currently considering making changes to the following APIs:

MGLStyle
✨ Introduce -[MGLStyle initWithURL:]
✨ Introduce -[MGLStyle initWithJSONData:encoding:error:]

MGLMapView
✨ Introduce -[MGLMapView initWithFrame:style]
✂️ Deprecate -[MGLMapView initWithFrame:styleURL:

Support for completion handlers, offline APIs, and additional methods such as -[MGLStyle initWithSources:layers:] may be scoped into later work.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Core The cross-platform C++ core, aka mbgl gl-ios iOS Mapbox Maps SDK for iOS macOS Mapbox Maps SDK for macOS refactor
Projects
None yet
Development

No branches or pull requests

4 participants