-
Notifications
You must be signed in to change notification settings - Fork 9
error handling and add custom keyboard key listener #19
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
base: main
Are you sure you want to change the base?
Conversation
|
I get an error: |
Goddchen
left a comment
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.
suggestion: I'd also suggest to issue PRs that only tackle one feature / bug / issue per PR. Makes the whole process easier.
| ), | ||
| ), | ||
| (final SendPort sendPort) { | ||
| num x = details.localPosition.dx / |
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.
| num x = details.localPosition.dx / | |
| final num x = details.localPosition.dx / |
| num x = details.localPosition.dx / | ||
| _remoteFrameBufferWidgetSize.width * | ||
| _image.width; | ||
| num y = details.localPosition.dy / |
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.
| num y = details.localPosition.dy / | |
| final num y = details.localPosition.dy / |
| ), | ||
| ), | ||
| (final SendPort sendPort) { | ||
| num x = details.localPosition.dx / |
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.
| num x = details.localPosition.dx / | |
| final num x = details.localPosition.dx / |
| num x = details.localPosition.dx / | ||
| _remoteFrameBufferWidgetSize.width * | ||
| _image.width; | ||
| num y = details.localPosition.dy / |
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.
| num y = details.localPosition.dy / | |
| final num y = details.localPosition.dy / |
| ); | ||
| } | ||
|
|
||
| void _rawKeyEventListener(final RawKeyEvent rawKeyEvent) { |
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.
format: I prefer those one-liners to be expression methods.
| hostname: initMessage.hostName, | ||
| password: initMessage.password.toNullable(), | ||
| port: initMessage.port, | ||
| timeout: initMessage.timeout, |
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.
question: the current client implementation does not have a timeout parameter. Are you planing on creating a PR in that project as well?
| required final String hostName, | ||
| required final Option<String> password, | ||
| required final int port, | ||
| required final Duration? timeout, |
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.
suggestion: I want to avoid all null-aware code in this package. My suggestion would be to make this an Option<Duration> instead.
| final Option<String> _password; | ||
| final int _port; | ||
| final Option<ValueNotifier<Map<String, dynamic>>> _keyEventNotifier; | ||
| final Option<bool> _monitorClipBoard; |
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.
suggestion: can't this be a simple bool? Makes the using code a lot shorter and more readable as well.
| final void Function(Object error)? onError, | ||
| final String? password, | ||
| final int port = 5900, | ||
| final ValueNotifier<Map<String, dynamic>>? keyEventNotifier, |
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.
question: is it a usecase that you need right now? Knowing from outside of this widget, which keys are being pressed? I'd expect that you would just add a surrounding RawKeyboardListener widget (https://api.flutter.dev/flutter/widgets/RawKeyboardListener-class.html) to achieve this.
| (final ValueNotifier<Map<String, dynamic>> notifier) { | ||
| final Map<String, dynamic> event = notifier.value; | ||
| final LogicalKeyboardKey keyboardKey = event['keyboardKey']; | ||
| sendPort.send( |
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.
question: aren't we sending key events twice now?
No description provided.