Skip to content

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

Merged
fluttergithubbot merged 3 commits intoflutter:masterfrom
twinsunllc:reland-scribble
Jan 27, 2022
Merged

Re-land "Support Scribble Handwriting" (#96615)#96881
fluttergithubbot merged 3 commits intoflutter:masterfrom
twinsunllc:reland-scribble

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