Skip to content

[shared_preferences] Convert legacy codepath to Kotlin#12139

Open
stuartmorgan-g wants to merge 10 commits into
flutter:mainfrom
stuartmorgan-g:shared-prefs-legacy-kotlin
Open

[shared_preferences] Convert legacy codepath to Kotlin#12139
stuartmorgan-g wants to merge 10 commits into
flutter:mainfrom
stuartmorgan-g:shared-prefs-legacy-kotlin

Conversation

@stuartmorgan-g

Copy link
Copy Markdown
Collaborator

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:

  • use the Android Studio auto-conversion to Kotlin
  • fix up type nullability (the auto-convert assumes non-annotated types, including any generics, are nullable)
  • remove Throws annotations that aren't needed in Kotlin
  • autoformat
  • do a couple of conversions from direct Java translation to more idiomatic Dart, especially around collection filtering and mutation

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

Footnotes

  1. 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

Context context = Mockito.mock(Context.class);
SharedPreferences sharedPrefs = new FakeSharedPreferences();

mockMessenger = Mockito.mock(BinaryMessenger.class);

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

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 :)

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

Comment on lines +105 to +112
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!!) }
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

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!!.

Suggested change
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))
}
}
}
}

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Changed.

Using .filter { ... }.mapValues { ... } creates an intermediate map and requires a force-unwrap (value!!). Using buildMap is 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()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

Instead of an unchecked cast with @Suppress("UNCHECKED_CAST"), you can use filterIsInstance<String>() which is fully type-safe and idiomatic in Kotlin.

Suggested change
@Suppress("UNCHECKED_CAST") val listValue = (value as Set<String>).toList()
val listValue = value.filterIsInstance<String>()

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

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>

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

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.

Suggested change
@Suppress("UNCHECKED_CAST") return stream.readObject() as List<String>
return (stream.readObject() as List<*>).filterIsInstance<String>()

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Same as above.

@github-actions github-actions Bot removed the CICD Run CI/CD label Jul 8, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant