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 Sep 26, 2021

With this PR, the state of the keyboard system will be reset when the engine is reset (typically during a hot restart.)

Fixes flutter/flutter#89729 and the linux part of flutter/flutter#91064.

Exemption from testing

I don't think we can test FlView within reasonable effort.

  • 80% of FlView's code is related to GtkWidget, which can't be run headlessly without bootstrapping the entire GTK environment (which might still be possible, but honestly too much to research given my current task queue.)
  • I tried to isolate the rest of FlView's code into a headless class, but it turns out to be huge amount of boilerplate with like 10 lines of effective code (creation, connection, restarting.)

Request to test has been filed at flutter/flutter#91345.

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. See testing the engine for instructions on
    writing and running engine tests.
  • I updated/added relevant documentation (doc comments with ///).
  • I signed the CLA.
  • All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

@flutter-dashboard flutter-dashboard bot added embedder Related to the embedder API platform-linux labels Sep 26, 2021
@google-cla google-cla bot added the cla: yes label Sep 26, 2021
@dkwingsmt dkwingsmt force-pushed the linux-keyboard-restart branch from 1778099 to 9263c95 Compare September 28, 2021 23:18
@dkwingsmt dkwingsmt changed the title [Linux] Add engine on restarted handler; reset keyboard on restart [Linux] Reset keyboard states on engine restart Sep 28, 2021
@Hixie
Copy link
Contributor

Hixie commented Oct 5, 2021

Do we have any end-to-end tests for GTK desktop builds that we could sneak some keyboard testing into?

cc @cbracken @robert-ancell @stuartmorgan

In the absence of any reasonable way to test this:
test-exempt: untestable

Please file a bug noting describing the scenarios we should test, though, if you don't check in tests here.

@cbracken
Copy link
Member

cbracken commented Oct 5, 2021

Most of the text editing testing we have is either unit tests for text editing or keyboard input channel or written against text editing APIs and implemented with a dart test harness.

We don't currently have test harness abstractions for integration tests that would allow an engine restart and a way to send key events, but I can imagine writing a new class of devicelab test harness that would support such a thing.

I'll file a bug.

@dkwingsmt dkwingsmt requested a review from gspencergoog October 5, 2021 23:57
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

@dkwingsmt
Copy link
Contributor Author

Do we have any end-to-end tests for GTK desktop builds that we could sneak some keyboard testing into?

cc @cbracken @robert-ancell @stuartmorgan

In the absence of any reasonable way to test this: test-exempt: untestable

Please file a bug noting describing the scenarios we should test, though, if you don't check in tests here.

Filed at flutter/flutter#91345

@dkwingsmt dkwingsmt merged commit 1a4d906 into flutter:master Oct 6, 2021
@dkwingsmt dkwingsmt deleted the linux-keyboard-restart branch October 6, 2021 19:26
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Oct 6, 2021
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Oct 6, 2021
gaaclarke pushed a commit to flutter/flutter that referenced this pull request Oct 6, 2021
* f30362b Roll Skia from 47da0ac577aa to 6d0234673a40 (4 revisions) (flutter/engine#29047)

* 1a4d906 [Linux] Reset keyboard states on engine restart (flutter/engine#28877)

* 6a00ce5 Roll Skia from 6d0234673a40 to a1b44b72eb47 (3 revisions) (flutter/engine#29049)
clocksmith pushed a commit to clocksmith/flutter that referenced this pull request Oct 29, 2021
* f30362b Roll Skia from 47da0ac577aa to 6d0234673a40 (4 revisions) (flutter/engine#29047)

* 1a4d906 [Linux] Reset keyboard states on engine restart (flutter/engine#28877)

* 6a00ce5 Roll Skia from 6d0234673a40 to a1b44b72eb47 (3 revisions) (flutter/engine#29049)
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.

[Linux] Unable to take Keyboard Input

4 participants