davdroman / Bohr

Settings screen composing framework
MIT License
1.26k stars 83 forks source link

Defaults persistence storage abstraction #32

Open gmoledina opened 8 years ago

gmoledina commented 8 years ago

Settings always need default values so that :

The suggestion is to add this simple method in the Setting model class that will serve just that : easily specify default values for when no values have been set yet.

Usage example:

+ (void)applyDefaultValues
{
    [[BOSetting settingWithKey:@"fontfamily"] setDefaultValue:@"verdana"];
    [[BOSetting settingWithKey:@"fontsize"] setDefaultValue:@(16)];
}

This method would be called in the app delegate during app launch so that all ViewControllers already have valid values for the settings to start with.

davdroman commented 8 years ago

I don't know if you already know this, but NSUserDefaults offers a method called registerDefaults: in order to do just this (which is used in the demo app's AppDelegate). I don't see a need to offer a different implementation.

gmoledina commented 8 years ago

Oh I had not seen the code of the demo - just checked it out.

The fact that this lib uses NSUserDefaults to store the settings is an internal implementation detail. Any app using this lib shouldn't assume this detail. If one day you change the persistence storage of the settings, it should be transparent for all the apps.

Also, there's no guarantee that the key that the app specifies is exactly the key the lib uses to store the settings - the lib could easily had prefixes for example or not use the standard NSUserDefaults.

For these reasons, I think the lib should offer a way to specify default values.

In the future, the persistence storage could be abstracted and the lib could even support multiple storage options (ex: CoreData, sqlite, Parse, Realm, etc...)

davdroman commented 8 years ago

Ok, so now we're discussing a larger matter: abstracting the defaults persistence storage.

I'm curious about how well we could implement this. My bet is we can make a pretty good job. I'll open an issue to discuss future breaking changes for 4.0 (which I'm planning to rewrite entirely in Swift) and reference this one there.

gmoledina commented 8 years ago

@DavdRoman I dont think this PR should be renamed. The persistence storage abstraction is not a requirement to adopt good habits of hiding implementation details.

davdroman commented 8 years ago

@gmoledina I'm sorry if that bothered you, I didn't mean to. The thing is I really want to hide those implementation details, but I don't think is worth it in this current version. Bohr already provides what this PR is trying to achieve indirectly because it relies uniquely on NSUserDefaults, which can provide defaults values natively. Providing an additional mechanisms for the same task right now seems like a confusing and repetitive thing to me as long as Bohr relies on NSUserDefaults (which again, I recognise was a terrible API design decision and needs to change).

I think it'd be far more convenient and straightforward to do the right thing and abstract the storage layer in the next major version and leaving this one depending on NSUserDefaults for providing default values.

gmoledina commented 8 years ago

@DavdRoman no no I'm not bothered at all, dont worry ! Alright no problem, I see your point.