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

franciscojma86 · 2019-06-19T22:52:03Z

Description

Prefer using the unmodified unicode character obtained from GLFW, instead of a code point, which is obtained from a separate callback. This new logic now mimics the macOS implementation.
Part of #33395

Tests

Update raw_keyboard_test.dart

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
I signed the [CLA].
I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement].
I updated/added relevant documentation (doc comments with ///).
All existing and new tests are passing.
The analyzer (flutter analyze --flutter-repo) does not report any problems on my PR.
I am willing to follow-up on review comments in a timely manner.

Breaking Change

Does your PR require Flutter developers to manually update their apps to accommodate your change?

Yes, this is a breaking change (Please read [Handling breaking changes]). Replace this with a link to the e-mail where you asked for input on this proposed change.
No, this is not a breaking change.

packages/flutter/lib/src/services/raw_keyboard_linux.dart

franciscojma86 · 2019-06-20T18:36:14Z

Comments addressed, PTAL!

goderbauer

I am not sure about the implications of this change for our other platforms. Greg would probably be the best person to know.

goderbauer · 2019-06-20T20:50:31Z

dev/manual_tests/lib/raw_keyboard.dart

            dataText.add(Text('keyCode: ${data.keyCode} (${_asHex(data.keyCode)})'));
            dataText.add(Text('scanCode: ${data.scanCode}'));
-            dataText.add(Text('codePoint: ${data.codePoint}'));
+            dataText.add(Text('charactersIgnoringModifiers: ${data.charactersIgnoringModifiers}'));


Does this also work for all the other platforms we support? I remember @gspencergoog saying that not all data is available everywhere...

(or should we provide both properties?)

Each platform has their own RawKeyEventData with different properties, so this change wouldn't affect them. There's no need to send both since codePoint is just the decimal representation of the characters (which we were converting to string anyway).

franciscojma86 · 2019-06-20T21:06:26Z

This PR only affects the Linux raw keyboard data, which is not really being used at all. The glfw shell was sending the key data as Android events, due to some limitations with the GLFW apis. With this PR and this one (flutter/engine#9386), the shell will be able to send the key data as GLFW events. It's safe to land this PR, since this code isn't really being used, but also not urgent.

packages/flutter/test/services/raw_keyboard_test.dart

dkwingsmt · 2019-06-20T21:46:05Z

packages/flutter/lib/src/services/raw_keyboard_linux.dart

    @required this.keyHelper,
+    this.charactersIgnoringModifiers = '',
    this.scanCode = 0,
-    this.codePoint = 0,


Does this need a breaking change proposal? I wonder if it's an exception since it's related to engine/platform.
+cc @Hixie

Keyboard mapping is still a WIP, the linux part hasn't been implemented by the engine (will be after this PR is merged). I don't think it'd be worth it to add a proposal for this since these apis are still subject to change quickly and desktop is still not a stable platform.

We should still announce it on flutter-announce and label this as a break if it is a break.

packages/flutter/lib/src/services/raw_keyboard_linux.dart

cbracken

LGTM for the code change, though I wonder if there's a less ambiguous name for the charactersIgnoringModifiers parameter.

cbracken · 2019-06-20T21:58:40Z

packages/flutter/lib/src/services/raw_keyboard_linux.dart

+  /// [modifiers], arguments must not be null.
  const RawKeyEventDataLinux({
    @required this.keyHelper,
+    this.charactersIgnoringModifiers = '',


Should this be characterIncludingModifiers (character singular)? The doc comment below describes this as "the name of the printable key" -- is this something like e, E, é, ¸ (cédille), return (for 0x0d)?

I wonder if there's a way to avoid the use of the word 'character' which is pretty ambiguous when you get down to the level of differentiating between codepoints, glyphs, bytes and other things frequently referred to as 'characters' in discussions at this level.

I got that name from the macos implementation, which is the name Apple uses for their own API. So maybe it's good to keep for consistency? I agree that it being plural makes it a bit more ambiguous since it can be one character, but two codepoints. I could make it singular, but don't really have any other suggestions to change the word "character" :) WDYT?

On a second thought, given that the macOS implementation has the same name, I'd like to keep the same name for consistency. WDYT?

I agree it's a good idea to keep ourselves consistent across platforms, but we don't need to necessarily leak macOS concepts into Flutter APIs. To be honest, I'm still confused as to which meaning of 'character' this refers to and what that would look like in the presence of e.g. an IME, even after reading Apple's docs. I'd guess (but am not at all sure), that a keypress of the 'c' key when using a Japanese keyboard layout in 'romaji' input mode would emit 'c', but that in 'kana' input mode it might emit 'そ' (see my profile picture).

As noted above the word 'character' is really ambiguous and feels like a leftover from the old days where byte/grapheme/glyph/codepoint were regularly conflated, and APIs that don't communicate concepts precisely to users result in assumptions + bugs.

In any case, depending on what the behaviour is, I wonder if we can find a better name to expose than 'character'. @Hixie almost certainly has thoughts on this.

Definitely don't use the word "character". It's a meaningless word.

cbracken

Added a reply. I'd like @Hixie's thoughts on character-related terminology here.

Hixie · 2019-06-21T20:52:05Z

I would use "codepoint" rather than "characterIgnoringModifier". It's not at all clear to me what "characterIgnoringModifier" means ("character" is meaningless, "modifier" means many things depending on context, "ignoring" is ambiguous, and even if you get past all of that, I'm convinced that it'd be wrong in many cases, e.g. presumably hitting a dead key on layouts with dead keys would be reported as only the "modifier", and hitting a non-dead key like the backquote on a US 101-key layout would be a "modifier" too).

Hixie · 2019-06-21T20:55:42Z

packages/flutter/lib/src/services/raw_keyboard_linux.dart

+      if (charactersIgnoringModifiers.length == 2) {
+        final int secondCode = charactersIgnoringModifiers.codeUnitAt(1);
+        codeUnit = (codeUnit << 16) | secondCode;
+      }


I don't understand this code at all.

It appears this code is untested. We should definitely test whatever this code is doing.

Greg suggested this code as a safeguard against any really rare/custom keyboard that would send a unicode character with more than one code unit on a single key press. I can add a test and documentation to clarify this.

I'm not sure what you mean by "unicode character" or "code unit" or how a keyboard could do this. Can you elaborate?

By "unicode character" I meant the code point. By "code unit" I was referring to the UTF-16 representation of the code point. This check covers the case were charactersIgnoringModifiers is a high code point and so it would be a surrogate pair.

This check might be a bit paranoid since it's highly unlikely that a keyboard would do this, but wanted to cover it anyway given the possibility in the future. I can remove it if you think it just adds complexity and little value.

let's use the terms "Unicode Scalar Value" and "UTF-16 words" for those two terms, to maximally avoid ambiguity.

Regarding the code itself, IMHO we should either support arbitrary length strings, or we should only allow one Unicode scalar value, and if the latter, we must document this and assert it throughout (ideally by changing the API to not accept an arbitrary string at all, e.g. take a Unicode scalar value). We should definitely not have an API that looks like it takes an arbitrary string but doesn't.

FWIW, I think the code you have here is equivalent to just codeUnit = codePoint.runes.single.

By suggesting the addition of handling the extra scalar value in the macOS code, I was trying to handle the possible situation when a single keystroke returns multiple scalar values. The docs for macOS's charactersIgnoringModifiers aren't very clear about how many values could be stuffed into the field, but our id scheme doesn't allow for more than two scalars, since it's only 32 bits. Since we could accept up to two, I thought we should support up to the limit we could accept. We should assert throughout that it doesn't contain more than two, however (the reading code does assert already if it's more than two). Unfortunately, Dart doesn't have fixed-length arrays, or I'd also use one of those to indicate that the member can't have more than two characters, but we should document it. Using a packed int could work, however.

I'd rather not limit us to one value, since there may be some common cases (I'm thinking of combining characters here) where there are more than one, depending on how they handle those.

I'm not sure how clear the Linux docs are about this same situation, but if it's supplying a string, then it's probably similar enough to macOS to use a similar strategy.

And yes, it's equivalent to the first rune of the string (although I try and avoid working with runes, since it's such Dart specific terminology).

Hixie · 2019-06-21T20:56:25Z

packages/flutter/test/services/raw_keyboard_test.dart

          'keyCode': 0x04,
          'scanCode': 0x01,
-          'codePoint': 0x10,
+          'charactersIgnoringModifiers': 'a',


how are 0x10 and 'a' related?

I assume they're not -- this is test data. My assumption stems from the fact that a key-event for a tap on the 'a' key is something likely, whereas I'm not aware of any keyboard that'll emit keydown events for 0x10 (DLE: "data link escape" according to my local ASCII table).

That said, I'm sure that at some point such a keyboard has existed and I'd be very curious to know what the value would be according to Apple's APIs. 0x1b (ESC), on the other hand, seems like something we could easily test their APIs against to answer what their APIs currently return.

They are not. This test only really cares about modifiers. The rest of the key data is there just to show all the fields in object. IIRC, 0x10 is just a random value.

In that case, this entire change appears to have had no practical impact on the tests at all, which is quite concerning! We should make sure to test this code.

This particular test is not affected by the codePoint since it's only testing for the modifiers. The tests for key translations are below.

I think the keyCode and codePoint from the previous patch were just random numbers but I changed them now to mimic an actual 'A' key press in all fields.

franciscojma86 · 2019-06-25T16:13:09Z

Comments addressed, PTAL!

Hixie · 2019-07-01T19:50:53Z

packages/flutter/test/services/raw_keyboard_test.dart

+        'codePoint': '',
      });
      final RawKeyEventDataLinux data = shiftLeftKeyEvent.data;
      expect(data.physicalKey, equals(PhysicalKeyboardKey.shiftLeft));


we still need to add tests for this new code. As far as I can tell the tests don't test the new code.

The test that checks for this code is on line 534 https://github.com/flutter/flutter/pull/34752/files#diff-1e6ff69b318c8b31b82b2086c0f8a12eR534. Is this what you are referring to?

No, I think he means it's not testing the "multiple code points" part of the code.

Thanks, totally missed that. Added test for handling 2 and 3 values

Since this is now an int, there's no need for these tests. Existing tests cover the creation of the logical key.

franciscojma86 · 2019-07-09T20:46:58Z

Comments addressed, PTAL! @Hixie. @gspencergoog

gspencergoog · 2019-07-10T22:27:40Z

packages/flutter/lib/src/services/raw_keyboard_linux.dart

  /// (GLFW, GTK, QT, etc) may have a different key code mapping.
  final KeyHelper keyHelper;

+  /// The name of the printable key. This is typically the character that [keyCode] would


This isn't really a name. It's the set of Unicode scalar values generated by a single keystroke.

gspencergoog · 2019-07-10T22:29:22Z

packages/flutter/lib/src/services/raw_keyboard_linux.dart

+  /// The name of the printable key. This is typically the character that [keyCode] would
+  /// produce without any modifier keys. For dead keys, it is typically the diacritic
+  /// it would add to a character. Defaults to an empty string, asserted to be not null.
+  final String codePoint;


codePoint isn't a great name, since it's a string (and could contain multiple "code points").

I'd call it something like unicodeScalarValuesProduced.

You could also just leave it as an int, and pack the first two scalars into it. Then it would represent exactly the amount of data we can accept. Definitely document it.

Changed the name and documented that we handle two at most, but it's typically one value

gspencergoog · 2019-07-10T22:41:38Z

packages/flutter/lib/src/services/raw_keyboard_linux.dart

+      if (charactersIgnoringModifiers.length == 2) {
+        final int secondCode = charactersIgnoringModifiers.codeUnitAt(1);
+        codeUnit = (codeUnit << 16) | secondCode;
+      }


By suggesting the addition of handling the extra scalar value in the macOS code, I was trying to handle the possible situation when a single keystroke returns multiple scalar values. The docs for macOS's charactersIgnoringModifiers aren't very clear about how many values could be stuffed into the field, but our id scheme doesn't allow for more than two scalars, since it's only 32 bits. Since we could accept up to two, I thought we should support up to the limit we could accept. We should assert throughout that it doesn't contain more than two, however (the reading code does assert already if it's more than two). Unfortunately, Dart doesn't have fixed-length arrays, or I'd also use one of those to indicate that the member can't have more than two characters, but we should document it. Using a packed int could work, however.

I'd rather not limit us to one value, since there may be some common cases (I'm thinking of combining characters here) where there are more than one, depending on how they handle those.

I'm not sure how clear the Linux docs are about this same situation, but if it's supplying a string, then it's probably similar enough to macOS to use a similar strategy.

And yes, it's equivalent to the first rune of the string (although I try and avoid working with runes, since it's such Dart specific terminology).

gspencergoog · 2019-07-10T22:44:50Z

packages/flutter/test/services/raw_keyboard_test.dart

+        'codePoint': '',
      });
      final RawKeyEventDataLinux data = shiftLeftKeyEvent.data;
      expect(data.physicalKey, equals(PhysicalKeyboardKey.shiftLeft));


No, I think he means it's not testing the "multiple code points" part of the code.

franciscojma86 · 2019-07-17T22:48:26Z

@gspencergoog ping

gspencergoog · 2019-07-17T23:03:51Z

packages/flutter/lib/src/services/raw_keyboard_linux.dart

  /// (GLFW, GTK, QT, etc) may have a different key code mapping.
  final KeyHelper keyHelper;

+  /// A string with one or more unicode scalar values in a single keystroke. Two values are handled


Suggested change

/// A string with one or more unicode scalar values in a single keystroke. Two values are handled

/// A string with one or more Unicode scalar values in a single keystroke. Two values are handled

gspencergoog · 2019-07-17T23:06:04Z

packages/flutter/lib/src/services/raw_keyboard_linux.dart

+  /// at most. Otherwise, an assertion will be raised. This is typically the character
+  /// that [keyCode] would produce without any modifier keys. For dead keys, it is typically
+  /// the diacritic it would add to a character. Defaults to an empty string, asserted to be not null.
+  final String unicodeScalarValuesProduced;


This could be a 32-bit int that packs the first two values into it. It's all we can handle anyhow, and it would make it seem less like you can pack any number of values into it (which is one of Ian's concerns). I realize it would mean changing the engine, but then it definitely wouldn't give the impression that it can be an arbitrary-length string.

Changed to an int. Made the relevant changes on the engine repo as well flutter/engine#9386

goderbauer · 2019-08-07T21:40:06Z

@franciscojma86 Any plans to follow-up on the review?

franciscojma86 · 2019-08-07T22:35:01Z

Just finished my rotation, working on this right now. Should be ready soon.

packages/flutter/lib/src/services/raw_keyboard_linux.dart

gspencergoog · 2019-08-12T23:22:27Z

packages/flutter/lib/src/services/raw_keyboard_linux.dart

+  /// at most. Otherwise, an assertion will be raised. This is typically the character
+  /// that [keyCode] would produce without any modifier keys. For dead keys, it is typically
+  /// the diacritic it would add to a character. Defaults to 0, asserted to be not null.
+  final int unicodeScalarValuesProduced;


I know, I'm the one that suggested it, but would it be better to just be unicodeScalarValues? Reading it now, I feel like the "Produced" is redundant: we know it's being produced.

gspencergoog · 2019-08-12T23:24:40Z

packages/flutter/test/services/raw_keyboard_test.dart

        'keyCode': 65,
        'scanCode': 0x00000026,
-        'codePoint': 97,
+        'unicodeScalarValuesProduced': 113,


Can you also add back in some tests that test two and three scalar values encoded in the unicodeScalarValuesProduced member? (the "three" scenario should assert).

Co-Authored-By: Greg Spencer <[email protected]>

gspencergoog

googlebot added the cla: yes label Jun 19, 2019

franciscojma86 requested review from goderbauer and gspencergoog June 19, 2019 22:52

franciscojma86 mentioned this pull request Jun 19, 2019

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

Merged

franciscojma86 changed the title ~~Linux~~ [linux] Sends the unmodified characters obtained from GLFW Jun 19, 2019

goderbauer added a: desktop Running on desktop framework flutter/packages/flutter repository. See also f: labels. labels Jun 19, 2019

franciscojma86 changed the title ~~[linux] Sends the unmodified characters obtained from GLFW~~ [linux] Receives the unmodified characters obtained from GLFW Jun 20, 2019

franciscojma86 added 3 commits June 20, 2019 10:12

[linux] Sends the unmodified characters obtained from GLFW

acd95cf

Update tests

8e33760

Remove whitespace

685937c

franciscojma86 force-pushed the linux branch from fc848bd to 685937c Compare June 20, 2019 17:12

Piinks reviewed Jun 20, 2019

View reviewed changes

Address documentation comments

3643089

franciscojma86 requested a review from Piinks June 20, 2019 18:36

goderbauer reviewed Jun 20, 2019

View reviewed changes

dkwingsmt reviewed Jun 20, 2019

View reviewed changes

packages/flutter/test/services/raw_keyboard_test.dart Outdated Show resolved Hide resolved

dkwingsmt reviewed Jun 20, 2019

View reviewed changes

packages/flutter/lib/src/services/raw_keyboard_linux.dart Outdated Show resolved Hide resolved

cbracken approved these changes Jun 20, 2019

View reviewed changes

Address formatting comments

b90648c

cbracken reviewed Jun 21, 2019

View reviewed changes

Hixie reviewed Jun 21, 2019

View reviewed changes

franciscojma86 added 2 commits June 24, 2019 13:35

Rename to codePoint

8a764dc

Fix formatting

9f7b138

Hixie reviewed Jul 1, 2019

View reviewed changes

Fix documentation

8d47d0e

franciscojma86 requested a review from Hixie July 8, 2019 18:22

gspencergoog reviewed Jul 10, 2019

View reviewed changes

franciscojma86 added 2 commits July 12, 2019 10:03

Add tests for multiple values

4ed2984

Rename codepoint to unicodeScalarValues

f4a17a6

franciscojma86 requested a review from gspencergoog July 12, 2019 17:56

franciscojma86 added 3 commits July 16, 2019 11:13

Fix test

d337a6a

Fix test again

f1c14b8

Whitespace

5e70d22

gspencergoog reviewed Jul 17, 2019

View reviewed changes

Piinks removed their request for review July 30, 2019 19:58

franciscojma86 added 2 commits August 7, 2019 17:38

Change to int

db7988f

Fix tests

d932f54

franciscojma86 requested a review from gspencergoog August 8, 2019 01:17

franciscojma86 mentioned this pull request Aug 8, 2019

Support keydata mapping for GLFW shell #37830

Closed

gspencergoog reviewed Aug 12, 2019

View reviewed changes

franciscojma86 and others added 4 commits August 13, 2019 10:44

Update packages/flutter/lib/src/services/raw_keyboard_linux.dart

e715260

Co-Authored-By: Greg Spencer <[email protected]>

Update packages/flutter/lib/src/services/raw_keyboard_linux.dart

b47f00b

Co-Authored-By: Greg Spencer <[email protected]>

Address comments

b69e891

Fix comment

598b290

gspencergoog approved these changes Aug 13, 2019

View reviewed changes

whitespace

fb6bd9f

franciscojma86 merged commit 760635e into flutter:master Aug 14, 2019

franciscojma86 deleted the linux branch August 14, 2019 01:14

github-actions bot locked as resolved and limited conversation to collaborators Aug 4, 2021

	/// A string with one or more unicode scalar values in a single keystroke. Two values are handled
	/// A string with one or more Unicode scalar values in a single keystroke. Two values are handled

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

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

Uh oh!

Conversation

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

Uh oh!

Description

Tests

Checklist

Breaking Change

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

franciscojma86 commented Jun 20, 2019

Uh oh!

goderbauer 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!

franciscojma86 commented Jun 20, 2019

Uh oh!

Uh oh!

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

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!

Uh oh!

cbracken 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!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cbracken left a comment

Choose a reason for hiding this comment

Uh oh!

Hixie commented Jun 21, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

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!

franciscojma86 commented Jun 19, 2019 •

edited

Loading

dkwingsmt Jun 20, 2019 •

edited

Loading

cbracken Jun 21, 2019 •

edited

Loading

franciscojma86 Jun 22, 2019 •

edited

Loading