Skip to content

Conversation

@jakobr-google
Copy link
Contributor

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.

Copy link
Contributor

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

Copy link
Contributor

Choose a reason for hiding this comment

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

(instancetype)initWithController

Copy link
Contributor

@collinjackson collinjackson Apr 5, 2017

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.

Copy link
Contributor Author

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.

Copy link
Contributor

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.

Copy link
Contributor

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor

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?

Copy link
Contributor Author

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.

Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor

@collinjackson collinjackson Apr 5, 2017

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.

Copy link
Contributor Author

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.

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.
@jakobr-google jakobr-google merged commit 7ffa82a into flutter:master Apr 10, 2017
@jakobr-google jakobr-google deleted the injection branch April 10, 2017 13:44
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 13, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Support consuming a plugin without having to write native code

3 participants