search

davdroman / Bohr

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

ChoiceTableViewCell : allow specific setting values so the option's indexes aren't used #37

Closed gmoledina closed 8 years ago

gmoledina commented 8 years ago

Same concept as we had for the OptionTableViewCell - this allows specifying the setting value for each choice. Extremely useful when paired up with a separate destinationViewController as well.

Thanks again for making this open source, it's working out very well for me !

Also, would it be possible to tag a release after this PR is merged ?

davdroman commented 8 years ago

this allows specifying the setting value for each choice

I don't understand what a useful use case would be for this one. Could you explain a bit more the benefits of this change?

Thanks again for making this open source, it's working out very well for me !

I'm so glad to hear that :smile:

Also, would it be possible to tag a release after this PR is merged ?

Absolutely! Although we'd need to take a look at #36 (from #31) and see what's causing it first.

gmoledina commented 8 years ago

To answer your first question, the goal is the same as my PR in the past regarding OptionTableViewCell.

Problem : the ChoiceTableViewCell uses the index of the array of strings to compare it to the settings value. So for example, let's say I have these 3 choices from my database :

Unique ID Title
23 Red
45 Green
76 Orange

When the user selects Green, I need the setting value to be 45, not 1.

Currently, the setting value for each of these will be their index in the NSArray of options (so 0 for Red, 1 for Green, and 2 for Orange). But what if my DB query changes and now I have more results between any on these choices ? The settings value gets messed up.

(in my case, I have more choices but depending on the user, some choices are hidden. So choices values don't start at 0 and aren't ordered).

So basically, it's useful in scenarios where the choices values are not starting integers starting from 0 and necessary in order.

The point is the same as the other PR : we shouldn't assume a setting value automatically based on array index. Instead, we should let the app specify it based on its own logic.

Regarding #36, I experienced this same issue as well. I'll comment on it.

davdroman commented 8 years ago

Oh I see what you mean, but why not use an NSDictionary for the options then?

I know the whole "number as NSString" thing is not the safest mechanism to provide (and Swift's Dictionary will probably be an amazing way to achieve this in the future), but I think separate int/value arrays are even more unsafe in this case.

gmoledina commented 8 years ago

NSDictionary would be safer, I totally agree. But that would make this change break with previous versions and everyone will have to update their code.

Also, with NSDictionary, you lose control of the order in which the choices appear !

The current implementation makes it optional for other people to specify the values or stick with the defaults.

davdroman commented 8 years ago

But that would make this change break with previous versions and everyone will have to update their code.

You're right. We'll leave it as a possible enhancement for #33 then.

you lose control of the order in which the choices appear !

That's not really true, since you can get all the keys in the dictionary, map them to integers and look for the nearest integer after the current one. Plus, trusting NSArray's order isn't always the best thing to do.

Anyways, I agree with you on not making breaking changes. Merging! :+1:

gmoledina commented 8 years ago

Thanks @DavdRoman !