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

[ios] Adds support for MGLCollisionBehaviorPre4_0 in NSUserDefaults #13426

Merged
merged 6 commits into from
Nov 21, 2018

Conversation

julianrex
Copy link
Contributor

Addresses #13155 - adds support for setting MGLCollisionBehaviorPre4_0 via NSUserDefaults.standardUserDefaults.

Setting via NSUserDefaults takes priority over the value in the application's Info.plist file.

@julianrex julianrex added iOS Mapbox Maps SDK for iOS ⚠️ DO NOT MERGE Work in progress, proof of concept, or on hold labels Nov 20, 2018
@julianrex julianrex requested a review from 1ec5 as a code owner November 20, 2018 20:24
@julianrex julianrex removed the ⚠️ DO NOT MERGE Work in progress, proof of concept, or on hold label Nov 20, 2018
@julianrex julianrex added the macOS Mapbox Maps SDK for macOS label Nov 20, 2018
@julianrex julianrex force-pushed the jrex/13155-MGLCollisionBehaviorPre4_0 branch from d096450 to 1b5dead Compare November 21, 2018 04:22
@friedbunny friedbunny added this to the release-iowaska milestone Nov 21, 2018
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 leave two comments but already approved since are minor changes. Regarding the previous public function going private I looked our API docs and is not documented so I think we are fine making this private.

if (collisionBehaviourNumber) {
_perSourceCollisions = collisionBehaviourNumber.boolValue;
} else {
// Also support NSString to correspond with the behavior of `-[NSUserDefaults boolForKey:]`
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this is necessary because boolForKey coerces "YES"/YES/1/"1"/"NO"/NO/0/"0" as BOOL

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's only on NSUserDefaults unfortunately - this is for the property from the NSDictionary (from the bundle/info.plist). So, I think we need this?

Though strictly speaking it's not documented that @"YES"/@"NO" etc. are supported, but given that this PR introduces support for NSUserDefaults I figured this ought to be included.

(An mgl_boolForKey might be a nice category method to add to NSDictionary)

@interface MGLRendererConfiguration ()
@property (nonatomic, readwrite) BOOL perSourceCollisions;
@end


@implementation MGLRendererConfiguration

+ (instancetype)currentConfiguration {
return [[self alloc] init];
Copy link
Contributor

Choose a reason for hiding this comment

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

With the addition of the initializers I think we need to make this class a proper singleton. Or is there a reason to have more than one instance for this class?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not aware of the history behind currentConfiguration, but I think it's like this so that it captures the current state, which could change. I agree it's a little confusing - hence my comment above:

(An aside: since currentConfiguration doesn't do anything I'm tempted to remove it too).

@julianrex julianrex merged commit fd4b470 into master Nov 21, 2018
@julianrex julianrex deleted the jrex/13155-MGLCollisionBehaviorPre4_0 branch November 21, 2018 18:56
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
iOS Mapbox Maps SDK for iOS macOS Mapbox Maps SDK for macOS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants