Skip to content

[shared_preferences] Platform implementations of async api #6965

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

Merged
auto-submit 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 1 commit
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
Prev Previous commit
Next Next commit
more legacy
  • Loading branch information
@tarrinneal
tarrinneal committed Jul 22, 2024
commit f3002f14e6e7ee308acaa9b5360c6d094dde16c9
8 changes: 4 additions & 4 deletions ...es/DeprecatedSharedPreferencesPlugin.java → ...rences/LegacySharedPreferencesPlugin.java
View file
Open in desktop
Copy link
Contributor

@reidbaker reidbaker Jul 30, 2024

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

@tarrinneal tarrinneal Jul 30, 2024

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

@reidbaker reidbaker Jul 30, 2024

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
Collaborator

@stuartmorgan-g stuartmorgan-g Jul 30, 2024

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

@reidbaker reidbaker Jul 30, 2024

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

@tarrinneal tarrinneal Jul 30, 2024

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

@tarrinneal tarrinneal Jul 30, 2024

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

@reidbaker reidbaker Jul 30, 2024

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

@tarrinneal tarrinneal Jul 30, 2024

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;

/** DeprecatedSharedPreferencesPlugin */
public class DeprecatedSharedPreferencesPlugin 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,12 +38,12 @@ public class DeprecatedSharedPreferencesPlugin implements FlutterPlugin, SharedP
private SharedPreferences preferences;
private SharedPreferencesListEncoder listEncoder;

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

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

Expand Down
6 changes: 3 additions & 3 deletions ...d/src/test/java/io/flutter/plugins/sharedpreferences/DeprecatedSharedPreferencesTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,9 +26,9 @@
import org.mockito.Mock;
import org.mockito.Mockito;

public class DeprecatedSharedPreferencesTest {
public class LegacySharedPreferencesTest {

DeprecatedSharedPreferencesPlugin plugin;
LegacySharedPreferencesPlugin plugin;

@Mock BinaryMessenger mockMessenger;
@Mock FlutterPlugin.FlutterPluginBinding flutterPluginBinding;
Expand All @@ -44,7 +44,7 @@ public void before() {
Mockito.when(flutterPluginBinding.getApplicationContext()).thenReturn(context);
Mockito.when(context.getSharedPreferences(anyString(), anyInt())).thenReturn(sharedPrefs);

plugin = new DeprecatedSharedPreferencesPlugin(new ListEncoder());
plugin = new LegacySharedPreferencesPlugin(new ListEncoder());
plugin.onAttachedToEngine(flutterPluginBinding);
}

Expand Down