Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Conversation

robert-ancell
Copy link
Contributor

@robert-ancell robert-ancell commented Sep 22, 2020

Description

Add FlEventChannel

Related Issues

flutter/flutter#65270

Tests

I added the following tests:

Tests added in shell/platform/linux/fl_event_channel_test.cc.

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 C++, Objective-C, Java style guides for the engine.
  • I read the tree hygiene wiki page, which explains my responsibilities.
  • I updated/added relevant documentation.
  • All existing and new tests are passing.
  • I am willing to follow-up on review comments in a timely manner.

Breaking Change

Did any tests fail when you ran them? Please read handling breaking changes.

Solve by keeping a reference to the FlEngine until a source completes.
@auto-assign auto-assign bot requested a review from GaryQian September 22, 2020 04:43
@robert-ancell robert-ancell marked this pull request as draft September 22, 2020 05:13
@robert-ancell
Copy link
Contributor Author

robert-ancell commented Sep 22, 2020

I just noticed while testing that EventChannel.receiveBroadcastStream passes arguments - I'll add support for these.

Copy link
Contributor

@stuartmorgan-g stuartmorgan-g left a comment

Choose a reason for hiding this comment

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

A few comments below. I've never actually used the event channel API, so (especially on top of my usual lack of experience with GObject) I'm not super confident in my review here.

Have you tried finding a live use case and ensuring that you can write a Linux version of that plugin (at least, that part, with other bits stubbed out)?

@robert-ancell
Copy link
Contributor Author

A few comments below. I've never actually used the event channel API, so (especially on top of my usual lack of experience with GObject) I'm not super confident in my review here.

Have you tried finding a live use case and ensuring that you can write a Linux version of that plugin (at least, that part, with other bits stubbed out)?

I'm also a bit unsure about the correct use of the API. I've got an example program that sends a stream of integers, but I haven't found a good real-world example to implement yet.

@robert-ancell robert-ancell marked this pull request as ready for review September 22, 2020 23:13
@robert-ancell
Copy link
Contributor Author

This change is going to require the Linux template to be updated due to the new header file. But that means there must be a transition point where Flutter gets the new engine but the template is broken as it will be missing it. @stuartmorgan is there a way around that?

Copy link
Member

@jpnurmi jpnurmi left a comment

Choose a reason for hiding this comment

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

Thank you for working on this. I bumped into missing event channels on Linux and found this. For what it's worth, this works fine for me with simple string values.

@stuartmorgan-g
Copy link
Contributor

This change is going to require the Linux template to be updated due to the new header file. But that means there must be a transition point where Flutter gets the new engine but the template is broken as it will be missing it. @stuartmorgan is there a way around that?

What would the actual breakage be? Wouldn't the effect just be that if someone manually deleted that one specific header (and none of the others), the rule wouldn't re-run?

If it actually is broken, then it's a significant problem. Any required template update means breaking everyone's existing project. Doing that post-alpha requires going through the full breaking change process at a minimum, and ideally adding project migration logic to flutter_tool.

On the other hand if it's just that the output list won't be comprehensive for older projects, then it should be fine to just update the template and not worry about existing projects.

slightfoot pushed a commit to fluttercommunity/plus_plugins that referenced this pull request Oct 2, 2020
slightfoot pushed a commit to fluttercommunity/plus_plugins that referenced this pull request Oct 2, 2020
slightfoot pushed a commit to fluttercommunity/plus_plugins that referenced this pull request Oct 2, 2020
slightfoot pushed a commit to fluttercommunity/plus_plugins that referenced this pull request Oct 2, 2020
@robert-ancell
Copy link
Contributor Author

What would the actual breakage be? Wouldn't the effect just be that if someone manually deleted that one specific header (and none of the others), the rule wouldn't re-run?

I'm not 100% sure but I think the following might happen:

  1. You create a new flutter project using Flutter before FlEventChannel was created:
    $ flutter create my_new_app
  2. This app contains linux/flutter/CMakeLists.txt which refers to all the flutter_linux headers, e.g. flutter_linux.h, fl_method_codec.h etc.
  3. You update Flutter, which now includes the engine that supports FlEventChannel.
  4. You do flutter run -d linux, which copies the headers as specified in CMakeLists.txt. This means it wont copy the new fl_event_channel.h but will copy the version flutter_linux.h that refers to it. The program will fail to build.
  5. You re-run flutter create to get the new CMakeLists.txt, the build now works.

It seems to me that CMakeLists.txt should copy all the header files found in the flutter_linux directory, though I'm not sure the best way to do that with CMake.

@stuartmorgan-g
Copy link
Contributor

You do flutter run -d linux, which copies the headers as specified in CMakeLists.txt.

Ah, that's not how the copy works. flutter copies the headers (here), and the artifact that it's copying there is the entire flutter_linux folder. It doesn't care what's in CMakeLists.txt.

Copy link
Contributor

@stuartmorgan-g stuartmorgan-g left a comment

Choose a reason for hiding this comment

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

LGTM

@chinmaygarde
Copy link
Member

Tree is open now. Landing.

@chinmaygarde chinmaygarde merged commit 5ca5e26 into flutter:master Oct 22, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Oct 23, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Oct 23, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Oct 23, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Oct 23, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Oct 23, 2020
zanderso pushed a commit to flutter/flutter that referenced this pull request Oct 23, 2020
* 000bf4b Roll Skia from 2d2f82c00aeb to 5c7bb326a7b3 (33 revisions) (flutter/engine#22059)

* ae92dbf Roll Fuchsia Linux SDK from lPMs_KwnU... to gqS_DIjN4... (flutter/engine#22057)

* 92cd74e Roll Fuchsia Mac SDK from pZ9FgVZTK... to WLxBkBnZa... (flutter/engine#22055)

* e51c710 Roll Dart SDK from a3d902d8598e to 9f907e198970 (2 revisions) (flutter/engine#22058)

* 326b202 Reland fuchsia external view embedder will be shared with platform view (flutter/engine#22008)

* a9a9a2f Roll Skia from 5c7bb326a7b3 to 65674e4c2e56 (3 revisions) (flutter/engine#22060)

* 1233fe4 Revert "Revert "Explicitly make the X connection for EGL. (#21831)" (#21851)" (flutter/engine#21871)

* aed8e01 Fixes Edge trigger route change announcement (flutter/engine#21975)

* 6bc70e4 Reland: Migration to PlatformDispatcher and multi-window (flutter/engine#21932)

* 5ca5e26 Add FlEventChannel (flutter/engine#21316)

* 77b0052 Roll Skia from 65674e4c2e56 to 01b05e5b830b (3 revisions) (flutter/engine#22062)

* 3d27fd5 Support loading assets from Android dynamic feature modules (flutter/engine#21504)

* 742dfbe support uri intent launcher in android (flutter/engine#21275)

* cde1e3f Auto detect mode to determine which rendering backend to use. (flutter/engine#21852)

* 329ccf7 Roll Skia from 01b05e5b830b to 53281c712159 (1 revision) (flutter/engine#22065)

* cde78c1 Add a golden scenario test for fallback font rendering on iOS take 2 (flutter/engine#22033)

* 4f4599b Roll Dart SDK from 9f907e198970 to 37ccceacad41 (3 revisions) (flutter/engine#22069)

* f0b10c5 [web] Prevent using DOM nodes for canvas with large number of draws (flutter/engine#22064)

* a86ba57 Roll Fuchsia Mac SDK from WLxBkBnZa... to zDfaxkqlv... (flutter/engine#22073)

* 645198a Roll Fuchsia Linux SDK from gqS_DIjN4... to vuKxZmSVj... (flutter/engine#22074)

* 0b26570 Revert dart rolls (flutter/engine#22078)
@robert-ancell robert-ancell deleted the linux-shell-event-channel branch October 30, 2020 02:27
chaselatta pushed a commit to chaselatta/engine that referenced this pull request Nov 30, 2020
@Gustl22
Copy link

Gustl22 commented May 6, 2023

@robert-ancell I wonder where the method fl_event_channel_class_init is called, which initializes the fl_event_channel_dispose method? I searched the whole repository. My fear is, that I can't use the channel again, once it is disposed, but not properly. I get an error MissingPluginException(No implementation found for method listen on channel xyz.luan/audioplayers/events/somePlayerId). I may miss something, but I don't see where this method is used.
I get the error on our repository. If confirmed or disagreed, I can sent a more detailed example or search some where else. I appreciate your help :D

@stuartmorgan-g
Copy link
Contributor

I wonder where the method fl_event_channel_class_init is called

See https://docs.gtk.org/gobject/concepts.html#object-instantiation

I get an error MissingPluginException(No implementation found for method listen on channel xyz.luan/audioplayers/events/somePlayerId)

That suggests that dispose has been called, not that it hasn't.

@Gustl22
Copy link

Gustl22 commented May 7, 2023

Thank you @stuartmorgan, wasn't aware of the concept.
Yeah the event channel seems to be disposed, but a new one has been created, which still is linked to the old message handler, sth like that. Need to dig deeper I guess.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants