[shared_preferences] Convert legacy codepath to Kotlin#12139
[shared_preferences] Convert legacy codepath to Kotlin#12139stuartmorgan-g wants to merge 10 commits into
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.
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