Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
fe2b7ea
Add settings module for android
Mar 23, 2017
7add8a6
Merge remote-tracking branch 'facebook/master' into feature-settings-…
Mar 25, 2017
2ddc9e6
Add settings module for android
Mar 23, 2017
47fc744
Merge branch 'feature-settings-module-android' of github.com:skillz/r…
Mar 25, 2017
11ef48f
Fix Settings test
Mar 25, 2017
63d4d55
Add buck files for settings module
Mar 27, 2017
d5e7a55
Merge branch 'feature-settings-module-android' of github.com:skillz/r…
Mar 27, 2017
5b96905
fix settings buck file
Mar 27, 2017
57ca0d0
Add a default settings file name
Mar 27, 2017
bd5b23d
Add settings module for android
Mar 23, 2017
0fef2c3
Fix Settings test
Mar 25, 2017
dbb65a6
Add buck files for settings module
Mar 27, 2017
9b2a396
fix settings buck file
Mar 27, 2017
1aee785
Add a default settings file name
Mar 27, 2017
724c18c
Merge branch 'feature-settings-module-android' of github.com:skillz/r…
Mar 27, 2017
f776849
Merge remote-tracking branch 'facebook/master' into feature-settings-…
Mar 27, 2017
1214f84
Clarify tests. Make value conversion more abstract. Fix constants exp…
Apr 4, 2017
c1a7ed8
Merge branch 'feature-settings-module-android' of github.com:skillz/r…
Apr 4, 2017
4f11f67
Change Settings.ios.js to Settings.js in docsList.js
Apr 5, 2017
18e3561
Merge branch 'feature-settings-module-android' of github.com:skillz/r…
Apr 5, 2017
d494eaa
Rename setFilename to setSharedPreferencesFilename and javadoc-ed it.…
Apr 7, 2017
8ffa9c7
Merge remote-tracking branch 'facebook/master' into feature-settings-…
Apr 7, 2017
92371df
Merge remote-tracking branch 'facebook/master' into feature-settings-…
Apr 8, 2017
5c8d616
Make switch statement more efficient.
Apr 8, 2017
94bd032
Removed thrown exception when unexportable type is encountered.. Logg…
Apr 8, 2017
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 0 additions & 34 deletions Libraries/Settings/Settings.android.js

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -11,25 +11,44 @@
*/
'use strict';

var RCTDeviceEventEmitter = require('RCTDeviceEventEmitter');
var RCTSettingsManager = require('NativeModules').SettingsManager;
const RCTDeviceEventEmitter = require('RCTDeviceEventEmitter');
const RCTSettingsManager = require('NativeModules').SettingsManager;

var invariant = require('fbjs/lib/invariant');
const invariant = require('fbjs/lib/invariant');

var subscriptions: Array<{keys: Array<string>, callback: ?Function}> = [];
const subscriptions: Array<{keys: Array<string>; callback: ?Function}> = [];

