-
Notifications
You must be signed in to change notification settings - Fork 29.8k
Inject plugin registration. #9216
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you want to match what we're doing in the plugin template and what Apple recommends in Adopting Modern Objective-C, this should be (instancetype)initWithController with no space between
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(instancetype)initWithController
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we had the plugin registry code happen in FlutterActivity's onCreate we could do this automatically for all created FlutterActivities. It would be more robust to developers accidentally deleting the auto-registration code, or the registry API changing and breaking client apps.
Another possibility that's maybe better long-term is for the registry to be owned by the FlutterApplication with lifecycle callbacks rather than having registration code happen in Activity onCreate. This is useful for plugins that want to do something when the application is running but has no visible activities (e.g. push messaging). It's fine if this is beyond the scope of what you're trying to do with change, but the nice thing about moving plugin registration out of the client code is that we'd be able to do it invisibly without breaking apps.
/cc @goderbauer
There might be an issue where we can't compile FlutterActivity/FlutterApplication into the Flutter engine because it references a dynamically generated class. One way to resolve this would be to have a generic registry in engine that maintains a Map of class names to registered plugins, and applications that want to call methods of a plugin can call a method on the plugin registry that look up a plugin of a particular class.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mravn-google will look into this, see #9215.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar to the Android concept discussed above, what if the PluginRegistry had a static +(instancetype)defaultRegistry getter and the FlutterViewControllers automatically registered themselves in their constructor. This would allow us to mess around with the auto-registration process without having to worry about changing all the client's AppDelegates every time the plugin registry changes.
AFAIK, there can only be only one AppDelegate per app, so having a global default plugin registry seems analogous to having the registry be owned by the FlutterApplication on Android.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apple seems to prefer {{pluginClass}} *{{pluginProjectName}} and we should do that for consistency with other templates.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should put the version in the root of the pubspec.yaml instead of having it live as a child of plugin. That would require it to be a string, but it would keep versions numbers in sync with what's published in pub.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was the plugin interface version that the plugin implemented, not the version of the plugin itself.
Removed to avoid confusion. When we need to introduce it, we'll have to pick a better name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is used anywhere yet. Maybe it should be?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was really just future-proofing. All plugins are currently on version 1 of the plugin interface, so there was no need to check.
Removed for now, to avoid confusing things. We can always add the version, and treat anything with a missing version tag as version 1.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think maybe the visibility of these should be explicitly specified. If you want them to be public then consider making them private and providing a getter so it's clear that they're not meant to be mutated externally. Or maybe have them be final and set in the PluginRegistry's constructor.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added public visibility.
I'm not too worried about users modifying the fields, since the PluginRegistry is only used by the app. If it was used by the framework, I'd want to lock it down.
We should revisit the plugin "interface" when/if we move initialization to FlutterApplication, since that'll heavily influence how they're registered.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm kind of surprised this works, since {{name}} is the name of a readonly synthesized getter. I would put an underscore before {{name}} to make it clear you're not calling the synthesized accessor method, but rather mutating the synthesized member variable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was actually kind of surprised myself, by figured it was a feature. :)
Added underscore to make it clear. Also removed @synthesize, since they're auto-synthesized.
7df3465 to
8ecad34
Compare
Added a PluginRegistry to the new project template. The registry files will be automatically updated at build time to register the native plugins. Fixes flutter#7814.
8ecad34 to
41a6d0a
Compare
Added a PluginRegistry to the new project template. The registry files will be automatically updated at build time to register the native plugins.
Fixes #7814.