Convert Shared Preferences Android to Pigeon #7015

AmanNegi · 2023-01-24T08:30:11Z

Changes

Converted the Android Implementation of shared_preferences to Pigeon. The files messages.g.dart and Messages.java are autogenerated by Pigeon according to the template provided in pigeons/messages.dart.

I took reference from the path_provider package as it is already converted to pigeon.
The tests were heavily inspired by the SharedPreferences for MacOS as it had been already converted using Pigeon.
All the tests are running successfully and the example is also working as expected.

Issue

Part of flutter/flutter#117914

Pre-launch Checklist

[✅] I read the Contributor Guide and followed the process outlined there for submitting PRs.
[✅] I read the Tree Hygiene wiki page, which explains my responsibilities.
[✅] I read and followed the relevant style guides and ran the auto-formatter. (Unlike the flutter/flutter repo, the flutter/plugins repo does use dart format.)
[✅] I signed the CLA.
[✅] The title of the PR starts with the name of the plugin surrounded by square brackets, e.g. [shared_preferences]
[✅] I listed at least one issue that this PR fixes in the description above.
[✅] I updated pubspec.yaml with an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes.
[✅] I updated CHANGELOG.md to add a description of the change, following repository CHANGELOG style.
[✅] I updated/added relevant documentation (doc comments with ///).
[✅] I added new tests to check the change I am making, or this PR is test-exempt.
[✅] All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

…h-sharedprefs-android-to-pigeon

…7914

AmanNegi · 2023-01-24T11:18:31Z

@tarrinneal Could you please guide me with how can I pass through the CI/CD Pipeline, the tests were running and the example is also running fine as expected.

stuartmorgan-g

Thanks for the contribution!

packages/shared_preferences/shared_preferences_android/AUTHORS

packages/shared_preferences/shared_preferences_android/CHANGELOG.md

...ndroid/android/src/main/java/io/flutter/plugins/sharedpreferences/MethodCallHandlerImpl.java

...roid/android/src/main/java/io/flutter/plugins/sharedpreferences/SharedPreferencesPlugin.java

packages/shared_preferences/shared_preferences_android/pigeons/messages.dart

...roid/android/src/main/java/io/flutter/plugins/sharedpreferences/SharedPreferencesPlugin.java

tarrinneal · 2023-01-24T18:20:11Z

@tarrinneal Could you please guide me with how can I pass through the CI/CD Pipeline, the tests were running and the example is also running fine as expected.

Seems like you got it, nice work. I'll go ahead and review after you fix Stuarts requests. Thank you for contributing!

…on upgrade to 3.0.0

AmanNegi · 2023-01-25T14:56:06Z

I have made the changes but I am facing issues with File Formatting, however I'm trying to fix it. Once I fix it I will raise a review check. Sorry for the delay

…h-sharedprefs-android-to-pigeon

AmanNegi · 2023-01-25T17:54:01Z

Hey, there I successfully fixed the error regarding Code Formatting.

packages/shared_preferences/shared_preferences_android/CHANGELOG.md

…h-sharedprefs-android-to-pigeon

AmanNegi · 2023-01-26T05:42:42Z

Hey, there I reverted the unwanted changes caused by linting.

AmanNegi

Fixed all reviews successfully :)

AmanNegi · 2023-01-27T17:37:27Z

Hey, is there some issue with the PR? Can I help with it?

tarrinneal · 2023-01-27T18:34:40Z

...ndroid/android/src/main/java/io/flutter/plugins/sharedpreferences/MethodCallHandlerImpl.java

+
+/** Helper class to save data to `android.content.SharedPreferences` */
+
+// Rename class and file to match it's purpose, preferably SharedPreferencesHelper


If you would still like to make this change, I think another pr that does only this would be a good idea.

Should I change the name perhaps? I was asked to make minimal changes, so I didn't change the file name as well.

I think both in a follow up pr would be ideal

Yeah sure, do I have to do it after this PR is merged? I am sorry for the lame question😔, this is my first time contributing.

you can put the pr up whenever, I think it would be easier to wait till this one is landed though

Okay sure! Thanks a lot again(🙂).

stuartmorgan-g · 2023-01-27T18:36:51Z

Hey, is there some issue with the PR?

What issue are you referring to?

AmanNegi · 2023-01-27T18:46:47Z

Hey, is there some issue with the PR?

What issue are you referring to?

I thought it wasn't getting merged perhaps I am missing something at my end.

stuartmorgan-g · 2023-01-27T18:51:49Z

I thought it wasn't getting merged perhaps I am missing something at my end.

PRs don't get merged before they are approved by reviewers; please read https://github.com/flutter/flutter/wiki/Tree-hygiene#getting-a-code-review (and 11 hours since your last update is not a reasonable window of time before pinging reviewers; re-review can take a while depending on people's schedules).

AmanNegi · 2023-01-28T02:35:36Z

I am really sorry, I will make sure not to tag any contributor/maintainer.

AmanNegi · 2023-02-14T16:56:40Z

Hey there, any updates regarding this PR? 😊

stuartmorgan-g

Apparently I hadn't published my review comments; they were still in Pending mode.

stuartmorgan-g · 2023-02-12T22:31:13Z

packages/shared_preferences/shared_preferences_android/pigeons/messages.dart

+@HostApi(dartHostTestHandler: 'TestSharedPreferencesApi')
+abstract class SharedPreferencesApi {
+  @TaskQueue(type: TaskQueueType.serialBackgroundThread)
+  bool remove(String key);


Please add documentation comments on each method explaining what they do.

stuartmorgan-g · 2023-02-12T22:31:53Z

packages/shared_preferences/shared_preferences_android/pubspec.yaml

@@ -25,3 +25,4 @@ dependencies:
 dev_dependencies:
  flutter_test:
    sdk: flutter
+  pigeon: ^7.0.2


This can be updated to ^8.0.0.

stuartmorgan-g · 2023-02-12T22:33:07Z

...ages/shared_preferences/shared_preferences_android/test/shared_preferences_android_test.dart

+import '../test/messages_test.g.dart';
+
+// Mock Implementation of the SharedPreferencesApi
+class _MockAPI extends TestSharedPreferencesApi {


This is a fake, not a mock, so should be named accordingly.

Okay, I will rename it. 😊

stuartmorgan-g · 2023-02-12T22:35:23Z

...ndroid/android/src/main/java/io/flutter/plugins/sharedpreferences/MethodCallHandlerImpl.java

+
+/** Helper class to save data to `android.content.SharedPreferences` */
+
+// Rename class and file to match it's purpose, preferably SharedPreferencesHelper


Please see https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo#avoid-checking-in-comments-that-ask-questions for TODO comment style.

stuartmorgan-g · 2023-02-12T22:41:03Z

...ndroid/android/src/main/java/io/flutter/plugins/sharedpreferences/MethodCallHandlerImpl.java

+                .remove(key)
+                .putString(key, LIST_IDENTIFIER + encodeList(listValue))
+                .commit();
+          } catch (RuntimeException e) {


It doesn't look like you addressed my question from before: why are you throwing an exception for the sole purpose of rethrowing it? This doesn't accomplish anything unless I'm missing something.

Sorry for that, I have now fixed it. Now I have mitigated the functions to handle IOException and the wrapper functions then convert it to RuntimeException.

stuartmorgan-g · 2023-02-14T17:55:29Z

...ndroid/android/src/test/java/io/flutter/plugins/sharedpreferences/SharedPreferencesTest.java

+    api.setDouble("Pie", 3.14);
+    api.setStringList("Names", Arrays.asList("Flutter", "Dart"));
+    api.setBool("NewToFlutter", false);
+  }


I'm confused by the changes in this file; you created a mock, and then you wrote unit tests of the mock. There is absolutely no production code being tested in these tests.

The point of unit tests is to test the actual code. If you need to mock out the underlying system API that's one thing (although it's not clear to me that that's necessary here), but you can't mock out everything.

I wrote these tests so that the user can easily understand what all functions are available and what all values can be passed. I am using Fake Tests because we can't access the SharedPreferencesPlugin in Unit Tests. If I am wrong about it, please let me know.

I wrote these tests so that the user can easily understand what all functions are available and what all values can be passed.

The purpose of unit tests is to test code; documentation belongs in comments.

I am using Fake Tests because we can't access the SharedPreferencesPlugin in Unit Tests.

Why wouldn't unit tests be able to access code under test?

Because under SharedPreferencesPlugin.java we are using android.content.SharedPreferences and other libraries which can only execute when we test on an Emulator/device.

I took inspiration for tests from here.

Let me know if I'm wrong here.

Because under SharedPreferencesPlugin.java we are using android.content.SharedPreferences and other libraries which can only execute when we test on an Emulator/device.

Internal implementation details that aren't available in unit tests can be mocked out, per my comment above.

I took inspiration for tests from here.

Let me know if I'm wrong here.

That is testing a small utility, not the plugin itself, so isn't going to be very useful as an example. But even then, that's testing actual code, not testing test mocks, so is serving a useful test purpose.

Examples of substantive unit tests include this and this.

Hey there, I went through the examples and I was able to create some tests using MockedSharedPreferencesPlugin.

I tried, mocking android.content.SharedPreferences: I tried this method but it results in a chain of mapping methods similar to the approach I have followed.

I don't think we will be able to test the production code like the code under MethodCallHandlerImpl because it requires several mocked objects with chained methods Mockito.when() to map methods.

Here, is the updated test. I am really unsure about how to write tests for this because there are not many references of tests with packages using Pigeon.

stuartmorgan-g · 2023-02-14T17:56:06Z

...roid/android/src/main/java/io/flutter/plugins/sharedpreferences/SharedPreferencesPlugin.java

+    plugin.setup(registrar.messenger(), registrar.context());
+  }
+
+  @SuppressLint("LongLogTag")


Why are you using a tag so long that it violates linter rules?

stuartmorgan-g · 2023-02-14T17:58:30Z

packages/shared_preferences/shared_preferences_android/lib/shared_preferences_android.dart

    return preferences;
  }
+
+  // Call the function according to the type of value provided
+  Future<bool?> _handleSetValue(


Why is the future type nullable?

stuartmorgan-g · 2023-02-14T17:59:15Z

packages/shared_preferences/shared_preferences_android/lib/shared_preferences_android.dart

-      'set$valueType',
-      <String, dynamic>{'key': key, 'value': value},
-    ))!;
+    return await _handleSetValue(valueType, key, value) ?? false;


What's the purpose of extracting the helper here?

Earlier setDataType was used, as at the Android end we used to get its name from callMethod. However, it makes more sense to filter that at the Dart End. Either way, this function has to be created at either Dart or Android end (which maps setDataType string to a function according to the dataType).

I'm not asking why the code that's currently in the helper exists, I'm asking why it's in a helper that's only called from one place, where that place ends up being just a single line calling that helper, rather than that code being here. I don't see what value the indirection is providing.

stuartmorgan-g · 2023-02-14T18:01:32Z

...ndroid/android/src/main/java/io/flutter/plugins/sharedpreferences/MethodCallHandlerImpl.java

+      List<String> data = (List<String>) stream.readObject();
+      if (stream != null) {
+        stream.close();
+      }


Why did you remove the cleanup steps from finally blocks? That regresses behavior.

Yes, sorry for removing the cleanup process. I have updated the finally block in all required scenarios.

stuartmorgan-g · 2023-02-22T15:51:15Z

We've just completed the migration of the plugin code to the flutter/packages repository, as described in https://flutter.dev/go/flutter-plugins-repo-migration, and this repository is now being archived. Unfortunately that means that all in-progress PRs here must be moved to flutter/packages.

Please see our instructions for an explanation of how to move your PR, and if you have any issues moving your PR please don't hesitate to reach out in the #hackers-ecosystem channel in Discord.

Our apologies that your PR was caught in this one-time transition. We're aware that it's disruptive in the short term, and appreciate your help in getting us to a better long-term state!

AmanNegi added 6 commits January 23, 2023 15:17

[shared_preferences]: Switched to Piegon, created setters

4ea5bf3

Merge branch 'main' of https://github.com/AmanNegi/plugins into switc…

cfb8351

…h-sharedprefs-android-to-pigeon

[shared_preferences] Convert shared_preferences_android to Pigeon #11…

bd8a0df

…7914

shared_preferences] Upgraded the version pubspec.yaml

c6af29c

[shared_preferences] Updated CHANGELOG.md

04d8675

shared_preferences] Added helper comments

