[shared_preferences] switch to ffi/jni pigeon - #12329
Conversation
|
@stuartmorgan-g this is a working example of switching to ffi/jni pigeon. the pr contains all of the pigeon changes, but you can just look at the shared preferences stuff |
stuartmorgan-g
left a comment
stuartmorgan-g
left a comment
There was a problem hiding this comment.
The iOS part generally looks good, but one question on the binary file. The Android part I have more questions about the structure of main package vs example package.
| channel.setMessageHandler(null) | ||
| } | ||
| } | ||
| error("SharedPreferencesApi has not been set") |
There was a problem hiding this comment.
A minor nit: should we say "registered" instead of "set" to connect this more clearly to the register method that needs to be called?
| @@ -52,3 +52,89 @@ dependencies { | |||
| androidTestImplementation("androidx.test.espresso:espresso-core:3.2.0") | |||
| api("androidx.test:core:1.4.0") | |||
| } | |||
| // Gradle stub for listing dependencies in JNIgen. If found in | |||
There was a problem hiding this comment.
I know this isn't in our direct control, but something we should raise for long-term jnigen discussions: I worry about the maintenance of this code. It's in a user-controlled file, not a generated file, which means that if it needs to change in the future there will need to be some potentially non-trivial migration.
(On the other hand, that's also true of the Flutter plugin system itself.)
There was a problem hiding this comment.
Why do these files exist in the example app? Is this cruft from an earlier mistake in generation path?
There was a problem hiding this comment.
It feels weird to have these in the example rather than the main plugin. Can't we have them in the main plugin package, and use androidExample directory to point into the subdirectory?
| @@ -9,12 +9,8 @@ environment: | |||
| dependencies: | |||
| flutter: | |||
| sdk: flutter | |||
| jni: ^1.0.0 | |||
There was a problem hiding this comment.
This worries me; having to make production dependency changes to the example means that we are not testing the same configuration that everyone using shared_preferences will have, which could mask mistakes.
Is this only because the tool/ directory is part of the example rather than the main bundle?
| Directory.current = Platform.script.resolve('../..').toFilePath(); | ||
| await generateJniBindings( | ||
| Config( | ||
| androidSdkConfig: AndroidSdkConfig(addGradleDeps: true, androidExample: './'), |
There was a problem hiding this comment.
Is androidExample correct here? If so that seems pretty confusing; how can the example be the current directory rather than example/?
| @@ -12,7 +12,7 @@ Wraps NSUserDefaults, providing a persistent store for simple key-value pairs. | |||
| s.license = { :type => 'BSD', :file => '../LICENSE' } | |||
| s.author = { 'Flutter Team' => 'flutter-dev@googlegroups.com' } | |||
| s.source = { :http => 'https://github.com/flutter/packages/tree/main/packages/shared_preferences/shared_preferences_foundation' } | |||
| s.source_files = 'shared_preferences_foundation/Sources/shared_preferences_foundation/**/*.swift' | |||
| s.source_files = 'shared_preferences_foundation/Sources/**/*.{swift,m,h}' | |||
There was a problem hiding this comment.
Are we still expecting there to be .m code generated, or just the .o?
There was a problem hiding this comment.
the .m file isn't always generated, this api surface is simple enough that it isn't needed. The skill currently says to add both since it will always work. Do you think it's a bad idea?
2 participants
for example