Skip to content
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

[platform_channels] adds EventChannel Demo #462

Merged
merged 12 commits into from
Jun 12, 2020
Merged

[platform_channels] adds EventChannel Demo #462

merged 12 commits into from
Jun 12, 2020

Conversation

AyushBherwani1998
Copy link
Member

Description

Adds an EventChannel Demo where it streams the value of Accelerometer sensor.

GIF

Comment on lines 11 to 17
testWidgets('EventChannel accelerometer Stream test', (tester) async {
await tester.pumpWidget(MaterialApp(
home: EventChannelDemo(),
));

expect(find.text('No Data Available'), findsOneWidget);
});
Copy link
Member Author

@AyushBherwani1998 AyushBherwani1998 Jun 6, 2020

Choose a reason for hiding this comment

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

I'm not sure how can I mock the EventChannel.recieveBroadcastStream. Suggestions and ideas for testing are much appreciated.

Copy link
Member Author

@AyushBherwani1998 AyushBherwani1998 Jun 7, 2020

Choose a reason for hiding this comment

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

The EventChannel test in the framework helped me.

Here

builder: (context, snapshot) {
if (snapshot.hasData) {
return Center(
child: Column(
Copy link
Member Author

@AyushBherwani1998 AyushBherwani1998 Jun 6, 2020

Choose a reason for hiding this comment

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

Any suggestions for improving UI are welcome. We can also have three Contianers which change properties when a value changes. But since the sample is for learning purpose I kept it simple enough. I'm happy to iterate over sample to improve the UI.

Copy link
Contributor

Choose a reason for hiding this comment

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

As we discussed offline, I think you could do something simple with Animated Container, which always manages to make UI look cool.

Copy link
Member Author

Choose a reason for hiding this comment

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

@RedBrogdon Instead of using AnimatedContainer I tried Transform to rotate the containers whenever the value changes. There are three containers which resemble 3 Axis. Each Container rotates on a single axis.

GIF

The above example works fine on the Emulator, but when testing on the real device, the value changes too fast and hence the Containers keep rotating even while you are holding the phone.

I also tried with the AnimatedContainer, based on the values of x and y-axis from the Stream I was changing the height and width of the AnimatedContainer respectively, and on the value of z-axis, I'm changing the borderRadius, which works fine on the real device too.

GIF

Copy link
Member Author

Choose a reason for hiding this comment

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

The recent commit uses AnimatedContainer example.

@RedBrogdon RedBrogdon self-requested a review June 9, 2020 06:10
Copy link
Contributor

@RedBrogdon RedBrogdon left a comment

Choose a reason for hiding this comment

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

Looking great. I have a few notes, but nothing huge.

private lateinit var eventSink: EventChannel.EventSink

override fun onListen(arguments: Any?, events: EventChannel.EventSink?) {
if (events != null)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure that events could ever be null here without something having gone very, very wrong. If you're going to have a null check, I would probably put the registerListener call in there as well. No point in having the listener if you can't send events back.

// Creates a EventChannel, and sets a StreamHandler. The AccelerometerStreamHandler
// implements a StreamHandler and SensorEventListener to listen the value changes from
// the sensor and send it to dart side.
EventChannel(flutterEngine.dartExecutor, "accelerometer")
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider aligning the names of the two channels a little more. The previous one is "methodChannelDemo," and this one is "accelerometer" rather than "eventChannelDemo" or something similar. You could always rename the method channel one to "counter."

@@ -27,5 +35,14 @@ class MainActivity : FlutterActivity() {
}
}
}

val sensorManger: SensorManager = getSystemService(Context.SENSOR_SERVICE) as SensorManager
// Use instance of Sensor Service to get the ACCELEROMETER sensor.
Copy link
Contributor

Choose a reason for hiding this comment

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

I would recommend cutting the comments on line 40 and 42-44. Your code is easy to read and very direct. I don't think people need the comments to understand what you're doing.


import 'package:flutter/services.dart';

/// This class includes the implementation for [EventChannel] to listen value
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: "listen to value" and "from the accelerometer."

class Accelerometer {
static final _eventChannel = const EventChannel('accelerometer');

/// Method responsible to provide a stream of [AccelerometerReadings] to listen
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: "responsible for providing" and "to listen to"

/// Acceleration force along the z-axis.
final double z;

AccelerometerReadings(this.x, this.y, this.z);
Copy link
Contributor

Choose a reason for hiding this comment

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

Optional suggestion, but you could make this a const constructor.

import 'package:flutter/material.dart';
import 'package:platform_channels/src/accelerometer_event_channel.dart';

/// The widget demonstrates how to use [EventChannel] to listen continuous values
Copy link
Contributor

Choose a reason for hiding this comment

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

We're getting way into the Dart style guide here, but it's recommended to start class comments like these with a verb:

https://dart.dev/guides/language/effective-dart/documentation#prefer-starting-function-or-method-comments-with-third-person-verbs

In this case, you'd just drop "the widget" from the beginning.

/// The widget demonstrates how to use [EventChannel] to listen continuous values
/// of Accelerometer Sensor from platform.
///
/// The widget uses a [StreamBuilder] to rebuild it's descendant whenever it
Copy link
Contributor

Choose a reason for hiding this comment

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

Here, "the widget" is fine. It's just for the initial sentence that the noun is typically dropped.

builder: (context, snapshot) {
if (snapshot.hasData) {
return Center(
child: Column(
Copy link
Contributor

Choose a reason for hiding this comment

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

As we discussed offline, I think you could do something simple with Animated Container, which always manages to make UI look cool.

body: StreamBuilder<AccelerometerReadings>(
stream: Accelerometer.getReadings(),
builder: (context, snapshot) {
if (snapshot.hasData) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider checking for errors and displaying them as well. It's a good practice, and one some devices it's possible a user could get one.

@RedBrogdon
Copy link
Contributor

@amirh, would you mind taking a quick look at the tests in event_channel_demo_test.dart? I'm not hugely familiar with testing with mocked event channel code, and I imagine you (or someone on your team) are.

final height = 150 + snapshot.data.x * 10;
final borderRadius = snapshot.data.z;

return AnimatedContainer(
Copy link
Contributor

Choose a reason for hiding this comment

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

The changes you've made everywhere else are solid, so I think the only remaining question is what to do about displaying the data. While the AnimatedContainer code you have here is clean, valid, Flutter, I don't think it's obvious to someone looking at the app how the container visually relates to the numbers coming out of the accelerometer (I don't have the eyes to see small changes in border radius, for example).

For now, I would consider just going back to Text widgets, so you can put your time into finishing out the rest of the app (and the plugin app). Once those are done, you'll likely have time to come back to this section of the code, and (probably) you'll have had an idea for a great way to present the data.

That's just a suggestion, though. If you want to iterate on a few designs and try to find a great one, that's cool. You might try an animated bar chart, similar to what audio equalizers do, for example.

Copy link
Member Author

@AyushBherwani1998 AyushBherwani1998 Jun 12, 2020

Choose a reason for hiding this comment

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

The changes you've made everywhere else are solid, so I think the only remaining question is what to do about displaying the data. While the AnimatedContainer code you have here is clean, valid, Flutter, I don't think it's obvious to someone looking at the app how the container visually relates to the numbers coming out of the accelerometer (I don't have the eyes to see small changes in border radius, for example).

Completely makes sense to me.

For now, I would consider just going back to Text widgets, so you can put your time into finishing out the rest of the app (and the plugin app). Once those are done, you'll likely have time to come back to this section of the code, and (probably) you'll have had an idea for a great way to present the data.

SGTM, as we finish the project we can iterate over the design. Also, if I come across any idea for displaying data, I will submit a separate PR for that, so we don't have our progress on hold.

@RedBrogdon RedBrogdon merged commit 87c9cfa into flutter:master Jun 12, 2020
@RedBrogdon
Copy link
Contributor

Thanks for some more great code!

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

Successfully merging this pull request may close these issues.

None yet

2 participants