[shared_preferences] Convert legacy codepath to Kotlin#12139
Conversation
| Context context = Mockito.mock(Context.class); | ||
| SharedPreferences sharedPrefs = new FakeSharedPreferences(); | ||
|
|
||
| mockMessenger = Mockito.mock(BinaryMessenger.class); |
There was a problem hiding this comment.
This change was needed because the tests used to get away with returning a null binary messenger because the Java code never actually tried to use it. In Kotlin it exploded right away because that property isn't nullable, and Kotlin actually enforces nullability :)
There was a problem hiding this comment.
Code Review
This pull request converts LegacySharedPreferencesPlugin and SharedPreferencesListEncoder from Java to Kotlin, alongside minor updates to SharedPreferencesPlugin.kt and LegacySharedPreferencesTest.java. The review feedback suggests several improvements to the Kotlin implementation, including resolving method call ambiguity by explicitly referencing SharedPreferencesApi.setUp, optimizing collection operations in clear and getAllPrefs to avoid intermediate allocations, and replacing unchecked casts with type-safe filterIsInstance calls.
| private fun getAllPrefs(prefix: String, allowList: Set<String>?): Map<String, Any> { | ||
| return preferences.all | ||
| .filter { (key, value) -> | ||
| key.startsWith(prefix) && value != null && (allowList == null || allowList.contains(key)) | ||
| } | ||
| // Force-unwrapping is safe here due to the `value != null` in the filter above. | ||
| .mapValues { (key, value) -> transformPref(key, value!!) } | ||
| } |
There was a problem hiding this comment.
Using .filter { ... }.mapValues { ... } creates an intermediate map and requires a force-unwrap (value!!). Using buildMap is more idiomatic, performs the filtering and transformation in a single pass, and allows Kotlin to smart-cast value to a non-nullable type, eliminating the need for value!!.
| private fun getAllPrefs(prefix: String, allowList: Set<String>?): Map<String, Any> { | |
| return preferences.all | |
| .filter { (key, value) -> | |
| key.startsWith(prefix) && value != null && (allowList == null || allowList.contains(key)) | |
| } | |
| // Force-unwrapping is safe here due to the `value != null` in the filter above. | |
| .mapValues { (key, value) -> transformPref(key, value!!) } | |
| } | |
| private fun getAllPrefs(prefix: String, allowList: Set<String>?): Map<String, Any> { | |
| return buildMap { | |
| preferences.all.forEach { (key, value) -> | |
| if (key.startsWith(prefix) && value != null && (allowList == null || allowList.contains(key))) { | |
| put(key, transformPref(key, value)) | |
| } | |
| } | |
| } | |
| } |
There was a problem hiding this comment.
Changed.
Using
.filter { ... }.mapValues { ... }creates an intermediate map and requires a force-unwrap (value!!). UsingbuildMapis more idiomatic
Which is funny because Gemini suggested the code being reviewed here when I did a local Gemini review specifically prompting for a focus on idiomatic Kotlin.
| // https://github.com/flutter/flutter/issues/124420 | ||
|
|
||
| // This only happens for previous usage of setStringSet. The app expects a list. | ||
| @Suppress("UNCHECKED_CAST") val listValue = (value as Set<String>).toList() |
There was a problem hiding this comment.
There was a problem hiding this comment.
Gemini suggested this locally as well; I chose not to do it since while it is "safer", it's a behavioral change from the old code, and would cause invalid data to be silently dropped instead of erroring, which I'm not convinced we want in the very unlikely event that it does happen. And if we do want that, it should be a separate intentional change, rather than a side-effect of the Kotlin conversion.
| try { | ||
| val stream: ObjectInputStream = | ||
| StringListObjectInputStream(ByteArrayInputStream(Base64.decode(listString, 0))) | ||
| @Suppress("UNCHECKED_CAST") return stream.readObject() as List<String> |
There was a problem hiding this comment.
Instead of an unchecked cast with @Suppress("UNCHECKED_CAST"), you can use (stream.readObject() as List<*>).filterIsInstance<String>(). This is safer, avoids the warning suppression, and is consistent with the implementation in SharedPreferencesPlugin.kt.
| @Suppress("UNCHECKED_CAST") return stream.readObject() as List<String> | |
| return (stream.readObject() as List<*>).filterIsInstance<String>() |
There was a problem hiding this comment.
Same as above.
tarrinneal
left a comment
There was a problem hiding this comment.
Besides a couple nits, this is fine. I'm curious if you looked into overlapping code with the newer SP for clean up at all?
| } | ||
|
|
||
| // Deprecated, for testing purposes only. | ||
| @Deprecated("") |
There was a problem hiding this comment.
Probably wasn't anything here before either, but the comment could be in here instead.
There was a problem hiding this comment.
Moved into the string.
| } | ||
|
|
||
| // Gets all shared preferences, filtered to only those set with the given prefix. | ||
| // Optionally filtered also to only those items in |
There was a problem hiding this comment.
Huh, I wonder how I did that. Restored the rest of this comment.
I intentionally did not, but I did mean to add a TODO in the legacy file about that and forgot. I'll add it. |
…er#189387) flutter/packages@20928d5...ad2eab1 2026-07-13 35750184+motucraft@users.noreply.github.com [camera] Guard CameraController value updates after dispose (flutter/packages#11861) 2026-07-13 dkwingsmt@users.noreply.github.com [material_ui] Remove unused example files (flutter/packages#12179) 2026-07-13 puneetkukreja98@gmail.com [google_maps_flutter_platform_interface] Adds support for mapTypeControlEnabled, fullscreenControlEnabled, and streetViewControlEnabled for web (flutter/packages#12191) 2026-07-13 44747303+theprantadutta@users.noreply.github.com [pigeon] Report a clear error for enhanced enums (flutter/packages#11880) 2026-07-13 49699333+dependabot[bot]@users.noreply.github.com Bump actions/labeler from 6.1.0 to 6.2.0 in the all-github-actions group (flutter/packages#12190) 2026-07-13 stuartmorgan@google.com [shared_preferences] Convert legacy codepath to Kotlin (flutter/packages#12139) 2026-07-12 engine-flutter-autoroll@skia.org Roll Flutter from f7b66f3 to cf9e8af (11 revisions) (flutter/packages#12188) 2026-07-11 44525804+sailendrabathi@users.noreply.github.com [video_player] Improve seek performance on Android (flutter/packages#11810) 2026-07-11 stuartmorgan@google.com [pigeon] Disable iOS test parallelization (flutter/packages#12177) 2026-07-11 dkwingsmt@users.noreply.github.com [material_ui, cupertino_ui] Migrate snippet TODOs to `<callout-box>` (flutter/packages#12146) 2026-07-11 dkwingsmt@users.noreply.github.com [cupertino_ui] Fix example path (flutter/packages#12151) 2026-07-10 engine-flutter-autoroll@skia.org Roll Flutter from dc2a870 to f7b66f3 (12 revisions) (flutter/packages#12175) 2026-07-10 engine-flutter-autoroll@skia.org Roll Flutter (stable) from f94f4fc to ee80f08 (3 revisions) (flutter/packages#12171) 2026-07-10 engine-flutter-autoroll@skia.org Manual roll Flutter from 91939cc to dc2a870 (50 revisions) (flutter/packages#12169) 2026-07-09 jessiewong401@gmail.com Update `legacy` test to be in Warn Range (flutter/packages#12168) 2026-07-09 dkwingsmt@users.noreply.github.com [material_ui, cupertino_ui] Migrate dartpad TODOs to `<callout-box>` (flutter/packages#12120) 2026-07-09 73310711+shrabanti722@users.noreply.github.com [video_player_avfoundation] Implement preventsDisplaySleepDuringVideoPlayback (flutter/packages#11547) 2026-07-09 44747303+theprantadutta@users.noreply.github.com [pigeon] Add usage docs to generated Dart event channel methods (flutter/packages#11894) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages-flutter-autoroll Please CC flutter-ecosystem@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
Since this plugin already uses Kotlin code for the new codepaths, it doesn't fall under our usual policy of currently not converting code to Kotlin, we just never bothered to do the conversion for the old code. This converts the legacy implementation to Kotlin so that the plugin is consistent. The process for this conversion was:
I did also remove some duplicate string constants and switched to the copies that the non-legacy Kotlin code already had, since sharing those constants was now trivial, and less duplication is better.
Per our usual best practice, this doesn't convert the tests in the same PR, so that we guarantee that we aren't masking a regression with an accidental test change. I'll convert the unit tests in a separate PR with no changes to functional code.
Part of flutter/flutter#188630
Pre-Review Checklist
[shared_preferences]///).Footnotes
Regular contributors who have demonstrated familiarity with the repository guidelines only need to comment if the PR is not auto-exempted by repo tooling. ↩ ↩2