091ec62

AmanNegi requested a review from tarrinneal as a code owner January 24, 2023 08:30

github-actions bot added p: shared_preferences platform-android labels Jan 24, 2023

AmanNegi added 2 commits January 24, 2023 14:11

[shared_preferences] Formatted Document According to Error Request

5d900ad

shared_preferences] Updated Code Format

a874e29

[shared_preferences] Reformatted Code according to Google Format

0c3f92b

stuartmorgan-g suggested changes Jan 24, 2023

View reviewed changes

AmanNegi added 5 commits January 25, 2023 16:58

[shared_preferences] Added android tests and fixed changes requested

a4fb46e

[shared_preferences] Merged Upcoming Changes of minimum Flutter versi…

3c80145

…on upgrade to 3.0.0

[shared_preferences] Formatted Code using the format command

4ae8079

[shared_preferences] Formatted Code

4ded5c0

[shared_preferences] Restored error causing file

19d61cb

AmanNegi added 3 commits January 25, 2023 22:57

[shared_preferences] Fixed all Formatting errors

7a2c7d4

Merge branch 'main' of https://github.com/AmanNegi/plugins into switc…

3067853

…h-sharedprefs-android-to-pigeon

[shared_preferences] Updated the release Notes

2b62b5f

AmanNegi requested review from stuartmorgan-g and removed request for tarrinneal January 25, 2023 17:54

[shared_preferences] Fixed the AUTHORS file

40614ee

tarrinneal reviewed Jan 25, 2023

View reviewed changes

packages/shared_preferences/shared_preferences_android/CHANGELOG.md Outdated Show resolved Hide resolved

[shared_preferences] Fixed Linter Formatting Issue in CHANGELOG.md

9290ed1

Merge branch 'main' of https://github.com/AmanNegi/plugins into switc…

83c2adc

…h-sharedprefs-android-to-pigeon

AmanNegi requested review from tarrinneal and removed request for stuartmorgan-g January 26, 2023 05:58

This comment was marked as duplicate.

Sign in to view

AmanNegi commented Jan 27, 2023

View reviewed changes

AmanNegi requested review from stuartmorgan-g and removed request for tarrinneal January 27, 2023 07:26

tarrinneal reviewed Jan 27, 2023

View reviewed changes

stuartmorgan-g suggested changes Feb 14, 2023

View reviewed changes

Merge branch 'flutter:main' into switch-sharedprefs-android-to-pigeon

ddbf133

stuartmorgan-g closed this Feb 22, 2023

AmanNegi mentioned this pull request Feb 28, 2023

[shared_preferences] Ports SharedPreferencesAndroid to Pigeon flutter/packages#3321

Merged


		/** Helper class to save data to `android.content.SharedPreferences` */

		// Rename class and file to match it's purpose, preferably SharedPreferencesHelper

Convert Shared Preferences Android to Pigeon #7015

Convert Shared Preferences Android to Pigeon #7015

Uh oh!

Conversation

AmanNegi commented Jan 24, 2023

Changes

Issue

Pre-launch Checklist

Uh oh!

AmanNegi commented Jan 24, 2023

Uh oh!

stuartmorgan-g left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tarrinneal commented Jan 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AmanNegi commented Jan 25, 2023

Uh oh!

AmanNegi commented Jan 25, 2023

Uh oh!

Uh oh!

AmanNegi commented Jan 26, 2023

Uh oh!

This comment was marked as duplicate.

Uh oh!

AmanNegi left a comment

Choose a reason for hiding this comment

Uh oh!

AmanNegi commented Jan 27, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AmanNegi Jan 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stuartmorgan-g commented Jan 27, 2023

Uh oh!

AmanNegi commented Jan 27, 2023

Uh oh!

stuartmorgan-g commented Jan 27, 2023

Uh oh!

AmanNegi commented Jan 28, 2023

Uh oh!

AmanNegi commented Feb 14, 2023

Uh oh!

stuartmorgan-g left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AmanNegi Feb 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

tarrinneal commented Jan 24, 2023 •

edited

Loading

AmanNegi Jan 27, 2023 •

edited

Loading

stuartmorgan-g left a comment •

edited

Loading

AmanNegi Feb 15, 2023 •

edited

Loading