Allow spell check to fail silently on unsupported platforms & spell checkers by camsim99 · Pull Request #122715 · flutter/flutter

camsim99 · 2023-03-15T16:07:44Z

Removes assertion error for cases where platform/native spell checker is not supported and instead, allows spell check to fail silently for these cases.

Fixes #120611.

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.

justinmc

I think I found a logic problem but I'm not sure. Otherwise just nits.

justinmc · 2023-03-15T20:18:38Z

packages/flutter/lib/src/widgets/editable_text.dart

-    if (configuration == null || configuration == const SpellCheckConfiguration.disabled()) {
+    final SpellCheckService? spellCheckService = configuration?.spellCheckService;
+    final bool spellCheckServiceIsConfigured = spellCheckService != null || spellCheckService == null && WidgetsBinding.instance.platformDispatcher.nativeSpellCheckServiceDefined;
+    if (configuration == null || configuration == const SpellCheckConfiguration.disabled() || !spellCheckServiceIsConfigured) {


I think the logic might be off here, but tell me if I'm wrong... If configuration is null, but WidgetsBinding.instance.platformDispatcher.nativeSpellCheckServiceDefined is true, then this will return SpellCheckConfiguration.disabled(). Is that right?

That is right and I believe that's what we want because we don't want the spell check to be enabled by default. A configuration (even if it's just SpellCheckConfiguration()) must be specified for it to be enabled at this point.

justinmc · 2023-03-15T20:21:40Z

packages/flutter/test/widgets/editable_text_test.dart


    testWidgets(
-      'Error thrown when spell check enabled but no default spell check service available',
+      'Spell check disabled when spell check enabled but no default spell check service available',


"Spell check disabled when spell check enabled" is contradictory, maybe instead say: "Spell check disabled when spellCheckConfiguration enabled"

justinmc · 2023-03-15T20:25:53Z

packages/flutter/lib/src/widgets/editable_text.dart

+      // a native spell checker must be supported.
      return const SpellCheckConfiguration.disabled();
    }



Should we add a warning if configuration != null && spellCheckService == null && !WidgetsBinding.instance.platformDispatcher.nativeSpellCheckServiceDefined? Something like:

"Spell check was enabled with spellCheckConfiguration, but the current platform does not have a supported spell check service, and none was provided."

Here's something similar to what I'm thinking of:

flutter/packages/flutter/lib/src/widgets/navigator.dart

Lines 3527 to 3541 in 5b40de6

assert(() {

if (widget.pages.isEmpty) {

FlutterError.reportError(

FlutterErrorDetails(

exception: FlutterError(

'The Navigator.pages must not be empty to use the '

'Navigator.pages API',

),

library: 'widget library',

stack: StackTrace.current,

),

);

}

return true;

}());

This will not run in production mode since it's wrapped in an assert. Also, FlutterError.reportError won't halt execution.

justinmc

LGTM with nits 👍

I'm assuming the customer tests just need a merge to pass.

packages/flutter/lib/src/widgets/editable_text.dart

… spell checkers (flutter/flutter#122715)

Add fix

4918842

flutter-dashboard bot added a: text input Entering text in a text field or keyboard related problems framework flutter/packages/flutter repository. See also f: labels. labels Mar 15, 2023

Merge remote-tracking branch 'upstream/master' into sc_silentfail

09fd073

camsim99 requested a review from justinmc March 15, 2023 16:08

justinmc reviewed Mar 15, 2023

View reviewed changes

camsim99 added 8 commits March 15, 2023 14:19

fix test name and add print

f448b42

Merge remote-tracking branch 'upstream/master' into sc_silentfail

871c103

Add warning

c295f29

Merge remote-tracking branch 'upstream/master' into sc_silentfail

2fcdaae

Remove trailing space

4b28f87

Fix test

4b1ff37

Merge remote-tracking branch 'upstream/master' into sc_silentfail

ffa4afd

Fix assertion

f05473f

camsim99 requested a review from justinmc March 21, 2023 21:20

justinmc approved these changes Mar 22, 2023

View reviewed changes

packages/flutter/lib/src/widgets/editable_text.dart Outdated Show resolved Hide resolved

packages/flutter/lib/src/widgets/editable_text.dart Outdated Show resolved Hide resolved

camsim99 added 2 commits March 25, 2023 16:05

Address nits

1b9360c

Merge remote-tracking branch 'upstream/master' into sc_silentfail

64f6f85

camsim99 added the autosubmit Merge PR when tree becomes green via auto submit App label Mar 27, 2023

auto-submit bot merged commit 5e76e11 into flutter:master Mar 27, 2023

engine-flutter-autoroll mentioned this pull request Mar 27, 2023

Roll Flutter from 4a1002e2e255 to 5e76e119d596 (2 revisions) flutter/packages#3559

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 27, 2023

5e76e11 Allow spell check to fail silently on unsupported platforms &…

990e1a9

… spell checkers (flutter/flutter#122715)

eliasyishak mentioned this pull request Mar 27, 2023

Mac_ios wide_gamut_ios is failing #123550

Closed

engine-flutter-autoroll mentioned this pull request Mar 27, 2023

Roll Flutter from 4a1002e2e255 to af94e9b4f54d (3 revisions) flutter/packages#3560

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 27, 2023

5e76e11 Allow spell check to fail silently on unsupported platforms &…

3beaf5e

… spell checkers (flutter/flutter#122715)

engine-flutter-autoroll mentioned this pull request Mar 28, 2023

Roll Flutter from 4a1002e2e255 to c0ddbf129e5f (29 revisions) flutter/packages#3567

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 28, 2023

5e76e11 Allow spell check to fail silently on unsupported platforms &…

0152a7c

… spell checkers (flutter/flutter#122715)

engine-flutter-autoroll mentioned this pull request Mar 28, 2023

Roll Flutter from 4a1002e2e255 to f528f9f56ce8 (31 revisions) flutter/packages#3568

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 28, 2023

5e76e11 Allow spell check to fail silently on unsupported platforms &…

691a901

… spell checkers (flutter/flutter#122715)

engine-flutter-autoroll mentioned this pull request Mar 28, 2023

Roll Flutter from 4a1002e2e255 to 8a6cb26f1acf (32 revisions) flutter/packages#3569

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 28, 2023

5e76e11 Allow spell check to fail silently on unsupported platforms &…

6c07265

… spell checkers (flutter/flutter#122715)

engine-flutter-autoroll mentioned this pull request Mar 28, 2023

Roll Flutter from 4a1002e2e255 to 12b36de8f7ac (33 revisions) flutter/packages#3570

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 28, 2023

5e76e11 Allow spell check to fail silently on unsupported platforms &…

6977004

… spell checkers (flutter/flutter#122715)

This was referenced Mar 28, 2023

i123643 print java version gradle #123644

Merged

Add Java-Gradle-AGP validation to flutter analyze #123916

Merged

reidbaker mentioned this pull request Apr 5, 2023

Update getGradleVersion to ignore commented out lines #124260

Merged

8 tasks

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 10, 2023

5e76e11 Allow spell check to fail silently on unsupported platforms &…

739f253

… spell checkers (flutter/flutter#122715)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 10, 2023

5e76e11 Allow spell check to fail silently on unsupported platforms &…

df085d8

… spell checkers (flutter/flutter#122715)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow spell check to fail silently on unsupported platforms & spell checkers#122715

Allow spell check to fail silently on unsupported platforms & spell checkers#122715
auto-submit[bot] merged 12 commits intoflutter:masterfrom
camsim99:sc_silentfail

camsim99 commented Mar 15, 2023 •

edited

Loading

Uh oh!

justinmc left a comment

Uh oh!

justinmc Mar 15, 2023

Uh oh!

camsim99 Mar 15, 2023

Uh oh!

justinmc Mar 15, 2023

Uh oh!

justinmc Mar 15, 2023

Uh oh!

justinmc Mar 15, 2023

Uh oh!

justinmc left a comment •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

	assert(() {
	if (widget.pages.isEmpty) {
	FlutterError.reportError(
	FlutterErrorDetails(
	exception: FlutterError(
	'The Navigator.pages must not be empty to use the '
	'Navigator.pages API',
	),
	library: 'widget library',
	stack: StackTrace.current,
	),
	);
	}
	return true;
	}());

Conversation

camsim99 commented Mar 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

justinmc left a comment

Choose a reason for hiding this comment

Uh oh!

justinmc Mar 15, 2023

Choose a reason for hiding this comment

Uh oh!

camsim99 Mar 15, 2023

Choose a reason for hiding this comment

Uh oh!

justinmc Mar 15, 2023

Choose a reason for hiding this comment

Uh oh!

justinmc Mar 15, 2023

Choose a reason for hiding this comment

Uh oh!

justinmc Mar 15, 2023

Choose a reason for hiding this comment

Uh oh!

justinmc left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

camsim99 commented Mar 15, 2023 •

edited

Loading

justinmc left a comment •

edited

Loading