Rework keyboard event / keyboard shortcut #338

pchampio · 2020-01-12T00:06:20Z

fixes: #314
fixes: #332
fixes: #358

Uses UTF-16 for textencoding: ref

key-codepoint.cpp

james-lawrence · 2020-04-05T23:18:45Z

keyboard_events demo didn't work at all:

An Observatory debugger and profiler on Flutter test device is available at: http://127.0.0.1:50300/
For a more detailed help message, press "h". To detach, press "d"; to quit, press "q".
flutter: ══╡ EXCEPTION CAUGHT BY SERVICES LIBRARY ╞══════════════════════════════════════════════════════════
flutter: The following _Exception was thrown during a platform message callback:
flutter: Exception: Unsupported platform RawKeyEventDataMacOs
flutter: 
flutter: When the exception was thrown, this was the stack:
flutter: #0      _KeyboardTestPageState.onKeyEvent (package:keyboard_event/app.dart:83:9)
flutter: #1      _RawKeyboardListenerState._handleRawKeyEvent (package:flutter/src/widgets/raw_keyboard_listener.dart:121:14)
flutter: #2      RawKeyboard._handleKeyEvent (package:flutter/src/services/raw_keyboard.dart:528:17)
flutter: #3      BasicMessageChannel.setMessageHandler.<anonymous closure> (package:flutter/src/services/platform_channel.dart:74:49)
flutter: #4      _DefaultBinaryMessenger.handlePlatformMessage (package:flutter/src/services/binding.dart:200:33)
flutter: #5      _invoke3.<anonymous closure> (dart:ui/hooks.dart:303:15)
flutter: #9      _invoke3 (dart:ui/hooks.dart:302:10)
flutter: #10     _dispatchPlatformMessage (dart:ui/hooks.dart:162:5)
flutter: (elided 3 frames from package dart:async)
flutter: ════════════════════════════════════════════════════════════════════════════════════════════════════
flutter: Another exception was thrown: Exception: Unsupported platform RawKeyEventDataMacOs

pchampio · 2020-04-06T10:11:14Z

I've update the keyboard_events accordingly. The issue was in the example.
ctrl + delete is handled by the flutter framework, I don't want to support Ctrl+delete.

The 2 other points needs to be address before merging this PR.

pchampio · 2020-04-06T15:16:03Z

I'm still a bit surprised by the fact that shift+arrow doesn't select text. :/
The text navigation, copy/paste combinations that I specified in the readme are for linux/windows.

Are the MacOS one working https://i.imgur.com/YPpx9Px.png working?
When I simulate an MacOS environment, everything seems to work 😞

https://transfer.sh/dynwB/dell_xps_pchampion.mp4

james-lawrence · 2020-04-12T18:34:48Z

shift, control, and command results in the following:

flutter: ══╡ EXCEPTION CAUGHT BY SERVICES LIBRARY ╞══════════════════════════════════════════════════════════
flutter: The following assertion was thrown during a platform message callback:
flutter: Platform key support for macos is producing unsupported modifier combinations.
flutter: 'package:flutter/src/services/raw_keyboard.dart':
flutter: Failed assertion: line 580 pos 14: 'et<LogicalKeyboard'
flutter: 
flutter: Either the assertion indicates an error in the framework itself, or we should provide substantially
flutter: more information in this error message to help you determine and fix the underlying cause.
flutter: In either case, please report this assertion by filing a bug on GitHub:
flutter:   https://github.com/flutter/flutter/issues/new?template=BUG.md
flutter: 
flutter: When the exception was thrown, this was the stack:
flutter: #2      RawKeyboard._synchronizeModifiers (package:flutter/src/services/raw_keyboard.dart:580:14)
flutter: #3      RawKeyboard._handleKeyEvent (package:flutter/src/services/raw_keyboard.dart:522:5)
flutter: #4      BasicMessageChannel.setMessageHandler.<anonymous closure> (package:flutter/src/services/platform_channel.dart:74:49)
flutter: #5      _DefaultBinaryMessenger.handlePlatformMessage (package:flutter/src/services/binding.dart:200:33)
flutter: #6      _invoke3.<anonymous closure> (dart:ui/hooks.dart:303:15)
flutter: #10     _invoke3 (dart:ui/hooks.dart:302:10)
flutter: #11     _dispatchPlatformMessage (dart:ui/hooks.dart:162:5)
flutter: (elided 5 frames from class _AssertionError and package dart:async)
flutter: ════════════════════════════════════════════════════════════════════════════════════════════════════

james-lawrence

overall LGTM, not sure how I feel about runtime.GOOS == "darwin" vs using build tags and methods.

will need the change I opened a PR for to work properly on macs though.

james-lawrence · 2020-04-18T13:22:59Z

internal/glfwkeyconversion/map-keycode-to-macos.go

@@ -0,0 +1,559 @@
+package glfwkeyconversion
+
+import "github.com/go-gl/glfw/v3.3/glfw"


we may be better served by making this platform specific.

internal/keyboard/keyboard_darwin.go // has all the mappings defined in these two files. internal/keyboard/keyboard_unix.go // with a +build unix !darwin internal/keyboard/keyboard_windows.go

each file with a single method:

func Normalize(keycode glfw.Key, scancode int) Event { /* ... */ }

// internal/keyboard/keyboard.go // replaces keyEventMessage type Event struct { // ... }

james-lawrence · 2020-04-18T13:26:56Z

key-events.go

+	utf8 := glfw.GetKeyName(key, scancode)
+	var event keyEventMessage
+
+	if runtime.GOOS == "darwin" {


if we do the build tags this code would collapse to:

event := keyboard.Normalize(key, scancode) if err := p.keyEventChannel.Send(event); err != nil { /* .... */ }

james-lawrence · 2020-04-18T13:27:58Z

key-events.go

+// code point will be used. There is unlikely to be more than one, but there
+// is no guarantee that it won't happen.
+func CodepointFromGLFWKey(utf8 []rune) uint32 {
+	return uint32(utf8[0])


lets handle the zero length here and return 0.

james-lawrence · 2020-04-18T13:33:09Z

textinput.go

+				return
+			}
+			// Word Backspace
+			if (runtime.GOOS == "darwin" && mods == glfw.ModAlt) || (runtime.GOOS != "darwin" && mods == glfw.ModControl) {


if keyboard.DetectDeleteWord(key, mods) { /* ... */ }

james-lawrence · 2020-04-18T13:39:12Z

internal/glfwkeyconversion/map-modifiers-to-macos.go

+
+// AsMacOSModifiers translate the keycode to the ModifierKey
+func AsMacOSModifiers(keycode glfw.Key) (int, bool) {
+	val, ok := modifierKeytoMods[keycode]


~~a simple switch statement is likely more performant.~~ could be wrong here =)

It's an interesting topic. According to this blogpost the map is actually faster, but not much difference in absolute times. https://hashrocket.com/blog/posts/switch-vs-map-which-is-the-better-way-to-branch-in-go

yeah, I ran across that which is why I reverted my thinking.

its actually an interesting optimization that could be done to the switch statements at the compiler level.

I suspect its because maps are hashing the key which is a fixed cost based on the key length. then doing a lookup. whereas a switch statement is doing multiple comparisons between the key and the case values.

james-lawrence · 2020-04-20T14:30:14Z

internal/glfwkeyconversion/map-modifiers-to-macos.go

+
+// ToMacOSModifiers takes a glfw ModifierKey and return his MacOS equivalent
+// as defined in https://github.com/flutter/flutter/blob/3e63411256cc88afc48044aa5ea06c5c9c6a6846/packages/flutter/lib/src/services/raw_keyboard_macos.dart#L241
+func ToMacOSModifiers(mods glfw.ModifierKey) int {


removes the need to initialize it below.

Suggested change

func ToMacOSModifiers(mods glfw.ModifierKey) int {

func ToMacOSModifiers(mods glfw.ModifierKey) (macOSmods int) {

The flutter framework sends utf16 vectors

GeertJohan · 2020-04-24T09:50:36Z

textinput.go

@@ -104,7 +105,7 @@ func (p *textinputPlugin) handleSetEditingState(arguments interface{}) (reply in
 		return nil, errors.Wrap(err, "failed to decode json arguments for handleSetEditingState")
 	}

-	p.word = []rune(editingState.Text)
+	p.word = utf16.Encode([]rune(editingState.Text))


utf16? Is that what Flutter uses?
I'd think utf8 would be the default for modern projects.... Especially with the "utf-8 everywhere" movement that's going on.
http://utf8everywhere.org/

utf16? Is that what Flutter uses?

Yes, that what uses Dart to be more specific!

Here is some readings:

Bring Dart's String support into the modern age dart-lang/sdk#28404

https://github.com/flutter/flutter/blob/e0c63cd35e15e407a80dc44281cc392535fcce25/packages/flutter/lib/src/services/text_formatter.dart#L197-L204

A bug we encounter: #314 due to a miss match between utf8 and utf16 indexes.

I'd think utf8 would be the default for modern projects.... Especially with the "utf-8 everywhere" movement that's going on.

Well, the glfw callback returns a utf8 code point, we then convert it to utf16 at the embedder level and then send it to flutter.
On the outside, we support as input utf8-based code point 😄... but on the inside.. it'sa Roller coaster casting character 🪂

In for the ride, choo choo! 🎢

pchampio · 2020-05-22T20:35:59Z

Ready for another review/fix cycle!

GeertJohan · 2020-05-24T10:31:34Z

text-input.go

+		return nil, errors.Wrap(err, "failed to decode json arguments for handleSetClient")
+	}
+
+	err = json.Unmarshal(args[0], &p.clientID)


Length of 'args' is never checked, will cause panic if the message format ever changes.

GeertJohan · 2020-05-24T10:34:14Z

key-events.go

-	}
-	err := p.keyEventChannel.Send(event)
-	if err != nil {
+	if err := p.channel.Send(event); err != nil {


I prefer to never inline the assignment to err unless it's a function that only fetches some information or performs a check. In this case, the call to Send is actually important for the function a a whole, not just for this error check, so it feels off to inline it. But that could just be personal preference.

GeertJohan · 2020-05-24T10:42:48Z

text-input.go

+	}
+	// Opinionated: If a flutter dev uses TextCapitalization.characters
+	//              in a TextField, that means he wants only to receive
+	//              uppercase characters.


Interesting. In some cases on Android it may be possible to switch back from the uppercased keyboard to the lowercased one... Although indeed that may not be the intention of the developer. We'll have to see how this lands with go-flutter users. For now I think this is the best approach yes.

GeertJohan

Just a missing check on length of a slice.

Otherwise LGTM!

pchampio · 2020-05-25T13:47:04Z

Ready for another review/fix cycle!

pchampio requested a review from GeertJohan January 12, 2020 00:06

pchampio changed the title ~~don't send key event when the textinput has client~~ [WIP] don't send key event when the textinput has client Jan 14, 2020

pchampio changed the title ~~[WIP] don't send key event when the textinput has client~~ [WIP] Rework keyboard event / keyboard shortcut Jan 15, 2020

pchampio mentioned this pull request Jan 31, 2020

Copy paste. Value is inserted twice. #349

Closed

pchampio mentioned this pull request Mar 3, 2020

Target platfrom linux & windows available #367

Closed

james-lawrence reviewed Mar 29, 2020

View reviewed changes

key-codepoint.cpp Outdated Show resolved Hide resolved

pchampio mentioned this pull request Apr 4, 2020

Fix keyboard shortcut #383

Closed

Working shortcuts on Linux/windows/macos

38bf8bd

pchampio force-pushed the fix_keyboard_shortcut branch from 05dc761 to 38bf8bd Compare April 4, 2020 22:38

pchampio added 2 commits April 5, 2020 12:38

small refactor

4651af0

fix multiple modifier

48058a0

james-lawrence reviewed Apr 18, 2020

View reviewed changes

james-lawrence reviewed Apr 20, 2020

View reviewed changes

pchampio added 2 commits April 23, 2020 18:45

switch to utf16

56711e7

The flutter framework sends utf16 vectors

support surrogate

7f352bd

GeertJohan reviewed Apr 24, 2020

View reviewed changes

pchampio mentioned this pull request May 19, 2020

Add TextInput stub implementations #446

Closed

pchampio added 3 commits May 22, 2020 17:04

adress review comments

6620ba8

fix doc

8e9be19

chore: PrintStack add go-flutter prefix

b3054ca

pchampio requested a review from GeertJohan May 22, 2020 20:35

pchampio changed the title ~~[WIP] Rework keyboard event / keyboard shortcut~~ Rework keyboard event / keyboard shortcut May 22, 2020

GeertJohan reviewed May 24, 2020

View reviewed changes

GeertJohan requested changes May 24, 2020

View reviewed changes

adress review comments

9154bf3

pchampio requested a review from GeertJohan May 25, 2020 11:31

pchampio added 5 commits May 25, 2020 13:43

chore: remove the assumtion that dev are all male

d8c0fed

Merge branch 'master' into fix_keyboard_shortcut

992dd1e

fix keyboardLayout removal

4280f8a

chore: remove runtime usage in text-input

78d2e17

rm new line

b80a6cf

GeertJohan approved these changes May 25, 2020

View reviewed changes

pchampio merged commit 51326a1 into master May 25, 2020

pchampio deleted the fix_keyboard_shortcut branch May 25, 2020 14:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rework keyboard event / keyboard shortcut #338

Rework keyboard event / keyboard shortcut #338

pchampio commented Jan 12, 2020 •

edited

Loading

james-lawrence commented Apr 5, 2020 •

edited

Loading

pchampio commented Apr 6, 2020

pchampio commented Apr 6, 2020

james-lawrence commented Apr 12, 2020

james-lawrence left a comment

james-lawrence Apr 18, 2020

james-lawrence Apr 18, 2020

james-lawrence Apr 18, 2020

james-lawrence Apr 18, 2020

james-lawrence Apr 18, 2020 •

edited

Loading

GeertJohan Apr 20, 2020

james-lawrence Apr 20, 2020

james-lawrence Apr 20, 2020

GeertJohan Apr 24, 2020

pchampio Apr 24, 2020 •

edited

Loading

GeertJohan Apr 28, 2020

pchampio commented May 22, 2020

GeertJohan May 24, 2020 •

edited

Loading

GeertJohan May 24, 2020

GeertJohan May 24, 2020 •

edited

Loading

GeertJohan left a comment

pchampio commented May 25, 2020

		@@ -0,0 +1,559 @@
		package glfwkeyconversion

		import "github.com/go-gl/glfw/v3.3/glfw"

	func ToMacOSModifiers(mods glfw.ModifierKey) int {
	func ToMacOSModifiers(mods glfw.ModifierKey) (macOSmods int) {

Rework keyboard event / keyboard shortcut #338

Rework keyboard event / keyboard shortcut #338

Conversation

pchampio commented Jan 12, 2020 • edited Loading

james-lawrence commented Apr 5, 2020 • edited Loading

pchampio commented Apr 6, 2020

pchampio commented Apr 6, 2020

james-lawrence commented Apr 12, 2020

james-lawrence left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

james-lawrence Apr 18, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pchampio Apr 24, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pchampio commented May 22, 2020

GeertJohan May 24, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GeertJohan May 24, 2020 • edited Loading

Choose a reason for hiding this comment

GeertJohan left a comment

Choose a reason for hiding this comment

pchampio commented May 25, 2020

pchampio commented Jan 12, 2020 •

edited

Loading

james-lawrence commented Apr 5, 2020 •

edited

Loading

james-lawrence Apr 18, 2020 •

edited

Loading

pchampio Apr 24, 2020 •

edited

Loading

GeertJohan May 24, 2020 •

edited

Loading

GeertJohan May 24, 2020 •

edited

Loading