[shared_preferences] Handling multiple files on Android #2040

axel-op · 2019-09-04T14:48:33Z

Description

This PR gives the possibility to use multiple files to save the SharedPreferences.
This for now only works on Android. On iOs, the previous behaviour is preserved.
See flutter/flutter#14337 (comment).

Related Issues

Fix flutter/flutter#14337

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

Breaking Change

Does your PR require plugin users to manually update their apps to accommodate your change?

Yes, this is a breaking change (please indicate a breaking change in CHANGELOG.md and increment major revision).
No, this is not a breaking change.

collinjackson · 2019-10-17T18:26:34Z

@mehmetf do you have any opinions on this? It relates to your comment on another shared_preferences PR.

mehmetf · 2019-10-17T20:02:18Z

This PR is fine.

I believe SharedPreferences instances are cached by name: https://developer.android.com/reference/android/content/Context#getSharedPreferences(java.lang.String,%20int) . So this should not be a noticeable change. It would be good to validate that there's no performance regression by calling putString() N times before and after.

collinjackson · 2019-10-17T20:25:23Z

packages/shared_preferences/lib/shared_preferences.dart

-  static void setMockInitialValues(Map<String, dynamic> values) {
-    _kChannel.setMockMethodCallHandler((MethodCall methodCall) async {
+  void setMockInitialValues(Map<String, dynamic> values) {
+    throw UnimplementedError("Mocking function seems unfinished");


I'm not sure why you commented this out. Would it make sense to bring back? It is useful for unit testing apps that rely on the SharedPreferences plugin (although now that we have e2e testing that might be a reasonable alternative).

@collinjackson I don't understand the purpose of this function. As you can see, its implementation doesn't seem complete. What do you want me to do with this exactly?

This method is used by apps that want to set mock SharedPreferences values for testing. It shouldn't be commented out.

axel-op · 2019-10-19T00:17:08Z

This PR is fine.
I believe SharedPreferences instances are cached by name: https://developer.android.com/reference/android/content/Context#getSharedPreferences(java.lang.String,%20int) . So this should not be a noticeable change. It would be good to validate that there's no performance regression by calling putString() N times before and after.

@mehmetf Do you want me to do this myself?

mehmetf · 2019-10-19T00:22:55Z

@axel-op Yes. We would not want to rollback the change because it introduced a regression.

collinjackson · 2019-10-31T18:13:16Z

@axel-op are you still working on this? I'm happy to help land this one @mehmetf's feedback is addressed.

axel-op · 2019-11-27T16:31:17Z

I believe SharedPreferences instances are cached by name: https://developer.android.com/reference/android/content/Context#getSharedPreferences(java.lang.String,%20int) . So this should not be a noticeable change. It would be good to validate that there's no performance regression by calling putString() N times before and after.

@mehmetf Here are my results with N = 10^5 (time is in microseconds):

.	Previous impl / write	Previous impl / read	New impl / write	New impl / read
min	28	0	36	0
max	149136	347	849831	452
mean	115.6	0.1	152.5	0.1
std dev	1098.9	1.6	4431.5	1.9

Is this what you wanted?

axel-op · 2019-11-27T16:58:27Z

After creating some sort of cache on the Java side with a HashMap, here's what I get:

.	New impl with caching / write
min	35
max	124522
mean	84.27
std dev	782.10

However I'm not sure these differences are significant

axel-op · 2019-11-27T17:00:24Z

Furthermore, I'm struggling with the setMockInitialValues method with this new implementation. Could you help me with this one?

… older Java versions

mehmetf

Thanks for doing the experiment. The numbers look find with caching.

packages/shared_preferences/shared_preferences/lib/shared_preferences.dart

mehmetf · 2019-11-28T00:17:03Z

packages/shared_preferences/shared_preferences/lib/shared_preferences.dart

+  ///
+  /// For Android, see https://developer.android.com/training/data-storage/shared-preferences.html for more details on the platform implementation.
+  static Future<SharedPreferences> getInstance(
+      {String filename = "FlutterSharedPreferences"}) async {


I would leave this API alone and define a new one for Android specifically:

getAndroidPreferencesForFile(String preferencesFile)

Otherwise, it is misleading. Your application code will potentially contain multiple shared preferences instances and on iOS, these will clobber each other.

I created the method getInstanceForFile, which will throw an AssertionError if the current platform isn't Android. What do you think?

Thanks Axel.

The more I think about it, the more I am worried about this API. What is your primary motivation?

Do you intend to create multiple files backing different shared preferences?

or are you in a migration scenario where you have an existing app that is using a different file name and you want to hook shared preferences plugin to it? (and you have no intention to use multiple files?)

The main problem with #1 is that multiple files is not supported on iOS so your app needs to be structured and behave differently on iOS when it comes to preferences. That's a tall order because this is not just a simple switch. For instance, on iOS, if you wanted to separate your preferences, you would need to namespace them. Let's say you were separating preferences for each user. You would do this on iOS:

user_id1:preference1
user_id1:preference2
user_id2:preference1
user_id2:preference2

but then you would write to separate files on Android? Why? You could do the same on Android too and you would not have to worry about differing behavior on different platforms.

I apologize for not spotting this earlier. This PR has been open for a while and we did not give proper attention to it.

My intention was to use multiple files in Flutter to separate the data. It seemed to me that it could be easier to manage because then you can use functions like clear and getAll on an entire file without the need to filter the preferences.
The reason why I didn't code the iOs-native part is because I don't have the knowledge to do so, but with your comment I now understand that it cannot be done at all. So it's probably better to not change the API indeed.

…le on Android

mehmetf · 2020-01-09T04:24:47Z

I am going to close this PR. Please see my update above date Nov 28 about my concerns with the API. If you want to do a migration from a non-conforming file, you can do so entirely in native code and then use this plugin.

axel-op requested a review from collinjackson as a code owner September 4, 2019 14:48

googlebot added the cla: yes label Sep 4, 2019

collinjackson reviewed Oct 17, 2019

View reviewed changes

collinjackson added in review labels Oct 31, 2019

[shared_preferences] Handling multiple files on Android

19c8734

axel-op force-pushed the master branch from 18bf1bb to 19c8734 Compare November 27, 2019 15:22

[shared_preferences] Cache Java instances

7627c2e

axel-op added 2 commits November 27, 2019 18:04

Format

7e9f147

[shared_preferences] Replace lambda expression for compatibility with…

5cdcecb

… older Java versions

axel-op force-pushed the master branch from 99505a9 to 5cdcecb Compare November 27, 2019 19:39

Fix a logic error

c8a8384

mehmetf reviewed Nov 28, 2019

View reviewed changes

axel-op added 2 commits November 28, 2019 12:10

[shared_preferences] Create a new method to get an instance with a fi…

2388916

…le on Android

Merge remote-tracking branch 'upstream/master' (CONTAINS CONFLICTS)

66aa50d

axel-op force-pushed the master branch from 2d820da to 66aa50d Compare November 28, 2019 11:48

mehmetf closed this Jan 9, 2020

julianscheel pushed a commit to jusst-engineering/plugins that referenced this pull request Mar 11, 2020

Add pedantic to all Firebase plugins (flutter#2040)

ca7eb26

moldstadt mentioned this pull request Mar 28, 2020

Allow setting prefix in shared preference - for migrating from old app that did not use flutter flutter/flutter#52544

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[shared_preferences] Handling multiple files on Android #2040

[shared_preferences] Handling multiple files on Android #2040

axel-op commented Sep 4, 2019 •

edited

Loading

collinjackson commented Oct 17, 2019

mehmetf commented Oct 17, 2019

collinjackson Oct 17, 2019

axel-op Oct 19, 2019

collinjackson Oct 31, 2019

axel-op commented Oct 19, 2019

mehmetf commented Oct 19, 2019

collinjackson commented Oct 31, 2019

axel-op commented Nov 27, 2019

axel-op commented Nov 27, 2019

axel-op commented Nov 27, 2019 •

edited

Loading

mehmetf left a comment

mehmetf Nov 28, 2019

axel-op Nov 28, 2019

mehmetf Nov 28, 2019

mehmetf Nov 28, 2019

axel-op Jan 21, 2020

mehmetf commented Jan 9, 2020

[shared_preferences] Handling multiple files on Android #2040

[shared_preferences] Handling multiple files on Android #2040

Conversation

axel-op commented Sep 4, 2019 • edited Loading

Description

Related Issues

Checklist

Breaking Change

collinjackson commented Oct 17, 2019

mehmetf commented Oct 17, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

axel-op commented Oct 19, 2019

mehmetf commented Oct 19, 2019

collinjackson commented Oct 31, 2019

axel-op commented Nov 27, 2019

axel-op commented Nov 27, 2019

axel-op commented Nov 27, 2019 • edited Loading

mehmetf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mehmetf commented Jan 9, 2020

axel-op commented Sep 4, 2019 •

edited

Loading

axel-op commented Nov 27, 2019 •

edited

Loading