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

[macOS] Fix IME input issue caused on FlutterTextInputPlugin dealloc. #31879

Merged
fluttergithubbot merged 1 commit into from
Apr 21, 2022
Merged

[macOS] Fix IME input issue caused on FlutterTextInputPlugin dealloc. #31879

fluttergithubbot merged 1 commit into from
Apr 21, 2022

Conversation

@GetToSet
Copy link
Contributor

@GetToSet GetToSet commented Mar 7, 2022

This PR fixes an issue causing native textView not accepting text input caused by unnecessary NSTextInputContext calls on FlutterTextInputPlugin dealloc.

Fixes: flutter/flutter#99695

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.

@chinmaygarde
Copy link
Contributor

chinmaygarde commented Mar 17, 2022

I believe this is an accurate fix. Specifically, the documentation for deactivate states that You should not call this method directly; it is invoked by the system. It is provided as an override point for subclasses.. I have no guidance to provide on how to test this however. cc @cbracken

@cbracken cbracken self-requested a review March 17, 2022 20:46
@cbracken
Copy link
Member

cbracken commented Mar 17, 2022

I spotted this when I was while working on #31849 recently (where I avoided calling that method) and was going to send a PR that removes existing activate and deactivate calls.

I do think we should keep the call to clear the mark region though. If we're ending editing, we almost certainly don't want the mark text floating around.

cbracken
cbracken suggested changes Mar 17, 2022
View reviewed changes
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.

Thanks for sending this PR! I'll give some thought as to how we could test this.

@GetToSet GetToSet requested a review from cbracken March 18, 2022 17:54
@zanderso
Copy link
Member

zanderso commented Mar 31, 2022

From PR review triage: Ping @cbracken

@chinmaygarde
Copy link
Contributor

chinmaygarde commented Apr 14, 2022

cc @cbracken. If you don't have the cycles to comment on a test strategy, can you provide guidance instead of if this is a sound fix? If this improves things, we can land this and file a followup issue to figure out testing such changes.

cbracken
cbracken approved these changes Apr 18, 2022
View reviewed changes
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 stamp from a Japanese personal seal

@cbracken cbracken mentioned this pull request Apr 18, 2022
Open
@cbracken
Copy link
Member

cbracken commented Apr 18, 2022 •
edited
Loading

@chinmaygarde I don't have a great idea of how we could test this with our current test setup, but adding IME testing is something I'm looking at for Win/Mac/Linux. I've filed an issue that points back to this patch. flutter/flutter#102101

@cbracken cbracken added the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Apr 18, 2022
@fluttergithubbot
Copy link
Contributor

fluttergithubbot commented Apr 18, 2022

This pull request is not suitable for automatic merging in its current state.

  • Please get at least one approved review if you are already a member or two member reviews if you are not a member before re-applying this label. Reviewers: If you left a comment approving, please use the "approve" review action instead.
  • This commit has no checks. Please check that ci.yaml validation has started and there are multiple checks. If not, try uploading an empty commit.

@fluttergithubbot fluttergithubbot removed the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Apr 18, 2022
zhongwuzw
zhongwuzw approved these changes Apr 19, 2022
View reviewed changes
Copy link
Member

@zhongwuzw zhongwuzw left a comment

Choose a reason for hiding this comment

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

LGTM!

@zhongwuzw zhongwuzw added the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Apr 19, 2022
@fluttergithubbot
Copy link
Contributor

fluttergithubbot commented Apr 19, 2022

This pull request is not suitable for automatic merging in its current state.

  • This commit has no checks. Please check that ci.yaml validation has started and there are multiple checks. If not, try uploading an empty commit.

@fluttergithubbot fluttergithubbot removed the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Apr 19, 2022
@cbracken cbracken added the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Apr 19, 2022
@fluttergithubbot
Copy link
Contributor

fluttergithubbot commented Apr 19, 2022

This pull request is not suitable for automatic merging in its current state.

  • The status or check suite Linux Unopt has failed. Please fix the issues identified (or deflake) before re-applying this label.

@fluttergithubbot fluttergithubbot removed the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Apr 19, 2022
@GetToSet
Copy link
Contributor Author

GetToSet commented Apr 19, 2022

@zhongwuzw I've rebased this commit to latest main branch and made the CI checks pass.

@cbracken cbracken added the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Apr 21, 2022
@fluttergithubbot fluttergithubbot merged commit 4f05631 into flutter:main Apr 21, 2022
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Apr 21, 2022
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Apr 21, 2022
@engine-flutter-autoroll
4f05631 [macOS] Fix IME input issue caused on FlutterTextInputPlugin …
c8c5e7d 
…dealloc. (flutter/engine#31879)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

2 more reviewers

@cbracken cbracken cbracken approved these changes

@zhongwuzw zhongwuzw zhongwuzw approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[macOS] Unable to type text into native textView with IME after destroying Flutter window.

6 participants

@GetToSet @chinmaygarde @cbracken @zanderso @fluttergithubbot @zhongwuzw