First pass at using platform abstraction for plugins#92672
Conversation
flutter-dashboard
bot
added
the
tool
|
It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat. If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
|
This is a first pass at the template updates for the PlatformInterface abstraction. This doesn't hit some of the requests of the ticket including showing an FFI implementation or any documentation updates on PlatformInterface. But, I would love some guidance on whether that should be part of this PR or part of a different one, and what we might want to update. Also, I did not add any tests for this, but all current tests pass. I'll double-check to see if any tests need to change or if I need to add one. Otherwise, I'll ask for a test exemption. Would love guidance on that as well. |
|
I've looked through current tests and it looks like none look for specific files, only ensuring that the _web file doesn't show when the web platform is asked for, so I think no new tests are needed. |
I think you misread the issue slightly. I wasn't saying that the template should demonstrate using FFI, but that using a platform interface in the template makes it easier for people developing plugins to adopt non-platform-channel implementations (such as FFI) on some platforms. |
|
This kind of change should be tested (even though similar code currently lacks tests). |
|
I've added a test to check that we create the plugin platform interface file, and added lines that check that the generated projects and examples pass analysis and unit tests. Since the internal test uses the platform channel, I think this covers the functionality I'm looking to test. |
fuzzybinary
force-pushed
the
feature/plugin_platform_interface_template
branch
2 times, most recently
from
b60198d to
bfd368c
Compare
flutter-dashboard
bot
added
the
c: contributor-productivity
|
@stuartmorgan can you take another look at this one? |
|
@fuzzybinary it looks like you have merge conflicts, can you resolve them by rebasing upstream? |
|
This is near the top of my review queue BTW; sorry it has taken so long to get back to. |
fuzzybinary
force-pushed
the
feature/plugin_platform_interface_template
branch
from
5152bc0 to
bb8c079
Compare
|
Gold has detected about 59 new digest(s) on patchset 4. |
fuzzybinary
force-pushed
the
feature/plugin_platform_interface_template
branch
from
bb8c079 to
b0fd7b7
Compare
|
Rebased but it looks like some tests are failing on Mac. I'll come back and take a look Monday when I have some time. |
Change the flutter create --template plugin command to create a PlatformInterface as an abstraction for plugins.
dart_plugin_registry_test creates a plugin called plugin_platform_interface, which conflicts with an actual plugin now used for platform abstractions. Renamed to plugin_platform_base.
fuzzybinary
force-pushed
the
feature/plugin_platform_interface_template
branch
from
b0fd7b7 to
a86c806
Compare
|
Okay! Re-rebased and looks like tests are passing (no changes) so must have been an infra thing. Sorry for the delay in checking. |
stuartmorgan-g
left a comment
stuartmorgan-g
left a comment
There was a problem hiding this comment.
Sorry again for the delay. Overall this looks great, but some specific comments.
@ditman should look at the web template changes.
packages/flutter_tools/templates/plugin/lib/projectName_platform_interface.dart.tmpl
Outdated
Show resolved
Hide resolved
packages/flutter_tools/templates/plugin/lib/projectName_method_channel.dart.tmpl
Outdated
Show resolved
Hide resolved
packages/flutter_tools/templates/plugin/lib/projectName_method_channel.dart.tmpl
Outdated
Show resolved
Hide resolved
packages/flutter_tools/templates/plugin/lib/projectName_method_channel.dart.tmpl
Outdated
Show resolved
Hide resolved
packages/flutter_tools/templates/plugin/lib/projectName_method_channel.dart.tmpl
Outdated
Show resolved
Hide resolved
packages/flutter_tools/templates/plugin/lib/projectName.dart.tmpl
Outdated
Show resolved
Hide resolved
packages/flutter_tools/templates/plugin/lib/projectName.dart.tmpl
Outdated
Show resolved
Hide resolved
| pluginImplPubspecContent = pluginImplPubspecContent.replaceFirst( | ||
| ' platforms:\n', | ||
| ' implements: plugin_platform_interface\n' | ||
| ' implements: plugin_platform_base\n' |
There was a problem hiding this comment.
This is a really confusing name. The better fix here would be to change plugin to aplugin or someplugin everywhere in this test, and keep the standard suffixes.
There was a problem hiding this comment.
The web implementation looks correct. This should be easier to demonstrate by creating a plugin with this template and ensuring that the example app works correctly with flutter run -d chrome
Also: do we want to update the plugin template, or create a new template for this example, @stuartmorgan ?
7 participants
Change the flutter create --template plugin command to create a PlatformInterface as an abstraction for plugins.
Work to fix #92279
Pre-launch Checklist
///).