var Settings = {
/**
* @class
* @description
* An interface to the native "preferences" file storage that uses simple key-value pairs
*
*/
const Settings = {
_settings: RCTSettingsManager && RCTSettingsManager.settings,

get(key: string): mixed {
return this._settings[key];
},

set(settings: Object) {
this._settings = Object.assign(this._settings, settings);
/**
* Sets the value for a name-value pairs
* @param settings the object with the name/value pairs to set
*
* Note that on android the only allowed value types are number, string and boolean
*/
set(settings: Object): void {
Object.assign(this._settings, settings);
RCTSettingsManager.setValues(settings);
},


/**
* Monitor one or more keys for changed values
* @param keys a string or an array of strings naming the keys to monitor
* @callback the callback to invoke when one of the specified keys updates its value
* @returns a number representing the watch ID which can be used to clear the watch
*/
watchKeys(keys: string | Array<string>, callback: Function): number {
if (typeof keys === 'string') {
keys = [keys];
Expand All @@ -40,11 +59,15 @@ var Settings = {
'keys should be a string or array of strings'
);

var sid = subscriptions.length;
const sid = subscriptions.length;
subscriptions.push({keys: keys, callback: callback});
return sid;
},

/**
*
* @param {*} watchId a number identifying which set of monitoried keys is to be cleared
*/
clearWatch(watchId: number) {
if (watchId < subscriptions.length) {
subscriptions[watchId] = {keys: [], callback: null};
Expand All @@ -53,8 +76,8 @@ var Settings = {

_sendObservations(body: Object) {
Object.keys(body).forEach((key) => {
var newValue = body[key];
var didChange = this._settings[key] !== newValue;
const newValue = body[key];
const didChange = this._settings[key] !== newValue;
this._settings[key] = newValue;

if (didChange) {
Expand Down
46 changes: 46 additions & 0 deletions Libraries/Settings/__tests__/Settings-test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
/**
* Copyright (c) 2015-present, Facebook, Inc.
* All rights reserved.
*
* This source code is licensed under the BSD-style license found in the
* LICENSE file in the root directory of this source tree. An additional grant
* of patent rights can be found in the PATENTS file in the same directory.
*/
'use strict';

jest.unmock('../Settings');
jest.unmock('Platform');

jest.unmock('BatchedBridge');
jest.unmock('defineLazyObjectProperty');
jest.unmock('MessageQueue');
jest.unmock('NativeModules');

let Platform = require('Platform');

const Settings = require('Settings');

describe('Settings', () => {

describe('setting and getting', () => {
it('should set values and retrieve them', () => {

if (Platform.OS == 'ios') {
expect(true).toEqual(true);
return;
}

Settings.set({oneKey: 1});
Settings.set({twoKey: 2.0});
Settings.set({threeKey: 'three'});

const oneValue = Settings.get('oneKey');
const twoValue = Settings.get('twoKey');
const threeValue = Settings.get('threeKey');

expect(oneValue === 1).toBe(true);
expect(twoValue === 2.0).toBe(true);
expect(threeValue === 'three').toBe(true);
});
});
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
include_defs("//ReactAndroid/DEFS")

android_library(
name = "settings",
srcs = glob(["**/*.java"]),
visibility = [
"PUBLIC",
],
deps = [
react_native_dep("third-party/java/infer-annotations:infer-annotations"),
react_native_dep("third-party/java/jsr-305:jsr-305"),
react_native_target("java/com/facebook/react/bridge:bridge"),
react_native_target("java/com/facebook/react/common:common"),
react_native_target("java/com/facebook/react/module/annotations:annotations"),
react_native_target("java/com/facebook/react/modules/core:core"),
],
)

Original file line number Diff line number Diff line change
@@ -0,0 +1,180 @@
/*
* Copyright (c) 2015-present, Facebook, Inc.
* All rights reserved.
*
* This source code is licensed under the BSD-style license found in the
* LICENSE file in the root directory of this source tree. An additional grant
* of patent rights can be found in the PATENTS file in the same directory.
*/

package com.facebook.react.modules.settings;

import android.content.Context;
import android.content.SharedPreferences;
import android.util.Log;

import com.facebook.react.bridge.Arguments;
import com.facebook.react.bridge.ReactApplicationContext;
import com.facebook.react.bridge.ReactContextBaseJavaModule;
import com.facebook.react.bridge.ReactMethod;
import com.facebook.react.bridge.ReadableMap;
import com.facebook.react.bridge.ReadableMapKeySetIterator;
import com.facebook.react.bridge.ReadableType;
import com.facebook.react.bridge.WritableMap;
import com.facebook.react.module.annotations.ReactModule;
import com.facebook.react.modules.core.DeviceEventManagerModule;

import java.util.HashMap;
import java.util.Map;

@ReactModule(name = SettingsModule.NAME)
public class SettingsModule extends ReactContextBaseJavaModule implements SharedPreferences.OnSharedPreferenceChangeListener{

@SuppressWarnings("WeakerAccess")
static final String NAME = "SettingsManager";
static final String DEFAULT_FILE_NAME = "ReactNative";
static final String TAG = "SettingsMmodule";

private static boolean sSettingsModuleInstantiated = false;

static private String sFilename = "";
private Boolean ignoringUpdates = false;
private SharedPreferences mPreferences;

public SettingsModule(ReactApplicationContext context) {
super(context);

}

@Override
public String getName() {
return SettingsModule.NAME;
}

private SharedPreferences ensurePreferences() {
if (mPreferences == null) {
if (SettingsModule.sFilename.length() > 0) {
mPreferences = getReactApplicationContext().getSharedPreferences(SettingsModule.sFilename, Context.MODE_PRIVATE);
} else if (getCurrentActivity() != null) {
mPreferences = getCurrentActivity().getPreferences(Context.MODE_PRIVATE);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@greghe greghe Apr 6, 2017

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 vonovak Apr 6, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 greghe Apr 7, 2017

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

} else {
mPreferences = getReactApplicationContext().getSharedPreferences(DEFAULT_FILE_NAME, Context.MODE_PRIVATE);
}
sSettingsModuleInstantiated = true;
mPreferences.registerOnSharedPreferenceChangeListener(this);
}

return mPreferences;
}

/***
* Configure the Settings Module to use a SharedPreferences file with a specific name as its backing store.
* Useful if the Settings Module needs to use an existing SharedPreferences file or to ensure that Settings Modules
* belonging to different ReactPackages will all share the same SharedPreferences file for their backing store.
* <p></p>
* <b>Note:</b>this method may only be called before a Settings Module initialization (i.e. before a ReactMainPackage is instantiated)
* An app would typically invoke this function in the app's main activity before setting up the React Native environment
* @param filename the name of the SharedPreferences file to use or create for the backing store
* @throws IllegalStateException if called after instantiation of a Settings module
*/
static public void setSharedPreferencesFilename(String filename) {
if (sSettingsModuleInstantiated) {
throw new IllegalStateException("Attempt to set Settings Module SharedPreferences file name after a Settings Module instantiation");
}
sFilename = filename;
}

@Override
public void onCatalystInstanceDestroy() {
ensurePreferences().unregisterOnSharedPreferenceChangeListener(this);
}

@ReactMethod
public void setValues(ReadableMap values) {
this.ignoringUpdates = true;

ReadableMapKeySetIterator iterator = values.keySetIterator();
SharedPreferences.Editor editor = mPreferences.edit();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


while (iterator.hasNextKey()) {
String key = iterator.nextKey();
ReadableType type = values.getType(key);

switch (type) {
case Null:
editor.remove(key);
break;
case Boolean:
editor.putBoolean(key, values.getBoolean(key));
break;
case Number:
editor.putFloat(key, (float) values.getDouble(key));

@vonovak vonovak Apr 10, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 greghe Apr 11, 2017

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 vonovak Apr 12, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

break;
case String:
editor.putString(key, values.getString(key));
break;
case Map: // Not supported
throw new IllegalArgumentException(TAG + ": Cannot not store Map as value in Settings");
case Array: // Not supported
throw new IllegalArgumentException(TAG + ": Cannot not store Array as value in Settings");
}
}
editor.apply();

this.ignoringUpdates = false;
}

public Map<String, Object> getConstants() {
Map<String, Object> exportedValuePairs = new HashMap<String, Object>();
Map<String, ?> prefsMap = ensurePreferences().getAll();

for (String key : prefsMap.keySet()) {
Object value = prefsMap.get(key);
if (value instanceof String || value.getClass().isPrimitive()) {
exportedValuePairs.put(key, value);
} else {
// StringSets are not exported - could be used in the future to store JSON encoded arrays and maps
Log.d(TAG, "unexported setting: " + key);
}
}

HashMap<String, Object> constants = new HashMap<>();

constants.put("settings", exportedValuePairs);
return constants;
}

@Override
public void onSharedPreferenceChanged(SharedPreferences sharedPreferences, String prefKey) {
if (ignoringUpdates) {
return;
}
Map<String, ?> prefsMap = ensurePreferences().getAll();

WritableMap map = Arguments.createMap();

for (String key: prefsMap.keySet()) {
Object value = prefsMap.get(key);

if (value == null) {
map.putNull(key);
} else if (value instanceof String) {
map.putString(key, (String) value);
} else if (value instanceof Number) {
if (value instanceof Integer) {
map.putInt(key, (Integer) value);
} else {
map.putDouble(key, ((Number) value).doubleValue());
}
} else if (value instanceof Boolean) {
map.putBoolean(key, (Boolean) value);
} else {
Log.d(TAG, "ignoring setting: " + key);
}
}
if (getReactApplicationContext().hasActiveCatalystInstance()) {
getReactApplicationContext().getJSModule(DeviceEventManagerModule.RCTDeviceEventEmitter.class)
.emit("settingsUpdated", map);
}
}
}
1 change: 1 addition & 0 deletions ReactAndroid/src/main/java/com/facebook/react/shell/BUCK
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ android_library(
react_native_target("java/com/facebook/react/modules/network:network"),
react_native_target("java/com/facebook/react/modules/permissions:permissions"),
react_native_target("java/com/facebook/react/modules/share:share"),
react_native_target("java/com/facebook/react/modules/settings:settings"),
react_native_target("java/com/facebook/react/modules/statusbar:statusbar"),
react_native_target("java/com/facebook/react/modules/storage:storage"),
react_native_target("java/com/facebook/react/modules/timepicker:timepicker"),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@
import com.facebook.react.modules.netinfo.NetInfoModule;
import com.facebook.react.modules.network.NetworkingModule;
import com.facebook.react.modules.permissions.PermissionsModule;
import com.facebook.react.modules.settings.SettingsModule;
import com.facebook.react.modules.share.ShareModule;
import com.facebook.react.modules.statusbar.StatusBarModule;
import com.facebook.react.modules.storage.AsyncStorageModule;
Expand Down Expand Up @@ -211,6 +212,12 @@ public NativeModule get() {
return new PermissionsModule(context);
}
}),
new ModuleSpec(SettingsModule.class, new Provider<NativeModule>() {
@Override
public NativeModule get() {
return new SettingsModule(context);
}
}),
new ModuleSpec(ShareModule.class, new Provider<NativeModule>() {
@Override
public NativeModule get() {
Expand Down
2 changes: 1 addition & 1 deletion website/server/docsList.js
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ const apis = [
'../Libraries/PermissionsAndroid/PermissionsAndroid.js',
'../Libraries/Utilities/PixelRatio.js',
'../Libraries/PushNotificationIOS/PushNotificationIOS.js',
'../Libraries/Settings/Settings.ios.js',
'../Libraries/Settings/Settings.js',
'../Libraries/Share/Share.js',
'../Libraries/Components/StatusBar/StatusBarIOS.ios.js',
'../Libraries/StyleSheet/StyleSheet.js',
Expand Down