Add Settings Module Support for the Android Platform

[greghe]

Motivation

Allows the current iOS Settings module to be used on both android and iOS platforms for the keyed value storage of primitive types. Solves the problem that android applications cannot currently store primitive types in a shared preferences file with the native app in the same way that iOS applications can.

Test Plan

I've added a tests directory to the Settings that performs basic tests of Settings on the android platform.

I wrote a SettingsDemo React Native app that shows the storage of different primitive types as well as the ability to watch pref values for modification by the native app.

Moreover, The android implementation exactly mirrors the iOS Settings implementation, even using the same consolidated Settings.js file.

[vonovak]

Good job. In my opinion, there are still some parts that need a bit of improvement though.

[vonovak]

I believe using toBe() can improve readability. Either by saying expect(oneValue === 1).toBe(true); or expect(oneValue).toBe(1);

[vonovak]

just Object.assign(this._settings, settings); would imho be more readable. Syntax is Object.assign(target, ...sources), so no need for the assignment.

[greghe]

I didn't actually change any code in Settings.js (other than rename the file from Settings.ios.js). But I agree we should probably improve this file's code as part of this change.

[vonovak]

It would be great to stay at the interface level, without any casting.
Why not pass the ReadableMap to setValuesToPreference, and then instead of working with the HashMap, you can use ReadableMapKeySetIterator keySetIterator(); to iterate over the map and call ReadableType getType(String name); to find out the type instead of using instanceof. What do you think?

[greghe]

This is how I originally implemented it and it is the cleaner implementation. I will restore it.

[vonovak]

this may cause trouble: the original map is being mutated - you don't want that. You will probably need an extra Map instance to cover the String Set case. Note you should be able to use ArrayList<?> list = new ArrayList<>((Set<?>) value); to avoid the unchecked casting complaint.

[greghe]

Should be fixed now.

[vonovak]

Some more observations :)

[vonovak]

The case when the value is out of float range should be covered.
Also, I think having mPreferences.edit().putXYZ.apply(); for each case is unnecessarily lengthy. Why not assign mPreferences.edit() to something and then call putXYZ() on it and use just one apply() at the end.

Also, removing the empty lines between cases will likely improve readability.

[greghe]

done

[vonovak]

isn't mPreferencess null at this moment? what is the purpose of having getPreferences() method and mPreferences member variable at the same time? There should be only one way of working with them.

[greghe]

No, mPreferences is not null here because that would cause a crash. 🙂

getPreferences would be better named getPreferencesAndInstantiateIfNecessary which I abbreviated to ensurePreferences() when I renamed this method.

[vonovak]

Imho the else branch should be covered, and maybe a comment on the meaning of the condition would help.

[vonovak]

I think a little less generic message would be great, something that tells you right away what module the error relates to.

[greghe]

will do

[vonovak]

I'm not familiar with this android functionality, but I believe there is a difference in using getSharedPreferences vs getPreferences

[greghe]

Yes, and differing calls here are entirely intentional. getSharedPreferences() lets the caller specify a specific file name in which to store the shared preferences, whereas getPreferences() stores the preferences values in a file with a default name.

In the section above we allow the programmer the option of storing the preferences in a file with a specific name (say, to match an existing shared preferences file already in use by the application), or, to simply store the preferences in a file with a default name.

[vonovak]

according to the docs, getPreferences returns

a SharedPreferences object for accessing preferences that are private to this activity

So by default, the preferences file created by this module would be available only to the activity that created it. That may or may not be what you want, and could be changed by calling the setFilename method. That brings us to another question - how do you call setFilename from the javascript side?

[greghe]

The settings file created by the Settings Module is accessible to that Settings module and therefore indirectly accessible to any Activity or to any other Java class that imports the Settings module.

The only potential concern raised by calling getPreferences() here is for apps that have multiple Activities each instantiating a MainReactPackage. This is by no means a common occurrence, but if an app is engaging in this behavior - then having each Settings Module in each MainReactPackage use its own, private settings store seems like the most reasonable default behavior.

Nevertheless, if separate settings files are not desired, that is, if the developer wants to ensure that every Settings Module instantiated anywhere in the the app will always use the same settings file - then that developer merely has to provide a file name to the Settings Module, and - as long as they take care that only one Settings Module is ever extant at one time - it will be the case that every Settings Module created anywhere in the app will use the same settings file which will have the provided name.

[vonovak]

Thanks for the explanation @greghe. I'm just unsure if it is possible to call setFilename from Javascript with the current impl?

[greghe]

I've renamed setFilename to setSharedPreferencesFilename and documented its proper use. setSharedPreferencesFilename may not be called after a Settings Module has been instantiated (this is, before a ReactMainPackage initialization) - and will throw an exception if this requirement is not met.

Basically, this method exists as a convenient customization point for a developer with specialized requirements regarding handling of the Shared Preferences file - and would typically be invoked from the app's main activity just before it sets up the RN environment.

Therefore invoking setSharedPreferencesFilename is not possible from any JS code -because by the time the JS code would be executing, the Settings Module and its shared preferences file will already be up and running.

[vonovak]

I hope I am not becoming annoying :D but why do you use mPreferences here, while you call ensurePreferences() in other methods? I believe there should be only one way of working with the preferences.

[vonovak]

We still need to have the case when the value is out of the range of Float covered. I don't want to make it more complicated for you than necessary, but personally I'd do it so that this method returns a promise that resolves / rejects with error message if there is one. This would require changes to the ios module as well though.

[greghe]

Remember these are the persisted settings values, not the current values of the Settings Module. Whereas AsyncStorage has to read and write its stored values asynchronously through native code, Settings reads and writes its settings immediately and synchronously all within its javascript code. It is true that the JS-side settings value are then lazily and asynchronously propagated to native code - both to inform the native app of any changes and also to persist the settings values for a subsequent relaunch of the app. But the values being stored here are not the values that Settings Module is actually providing to its clients.

Therefore, there is no reason to use a promise here because the Settings module is basically unaware and unaffected by what this method is doing.

The double-to-float conversion does affect the persisted numeric values that populate the Settings Module upon launch. Because these loaded numeric values have been "round tripped" through a smaller type, they may wind up losing precision or over/under flowing their representation. Although this information loss is unfortunate - as a practical matter, few settings are likely to be affected - either because they are integers or they are far, far smaller than 3.4 × 10³⁸ - the upper range of a float.

Moreover, it is not a error for a number to be too large or too small for a floating point type to represent - for such numbers, infinity is used for the huge ones and zero for the tiny values.

[vonovak]

I am aware that the settings values live in javascript and are also asynchronously (although I would't say lazily) sent to native land and persisted.

The problem here is that you can store an object to preferences, for example {name: "John", age: null } and when you read the preferences somewhere else, you the module returns the same what you have stored (because the value is read from JS). But then you restart the app and what you get is {name: "John"} - the age field is lost!

With the current implementation, people can store things without knowing they weren't saved or that they weren't saved exactly the way they were meant to be saved (e.g. because the settings module does not support storing nulls or because there was a trouble casting their large number they wanted to save). This silent failing is very developer-unfriendly. That is why I suggested that a promise would be helpful - if such case happens, the promise could reject.

I think it pays off to do things right here - many people will likely use this module (myself included, which is why I did the review). Now, I am not a person that has the rights to merge this PR, so maybe you can go ahead and ask some core contributor to merge, but I think they will also want to make the module more error-proof. I think I have said all that I wanted, so good luck getting this merged 👍

[vonovak]

Maybe the easiest and the best way to handle this would be to put a simple check to the set method in Javascript. It would make sure only non-null strings / numbers / bools can be saved.

[facebook-github-bot]

@greghe I tried to find reviewers for this pull request and wanted to ping them to take another look. However, based on the blame information for the files in this pull request I couldn't find any reviewers. This sometimes happens when the files in the pull request are new or don't exist on master anymore. Is this pull request still relevant? If yes could you please rebase? In case you know who has context on this code feel free to mention them in a comment (one person is fine). Thanks for reading and hope you will continue contributing to the project.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Maybe the issue has been fixed in a recent release, or perhaps it is not affecting a lot of people. If you think this issue should definitely remain open, please let us know why. Thank you for your contributions.

[sryze]

Can somebody else work on this PR (e.g. me) to fix the remaining issues and get it merged? or should I create one from scratch? (I'm not sure what the possible copyright issues are and so on).

I was a little surprised to learn that the basic native settings storage feature of Android is not implemented in React Native. I've actually created my own internal RN module for this, but it's pretty basic (doesn't support watches for example).

[vonovak]

The core of react native is going to become smaller in order to be more maintainable, so working on a PR for RN is not meaningful. It'd be better to cover this by a separate native module that people can use as a dependency. I'd be surprised if there wasn't such module already.