Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Conversation

@dkwingsmt
Copy link
Contributor

@dkwingsmt dkwingsmt commented Jan 6, 2021

Description

This PR contains the Linux (GTK) changes for the Hardware Keyboard project.

Classes has been adjusted as follows:

Before After Explain
FlKeyEventPlugin FlKeyboardManager Handles redispatching and detects redispatched events.
FlKeyChannelResponder Sends raw events through method channel and collect responses.
- FlKeyEmbedderResponder Converts raw events to FlutterKeyEvent, sends them through embedder API and collect responses.

Related Issues

Tests

I added the following tests:

  • A number of tests for FlKeyboardManager
  • A number of tests for FlKeyEmbedderResponder

Migrated tests of FlKeyEventPlugin to FlKeyChannelResponder.

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 C++, Objective-C, Java style guides for the engine.
  • I read the tree hygiene wiki page, which explains my responsibilities.
  • I updated/added relevant documentation.
  • All existing and new tests are passing.
  • I am willing to follow-up on review comments in a timely manner.

Reviewer Checklist

Breaking Change

Did any tests fail when you ran them? Please read handling breaking changes.

@dkwingsmt
Copy link
Contributor Author

dkwingsmt commented May 19, 2021

@gspencergoog Can you take another look?
For your convenience, this is the since after your last review. The changes by this PR are everything under the linux folder (including fl_settings_plugin.cc).
https://github.com/flutter/engine/compare/3ecc134bc68f96b01e2dd3fd921e4c4406db454f..f36473a38a9e3694b62920accb5048e7fe7f65a6

@gspencergoog
Copy link
Contributor

@dkwingsmt I think that link compares the wrong revisions, unless you've been VERY busy :-) (it currently shows ~5000 lines of difference in 115 files for me). No worries, though, I'll just go through the code.

@dkwingsmt
Copy link
Contributor Author

dkwingsmt commented May 19, 2021

@gspencergoog That's because I've updated with master since your last review, and that's what I mean by "the changes by this PR are everything under the linux folder". (So everything out of it are from the master)

Edit: Ok I just learned that Github comes with the "PR compare" feature so here we go:
https://github.com/flutter/engine/pull/23467/files/3ecc134bc68f96b01e2dd3fd921e4c4406db454f..f36473a38a9e3694b62920accb5048e7fe7f65a6

Copy link
Contributor

@gspencergoog gspencergoog left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

32384589-a60f0e74-c078-11e7-9bc1-e5b5287aea9d

To be honest, I have looked this over a number of times, but there's so much code here, it's hard to be sure that things are all "correct". In general, it looks good though.


static void fl_mock_text_input_plugin_init(FlMockTextInputPlugin* self) {}

// static gboolean text_input_im_filter_by_mock(GtkIMContext* im_context,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove commented out code.

@dkwingsmt
Copy link
Contributor Author

@gspencergoog Thanks for the review!

@dkwingsmt dkwingsmt merged commit 94a40bc into flutter:master May 26, 2021
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request May 26, 2021
naudzghebre pushed a commit to naudzghebre/engine that referenced this pull request Sep 2, 2021
Linux (GTK) changes for the Hardware Keyboard project.
@dkwingsmt dkwingsmt deleted the keyboard-linux branch January 6, 2022 01:59
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants