Skip to content
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

Spelling error decorations and suggestions (Partial for #388) #2324

Merged
merged 17 commits into from
Oct 7, 2024
Merged

Spelling error decorations and suggestions (Partial for #388) #2324

merged 17 commits into from
Oct 7, 2024

Conversation

matthew-carroll
Copy link
Contributor

@matthew-carroll matthew-carroll commented Sep 18, 2024 •
edited
Loading

Spelling error decorations and suggestions (Partial for #388)

Screen.Recording.2024-09-17.at.7.09.40.PM.mov

@matthew-carroll
Copy link
Contributor Author

@miguelcmedeiros @brian-superlist - Do you guys wanna try out this PR and report back with any issues?

angelosilvestre
angelosilvestre reviewed Sep 19, 2024
View reviewed changes
super_editor/lib/src/default_editor/spelling_and_grammar/spelling_and_grammar_styler.dart Outdated Show resolved Hide resolved
super_editor/lib/src/default_editor/spelling_and_grammar/spelling_and_grammar_styler.dart Show resolved Hide resolved
...s/super-editor_text-entry_spelling-error-shows-underline_list-item_android_no-stylesheet.png Outdated
Copy link
Collaborator

Choose a reason for hiding this comment

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

The new golden isn't showing an underline. This seem to be the case for other goldens as well.

super_editor_spellcheck/.run/Mac Plugin Sample.run.xml Outdated Show resolved Hide resolved
super_editor_spellcheck/lib/src/super_editor/spelling_and_grammar_plugin.dart Show resolved Hide resolved
super_editor_spellcheck/lib/src/super_editor/spelling_error_suggestion_overlay.dart Outdated Show resolved Hide resolved
super_editor_spellcheck/lib/src/super_editor/spelling_error_suggestion_overlay.dart Outdated Show resolved Hide resolved
super_editor_spellcheck/lib/src/super_editor/spelling_error_suggestion_overlay.dart Show resolved Hide resolved
super_editor_spellcheck/lib/src/super_editor/spelling_error_suggestions.dart Outdated Show resolved Hide resolved
super_editor_spellcheck/lib/src/super_editor/spelling_error_suggestions.dart Outdated Show resolved Hide resolved
miguelcmedeiros
miguelcmedeiros reviewed Sep 19, 2024
View reviewed changes
Copy link
Collaborator

@miguelcmedeiros miguelcmedeiros left a comment

Choose a reason for hiding this comment

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

@matthew-carroll noticed a small issue (see comment) with the code.

Also tested the feature. Here's my initial feedback:

  • We should be able to pass a tapRegionGroupId to the suggestion toolbar so that it plays well with a TapRegion ancestor
  • Tested on android for potential regression and the following is thrown:
I/flutter ( 5003): (12.427) PlatformException(channel-error, Unable to establish connection on channel: "dev.flutter.pigeon.super_editor_spellcheck.SpellCheckMac.checkSpelling"., null, null)
I/flutter ( 5003): #0      SpellCheckMac.checkSpelling (package:super_editor_spellcheck/src/platform/messages.g.dart:277:7)
I/flutter ( 5003): <asynchronous suspension>
I/flutter ( 5003): #1      SuperEditorSpellCheckerMacPlugin.checkSpelling (package:super_editor_spellcheck/src/platform/spell_checker_mac.dart:37:20)
I/flutter ( 5003): <asynchronous suspension>
I/flutter ( 5003): #2      SpellingAndGrammarReaction._findSpellingAndGrammarErrors (package:super_editor_spellcheck/src/super_editor/spelling_and_grammar_plugin.dart:200:21)
I/flutter ( 5003): <asynchronous suspension>

super_editor/lib/src/default_editor/list_items.dart Outdated
}) : super(nodeId: nodeId, maxWidth: maxWidth, padding: padding) {
this.composingRegion = composingRegion;
this.showComposingRegionUnderline = showComposingRegionUnderline;

this.spellingErrorUnderlineStyle = spellingErrorUnderlineStyle;
this.spellingErrors = spellingErrors;

this.grammarErrorUnderlineStyle = grammarErrorUnderlineStyle;
this.grammarErrors = spellingErrors;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
this.grammarErrors = spellingErrors;
this.grammarErrors = grammarErrors;

super_editor_spellcheck/lib/src/super_editor/spelling_error_suggestion_overlay.dart Outdated
parentNode: widget.editorFocusNode,
child: Container(
decoration: BoxDecoration(
color: Colors.white,
Copy link
Collaborator

Choose a reason for hiding this comment

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

The current implementation looks fine on light mode, but not on dark mode.
Also, it would be great to be able to customize the suggestion overlay. Is it possible to expose this in the API?

@matthew-carroll
PR Updates:
4ccef4a 
 * Spellcheck plugin toolbar builder is configurable
 * Default toolbar changes colors in dark mode
 * Default toolbar takes a tap region ID
 * Spell check reaction fizzles when not on Mac desktop
 * Fixed list items that were passing spelling errors for grammar errors
@matthew-carroll
Copy link
Contributor Author

@miguelcmedeiros - I addressed your issues.

Screen.Recording.2024-09-22.at.10.17.55.AM.mov

Do you want to try out the configurable toolbar builder with tap region ID to make sure you can build the kind of toolbar you want with the given API?

The configuration point is at the plugin level. You pass your own toolbar builder to the plugin.

@miguelcmedeiros
Copy link
Collaborator

Do you want to try out the configurable toolbar builder with tap region ID to make sure you can build the kind of toolbar you want with the given API?

@matthew-carroll tried to give it another go with the latest changes, but it was not possible. Left a few comments related to why it was not possible.

@matthew-carroll
Copy link
Contributor Author

@miguelcmedeiros I updated the widget tree to use the toolbar builder, exported all source files, and removed the tap region ID from the toolbar build interface.

@miguelcmedeiros
Copy link
Collaborator

@matthew-carroll since commit 58f4769 it works well 🚀

@matthew-carroll
Copy link
Contributor Author

@miguelcmedeiros any comments at all? any adjustments? do you think you should try to ship with this PR before we merge, or do you feel good about us merging this?

@matthew-carroll matthew-carroll changed the title Spelling error decorations and suggestions (Resolves #388) Spelling error decorations and suggestions (Partial for #388) Sep 29, 2024
@matthew-carroll
Copy link
Contributor Author

@angelosilvestre can you take another look. I resolved most of your comments, and then followed up with a few of them.

I think as soon as you approve this PR, we're ready to merge in. Superlist seems happy with it.

angelosilvestre
angelosilvestre approved these changes Oct 1, 2024
View reviewed changes
Copy link
Collaborator

@angelosilvestre angelosilvestre left a comment

Choose a reason for hiding this comment

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

LGTM with a couple comments. Also, I'm still seeing a bunch of golden files that previously were showing an underline, and now aren't. I wanted to make sure this is correct.

super_editor_spellcheck/lib/src/super_editor/spelling_and_grammar_plugin.dart
_asyncRequestIds[textNode.id] = _asyncRequestIds[textNode.id]! + 1;

int startingOffset = 0;
TextRange prevError = TextRange.empty;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess it makes sense.

super_editor_spellcheck/lib/src/super_editor/spelling_and_grammar_plugin.dart
final spellingSuggestions = <TextRange, SpellingErrorSuggestion>{};
if (isSpellCheckEnabled) {
do {
prevError = await spellChecker.checkSpelling(
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we should pass the language to avoid any surprises, the code would be:

final locale = PlatformDispatcher.instance.locale;
final language = spellChecker.convertDartLocaleToMacLanguageCode(locale)!;

super_editor_spellcheck/lib/src/super_editor/spelling_error_suggestion_overlay.dart
_suggestionListenable.value = spellingSuggestion;

WidgetsBinding.instance.addPostFrameCallback((_) {
_suggestionToolbarOverlayController.show();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we check if we are still mounted?

@matthew-carroll
Copy link
Contributor Author

@angelosilvestre do you wanna do a final pass? I think the goldens should be OK now, but let me know if you see anything wrong there.

angelosilvestre
angelosilvestre reviewed Oct 6, 2024
View reviewed changes
Copy link
Collaborator

@angelosilvestre angelosilvestre left a comment

Choose a reason for hiding this comment

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

Code looks good. We still have some golden files that are removing the underlines. I left a comment at each of them.

...oldens/super-editor_text-entry_spelling-error-shows-underline_task_android_no-stylesheet.png Outdated
Copy link
Collaborator

Choose a reason for hiding this comment

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

The underline was removed from this golden. Is that correct?

...dens/super-editor_text-entry_spelling-error-shows-underline_task_android_with-stylesheet.png Outdated
Copy link
Collaborator

Choose a reason for hiding this comment

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

Another golden with the underline removed.

...ry/goldens/super-editor_text-entry_spelling-error-shows-underline_task_iOS_no-stylesheet.png Outdated
Copy link
Collaborator

Choose a reason for hiding this comment

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

Another golden with the underline removed.

.../goldens/super-editor_text-entry_spelling-error-shows-underline_task_iOS_with-stylesheet.png Outdated
Copy link
Collaborator

Choose a reason for hiding this comment

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

Another golden with the underline removed.

.../goldens/super-editor_text-entry_spelling-error-shows-underline_task_macOS_no-stylesheet.png Outdated
Copy link
Collaborator

Choose a reason for hiding this comment

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

Another golden with the underline removed.

...oldens/super-editor_text-entry_spelling-error-shows-underline_task_macOS_with-stylesheet.png Outdated
Copy link
Collaborator

Choose a reason for hiding this comment

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

Another golden with the underline removed.

@matthew-carroll
Copy link
Contributor Author

@angelosilvestre I fixed a task spelling error bug - can you check the goldens again?

angelosilvestre
angelosilvestre approved these changes Oct 6, 2024
View reviewed changes
Copy link
Collaborator

@angelosilvestre angelosilvestre left a comment

Choose a reason for hiding this comment

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

LGTM

@matthew-carroll matthew-carroll merged commit f32e4a3 into main Oct 7, 2024
12 of 14 checks passed
@matthew-carroll matthew-carroll deleted the 388_spelling-and-grammar-checking branch October 7, 2024 00:00
github-actions bot pushed a commit that referenced this pull request Oct 7, 2024
@matthew-carroll @web-flow
Spelling error decorations and suggestions (Partial for #388) (#2324)
84aacd2
@github-actions github-actions bot mentioned this pull request Oct 7, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@miguelcmedeiros miguelcmedeiros miguelcmedeiros left review comments

@angelosilvestre angelosilvestre angelosilvestre approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@matthew-carroll @miguelcmedeiros @angelosilvestre