-
Notifications
You must be signed in to change notification settings - Fork 18
Conversation
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a few questions and possibly some changes.
|
||
val view: FlutterView = registrar.view() | ||
fun viewField(name: String): Field { | ||
val field = FlutterView::class.java.getDeclaredField(name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this need a try/catch?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See below: if this process goes wrong for someone after a Flutter-internal change, we can't really save anything and probably want to let that error propagate so they can tell us it broke.
val rendererField = viewField("flutterRenderer") | ||
val renderer = rendererField.get(view) as FlutterRenderer | ||
val touchProcessor = GamepadAndroidTouchProcessor(renderer) | ||
touchProcessorField.set(view, touchProcessor) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The "Hack" comment seems to suggest that this solution is tenuous in that it could possibly break if Flutter changes something about how it is implemented. For example if the "androidTouchProcessor" field changes names perhaps? If something like that were to happen would this code throw an uncaught exception? If so, should we try to catch the exception here and deal with it gracefully?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes — it's relying on the presence of a (private!) field with that name in a class in the Flutter engine, which could change in a future version.
This patching step is crucial to the working of the plugin: if it goes wrong, then the plug-in can't possibly do its job (because we will have no access to key/motion events). In a sense, crashing feels like the “right” thing to do, then: users of the plug-in will make an issue here on GitHub, and it will be up to us to fix and accommodate the new Flutter version.
If we preempt such a failure and Log.warning("uh-oh, blah blah blah, here's a stacktrace")
and do nothing else, we will just be making the failure a little less obvious.
It would be a different story if the Flutter version wasn't “baked into” an app. That is: if the trick works (for the user) in a given build of an app, then it won't suddenly stop working. So users of this plug-in (i.e. Flutter app developers) don't have to worry about this hack suddenly breaking on them if it works once for them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this what RawKeyboard is for in combination with InputDevice using a SOURCE_GAMEPAD
so you don't need to call private methods ,and rather handle the Raw key events?
https://api.flutter.dev/flutter/services/RawKeyEventDataAndroid-class.html
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, you (as a Flutter app developer) can use a RawKeyboard Flutter widget to get gamepad button events.
However, at the time of writing, that works only on Android, and only buttons are supported: the motion events from thumbsticks are discarded by AndroidTouchProcessor. (The events also don't seem to have a deviceId
field, only vendorId
and productId
, so theoretically you couldn't tell two identical Dualshocks connected to the device apart. This is kind of a nitpick.)
If we want to offer a plug-in that has the same API on both iOS and Android, i.e. if we want to put button events onto the flutter_gamepad
event stream, then we need the trick, because Flutter widgets are immaterial to us at the plug-in level. So RawKeyboard's existence does not help us much.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unrelated to this PR, but just had the thought that it might be a good idea to specify on this https://pub.dev/packages/flutter_gamepad that it is tested and works with specific versions of Flutter and that it is potentially not compatible with versions that haven't been tested.
Also I'm curious, if the flutter_gamepad plugin did cause a crash due to incompatibility with the version of Flutter that the Flutter app developer is using it with, would it be obvious to the developer why/where it crashed (assuming the developer has no background in platform specific development so doing something like running the app through XCode is too foreign for them to try)? In other words, if it were to crash after said app developer did a "flutter run" on their app, would they be able to tell that it is the gamepad_plugin that is malfunctioning?
val keyEventChannel = keyEventChannelField.get(view) as KeyEventChannel | ||
val textInputPlugin = textInputPluginField.get(view) as TextInputPlugin | ||
val keyProcessor = GamepadAndroidKeyProcessor(keyEventChannel, textInputPlugin) | ||
keyProcessorField.set(view, keyProcessor) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment here regarding "Hack".
* An extension of Flutter's AndroidKeyProcessor that delegates KeyEvents to the GamepadStreamHandler. | ||
*/ | ||
class GamepadAndroidKeyProcessor(keyEventChannel: KeyEventChannel, textInputPlugin: TextInputPlugin) : AndroidKeyProcessor(keyEventChannel, textInputPlugin) { | ||
override fun onKeyDown(keyEvent: KeyEvent) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just curious, but why/how do gamepads trigger key events?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, on Android a gamepad button press is a kind of KeyEvent. :)
(And a gamepad thumbstick movement is a kind of MotionEvent, just like a mouse or pointer movement.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added comments explaining this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me.
Adds Android support to the gamepad plugin.
@apecoraro @nir-ziv To test:
Basically, you should see something like this: