[ios, macos] Remove duplicated variables in MGLMapSnapshotter #10702
Conversation
| property is non-nil, the camera’s altitude is ignored in favor of this | ||
| property’s value. | ||
| */ | ||
| @property (nonatomic) double zoomLevel; |
There was a problem hiding this comment.
Removing this public symbol would technically be a backwards-incompatible change. On the other hand, these properties are have no effect and are never set, so I guess we can assume no one is using them?
There was a problem hiding this comment.
This was released since 3.7.0 I think it's use is not spread. Since this is backwards-incompatible we may want to release it as part of 4.0 and not in a patch.
There was a problem hiding this comment.
That sounds reasonable to me. Alternatively, if a patch release is going out the door anyways, we can mark these methods as deprecated for now and remove them when v4.0.0 comes out.
| } | ||
| return self; | ||
| } | ||
|
|
||
| - (void)initMBGLMapSnapshotter:(MGLMapSnapshotOptions *)options |
There was a problem hiding this comment.
Can we go with a different method name here? Style-wise, I'm not a fan of leading method names with init if they're not a proper initializer (and I believe it breaks convention, although I'm in too much of a hurry to dig up a citation)
There was a problem hiding this comment.
-configureMapSnapshotterWithOptions: perhaps? or -setUpMapSnapshotterWithOptions:
There was a problem hiding this comment.
Good point. Beginning a non-initializer with init is problematic because ARC expects to retain its return value. There’s a memorable slide from WWDC11 about a hypothetical -copyMachine method that has to be renamed -copymachine for the same reason. 😬
fabian-guerra
force-pushed
the
fabian-ss-redundant-10435
branch
from
950badf to
372da68
Compare
|
|
||
| // Create the snapshotter | ||
| _mbglMapSnapshotter = std::make_unique<mbgl::MapSnapshotter>(*mbglFileSource, *_mbglThreadPool, styleURL, size, pixelRatio, cameraOptions, coordinateBounds); | ||
| [self configureMBGLMapSnapshotter:options]; |
There was a problem hiding this comment.
Both call sites to -configureMBGLMapSnapshotter: pass in the value of the options property. Since it isn’t necessary to support configuring the receiver based on an arbitrary options object, let’s remove this parameter and have -configureMBGLMapSnapshotter refer to the options property.
The usual name for a method like this is -commonInit, as seen in MGLMapView. However, this method is also called from a non-initializer, so it would be slightly misleading to name it that.
Since the intention is to update the snapshotter object whenever the options object is set, including when it is set for the first time, how about folding this method into -setOptions: and having the initializer call -setOptions:?
…s and MGLMapSnapshotter
fabian-guerra
force-pushed
the
fabian-ss-redundant-10435
branch
from
372da68 to
f6184d8
Compare
Fixes #10435