jamesmontemagno / Xamarin.Plugins

Cross-platform Native API Access from Shared Code!
MIT License
1.3k stars 380 forks source link

Settings: GetValueOrDefault<string>(key, null) returns empty string #195

Closed csoren closed 8 years ago

csoren commented 8 years ago

On iOS ISettings.GetValueOrDefault(key, null) returns null if the key does not exists. On Android, the same call returns the empty string, if the key does not exists.

I would expect the platforms to behave the same way and return the default value I specified (null).

jamesmontemagno commented 8 years ago

Agreed it should be consistent. Will require a bit of work because I need to check to see if the key exists and if not then return your default. This is what iOS does. I will say null is a bad idea and is actually an exception if you tried to pass it in to set it (well i convert it to string.empty). I highly recommend going with a different default.

csoren commented 8 years ago

The reason I'm using null is so I can determine whether the key has been set. Passing a default value, such as the empty string, will not allow me to do that. There's no "KeyExists" function, if null is discouraged, perhaps the solution would include adding that to ISettings?

dvshub commented 8 years ago

James, thanks for making this great effort to create a common Settings layer on all these platforms.

This issue (and the one I reported for AddOrUpdateValue) is actually pretty simple to fix. I debugged and found that the root cause is Convert.ToString(defaultValue) at Line# 96 and 219 on Android https://github.com/jamesmontemagno/Xamarin.Plugins/blob/1ed18269c3e123c67a1bd01685d0cdacd0ea4db3/Settings/Refractored.Xam.Settings.Android/Settings.cs

Since Convert.ToString() returns "The string representation of value, or String.Empty if value is an object whose value is null. If value is null, the method returns null." as per https://msdn.microsoft.com/en-us/library/astxcyeh(v=vs.110).aspx

So the fix is: defaultValue = (defaultValue == null)? null : Convert.ToString(defaultValue);

But this will result in removing the existing entry if the key was found since that is the default behavior of SharedPreferences on Android while adding an entry with null value.

Hence, the best solution is to say that null values are not supported by the Settings plugin and throw ArgumentException if the supplied value is null for case TypeCode.String. This will also make the Android behavior consistent with the behavior on iOS.

jamesmontemagno commented 8 years ago

This issue was moved to jamesmontemagno/SettingsPlugin#8