[glfw] Send the glfw key data to the framework. #9386

franciscojma86 · 2019-06-19T22:51:50Z

Sends GLFW keyboard data as Linux key data, as opposed to Android. Includes the unmodified unicode character generated by the key.

Depends on flutter/flutter#34752

Fixes flutter/flutter#33395

stuartmorgan-g · 2019-06-20T03:34:24Z

Please be sure to update the FDE testbed app's raw key event test page to expect Linux rather than Android at the same time as landing this.

pchampio · 2019-06-27T21:03:06Z

GLFW has a key event action named GLFW_REPEAT for repeated keys that are held down.

We had a discussion about it in go-flutter-desktop/go-flutter#136 (comment)
TLDR; we choose to interpret the GLFW_REPEAT action as GLFW_DOWN action.

What is your take on this?

stuartmorgan-g · 2019-06-29T04:27:20Z

What is your take on this?

I don't see any reason for desktop to deviate from the existing Android behavior at this point. If someone files an issue with a use case for adding Repeat as a separate event type, that can be revisited across all platforms.

@franciscojma86 Feel free to add repeat handling to this PR since it's a minor change.

cbracken · 2019-08-05T17:58:28Z

@franciscojma86 what's the status of this PR?

franciscojma86 · 2019-08-05T18:07:01Z

Will be ready once the flutter/flutter#34752 is landed, which I'll finish right after my rotation.

stuartmorgan-g

LGTM (although per above I'd suggest adding repeat-as-down to this since you're changing this code anyway and that's a simple change).

franciscojma86 · 2019-08-08T01:16:41Z

@stuartmorgan sorry for the re-review. Added a function to convert the utf8 encoded code point to GLFW to an int, which is what the framework accepts.

stuartmorgan-g · 2019-08-09T19:21:23Z

shell/platform/glfw/key_event_handler.cc

+  masks[2] = 0x1F;
+  masks[1] = 0xFF;
+
+  // The number of bits to shift a byte depending on it's position.


s/it's/its/

stuartmorgan-g · 2019-08-09T19:23:48Z

shell/platform/glfw/key_event_handler.cc

+  int shift = length;
+
+  // All possible masks for a 4 byte utf8 code point.
+  std::map<int, int> masks;


Why not use a vector for these rather than a map, since you are always only using the first 4 positions? (You'd want to zero-index rather than one-index, obviously)

stuartmorgan-g · 2019-08-09T19:25:41Z

shell/platform/glfw/key_event_handler.cc


+// Converts a utf8 const char* to a 32 bit int. The Flutter framework accepts
+// only one 32 bit int, therefore, only up to 4 bytes are accepted. If it's
+// larger than 4 bytes, only the first 4 will be used.


What happens if a multi-byte character straddles position 4? I didn't see any obvious handling of that; if I missed it maybe it just needs clear commenting at that point.

stuartmorgan-g · 2019-08-09T19:28:01Z

shell/platform/glfw/key_event_handler.cc

+    int current_byte = utf8[current_byte_index];
+    int mask = current_byte_index == 0 ? masks[length] : complement_mask;
+    current_byte_index++;
+    result += (current_byte & mask) << bits[shift--];


Why is there a mask for each individual byte? I think I'm not understanding the role the mask plays in this algorithm, so it would be helpful if you could expand on the algorithm in comments in the implementation. It's non-obvious to me why there's a mask at all.

I updated the documentation with a run through. LMK if it answers your question.

stuartmorgan-g

Makes a lot more sense now; just small comments.

stuartmorgan-g · 2019-08-12T21:46:41Z

shell/platform/glfw/key_event_handler.cc

+
 namespace flutter {

+// Information about the UTF-8 encoded code point.


Wrap this and the helper function in an anonymous namespace so they have internal linkage.

stuartmorgan-g · 2019-08-12T21:50:40Z

shell/platform/glfw/key_event_handler.cc

+
+// Queries GLFW for the printable key name given a [key] and [scan_code] and
+// converts it to UTF-32. The Flutter framework accepts only one 32 bit
+// int, therefore, if the [code_point]'s length is > 4 (e.g. has two code


// The Flutter framework accepts only one 32 bit // int, therefore, if the [code_point]'s length is > 4 (e.g. has two code // points)

This comment is misleading. First, it always sends only the first point, regardless of length (consider "ab"). Second, the reason isn't that it accepts "one 32-bit int" ("ab"'s UTF-8 encoding fits in a 32-bit int), but that the Flutter framework is expecting exactly one code point.

stuartmorgan-g · 2019-08-12T21:51:31Z

shell/platform/glfw/key_event_handler.cc

+// int, therefore, if the [code_point]'s length is > 4 (e.g. has two code
+// points), only the first will be used. This unlikely but there is no guarantee
+// that it won't happen.
+bool GetGLFWCodePoint(int key, int scan_code, uint32_t& code_point) {


Let's call this GetUTF32CodePointFromGLFWKey so it's more obvious what exactly it's doing.

stuartmorgan-g · 2019-08-12T21:52:10Z

shell/platform/glfw/key_event_handler.cc

+// int, therefore, if the [code_point]'s length is > 4 (e.g. has two code
+// points), only the first will be used. This unlikely but there is no guarantee
+// that it won't happen.
+bool GetGLFWCodePoint(int key, int scan_code, uint32_t& code_point) {


uint32_t*. The C++ style guide forbids using references for out-params.

stuartmorgan-g · 2019-08-12T21:54:32Z

shell/platform/glfw/key_event_handler.cc

+  int shift = length - 1;
+
+  // The number of bits to shift a byte depending on its position.
+  std::vector<int> bits = {0, 6, 12, 18};


Why is this a lookup table rather than just a multiplication by 6?

stuartmorgan-g · 2019-08-12T21:54:43Z

shell/platform/glfw/key_event_handler.cc

+  // The number of bits to shift a byte depending on its position.
+  std::vector<int> bits = {0, 6, 12, 18};
+
+  int complement_mask = 0x3F;


const int

stuartmorgan-g · 2019-08-12T21:55:14Z

shell/platform/glfw/key_event_handler.cc

+    return false;
+  }
+  // The first byte determines the length of the whole code point.
+  auto byte_info = GetUTF8CodePointInfo(utf8[0]);


stuartmorgan-g · 2019-08-12T21:57:00Z

shell/platform/glfw/key_event_handler.cc

+  }
+  // The first byte determines the length of the whole code point.
+  auto byte_info = GetUTF8CodePointInfo(utf8[0]);
+  size_t length = byte_info.length;


Consider just using byte_info.length in the two places it's used, rather than adding another variable that the reader needs to keep track of.

stuartmorgan-g

LGTM

stuartmorgan-g · 2019-08-12T22:35:59Z

shell/platform/glfw/key_event_handler.cc

+
+// Queries GLFW for the printable key name given a [key] and [scan_code] and
+// converts it to UTF-32. The Flutter framework accepts only one code point,
+// therefore, only the first code point will be used. This unlikely but there is


s/This is unlikely/There is unlikely to be more than one/ (the "This" currently doesn't refer to anything that was actually said before.)

…ne#9386)

[email protected]:flutter/engine.git/compare/c43739ba53ce...51bdf83 git log c43739b..51bdf83 --no-merges --oneline 2019-08-21 [email protected] Migrate Embedder API documentation to Doxygen format. (flutter/engine#11255) 2019-08-21 [email protected] Do not add null task observers (flutter/engine#11315) 2019-08-21 [email protected] When using a custom compositor, ensure the root canvas is flushed. (flutter/engine#11310) 2019-08-20 [email protected] [glfw] Send the glfw key data to the framework. (flutter/engine#9386) The AutoRoll server is located here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+/master/autoroll/README.md If the roll is causing failures, please contact the current sheriff ([email protected]), and stop the roller if necessary.

franciscojma86 requested a review from stuartmorgan-g June 19, 2019 22:51

googlebot added the cla: yes label Jun 19, 2019

franciscojma86 requested a review from chinmaygarde June 19, 2019 22:56

chinmaygarde approved these changes Jun 19, 2019

View reviewed changes

This was referenced Jun 20, 2019

[glfw] Use RawKeyEventDataLinux on testbed keyboard page google/flutter-desktop-embedding#437

Merged

[linux] Receives the unmodified characters obtained from GLFW flutter/flutter#34752

Merged

franciscojma86 force-pushed the keydata branch from 6f4de16 to 319bc7b Compare June 24, 2019 20:17

stuartmorgan-g approved these changes Aug 5, 2019

View reviewed changes

franciscojma86 requested a review from stuartmorgan-g August 8, 2019 01:15

franciscojma86 mentioned this pull request Aug 8, 2019

Support keydata mapping for GLFW shell flutter/flutter#37830

Closed

stuartmorgan-g reviewed Aug 9, 2019

View reviewed changes

franciscojma86 force-pushed the keydata branch 2 times, most recently from 6aa2051 to 7670985 Compare August 12, 2019 21:43

stuartmorgan-g suggested changes Aug 12, 2019

View reviewed changes

stuartmorgan-g approved these changes Aug 12, 2019

View reviewed changes

stuartmorgan-g added affects: desktop glfw labels Aug 14, 2019

franciscojma86 added 7 commits August 20, 2019 14:17

[glfw] Send the glfw key data to the framework.

4a6bf35

Formatting

359b4c0

Rename to codePoint

7c1aed7

Pack chars into an int

dfb3208

works, needs clean

ac57f09

Add unicode converter function

e88f7cc

Fix constant name

c3b354d

franciscojma86 added 6 commits August 20, 2019 14:17

Remove header

9b95963

Remove coment

cb548f8

Address comments

f037592

Fix comment

0eefde9

Rename variable

1ff0e2c

Format file

a65fb4d

franciscojma86 force-pushed the keydata branch from 1d78c5d to a65fb4d Compare August 20, 2019 21:18

franciscojma86 merged commit a79a31a into flutter:master Aug 20, 2019

franciscojma86 deleted the keydata branch August 20, 2019 22:55

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 21, 2019

a79a31a [glfw] Send the glfw key data to the framework. (flutter/engi…

814cd1b

…ne#9386)

engine-flutter-autoroll mentioned this pull request Aug 21, 2019

Roll engine c43739ba53ce..bf138516d729 (3 commits) flutter/flutter#38938

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 21, 2019

a79a31a [glfw] Send the glfw key data to the framework. (flutter/engi…

9b14802

…ne#9386)

engine-flutter-autoroll mentioned this pull request Aug 21, 2019

Roll engine c43739ba53ce..bf138516d729 (3 commits) flutter/flutter#38939

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 21, 2019

a79a31a [glfw] Send the glfw key data to the framework. (flutter/engi…

a61f719

…ne#9386)

engine-flutter-autoroll mentioned this pull request Aug 21, 2019

Roll engine c43739ba53ce..bf138516d729 (3 commits) flutter/flutter#38941

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 21, 2019

a79a31a [glfw] Send the glfw key data to the framework. (flutter/engi…

afaa6bb

…ne#9386)

engine-flutter-autoroll mentioned this pull request Aug 21, 2019

Roll engine c43739ba53ce..bf138516d729 (3 commits) flutter/flutter#38942

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 21, 2019

a79a31a [glfw] Send the glfw key data to the framework. (flutter/engi…

d5b7626

…ne#9386)

engine-flutter-autoroll mentioned this pull request Aug 21, 2019

Roll engine c43739ba53ce..bf138516d729 (3 commits) flutter/flutter#38945

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 21, 2019

a79a31a [glfw] Send the glfw key data to the framework. (flutter/engi…

2b66d90

…ne#9386)

engine-flutter-autoroll mentioned this pull request Aug 21, 2019

Roll engine c43739ba53ce..bf138516d729 (3 commits) flutter/flutter#38946

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 21, 2019

a79a31a [glfw] Send the glfw key data to the framework. (flutter/engi…

d33c69c

…ne#9386)

engine-flutter-autoroll mentioned this pull request Aug 21, 2019

Roll engine c43739ba53ce..bf138516d729 (3 commits) flutter/flutter#38949

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 21, 2019

a79a31a [glfw] Send the glfw key data to the framework. (flutter/engi…

8ee772b

…ne#9386)

engine-flutter-autoroll mentioned this pull request Aug 21, 2019

Roll engine c43739ba53ce..bf138516d729 (3 commits) flutter/flutter#38950

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 21, 2019

a79a31a [glfw] Send the glfw key data to the framework. (flutter/engi…

3d6847d

…ne#9386)

engine-flutter-autoroll mentioned this pull request Aug 21, 2019

Roll engine c43739ba53ce..bf138516d729 (3 commits) flutter/flutter#38951

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 21, 2019

a79a31a [glfw] Send the glfw key data to the framework. (flutter/engi…

2ef6711

…ne#9386)

engine-flutter-autoroll mentioned this pull request Aug 21, 2019

Roll engine c43739ba53ce..51bdf834203c (4 commits) flutter/flutter#38952

Merged


		namespace flutter {

		// Information about the UTF-8 encoded code point.

[glfw] Send the glfw key data to the framework. #9386

[glfw] Send the glfw key data to the framework. #9386

Uh oh!

Conversation

franciscojma86 commented Jun 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stuartmorgan-g commented Jun 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pchampio commented Jun 27, 2019

Uh oh!

stuartmorgan-g commented Jun 29, 2019

Uh oh!

cbracken commented Aug 5, 2019

Uh oh!

franciscojma86 commented Aug 5, 2019

Uh oh!

stuartmorgan-g left a comment

Choose a reason for hiding this comment

Uh oh!

franciscojma86 commented Aug 8, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stuartmorgan-g left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stuartmorgan-g left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

franciscojma86 commented Jun 19, 2019 •

edited

Loading

stuartmorgan-g commented Jun 20, 2019 •

edited

Loading