Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Linux] take first steps towards testable text input #33661

Merged
merged 9 commits into from
Jun 10, 2022

Conversation

jpnurmi
Copy link
Member

@jpnurmi jpnurmi commented May 27, 2022

This PR is a continuation of #33111 and includes necessary changes to test the TextInputType.setClient() platform channel call. Testing the actual text input flow requires a mock IM context that will follow in a separate PR.

Pre-launch Checklist

  • I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
  • I read the [Tree Hygiene] wiki page, which explains my responsibilities.
  • I read and followed the [Flutter Style Guide] and the [C++, Objective-C, Java style guides].
  • I listed at least one issue that this PR fixes in the description above.
  • I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt.
  • I updated/added relevant documentation (doc comments with ///).
  • I signed the [CLA].
  • All existing and new tests are passing.

This allows passing an offscreen window for testing purposes.
Keep track of registered message handlers and allow tests to simulate
receiving messages.
Copy link
Contributor

@robert-ancell robert-ancell left a comment

Choose a reason for hiding this comment

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

Code LGTM.

Copy link
Member

@cbracken cbracken left a comment

Choose a reason for hiding this comment

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

lgtm with a minor suggestion.

shell/platform/linux/testing/mock_binary_messenger.h Outdated Show resolved Hide resolved
@cbracken
Copy link
Member

Looks like there's a legitimate test failure to fix in FlTextInputPluginTest.SetClient (maybe due to running headless?).

@chinmaygarde
Copy link
Member

@jpnurmi Any updates on the test failures?

GdkOffscreenWindow requires display & screen which is a problem in
headless mode. Luckily, we don't really need a window instance in tests
because its only passed as a client window to the IM context which will
be mocked out when we get to tests that depend on it.
@jpnurmi
Copy link
Member Author

jpnurmi commented Jun 10, 2022

I was silly and ignored the harmless warnings from GdkWindow but the warnings would become a problem when made fatal. Luckily, we don't really need a window instance in the text input tests because it's only passed as a client window to the IM context which will be mocked out when we get to tests that depend on it.

@cbracken cbracken merged commit bae667e into flutter:main Jun 10, 2022
@jpnurmi jpnurmi deleted the text-input-window branch June 10, 2022 18:19
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 10, 2022
@jpnurmi jpnurmi mentioned this pull request Jun 21, 2022
8 tasks
houhuayong pushed a commit to houhuayong/engine that referenced this pull request Jun 21, 2022
This PR is a continuation of flutter#33111 and includes necessary changes to test the `TextInputType.setClient()` platform channel call. Testing the actual text input flow requires a mock IM context that will follow in a separate PR.

* [Linux] pass GdkWindow instead of FlView to FlTextInputPlugin
* [Linux] add MockBinaryMessenger::ReceiveMessage()
* [Linux] add test for TextInputType.setClient
* Drop the offscreen window

GdkOffscreenWindow requires display & screen which is a problem in
headless mode. Luckily, we don't really need a window instance in tests
because its only passed as a client window to the IM context which will
be mocked out when we get to tests that depend on it.
@jpnurmi
Copy link
Member Author

jpnurmi commented Jun 21, 2022

I've pushed the next step: #34186

cbracken pushed a commit that referenced this pull request Jun 21, 2022
This PR finishes what #33661 and #33111 started - most of `FlTextInputPlugin` is now covered by tests.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants