Skip to content

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

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
auto-submit merged 12 commits into from
Mar 27, 2023
Merged

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

auto-submit merged 12 commits into from
Mar 27, 2023

Conversation

@camsim99
Copy link
Contributor

@camsim99 camsim99 commented Mar 15, 2023 •
edited
Loading

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.

@flutter-dashboard 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
@camsim99 camsim99 requested a review from justinmc March 15, 2023 16:08
justinmc
justinmc reviewed Mar 15, 2023
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.

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

packages/flutter/lib/src/widgets/editable_text.dart Outdated
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) {
Copy link
Contributor

@justinmc justinmc Mar 15, 2023

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

@camsim99 camsim99 Mar 15, 2023

Choose a reason for hiding this comment

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

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.

packages/flutter/test/widgets/editable_text_test.dart Outdated

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',
Copy link
Contributor

@justinmc justinmc Mar 15, 2023

Choose a reason for hiding this comment

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

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

packages/flutter/lib/src/widgets/editable_text.dart
// a native spell checker must be supported.
return const SpellCheckConfiguration.disabled();
}

Copy link
Contributor

@justinmc justinmc Mar 15, 2023

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@justinmc justinmc Mar 15, 2023

Choose a reason for hiding this comment

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

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.

@camsim99 camsim99 requested a review from justinmc March 21, 2023 21:20
justinmc
justinmc approved these changes Mar 22, 2023
View reviewed changes
Copy link
Contributor

@justinmc justinmc left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM with nits 👍

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

@camsim99 camsim99 added the autosubmit Merge PR when tree becomes green via auto submit App label Mar 27, 2023
@auto-submit auto-submit bot merged commit 5e76e11 into flutter:master Mar 27, 2023
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Mar 27, 2023
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 27, 2023
@engine-flutter-autoroll
5e76e11 Allow spell check to fail silently on unsupported platforms &…
990e1a9 
… spell checkers (flutter/flutter#122715)
@eliasyishak eliasyishak mentioned this pull request Mar 27, 2023
Closed
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Mar 27, 2023
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 27, 2023
@engine-flutter-autoroll
5e76e11 Allow spell check to fail silently on unsupported platforms &…
3beaf5e 
… spell checkers (flutter/flutter#122715)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Mar 28, 2023
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 28, 2023
@engine-flutter-autoroll
5e76e11 Allow spell check to fail silently on unsupported platforms &…
0152a7c 
… spell checkers (flutter/flutter#122715)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Mar 28, 2023
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 28, 2023
@engine-flutter-autoroll
5e76e11 Allow spell check to fail silently on unsupported platforms &…
691a901 
… spell checkers (flutter/flutter#122715)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Mar 28, 2023
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 28, 2023
@engine-flutter-autoroll
5e76e11 Allow spell check to fail silently on unsupported platforms &…
6c07265 
… spell checkers (flutter/flutter#122715)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Mar 28, 2023
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 28, 2023
@engine-flutter-autoroll
5e76e11 Allow spell check to fail silently on unsupported platforms &…
6977004 
… spell checkers (flutter/flutter#122715)
This was referenced Mar 28, 2023
Merged
Merged
@reidbaker reidbaker mentioned this pull request Apr 5, 2023
Merged
8 tasks
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 10, 2023
@engine-flutter-autoroll
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
@engine-flutter-autoroll
5e76e11 Allow spell check to fail silently on unsupported platforms &…
df085d8 
… spell checkers (flutter/flutter#122715)
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

Assignees

No one assigned

Labels

a: text input Entering text in a text field or keyboard related problems autosubmit Merge PR when tree becomes green via auto submit App 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.

Allow spell check to fail silently on unsupported platforms & for unsupported spell checkers

2 participants

@camsim99 @justinmc