Skip to content
Closed
Show file tree
Hide file tree
Changes from 16 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) {
/**
* 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 {
this._settings = Object.assign(this._settings, settings);

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.

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

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 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.

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).toEqual(true);

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 believe using toBe() can improve readability. Either by saying expect(oneValue === 1).toBe(true); or expect(oneValue).toBe(1);

expect(twoValue === 2.0).toEqual(true);
expect(threeValue === 'three').toEqual(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,167 @@
/*
* 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 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.ReadableNativeMap;
import com.facebook.react.bridge.WritableMap;
import com.facebook.react.module.annotations.ReactModule;
import com.facebook.react.modules.core.DeviceEventManagerModule;

import java.util.ArrayList;
import java.util.HashMap;
import java.util.HashSet;
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 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 getPreferences() {
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);
}
mPreferences.registerOnSharedPreferenceChangeListener(this);
}

return mPreferences;
}

static public void setFilename(String filename) {
sFilename = filename;
}

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

@ReactMethod
public void setValues(ReadableMap map) {
ReadableNativeMap nativeMap = (ReadableNativeMap) map;

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.

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?

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.

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


this.ignoringUpdates = true;
this.setValuesToPreference(nativeMap.toHashMap(), getPreferences());
this.ignoringUpdates = false;
}

@SuppressWarnings("unchecked")
public Map<String, Object> getConstants() {
Map<String, Object> map;

try {
map = (Map<String, Object>) getPreferences().getAll();

for (String key : map.keySet()) {
Object value = map.get(key);
if (value instanceof HashSet) {
ArrayList<Object> list = new ArrayList<>((HashSet<Object>) value);
map.put(key, list);

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.

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.

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.

Should be fixed now.

}

}
} catch (ClassCastException ex) {
map = new HashMap<>();
}

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

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

@Override
public void onSharedPreferenceChanged(SharedPreferences sharedPreferences, String prefKey) {
if (ignoringUpdates) {
return;
}
Map<String, ?> prefsMap = getPreferences().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 {
throw new IllegalArgumentException("Could not convert " + value.getClass());

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 think a little less generic message would be great, something that tells you right away what module the error relates to.

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.

will do

}
}
if (getReactApplicationContext().hasActiveCatalystInstance()) {
getReactApplicationContext().getJSModule(DeviceEventManagerModule.RCTDeviceEventEmitter.class)
.emit("settingsUpdated", map);
}
}

private void setValuesToPreference(HashMap<String, Object> sourceMap, SharedPreferences pref) {
SharedPreferences.Editor editor = pref.edit();

for (HashMap.Entry<String, Object> entry : sourceMap.entrySet()) {
String key = entry.getKey();
Object value = entry.getValue();

if (value != null && !value.getClass().isPrimitive() && !(value instanceof String)) {
continue;
}

if (value == null) {
editor.remove(key);
} else if (value instanceof String) {
editor.putString(key, (String) sourceMap.get(key));
} else if (value instanceof Number) {
Float valueF = ((Number) value).floatValue();
editor.putFloat(key, valueF);
} if (value instanceof Boolean) {
editor.putBoolean(key, (Boolean) sourceMap.get(key));
}

}
editor.apply();
}
}
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