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

Defaults persistence storage abstraction #32

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Defaults persistence storage abstraction #32

wants to merge 1 commit into from

Conversation

gmoledina
Copy link
Collaborator

Settings always need default values so that :

  • the first time the user goes into the Settings section, the default values are selected
  • the setting values might be needed prior to user going in the Settings section of the app

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
Copy link
Owner

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
Copy link
Collaborator Author

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
Copy link
Owner

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.

@davdroman davdroman changed the title Support for default value for settings Defaults persistence storage abstraction Jan 15, 2016
@davdroman davdroman added this to the 4.0.0 milestone Jan 15, 2016
@davdroman davdroman mentioned this pull request Jan 15, 2016
@gmoledina
Copy link
Collaborator Author

@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
Copy link
Owner

@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
Copy link
Collaborator Author

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

@davdroman davdroman force-pushed the master branch 2 times, most recently from 3b2804d to 658a540 Compare August 24, 2020 14:04
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants