Skip to content
Merged
Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
e772224
update List<String> encode/decode and other simple breaking changes
tarrinneal Dec 20, 2024
089d11d
fix test set up and legacy android system
tarrinneal Jan 2, 2025
29f3662
Merge branch 'main' of github.com:flutter/packages into listString-en…
tarrinneal Jan 2, 2025
c0c08df
format analyze update to latest pigeon
tarrinneal Jan 3, 2025
2ef0a01
remove unneeded braces from switch statement
tarrinneal Jan 3, 2025
bad5268
revert all breaking changes
tarrinneal Jan 6, 2025
7a013b1
Merge branch 'main' of github.com:flutter/packages into listString-en…
tarrinneal Jan 6, 2025
2c10fb7
rest of the comments
tarrinneal Jan 6, 2025
614cbe4
update test getter to match setter
tarrinneal Jan 6, 2025
e622f04
fix tests that I broke fixing tests
tarrinneal Jan 7, 2025
d317354
fix android code for error handling changes reversion
tarrinneal Jan 7, 2025
79ebe43
split getList and other nits
tarrinneal Jan 14, 2025
3a45c25
Merge branch 'main' of github.com:flutter/packages into listString-en…
tarrinneal Jan 14, 2025
a49fb9f
add new method to kotlin class
tarrinneal Jan 16, 2025
e1f8fab
fix test
tarrinneal Jan 17, 2025
96d129b
comments and test fixes
tarrinneal Jan 24, 2025
e89421c
rename setStringList methods
tarrinneal Jan 24, 2025
88ce597
Merge branch 'main' of github.com:flutter/packages into listString-en…
tarrinneal Jan 24, 2025
7e28547
fix kotlin tests and format
tarrinneal Jan 24, 2025
1cdce8d
fix java tests too
tarrinneal Jan 24, 2025
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
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
## 2.4.1

* Moves `List<String>` encoding to dart `JSON` package.

## 2.4.0

* Adds `SharedPreferences` support within `SharedPreferencesAsyncAndroid` API.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,18 +25,21 @@
import java.util.HashSet;
import java.util.List;
import java.util.Map;
import java.util.Objects;
import java.util.Set;

/** LegacySharedPreferencesPlugin */
public class LegacySharedPreferencesPlugin implements FlutterPlugin, SharedPreferencesApi {
private static final String TAG = "SharedPreferencesPlugin";
private static final String SHARED_PREFERENCES_NAME = "FlutterSharedPreferences";
// All identifiers must match the SharedPreferencesPlugin.kt file, as well as the strings.dart file.
private static final String JSON_LIST_IDENTIFIER = "VGhpcyBpcyB0aGUgcHJlZml4IGZvciBhIGxpc3Qu!";
private static final String LIST_IDENTIFIER = "VGhpcyBpcyB0aGUgcHJlZml4IGZvciBhIGxpc3Qu";
private static final String BIG_INTEGER_PREFIX = "VGhpcyBpcyB0aGUgcHJlZml4IGZvciBCaWdJbnRlZ2Vy";
private static final String DOUBLE_PREFIX = "VGhpcyBpcyB0aGUgcHJlZml4IGZvciBEb3VibGUu";

private SharedPreferences preferences;
private SharedPreferencesListEncoder listEncoder;
private final SharedPreferencesListEncoder listEncoder;

public LegacySharedPreferencesPlugin() {
this(new ListEncoder());
Expand Down Expand Up @@ -100,7 +103,15 @@ public void onDetachedFromEngine(@NonNull FlutterPlugin.FlutterPluginBinding bin
}

@Override
public @NonNull Boolean setStringList(@NonNull String key, @NonNull List<String> value)
public @NonNull Boolean setStringList(@NonNull String key, @NonNull String value)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Couldn't we eliminate this method from the Pigeon protocol and just change the setString check to ((value.startsWith(LIST_IDENTIFIER) && ! value.startsWith(JSON_LIST_IDENTIFIER) || ...? Since the structure of the new design is that the Dart code is responsible for encoding it to just be a string, it seems odd to have a dedicated setter for that string.

Copy link
Contributor Author

@tarrinneal tarrinneal Jan 23, 2025

Choose a reason for hiding this comment

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

we could, but then the method could be used to add a string with that json list identifier, which is the whole point of the check.

I could move the check for that one prefix to dart though, if that is what you think is best.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah, right. I guess this is fine then.

Ideally what we would do is move all of the prefixing logic to Dart; the weirdness here is because we're now doing some special casing in native code, and some in Dart. But that's more refactoring than this PR needs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I don't love it. Moving the checks without updating the error type seems silly.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I've done it in some other plugins, with a comment explaining why I'm throwing a weird PlatformException from Dart. It's a different kind of odd, but I don't think it's silly, since it allows us to make progress on moving logic to Dart, which in many cases makes our lives easier, without gating it behind breaking changes.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's leave refactoring the existing logic to future work if we find that we are changing this enough to warrant the time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, for my understanding though. Is this error I wrote:

PlatformException(java.lang.RuntimeException: StorageError: This string cannot be stored as it
clashes with special identifier prefixes, RuntimeException, Cause: null, Stacktrace:
java.lang.RuntimeException: StorageError: This string cannot be stored as it clashes with special
identifier prefixes, null)

similar enough to

PlatformException(java.lang.RuntimeException: StorageError: This string cannot be stored as it
clashes with special identifier prefixes, RuntimeException, Cause: null, Stacktrace:
java.lang.RuntimeException: StorageError: This string cannot be stored as it clashes with special
identifier prefixes
        at
io.flutter.plugins.sharedpreferences.LegacySharedPreferencesPlugin.setString(LegacySharedPreferencesPlugin.java:83)
        at
io.flutter.plugins.sharedpreferences.Messages$SharedPreferencesApi$-CC.lambda$setUp$2(Messages.java:204)
        at
io.flutter.plugins.sharedpreferences.Messages$SharedPreferencesApi$$ExternalSyntheticLambda2.onMessage(D8$$SyntheticClass:0)
        at
io.flutter.plugin.common.BasicMessageChannel$IncomingMessageHandler.onMessage(BasicMessageChannel.java:261)
        at io.flutter.embedding.engine.dart.DartMessenger.invokeHandler(DartMessenger.java:292)
        at
io.flutter.embedding.engine.dart.DartMessenger.lambda$dispatchMessageToQueue$0$io-flutter-embedding-engine-dart-DartMessenger(DartMessenger.java:319)
        at
io.flutter.embedding.engine.dart.DartMessenger$$ExternalSyntheticLambda0.run(D8$$SyntheticClass:0)
        at io.flutter.embedding.engine.dart.DartMessenger$SerialTaskQueue.flush(DartMessenger.java:173)
        at
io.flutter.embedding.engine.dart.DartMessenger$SerialTaskQueue.lambda$dispatch$0$io-flutter-embedding-engine-dart-DartMessenger$SerialTaskQueue(DartMessenger.java:163)
        at
io.flutter.embedding.engine.dart.DartMessenger$SerialTaskQueue$$ExternalSyntheticLambda0.run(D8$$SyntheticClass:0)
        at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1137)
        at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:637)
        at java.lang.Thread.run(Thread.java:1012)
, null)
```?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think I can replicate the entire stack trace reliably

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, if someone is relying on the format of the stack trace, they are going to have a bad time. The important things to preserve are:

  • code, since that's the most likely thing for someone to match on, and
  • message, which is debatable, and depends a lot on the code; some of our plugins use the same code for everything so people trying to distinguish have, for lack of a better option, done string matching on the message, so in those cases I try to preserve the message too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Another important thing to note is that this error suffers from the pigeon error code swapping issue as well. (I couldn't find the filed issue to link)

throws RuntimeException {
return preferences.edit().putString(key, value).commit();
}

// Deprecated, for testing purposes only.
@Deprecated
@Override
public @NonNull Boolean setDeprecatedStringList(@NonNull String key, @NonNull List<String> value)
throws RuntimeException {
return preferences.edit().putString(key, LIST_IDENTIFIER + listEncoder.encode(value)).commit();
}
Expand Down Expand Up @@ -131,14 +142,13 @@ public void onDetachedFromEngine(@NonNull FlutterPlugin.FlutterPluginBinding bin

// Gets all shared preferences, filtered to only those set with the given prefix.
// Optionally filtered also to only those items in the optional [allowList].
@SuppressWarnings("unchecked")
private @NonNull Map<String, Object> getAllPrefs(
@NonNull String prefix, @Nullable Set<String> allowList) throws RuntimeException {
Map<String, ?> allPrefs = preferences.getAll();
Map<String, Object> filteredPrefs = new HashMap<>();
for (String key : allPrefs.keySet()) {
if (key.startsWith(prefix) && (allowList == null || allowList.contains(key))) {
filteredPrefs.put(key, transformPref(key, allPrefs.get(key)));
filteredPrefs.put(key, transformPref(key, Objects.requireNonNull(allPrefs.get(key))));
Copy link
Contributor

Choose a reason for hiding this comment

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

What was requireNonNull needed here? Also this code is getting a bit difficult to parse. (A loop with a conditional that sets some transformed filtered data) Consider breaking parts out into methods or adding comments.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a change suggested by a warning I was getting in Android Studio. Since the method being called requires nonnull arguments and getting a value from a map doesn't programmatically guarantee a value. In this context, there would always be a value though.

}
}

Expand All @@ -149,7 +159,13 @@ private Object transformPref(@NonNull String key, @NonNull Object value) {
if (value instanceof String) {
String stringValue = (String) value;
if (stringValue.startsWith(LIST_IDENTIFIER)) {
return listEncoder.decode(stringValue.substring(LIST_IDENTIFIER.length()));
// The JSON-encoded lists use an extended prefix to distinguish them from
// lists that are encoded on the platform.
if (stringValue.startsWith(JSON_LIST_IDENTIFIER)) {
return value;
} else {
return listEncoder.decode(stringValue.substring(LIST_IDENTIFIER.length()));
}
} else if (stringValue.startsWith(BIG_INTEGER_PREFIX)) {
// TODO (tarrinneal): Remove all BigInt code.
// https://github.com/flutter/flutter/issues/124420
Expand Down
Loading
Loading