Skip to content

Re-land "Support Scribble Handwriting" (#96615) #96881

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.

Sign up for GitHub

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

Jump to bottom
Merged
fluttergithubbot merged 3 commits into from
Jan 27, 2022
Merged

Re-land "Support Scribble Handwriting" (#96615) #96881

fluttergithubbot merged 3 commits into from
Jan 27, 2022

Conversation

@fbcouch
Copy link
Contributor

@fbcouch fbcouch commented Jan 19, 2022

Reverts #96615

@LongCatIsLooong

Fixes #61278

Original PR #75472

Migration Guide: https://github.com/flutter/website/blob/main/src/release/breaking-changes/scribble-text-input-client.md

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, including Features we expect every widget to implement.
  • I signed the CLA.
  • I listed at least one issue that this PR fixes in the description above.
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making, or this PR is test-exempt.
  • All existing and new tests are passing.

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

LongCatIsLooong
LongCatIsLooong approved these changes Jan 20, 2022
View reviewed changes
Copy link
Contributor

@LongCatIsLooong LongCatIsLooong left a comment

Choose a reason for hiding this comment

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

LGTM. Sorry I triggered the internal test on the wrong pull request. Testing this one now.

@goderbauer goderbauer changed the title Revert "Revert "Support Scribble Handwriting" (#96615)" Re-land "Support Scribble Handwriting" (#96615) Jan 20, 2022
@flutter-dashboard flutter-dashboard bot added a: tests "flutter test", flutter_test, or one of our tests a: text input Entering text in a text field or keyboard related problems f: cupertino flutter/packages/flutter/cupertino repository f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels. labels Jan 20, 2022
@LongCatIsLooong
Copy link
Contributor

LongCatIsLooong commented Jan 20, 2022 •
edited
Loading

Update: I need to get one more fix in to land this.

@LongCatIsLooong
Copy link
Contributor

LongCatIsLooong commented Jan 24, 2022

the fix is in @fbcouch could you resolve the merge conflict? I'll try relanding this.

@fbcouch
Copy link
Contributor Author

fbcouch commented Jan 27, 2022

@justinmc Friendly ping to see if you can take a look at this, thanks!

justinmc
justinmc approved these changes Jan 27, 2022
View reviewed changes
Copy link
Contributor

@justinmc justinmc left a comment

Choose a reason for hiding this comment

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

Code and migration guide LGTM 👍

I'm tempted to merge this now, but @LongCatIsLooong is out for the rest of the week. We should probably wait for him to be around in case something goes wrong.

@fluttergithubbot fluttergithubbot merged commit c54ce01 into flutter:master Jan 27, 2022
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Jan 27, 2022
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jan 27, 2022
@engine-flutter-autoroll
c54ce01 Re-land "Support Scribble Handwriting" (#96615) (flutter/flut…
1f498ae 
…ter#96881)
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jan 27, 2022
@engine-flutter-autoroll
c54ce01 Re-land "Support Scribble Handwriting" (#96615) (flutter/flut…
aca0481 
…ter#96881)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Jan 27, 2022
Merged
justinmc added a commit that referenced this pull request Jan 27, 2022
@justinmc
Revert "Re-land "Support Scribble Handwriting" (#96615) (#96881)"
36672f3 
This reverts commit c54ce01.
@justinmc justinmc mentioned this pull request Jan 27, 2022
Merged
justinmc added a commit that referenced this pull request Jan 28, 2022
@justinmc
Revert "Re-land "Support Scribble Handwriting" (#96615) (#96881)" (#9…
40c4da5 
…7405)

This reverts commit c54ce01.
@justinmc
Copy link
Contributor

justinmc commented Jan 28, 2022

I had to revert this due to a failure, but it wasn't in the internal Google tests, it was a test added in this PR. It failed on all of the CI platforms (Windows, Mac, Linux). Any idea why?

Here's on of the failed runs: https://ci.chromium.org/ui/p/flutter/builders/prod/Linux%20framework_tests_widgets/3425/overview

01:31 +1913 ~3 -1: /b/s/w/ir/x/w/flutter/packages/flutter/test/widgets/editable_text_test.dart: multiline Scribble fields can show a vertical placeholder (variant: TargetPlatform.iOS)                
══╡ EXCEPTION CAUGHT BY SCHEDULER LIBRARY ╞═════════════════════════════════════════════════════════
The following StateError was thrown during a scheduler callback:
Bad state: No element

When the exception was thrown, this was the stack:
#0      List.first (dart:core-patch/growable_array.dart:360:5)
#1      EditableTextState._updateSelectionRects.<anonymous closure> (package:flutter/src/widgets/editable_text.dart:2756:168)
#2      new _GrowableList.generate (dart:core-patch/growable_array.dart:136:28)
#3      EditableTextState._updateSelectionRects (package:flutter/src/widgets/editable_text.dart:2748:41)
#4      EditableTextState._updateSizeAndTransform (package:flutter/src/widgets/editable_text.dart:2783:7)
#5      EditableTextState._updateSizeAndTransform.<anonymous closure> (package:flutter/src/widgets/editable_text.dart:2785:49)
#6      SchedulerBinding._invokeFrameCallback (package:flutter/src/scheduler/binding.dart:1144:15)
#7      SchedulerBinding.handleDrawFrame (package:flutter/src/scheduler/binding.dart:1089:9)
#8      AutomatedTestWidgetsFlutterBinding.pump.<anonymous closure> (package:flutter_test/src/binding.dart:995:9)
#11     TestAsyncUtils.guard (package:flutter_test/src/test_async_utils.dart:71:41)
#12     AutomatedTestWidgetsFlutterBinding.pump (package:flutter_test/src/binding.dart:982:27)
#13     WidgetTester.pumpAndSettle.<anonymous closure> (package:flutter_test/src/widget_tester.dart:668:23)
#14     WidgetTester.pumpAndSettle.<anonymous closure> (package:flutter_test/src/widget_tester.dart:662:38)
#17     TestAsyncUtils.guard (package:flutter_test/src/test_async_utils.dart:71:41)
#18     WidgetTester.pumpAndSettle (package:flutter_test/src/widget_tester.dart:662:27)
#19     main.<anonymous closure> (file:///b/s/w/ir/x/w/flutter/packages/flutter/test/widgets/editable_text_test.dart:2050:18)
<asynchronous suspension>
<asynchronous suspension>
(elided 5 frames from dart:async and package:stack_trace)

@fbcouch
Copy link
Contributor Author

fbcouch commented Jan 28, 2022

😩 I'll take a look...wonder if something that was merged to master between when I last merged to fix that PR and now broke it...at least that's a starting point

@fbcouch
Copy link
Contributor Author

fbcouch commented Jan 28, 2022 •
edited
Loading

So, a bisect pointed to 55e8a2c, which was an engine roll (thankfully of only 1 revision) for this flutter/engine#28912 ... my guess is something about the text renderer change is messing with the selection rects. I will try to look more closely at it tomorrow.

engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Feb 4, 2022
@engine-flutter-autoroll
c54ce01 Re-land "Support Scribble Handwriting" (#96615) (flutter/flut…
2117b93 
…ter#96881)
clocksmith pushed a commit to clocksmith/flutter that referenced this pull request Mar 8, 2022
@fbcouch @clocksmith
Re-land "Support Scribble Handwriting" (flutter#96615) (flutter#96881)
a468bf8
clocksmith pushed a commit to clocksmith/flutter that referenced this pull request Mar 8, 2022
@justinmc @clocksmith
Revert "Re-land "Support Scribble Handwriting" (flutter#96615) (flutt…
e29fe30 
…er#96881)" (flutter#97405)

This reverts commit c54ce01.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@justinmc justinmc justinmc approved these changes

@LongCatIsLooong LongCatIsLooong LongCatIsLooong approved these changes

Assignees

No one assigned

Labels

a: tests "flutter test", flutter_test, or one of our tests a: text input Entering text in a text field or keyboard related problems f: cupertino flutter/packages/flutter/cupertino repository f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

TextField should support apple pencil input by default.

4 participants

@fbcouch @LongCatIsLooong @justinmc @fluttergithubbot