davdroman / Bohr

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

OptionTableViewCell : allow specific values so the cell's index is not used #29

Closed gmoledina closed 8 years ago

gmoledina commented 8 years ago

The current implementation of optionTableCell uses the cell's index as the setting value when it is selected.

This makes it problematic to change the order of appearance of the settings as their indexes is used as the value and therefore the new cell at that index will be selected in the next release of the application.

I suggest allowing the app to specify a string value for each option that can be used as the setting value when it is selected.

Example:

[self.boTableViewController addSection:[BOTableViewSection sectionWithHeaderTitle:@"favorite color" handler:^(BOTableViewSection *section) {
    [section addCell:[BOOptionTableViewCell cellWithTitle:@"Orange" key:@"favcolor" handler:^(BOOptionTableViewCell *cell) {
        cell.value = @"orange";
    }]];
    [section addCell:[BOOptionTableViewCell cellWithTitle:@"Red" key:@"favcolor" handler:^(BOOptionTableViewCell *cell) {
        cell.value = @"red";
    }]];
}]];
davdroman commented 8 years ago

Hi @gmoledina:

First of all, let me thank you for contributing so much to Bohr!

This is certainly a loose end with BOOptionTableViewCell. The thing is, I don't think their values should be strings, but rather integers, as options are quite often expressed as enums.

However, strings can sometimes be more useful than enums for things like colors (as you showed in your example), so I'm kinda curious about how useful would it be to integrate a mapping function into BOSetting in order to map values from/to NSUserDefaults (which already is ridiculously unsafe type-wise).

First solution coming to mind is 2 really simple mapToDefaults and mapFromDefaults block properties, which would operate over values before going both inside and outside NSUserDefaults.

This way, BOOptionTableViewCell's value would be an NSString by default, but would also allow the user to map @"0", @"1"... as pure integers (and therefore enums) in NSUserDefaults.

gmoledina commented 8 years ago

Thanks to making this component open-source @DavdRoman ! Saved me a lot of time :)

Your point is right that it's better to have the value as integers rather then strings as enums are a common use case. My use case is that I'm getting the options from a DB directly as a list of options. So I can use their unique ID (integer) as their value as well.

So if you'd like to change the value for an integer, I'm perfectly fine with that. I can update my PR.

For the mapping, it's a good idea as well but as the requirement for the value to be a string is very flexible, let's see if just changing the value to be an integer works out.

davdroman commented 8 years ago

I see your point there. I think having mappable defaults would be really powerful, so I don't discard it as a future feature. However, I'm planning a big rewrite in Swift for the next major update, so I'll probably wait until then to add it. Let's go ahead and use integers for now. An updated PR would be highly appreciated :)

gmoledina commented 8 years ago

@DavdRoman yup absolutely, the mapping feature would be good to support straight from the framework.

I updated the PR with the change. Let me know if that's what you had in mind as well.

davdroman commented 8 years ago

I can see the reasoning behind didSetValue, it's a bummer Objective-C primitives can't be nil. However what do you think about using NSInteger instead of NSUInteger so we can use -1 as a nil by default in order to get rid of the didSetValue juggling? We know for sure rows can't be less than 0, so I don't think it'd present any problems.

gmoledina commented 8 years ago

@DavdRoman sure, we can use NSInteger instead or it's unsigned counterpart, and for the default value, we can use NSIntegerMin. PR updated

davdroman commented 8 years ago

Seems nice to me :+1: