Repository navigation
Remove keyLabel and debugName from Logical/PhysicalKeyboardKey - #78263
Conversation
flutter-dashboard
Bot
added
a: tests
gspencergoog
left a comment
gspencergoog
left a comment
There was a problem hiding this comment.
Hey, this change is awesome! Much cleaner! Hopefully it won't be breaking for many folks, but I doubt it will be. About the only thing some developers might be doing is creating their own LogicalKeyboardKeys, so removing the keyLabel constructor arg might cause some breakage, but as you point out, it's not really used much.
| /// } else { | ||
| /// if (kReleaseMode) { | ||
| /// _message = 'Not a Q: Key label is "${event.logicalKey.keyLabel}"'; | ||
| /// _message = 'Not a Q: Pressed 0x${event.logicalKey.keyId.toRadixString(16)}'; |
There was a problem hiding this comment.
Maybe print the character or debugLabel here in addition? Otherwise it doesn't show which key was pressed, just a hex value.
There was a problem hiding this comment.
I would if this is a framework code. Since it's only an example, I'm aiming for simplicity.
There was a problem hiding this comment.
Yeah, but it's exactly in an example that it should be clear that you can print those things. People aren't going to print the hex string, they're going to print the debug string so that know what key was pressed, so that's the best example of the usage.
There was a problem hiding this comment.
Well, the problem is that the keyId is technically not connected with the character it represents. It's entirely coincidental that the key ID mirrors the ASCII code the key might represent (without a modifier) in the ASCII printable range. How should we convert 0x01000c018a into a string that says "launch mail"? In release mode all explanatory information of a key object will and should be stripped. The only thing the key object represents then is the ID as a bare index.
There was a problem hiding this comment.
Well, if it's part of the Unicode ID plane, it could at least include that Unicode character as a label.
|
This pull request is not suitable for automatic merging in its current state.
|
| return other is LogicalKeyboardKey | ||
| && other.keyId == keyId; | ||
| } | ||
| String? get debugName => _debugNames[keyId]; |
There was a problem hiding this comment.
Couldn't this also include a representation of the key as a Unicode character, if it is part of the Unicode ID plane?
There was a problem hiding this comment.
Are you saying to include it as the release information or the debug information?
- If as release information, isn't it redundant symbol since it's unnecessary for runtime logic?
- If as debug information, doesn't the current
debugNamealready convey the key name?
There was a problem hiding this comment.
As debug information only. It would include a lot more keys that aren't in your _debugNames map: any Unicode-producing key would have a debugName that includes the character it represents. You could just produce a string if it doesn't exist in the map. If you do that here, then it'll also get printed as part of the debugFillProperties output.
There was a problem hiding this comment.
I've tried to add a debugCharacterLabel as you describe. See dkwingsmt@c62cf08
It doesn't really work well in my opinion.
- As you said, this is debug information only. Therefore it doesn't solve the problem mentioned in Remove keyLabel and debugName from Logical/PhysicalKeyboardKey #78263 (comment), which asks to add more information to release mode code.
- The only "unicode" keys
LogicalKeyboardKeyrepresents are ASCII printable keys (just search for0x000inkeyboard_key.dart). There isn't aœlogical key: despite that this chacacter can be input by a macOS keyboard with Option-Q, it's still an Option key followed by a KeyQ key. ThedebugCharacterLabeldoes not add any information besides what's already available indebugName.
There was a problem hiding this comment.
I was thinking of something like this:
String? get debugName {
String? result;
assert(() {
result ??= _debugNames[keyId];
if (result == null && keyId & ~valueMask == unicodePlane) {
result = 'Key ${String.fromCharCode(keyId)}';
}
return true;
}());
return result;
}For the printable key with the key cap of "Ù" (letter U with grave) on the French keyboard, the function above will return 'Key Ù', but your current implementation would return null. It's more than just ASCII: any key that generates a Unicode character should get a Unicode plane key id. In this case the logical id for "Ù" is 0x000000D9. There is a dedicated key for this on the French keyboard, so that's exactly what it should report. Debugging will get a lot harder if all they get from debugName is null outside of the ASCII keys, and this adds more useful information to allow them to differentiate inputs.
There was a problem hiding this comment.
Actually, this might be even better, since it won't return null except in release mode.
String? get debugName {
String? result;
assert(() {
result ??= _debugNames[keyId];
if (result == null) {
if (keyId & ~valueMask == unicodePlane) {
result = 'Key ${String.fromCharCode(keyId)}';
} else {
result = 'Key with ID 0x${keyId.toRadixString(16).padLeft(digits, '0')}';
}
}
return true;
}());
return result;
}There was a problem hiding this comment.
Thanks for the advice. I've added this one, as well as similar code to PhysicalKeyboardKey, and tests.
There was a problem hiding this comment.
It's weird: I see your push with the debugName changes in it (0deb173), but I don't see them here in the "Files changed" tab on GitHub. Maybe you somehow reverted them?
| /// } else { | ||
| /// if (kReleaseMode) { | ||
| /// _message = 'Not a Q: Key label is "${event.logicalKey.keyLabel}"'; | ||
| /// _message = 'Not a Q: Pressed 0x${event.logicalKey.keyId.toRadixString(16)}'; |
There was a problem hiding this comment.
Well, if it's part of the Unicode ID plane, it could at least include that Unicode character as a label.
|
This pull request is not suitable for automatic merging in its current state.
|
…r into remove-logical-keylabel
| /// _message = 'Not a Q: Key label is "${event.logicalKey.keyLabel}"'; | ||
| /// _message = 'Not a Q: Pressed 0x${event.logicalKey.keyId.toRadixString(16)}'; | ||
| /// } else { | ||
| /// // This will only print useful information in debug mode. |
There was a problem hiding this comment.
| /// // This will only print useful information in debug mode. | |
| /// // debugName will only print useful information in debug mode. |
| return other is LogicalKeyboardKey | ||
| && other.keyId == keyId; | ||
| } | ||
| String? get debugName => _debugNames[keyId]; |
There was a problem hiding this comment.
It's weird: I see your push with the debugName changes in it (0deb173), but I don't see them here in the "Files changed" tab on GitHub. Maybe you somehow reverted them?
…t/flutter into remove-logical-keylabel
|
@gspencergoog should work now. Can you check again? :) |
…r into remove-logical-keylabel
dkwingsmt
added
the
a: text input
3 participants
This PR makes the following changes:
LogicalKeyboardKey.keyLabelPhysical/LogicalKeyboardKey.debugNamegettersPhysical/LogicalKeyboardKey()factory constructors, which cache non-predefined instances.Reasons:
keyLabelbecause it brings confusion to usage, and complexity to expanding code related with logical keys.keyLabelis a duplicate information toflutterId. It is possible to construct an instance with the sameflutterIdand differentkeyLabel, but what does it even mean?keyLabelas a getter, but for now it doesn't seem necessary. ThekeyLabelis not used anywhere in keyboard-related algorithms, nor does it seem like a necessary part for client-side processing. SinceflutterIdis the deciding property for a logical key,keyLabelonly serves for explanation. It's like anotherdebugNamebut available at run time.debugNamebeing turned into a getter, theLogical/PhysicalKeyboardKeyclasses no longer needs to overridehashCodeandoperator ==. This allows the key instances to be used in a const context and removes a linter exception (prefer_const_literals_to_create_immutables), such asPre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.