Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[shared_preferences] Platform implementations of async api #6965

Merged
merged 23 commits into from
Jul 30, 2024
Merged

[shared_preferences] Platform implementations of async api #6965

Show file tree
Hide file tree
Changes from 18 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
0b619b4
plat
tarrinneal Jul 18, 2024
b8b4643
analyze
tarrinneal Jul 18, 2024
3216359
newer version
tarrinneal Jul 18, 2024
351e189
revert gradle changes
tarrinneal Jul 18, 2024
167a191
revert gradle wrapper
tarrinneal Jul 19, 2024
751b7ab
gradle
tarrinneal Jul 19, 2024
966cc82
bom
tarrinneal Jul 19, 2024
f5a7965
move bom
tarrinneal Jul 19, 2024
6d2127b
new kotlin version
tarrinneal Jul 20, 2024
5e80acb
rename classes and files
tarrinneal Jul 22, 2024
6a148c4
legacy
tarrinneal Jul 22, 2024
f3002f1
more legacy
tarrinneal Jul 22, 2024
48a294f
one more legacy
tarrinneal Jul 23, 2024
9d4ce28
anotha one
tarrinneal Jul 23, 2024
2b7b3a6
fix tests
tarrinneal Jul 23, 2024
fa7389d
revert test fix, fix real issue
tarrinneal Jul 23, 2024
bc85c30
fix explode
tarrinneal Jul 24, 2024
9780d68
format
tarrinneal Jul 24, 2024
9249648
revert kotlin version, update changelog
tarrinneal Jul 25, 2024
475237f
bom
tarrinneal Jul 26, 2024
94498a0
Merge branch 'main' of github.com:flutter/packages into spr-plat
tarrinneal Jul 26, 2024
78ace3a
move bom
tarrinneal Jul 26, 2024
89ed80a
kotlin version, how do they work?
tarrinneal Jul 27, 2024
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
5 changes: 5 additions & 0 deletions packages/shared_preferences/shared_preferences_android/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,8 @@
## 2.3.0

* Renames `SharedPreferencesAndroid` to `LegacySharedPreferencesAndroid`.
* Creates new `SharedPreferencesAndroid` that extends `SharedPreferencesAsyncPlatform`.

## 2.2.3

* Updates minimum supported SDK version to Flutter 3.22/Dart 3.4.
Expand Down
19 changes: 18 additions & 1 deletion packages/shared_preferences/shared_preferences_android/android/build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,15 @@ group 'io.flutter.plugins.sharedpreferences'
version '1.0-SNAPSHOT'

buildscript {
ext.kotlin_version = '1.8.10'
Copy link
Contributor

Choose a reason for hiding this comment

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

@reidbaker Is this going to cause issues for clients? It looks like this would be the first of our plugins using 1.8.x.

@tarrinneal What exactly was the error when this was 1.7.10 like our other plugins?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://logs.chromium.org/logs/flutter/buildbucket/cr-buildbucket/8741408197539124465/+/u/Run_package_tests/native_unit_tests/stdout?format=raw

Integration tests run fine though, only unit tests fail. There may be some other underlying issue.

repositories {
google()
mavenCentral()
}

dependencies {
classpath 'com.android.tools.build:gradle:7.2.2'
classpath "org.jetbrains.kotlin:kotlin-gradle-plugin:$kotlin_version"
}
}

Expand All @@ -28,6 +30,7 @@ allprojects {
}

apply plugin: 'com.android.library'
apply plugin: 'kotlin-android'

android {
// Conditional for compatibility with AGP <4.2.
Expand All @@ -41,18 +44,32 @@ android {
targetCompatibility JavaVersion.VERSION_1_8
}

kotlinOptions {
jvmTarget = '1.8'
}

sourceSets {
main.java.srcDirs += 'src/main/kotlin'
}

defaultConfig {
minSdkVersion 19
testInstrumentationRunner "androidx.test.runner.AndroidJUnitRunner"
}
lintOptions {
checkAllWarnings true
warningsAsErrors true
disable 'AndroidGradlePluginVersion', 'InvalidPackage', 'GradleDependency'
disable 'AndroidGradlePluginVersion', 'InvalidPackage', 'GradleDependency', 'NewerVersionAvailable'
}
dependencies {
implementation 'androidx.datastore:datastore:1.0.0'
implementation 'androidx.datastore:datastore-preferences:1.0.0'
testImplementation 'junit:junit:4.13.2'
testImplementation 'androidx.test:core-ktx:1.5.0'
testImplementation 'androidx.test.ext:junit-ktx:1.1.5'
testImplementation 'org.robolectric:robolectric:4.12.1'
testImplementation 'org.mockito:mockito-inline:5.2.0'
testImplementation 'io.mockk:mockk:1.13.12'
}


Expand Down
12 changes: 6 additions & 6 deletions ...dpreferences/SharedPreferencesPlugin.java → ...rences/LegacySharedPreferencesPlugin.java
View file
Open in desktop
Copy link
Contributor

Choose a reason for hiding this comment

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

Are you stuck on legacy as the prefix? The meaning of legacy can change over time so v1 might be a better prefix.

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 think legacy is pretty fitting, as it is now legacy code. It implies future intent to deprecate the code

Copy link
Contributor

Choose a reason for hiding this comment

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

The issue is legacy assumes 2 states, Legacy and Current and does not account for a 3rd state to exist at the same time.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is something I don't remember to think about until you raise it in reviews, but I'm trying to internalize it into my normal review process :)

I agree it would have been better not to use that name, but in this case I think it'll end up okay, because this was a big change to an API surface that we very explicitly don't want a lot of churn to, and hasn't been fundamentally changed like this in the entire life of the plugin. It's a very different case than, e.g., Android older API version codepaths, where this has come up before, where we can easily get new ones every year.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep I did not view it as a blocker which was why it came paired with an approval. It was just the dismissal that "I think legacy is pretty fitting, as it is now legacy code." that I wanted to expand upon since that misses my entire reason for commenting on the name.

I should have linked to the style guide. https://github.com/flutter/flutter/blob/master/docs/contributing/Style-guide-for-Flutter-repo.md#avoid-newold-modifiers-in-code

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This code, and the names specifically, can also change any time without breaking anything, as this is just the native platform implementation. If there does end up being another massive overhaul before it's deprecated, we can just change it to something else at that 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.

There's always LegacyLegacy ;)

Copy link
Contributor

Choose a reason for hiding this comment

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

yes there is hahaha 😭

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 was just the dismissal that "I think legacy is pretty fitting, as it is now legacy code." that I wanted to expand upon since that misses my entire reason for commenting on the name.

Totally fair, I was missing the expanded context so I appreciate the comment :)

Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,8 @@
import java.util.Map;
import java.util.Set;

/** SharedPreferencesPlugin */
public class SharedPreferencesPlugin implements FlutterPlugin, SharedPreferencesApi {
/** LegacySharedPreferencesPlugin */
public class LegacySharedPreferencesPlugin implements FlutterPlugin, SharedPreferencesApi {
private static final String TAG = "SharedPreferencesPlugin";
private static final String SHARED_PREFERENCES_NAME = "FlutterSharedPreferences";
private static final String LIST_IDENTIFIER = "VGhpcyBpcyB0aGUgcHJlZml4IGZvciBhIGxpc3Qu";
Expand All @@ -38,19 +38,19 @@ public class SharedPreferencesPlugin implements FlutterPlugin, SharedPreferences
private SharedPreferences preferences;
private SharedPreferencesListEncoder listEncoder;

public SharedPreferencesPlugin() {
public LegacySharedPreferencesPlugin() {
this(new ListEncoder());
}

@VisibleForTesting
SharedPreferencesPlugin(@NonNull SharedPreferencesListEncoder listEncoder) {
LegacySharedPreferencesPlugin(@NonNull SharedPreferencesListEncoder listEncoder) {
this.listEncoder = listEncoder;
}

private void setUp(@NonNull BinaryMessenger messenger, @NonNull Context context) {
preferences = context.getSharedPreferences(SHARED_PREFERENCES_NAME, Context.MODE_PRIVATE);
try {
SharedPreferencesApi.setup(messenger, this);
SharedPreferencesApi.setUp(messenger, this);
} catch (Exception ex) {
Log.e(TAG, "Received exception while setting up SharedPreferencesPlugin", ex);
}
Expand All @@ -63,7 +63,7 @@ public void onAttachedToEngine(@NonNull FlutterPlugin.FlutterPluginBinding bindi

@Override
public void onDetachedFromEngine(@NonNull FlutterPlugin.FlutterPluginBinding binding) {
SharedPreferencesApi.setup(binding.getBinaryMessenger(), null);
SharedPreferencesApi.setUp(binding.getBinaryMessenger(), null);
}

@Override
Expand Down
20 changes: 10 additions & 10 deletions ...ferences_android/android/src/main/java/io/flutter/plugins/sharedpreferences/Messages.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
// Copyright 2013 The Flutter Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
// Autogenerated from Pigeon (v9.2.5), do not edit directly.
// Autogenerated from Pigeon (v16.0.4), do not edit directly.
// See also: https://pub.dev/packages/pigeon

package io.flutter.plugins.sharedpreferences;
Expand Down Expand Up @@ -88,14 +88,14 @@ public interface SharedPreferencesApi {
* Sets up an instance of `SharedPreferencesApi` to handle messages through the
* `binaryMessenger`.
*/
static void setup(
static void setUp(
@NonNull BinaryMessenger binaryMessenger, @Nullable SharedPreferencesApi api) {
{
BinaryMessenger.TaskQueue taskQueue = binaryMessenger.makeBackgroundTaskQueue();
BasicMessageChannel<Object> channel =
new BasicMessageChannel<>(
binaryMessenger,
"dev.flutter.pigeon.SharedPreferencesApi.remove",
"dev.flutter.pigeon.shared_preferences_android.SharedPreferencesApi.remove",
getCodec(),
taskQueue);
if (api != null) {
Expand All @@ -122,7 +122,7 @@ static void setup(
BasicMessageChannel<Object> channel =
new BasicMessageChannel<>(
binaryMessenger,
"dev.flutter.pigeon.SharedPreferencesApi.setBool",
"dev.flutter.pigeon.shared_preferences_android.SharedPreferencesApi.setBool",
getCodec(),
taskQueue);
if (api != null) {
Expand Down Expand Up @@ -150,7 +150,7 @@ static void setup(
BasicMessageChannel<Object> channel =
new BasicMessageChannel<>(
binaryMessenger,
"dev.flutter.pigeon.SharedPreferencesApi.setString",
"dev.flutter.pigeon.shared_preferences_android.SharedPreferencesApi.setString",
getCodec(),
taskQueue);
if (api != null) {
Expand Down Expand Up @@ -178,7 +178,7 @@ static void setup(
BasicMessageChannel<Object> channel =
new BasicMessageChannel<>(
binaryMessenger,
"dev.flutter.pigeon.SharedPreferencesApi.setInt",
"dev.flutter.pigeon.shared_preferences_android.SharedPreferencesApi.setInt",
getCodec(),
taskQueue);
if (api != null) {
Expand Down Expand Up @@ -207,7 +207,7 @@ static void setup(
BasicMessageChannel<Object> channel =
new BasicMessageChannel<>(
binaryMessenger,
"dev.flutter.pigeon.SharedPreferencesApi.setDouble",
"dev.flutter.pigeon.shared_preferences_android.SharedPreferencesApi.setDouble",
getCodec(),
taskQueue);
if (api != null) {
Expand Down Expand Up @@ -235,7 +235,7 @@ static void setup(
BasicMessageChannel<Object> channel =
new BasicMessageChannel<>(
binaryMessenger,
"dev.flutter.pigeon.SharedPreferencesApi.setStringList",
"dev.flutter.pigeon.shared_preferences_android.SharedPreferencesApi.setStringList",
getCodec(),
taskQueue);
if (api != null) {
Expand Down Expand Up @@ -263,7 +263,7 @@ static void setup(
BasicMessageChannel<Object> channel =
new BasicMessageChannel<>(
binaryMessenger,
"dev.flutter.pigeon.SharedPreferencesApi.clear",
"dev.flutter.pigeon.shared_preferences_android.SharedPreferencesApi.clear",
getCodec(),
taskQueue);
if (api != null) {
Expand Down Expand Up @@ -291,7 +291,7 @@ static void setup(
BasicMessageChannel<Object> channel =
new BasicMessageChannel<>(
binaryMessenger,
"dev.flutter.pigeon.SharedPreferencesApi.getAll",
"dev.flutter.pigeon.shared_preferences_android.SharedPreferencesApi.getAll",
getCodec(),
taskQueue);
if (api != null) {
Expand Down
Loading