Add binaryMessenger constructor argument to platform channels #30406

harryterkelsen · 2019-04-02T22:01:43Z

By abstracting out the calls to [BinaryMessages] the same code
can be used for platform channels on the platform side as on the
framework side.

Description

This PR adds base classes to MethodChannel and BasicMessageChannel, which abstracts out a 'BinaryMessenger' which is responsible for actually moving the bytes across the platform divide. The existing Channel implementations use BinaryMessages to implement their BinaryMessenger. The "dual" channel, which sends messages from the platform side to the framework side, would use a different BinaryMessenger.

Related Issues

Refactor Channel and Codec to be usable by platform side #29629

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I signed the CLA.
I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
I updated/added relevant documentation (doc comments with ///).
All existing and new tests are passing.
The analyzer (flutter analyze --flutter-repo) does not report any problems on my PR.
I am willing to follow-up on review comments in a timely manner.

Breaking Change

Does your PR require Flutter developers to manually update their apps to accommodate your change?

Yes, this is a breaking change (Please read Handling breaking changes). Link to flutter-announce email: https://groups.google.com/d/msg/flutter-announce/oWP9TCKys2E/1pExn2RVBQAJ
No, this is not a breaking change.

By abstracting out the calls to [BinaryMessages] the same code can be used for platform channels on the platform side as on the framework side.

amirh · 2019-04-02T22:08:36Z

Curious what do you think about the alternative of using the same mechanism setMockMessageHandler uses?

harryterkelsen · 2019-04-02T22:43:28Z

After several discussions, I think we plan to support 2 approaches for plugins:

You write a web plugin in Dart that is pretty a Dart port of the Android-specific code. In this approach, you need a platform channel that can send messages back to the framework side. I think it is cleaner to have Platform-side implementations of Channel than to register mock handlers on BinaryMessages.
In the plugin, you have an if (const bool('dart.library.html')) { ... } where you have web-specific logic. This avoids channels altogether.

…nels_2

ClassyRemade

My name was not my intention. It's out there now!

harryterkelsen · 2019-04-08T20:35:56Z

Ping!

goderbauer · 2019-04-10T22:07:58Z

@amirh @collinjackson @bparrishMines Can somebody review this?

harryterkelsen · 2019-04-10T22:20:24Z

@Hixie FYI this PR incorporates the changes we talked about in our meeting a few weeks ago.

packages/flutter/lib/src/services/binary_messenger.dart

packages/flutter/lib/src/services/platform_channel.dart

…nels_2

packages/flutter/lib/src/services/platform_channel.dart

Hixie · 2019-04-13T00:12:11Z

This ends up being a bit convoluted, with BinaryMessages, BinaryMessenger, MethodChannel, BaseMethodChannel, BasicMessageChannel, and BaseMessageChannel. I'm not sure I'd be able to draw an accurate diagram of which class inherited from which, and which instantiated which, and which was static vs which was instantiatable, without referencing the code.

Could we instead just make BinaryMessages non-static? i.e. refactor it so that you call through to an singleton instance of that class, much as we do with things like the ImageCache? Then for the framework side the binding could set it up, and on the plugin side the plugin could set it up.

harryterkelsen · 2019-04-15T20:51:51Z

That might not work for the web case. On the web we compile all of the code (platform side and client side) into one executable, so we wouldn't be able to have the two different compilations set up the binaryMessages singleton in two different ways.

What do you think about changing this CL so that instead of introducing a super-class, we just add an optional binaryMessenger argument to MethodChannel which defaults to one which uses BinaryMessages. On the platform side users can either use a normal method channel with their own binaryMessenger or we can supply a platform-side subclass of MethodChannel which has a binaryMessenger that sends data back to the framework side.

…nels_2

dnfield · 2019-05-01T22:07:36Z

@hterkelsen @Hixie what's the path forward here? Are there more changes needed to this, or is it ready to land?

Hixie · 2019-05-03T04:46:39Z

Sorry, haven't been checking my bugmail much recently.

The new PR is much closer to something reasonable, I think. Can we go further and just have BinaryMessages be an instance instead of static? Then we don't need both BinaryMessenger and BinaryMessages, we can just have the interface (public), and the default implementation (also public, but instance methods instead of static methods, and used as the default value to the arguments added in the current PR).

dnfield · 2019-05-15T22:14:45Z

@hterkelsen - do you plan to continue with this? @Hixie - are these concerns meant to block landing this?

At this point the PR has been open for over a month - if we don't think we can keep making progress forward with this approach perhaps we should close it.

Hixie · 2019-05-15T23:29:32Z

are these concerns meant to block landing this?

yes

packages/flutter/lib/src/services/platform_messages.dart

Hixie · 2019-05-20T23:01:53Z

I like the approach in this patch.

Please follow the breaking change policy (https://github.com/flutter/flutter/wiki/Tree-hygiene#handling-breaking-changes).

Hixie · 2019-05-20T23:02:17Z

I'll defer to @amirh, @dnfield, or @goderbauer for final review.

- Make the message handler typedef public. - Fix typos in comments. - Make clear that defaultBinaryMessenger has replaced BinaryMessages

harryterkelsen · 2019-05-20T23:32:58Z

I tried to send an email to flutter-announce@googlegroups.com but it seems I don't have permission to post there.

I tried to send the following:

As part of a change to generalize Platform Channels so they may be used in Web Plugins, I intend to deprecate BinaryMessages.

Justification

The platform channels use BinaryMessages to send messages from the application to the platform side. The proposed change
will abstract the "binary messenger" in the platform channels such that an implementation which sends messages from the
platform side back to the application can use the same platform channel classes (MethodChannel, OptionalMethodChannel, etc).

Part of this change is to replace the collection of static methods in BinaryMessages with an interface for sending messages,
called BinaryMessenger, and a default instance of BinaryMessenger, called defaultBinaryMessenger.

Links to Issue and PR

Issue: #29629
PR: #30406

Migration Steps

defaultBinaryMessenger is a drop-in replacement for BinaryMessages. Simply find and replace every instance of BinaryMessages with defaultBinaryMessages.

Before:

BinaryMessages.setMessageHandler('my-channel', myHandler);

After:

defaultBinaryMessenger.setMessageHandler('my-channel', myHandler);

Conclusion

Please comment on the issue or contact me directly if this change will cause you any issues.

Thanks!

Hixie · 2019-05-21T02:21:34Z

cc @kf6gpe for help with flutter-announce.

kf6gpe · 2019-05-22T16:23:03Z

@hterkelsen Thanks for the pending contribution! I can give you posting rights to flutter-announce --- reach out to me at the email on my github page and we'll get you set up. Thanks!

dnfield · 2019-05-22T16:40:28Z

packages/flutter/lib/src/services/platform_channel.dart

-  /// Neither [name] nor [codec] may be null.
-  const BasicMessageChannel(this.name, this.codec);
+  /// None of [name], [codec], or [binaryMessenger] may be null.
+  const BasicMessageChannel(this.name, this.codec, [ this.binaryMessenger = defaultBinaryMessenger ])


Can we make this a named parameter rather than an optional one? e.g. { this.binaryMessenger = defaultBinaryMessenger }? This ends up getting ugly/confusing if we have to add more parameters later.

Yes. I did it this way because MethodChannel has an optional argument for this.codec so I could only add binaryMessenger as another optional argument.

packages/flutter/lib/src/services/platform_channel.dart

Hixie · 2019-05-22T17:15:44Z

I believe it should be api.flutter.dev.

…

On Wed, May 22, 2019 at 9:45 AM Dan Field ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In packages/flutter/lib/src/services/platform_channel.dart <#30406 (comment)>: > @@ -287,12 +301,12 @@ class MethodChannel { /// [StandardMethodCodec]. /// * [JSONMessageCodec] which defines the payload values supported by /// [JSONMethodCodec]. - /// * <https://docs.flutter.io/javadoc/io/flutter/plugin/common/MethodCall.html> + /// * <https://docs.flutter.dev/javadoc/io/flutter/plugin/common/MethodCall.html> Is this ready? Last I heard the docs wasn't ready for .dev links. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#30406?email_source=notifications&email_token=AAEGSHG3QWETXIVLNUMA4MDPWV2CBA5CNFSM4HDE3AP2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOBZMZQJA#pullrequestreview-240752676>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAEGSHBGO7V5PZP6X7APU7TPWV2CBANCNFSM4HDE3APQ> .

-- Ian Hickson

kf6gpe · 2019-05-22T17:31:51Z

We're ready for links to api.flutter.dev at this point, yes. Thanks! On Wed, May 22, 2019 at 10:22 AM Ian Hickson <notifications@github.com> wrote:

…

I believe it should be api.flutter.dev. On Wed, May 22, 2019 at 9:45 AM Dan Field ***@***.***> wrote: > ***@***.**** commented on this pull request. > ------------------------------ > > In packages/flutter/lib/src/services/platform_channel.dart > <#30406 (comment)>: > > > @@ -287,12 +301,12 @@ class MethodChannel { > /// [StandardMethodCodec]. > /// * [JSONMessageCodec] which defines the payload values supported by > /// [JSONMethodCodec]. > - /// * < https://docs.flutter.io/javadoc/io/flutter/plugin/common/MethodCall.html> > + /// * < https://docs.flutter.dev/javadoc/io/flutter/plugin/common/MethodCall.html> > > Is this ready? Last I heard the docs wasn't ready for .dev links. > > — > You are receiving this because you were mentioned. > Reply to this email directly, view it on GitHub > < #30406?email_source=notifications&email_token=AAEGSHG3QWETXIVLNUMA4MDPWV2CBA5CNFSM4HDE3AP2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOBZMZQJA#pullrequestreview-240752676 >, > or mute the thread > < https://github.com/notifications/unsubscribe-auth/AAEGSHBGO7V5PZP6X7APU7TPWV2CBANCNFSM4HDE3APQ > > . > -- Ian Hickson — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#30406?email_source=notifications&email_token=AAK6QRDYFM4OQTHVGPE42JTPWV6M3A5CNFSM4HDE3AP2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODV7XI4Q#issuecomment-494892146>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAK6QRAYQFZXSXL3PPDPGKLPWV6M3ANCNFSM4HDE3APQ> .

- make binaryMessenger a named argument in BasicMessageChannel

harryterkelsen · 2019-05-22T17:56:13Z

Thanks, @dnfield. PTAL

dnfield

LGTM

harryterkelsen · 2019-05-22T20:23:35Z

Thanks! I'll land this tomorrow once the build is fixed and my breaking change announcement has been posted for 24 hours.

flutter/flutter#30406 deprecated the usage of static BinaryMessages methods. As we cannot use the new defaultBinaryMessages mechanism before it makes it to stable, for now, we suppress the warnings.

…r#30406) * Deprecates `BinaryMessages` in favor of a default instance of `BinaryMessenger`, called `defaultBinaryMessenger` * Platform channels use the `defaultBinaryMessenger` for their binaryMessenger default argument.

flutter/flutter#30406 deprecated the usage of static BinaryMessages methods. As we cannot use the new defaultBinaryMessages mechanism before it makes it to stable, for now, we suppress the warnings.

Add super-classes to MethodChannel and BasicMessageChannel.

d41be70

By abstracting out the calls to [BinaryMessages] the same code can be used for platform channels on the platform side as on the framework side.

googlebot added the cla: yes label Apr 2, 2019

harryterkelsen mentioned this pull request Apr 2, 2019

Split platform channel code into a separate package, flutter_channels #29759

Closed

10 tasks

harryterkelsen added 2 commits April 2, 2019 15:19

Fix formatting issues

ee22d14

More formatting

24a1833

Remove trailing whitespace

adc8e82

goderbauer added framework flutter/packages/flutter repository. See also f: labels. p: framework labels Apr 2, 2019

Merge branch 'master' of github.com:flutter/flutter into flutter_chan…

7c0be78

…nels_2

ClassyRemade reviewed Apr 5, 2019

View reviewed changes

yjbanov approved these changes Apr 11, 2019

View reviewed changes

harryterkelsen added 3 commits April 11, 2019 15:30

Merge branch 'master' of github.com:flutter/flutter into flutter_chan…

86ba8b2

…nels_2

Respond to review comments

7c7eea4

Merge branch 'master' into flutter_channels_2

b117b96

Hixie reviewed Apr 12, 2019

View reviewed changes

packages/flutter/lib/src/services/platform_channel.dart Outdated Show resolved Hide resolved

harryterkelsen added 2 commits April 15, 2019 13:58

Merge branch 'master' of github.com:flutter/flutter into flutter_chan…

38b7d7d

…nels_2

Instead of a superclass, just add an optional constructor argument.

dfa6ced

harryterkelsen changed the title ~~Add super-classes to MethodChannel and BasicMessageChannel.~~ Add binaryMessenger constructor argument to platform channels Apr 15, 2019

Hixie reviewed May 20, 2019

View reviewed changes

packages/flutter/lib/src/services/platform_messages.dart Show resolved Hide resolved

Respond to review comments.

6339961

- Make the message handler typedef public. - Fix typos in comments. - Make clear that defaultBinaryMessenger has replaced BinaryMessages

harryterkelsen requested a review from dnfield May 21, 2019 17:34

dnfield reviewed May 22, 2019

View reviewed changes

packages/flutter/lib/src/services/platform_channel.dart Outdated Show resolved Hide resolved

harryterkelsen added 2 commits May 22, 2019 10:48

- update url for Flutter javadocs

12bbba9

- make binaryMessenger a named argument in BasicMessageChannel

Merge branch 'master' into flutter_channels_2

1df8c6f

harryterkelsen requested a review from dnfield May 22, 2019 17:56

dnfield approved these changes May 22, 2019

View reviewed changes

Merge branch 'master' into flutter_channels_2

1ae5321

harryterkelsen merged commit 13e9bfc into flutter:master May 28, 2019

harryterkelsen deleted the flutter_channels_2 branch May 28, 2019 18:18

This was referenced May 28, 2019

Replaces usages of BinaryMessages in the plugins repo #33446

Closed

suppress deprecation warning for BinaryMessages flutter/plugins#1643

Merged

harryterkelsen mentioned this pull request Sep 3, 2019

Refactor Channel and Codec to be usable by platform side #29629

Closed

github-actions bot locked as resolved and limited conversation to collaborators Aug 6, 2021

Add binaryMessenger constructor argument to platform channels #30406

Add binaryMessenger constructor argument to platform channels #30406

Uh oh!

Conversation

harryterkelsen commented Apr 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issues

Checklist

Breaking Change

Uh oh!

amirh commented Apr 2, 2019

Uh oh!

harryterkelsen commented Apr 2, 2019

Uh oh!

ClassyRemade left a comment

Choose a reason for hiding this comment

Uh oh!

harryterkelsen commented Apr 8, 2019

Uh oh!

goderbauer commented Apr 10, 2019

Uh oh!

harryterkelsen commented Apr 10, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Hixie commented Apr 13, 2019

Uh oh!

harryterkelsen commented Apr 15, 2019

Uh oh!

dnfield commented May 1, 2019

Uh oh!

Hixie commented May 3, 2019

Uh oh!

dnfield commented May 15, 2019

Uh oh!

Hixie commented May 15, 2019

Uh oh!

Uh oh!

Hixie commented May 20, 2019

Uh oh!

Hixie commented May 20, 2019

Uh oh!

harryterkelsen commented May 20, 2019

Uh oh!

Hixie commented May 21, 2019

Uh oh!

kf6gpe commented May 22, 2019

Uh oh!

dnfield May 22, 2019

Choose a reason for hiding this comment

Uh oh!

harryterkelsen May 22, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Hixie commented May 22, 2019 via email

Uh oh!

kf6gpe commented May 22, 2019 via email

Uh oh!

harryterkelsen commented May 22, 2019

Uh oh!

dnfield left a comment

Choose a reason for hiding this comment

Uh oh!

harryterkelsen commented May 22, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

harryterkelsen commented Apr 2, 2019 •

edited

Loading