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

[fuchsia] Support multi-line text editing #34410

Merged
akbiggs merged 1 commit into from
Jul 28, 2022
Merged

[fuchsia] Support multi-line text editing #34410

akbiggs merged 1 commit into from
Jul 28, 2022

Conversation

@filmil
Copy link
Contributor

@filmil filmil commented Jun 30, 2022 •
edited
Loading

Multi-line text editing and actions other than DONE were
never implemented in Flutter on Fuchsia. This change
implements the feature, by plumbing the desired action
through to Fuchsia proper, and back, as Fuchsia's text
editing API expects.

Tested:

  • the new behavior was verified by Fuchsia-side integration tests.
  • added unit tests for the new code.

Issue: flutter/flutter#106905

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.

@filmil filmil marked this pull request as ready for review June 30, 2022 22:04
@chinmaygarde
Copy link
Member

chinmaygarde commented Jul 14, 2022

cc @akbiggs

@akbiggs akbiggs requested review from akbiggs and jaeheon July 15, 2022 13:50
akbiggs
akbiggs reviewed Jul 15, 2022
View reviewed changes
Copy link
Contributor

@akbiggs akbiggs left a comment

Choose a reason for hiding this comment

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

Really sorry for the slow review and thanks for the ping, missed this in my inbox while I was out sick.

@filmil
Copy link
Contributor Author

filmil commented Jul 16, 2022

Really sorry for the slow review and thanks for the ping, missed this in my inbox while I was out sick.

No worries, I'm AFK for a while as well.

@filmil filmil force-pushed the fix-multiline-text-flutter-may19 branch 7 times, most recently from dbf04b9 to 76320f1 Compare July 27, 2022 07:27
@filmil
Copy link
Contributor Author

filmil commented Jul 27, 2022

Tests added and passing. PTAL.

@filmil filmil requested a review from akbiggs July 27, 2022 08:14
akbiggs
akbiggs approved these changes Jul 27, 2022
View reviewed changes
Copy link
Contributor

@akbiggs akbiggs left a comment

Choose a reason for hiding this comment

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

Wow, awesome. Thanks for your patience and doing this.

@filmil filmil force-pushed the fix-multiline-text-flutter-may19 branch from 76320f1 to 665ac45 Compare July 27, 2022 17:34
@filmil
Copy link
Contributor Author

filmil commented Jul 27, 2022 •
edited
Loading

@akbiggs Would you mind merging this for me when/if the presubmit checks pass? I do not have merge rights.

@akbiggs akbiggs added the autosubmit Merge PR when tree becomes green via auto submit App label Jul 27, 2022
@auto-submit
Copy link
Contributor

auto-submit bot commented Jul 27, 2022

  • 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.

@auto-submit
Copy link
Contributor

auto-submit bot commented Jul 27, 2022

Validations Fail.

@auto-submit auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Jul 27, 2022
@akbiggs
Copy link
Contributor

akbiggs commented Jul 27, 2022

We'll have to get Jaeheon's review first per the bot comment above.

@akbiggs
Copy link
Contributor

akbiggs commented Jul 27, 2022

(alternatively we can add you to the flutter team. feel free to ping me if you want to do that, basically you join the Flutter Discord https://discord.com/invite/N7Yshp4 and then I send a message endorsing you.)

@filmil
Copy link
Contributor Author

filmil commented Jul 27, 2022

Ugh, the discord invite doesn't allow me use the invite and then to log in with my existing credentials because "that email is already in use". What to do?

@akbiggs
Copy link
Contributor

akbiggs commented Jul 27, 2022

Ugh sorry. I'll sort it after lunch, ping me your Discord username

jaeheon
jaeheon reviewed Jul 28, 2022
View reviewed changes
jaeheon
jaeheon approved these changes Jul 28, 2022
View reviewed changes
Copy link
Contributor

@jaeheon jaeheon left a comment

Choose a reason for hiding this comment

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

Some small fixes requested, otherwise looks great!

@filmil
[fuchsia] Support multi-line text editing
8574f8c 
Multi-line text editing and actions other than DONE were
never implemented in Flutter on Fuchsia.  This change
implements the feature, by plumbing the desired action
through to Fuchsia proper, and back, as Fuchsia's text
editing API expects.

Tested: the new behavior was verified by Fuchsia-side
integration tests.

Issue: flutter/flutter#106905
@filmil filmil force-pushed the fix-multiline-text-flutter-may19 branch from 665ac45 to 8574f8c Compare July 28, 2022 17:19
@chinmaygarde
Copy link
Member

chinmaygarde commented Jul 28, 2022

Is this good to go?

@filmil
Copy link
Contributor Author

filmil commented Jul 28, 2022

I think it is good to go. 2 committers have approved.
Can you merge it for me please? I do not have merge rights.

@akbiggs akbiggs merged commit 422f8c0 into flutter:main Jul 28, 2022
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Jul 28, 2022
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 28, 2022
@engine-flutter-autoroll
422f8c0 [fuchsia] Support multi-line text editing (flutter/engine#34410)
cca31b3
betrevisan pushed a commit to betrevisan/engine that referenced this pull request Jul 29, 2022
@filmil @betrevisan
[fuchsia] Support multi-line text editing (flutter#34410)
b4ed923 
Multi-line text editing and actions other than DONE were
never implemented in Flutter on Fuchsia.  This change
implements the feature, by plumbing the desired action
through to Fuchsia proper, and back, as Fuchsia's text
editing API expects.

Tested: the new behavior was verified by Fuchsia-side
integration tests.

Issue: flutter/flutter#106905
github-actions bot pushed a commit to gnoliyil/fuchsia that referenced this pull request Aug 22, 2022
@filmil
[input] Starts testing Flutter multiline
0397d7e 
If this modified test passes, this means that the Flutter
change for supporting multiline text entry has rolled
into Fuchsia.

Flutter was recently changed to support multiline text
editing properly.

See flutter/engine#34410 for
details.

Fixed: 79807
Change-Id: Ice51df53e72e144b46852f645fce413172ceba82
Reviewed-on: https://fuchsia-review.googlesource.com/c/fuchsia/+/710482
Fuchsia-Auto-Submit: Filip Filmar <[email protected]>
Reviewed-by: Konstantin Pozin <[email protected]>
Commit-Queue: Filip Filmar <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

2 more reviewers

@jaeheon jaeheon jaeheon approved these changes

@akbiggs akbiggs akbiggs approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

platform-fuchsia

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@filmil @chinmaygarde @akbiggs @jaeheon