Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Add Spell Check Support for Android Engine #30858

Merged
fluttergithubbot merged 72 commits into from
May 16, 2022
Merged

Add Spell Check Support for Android Engine #30858

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
72 commits
Select commit Hold shift + click to select a range
dc93719
Add session management
camsim99 Jan 10, 2022
cd234ad
Handle receiving results
camsim99 Jan 10, 2022
f463846
Add framework logic, modify engine
camsim99 Jan 12, 2022
3dfae0a
Make engine build
camsim99 Jan 12, 2022
546692b
Add retrieving locale
camsim99 Jan 13, 2022
5f43ff5
Added comments for experimentation
camsim99 Jan 18, 2022
ee73219
Send string back temporarily, fix results representation
camsim99 Feb 2, 2022
624f68b
Formatting
camsim99 Feb 2, 2022
846b195
Merge branch 'main' of github.com:flutter/engine into issue_34688_dev
camsim99 Feb 28, 2022
b27704c
Merge remote-tracking branch 'upstream/main' into issue_34688_dev
camsim99 Mar 16, 2022
64f0ac7
Add new method channel
camsim99 Mar 18, 2022
0cc3776
Add platform setting for spell check
camsim99 Mar 18, 2022
33f2f40
Change method channel name
camsim99 Mar 21, 2022
eb3c731
Remove print statements
camsim99 Mar 22, 2022
1877242
Clean up
camsim99 Mar 23, 2022
5372aaa
Formatting
camsim99 Mar 24, 2022
d9adbdc
Clean PR
camsim99 Mar 28, 2022
34809db
Undo platform plugin changes
camsim99 Mar 28, 2022
10d154d
Clean up code
camsim99 Mar 30, 2022
00c228e
Begin adding tests
camsim99 Mar 30, 2022
e08171f
Formatting
camsim99 Mar 30, 2022
5ccc144
Correct sessions, remove unecessary line
camsim99 Mar 31, 2022
87ab60f
Add tests
camsim99 Mar 31, 2022
f4083d7
Formatting
camsim99 Mar 31, 2022
ce45427
Remove variable
camsim99 Mar 31, 2022
6972b0b
Begin fixing tests
camsim99 Apr 1, 2022
3eba4de
Address reviews
camsim99 Apr 7, 2022
963d9ae
Start fixing systesm setting
camsim99 Apr 8, 2022
6ea2ede
Continue addressing reviews
camsim99 Apr 8, 2022
32c7702
Continue addressing reviews
camsim99 Apr 8, 2022
e9ebd5b
Fix spell checkers check
camsim99 Apr 8, 2022
8298436
Begin fixing tests
camsim99 Apr 9, 2022
993b6c8
Reformat FlutterViewTest
camsim99 Apr 11, 2022
4179f42
Fixing tests
camsim99 Apr 11, 2022
f999488
Merge remote-tracking branch 'upstream/main' into issue_34688_dev
camsim99 Apr 11, 2022
ade72bc
Fix tests minus log errors
camsim99 Apr 11, 2022
c20b62c
Add context shadow to stub tsm request
camsim99 Apr 13, 2022
49583d4
Formatting
camsim99 Apr 13, 2022
49939aa
Fix np error
camsim99 Apr 14, 2022
0cf00f3
Add license
camsim99 Apr 14, 2022
48a4354
Update licenses file
camsim99 Apr 14, 2022
510f83a
Add nativeSpellCheckServiceDefined to other platform dispatchers
camsim99 Apr 15, 2022
d8e333b
Merge remote-tracking branch 'upstream/main' into issue_34688_dev
camsim99 Apr 15, 2022
61908d3
Remove setting
camsim99 Apr 15, 2022
29b095f
Fix typos
camsim99 Apr 15, 2022
ee28058
Update lib/ui/platform_dispatcher.dart
camsim99 Apr 18, 2022
4f0a074
Update lib/ui/window.dart
camsim99 Apr 18, 2022
6c4d437
Update shell/platform/android/io/flutter/embedding/android/FlutterVie…
camsim99 Apr 18, 2022
d73b915
Update shell/platform/android/io/flutter/embedding/engine/systemchann…
camsim99 Apr 18, 2022
66c24ed
Update shell/platform/android/io/flutter/embedding/engine/systemchann…
camsim99 Apr 18, 2022
04a2ce1
Update shell/platform/android/io/flutter/embedding/engine/systemchann…
camsim99 Apr 18, 2022
15d17f5
Update shell/platform/android/io/flutter/embedding/engine/systemchann…
camsim99 Apr 18, 2022
e6414b2
Update shell/platform/android/io/flutter/embedding/engine/systemchann…
camsim99 Apr 18, 2022
2882e3b
Update shell/platform/android/io/flutter/embedding/engine/systemchann…
camsim99 Apr 18, 2022
dfe4689
Update shell/platform/android/io/flutter/plugin/editing/SpellCheckPlu…
camsim99 Apr 18, 2022
cdc6b9c
Update shell/platform/android/io/flutter/plugin/editing/SpellCheckPlu…
camsim99 Apr 18, 2022
65b8850
Update shell/platform/android/io/flutter/plugin/editing/SpellCheckPlu…
camsim99 Apr 18, 2022
036b15b
Make review fixes
camsim99 Apr 18, 2022
188c2ad
Merge branch 'issue_34688_dev' of github.com:camsim99/engine into iss…
camsim99 Apr 18, 2022
8198d4e
Formatting
camsim99 Apr 18, 2022
a2b6c90
Fix spell check test
camsim99 Apr 18, 2022
2aac372
Address nits
camsim99 Apr 21, 2022
894f4e9
Send text to framework
camsim99 Apr 28, 2022
2a4ea66
Push for visibility
camsim99 May 4, 2022
c2a6550
Modify result response protocol
camsim99 May 6, 2022
6125e74
Merge branch 'main' into issue_34688_dev
camsim99 May 9, 2022
74d1477
Merge remote-tracking branch 'upstream/master' into issue_34688_dev
camsim99 May 10, 2022
cc8bf5e
Merge branch 'issue_34688_dev' of github.com:camsim99/engine into iss…
camsim99 May 10, 2022
5a175ec
Formatting and pull
camsim99 May 10, 2022
1c02784
Merge remote-tracking branch 'upstream/master' into issue_34688_dev
camsim99 May 12, 2022
3e74d61
Emulator fix
camsim99 May 13, 2022
975673c
Merge remote-tracking branch 'upstream/main' into issue_34688_dev
camsim99 May 13, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Address nits
  • Loading branch information
@camsim99
camsim99 committed Apr 21, 2022
commit 2aac372eb25bc42a6ee8d43c42d411532fd804d0
39 changes: 22 additions & 17 deletions shell/platform/android/io/flutter/plugin/editing/SpellCheckPlugin.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,29 +99,34 @@ public void performSpellCheck(@NonNull String locale, @NonNull String text) {
public void onGetSentenceSuggestions(SentenceSuggestionsInfo[] results) {
ArrayList<String> spellCheckerSuggestionSpans = new ArrayList<String>();

if (results.length > 0) {
SentenceSuggestionsInfo spellCheckResults = results[0];
if (results.length == 0) {
mSpellCheckChannel.updateSpellCheckResults(spellCheckerSuggestionSpans);
return;
}

SentenceSuggestionsInfo spellCheckResults = results[0];

for (int i = 0; i < spellCheckResults.getSuggestionsCount(); i++) {
SuggestionsInfo suggestionsInfo = spellCheckResults.getSuggestionsInfoAt(i);
int suggestionsCount = suggestionsInfo.getSuggestionsCount();
for (int i = 0; i < spellCheckResults.getSuggestionsCount(); i++) {
SuggestionsInfo suggestionsInfo = spellCheckResults.getSuggestionsInfoAt(i);
int suggestionsCount = suggestionsInfo.getSuggestionsCount();

if (suggestionsCount > 0) {
String spellCheckerSuggestionSpan = "";
int start = spellCheckResults.getOffsetAt(i);
int length = spellCheckResults.getLengthAt(i);
if (suggestionsCount == 0) {
continue;
}

spellCheckerSuggestionSpan += (String.valueOf(start) + ".");
spellCheckerSuggestionSpan += (String.valueOf(start + (length - 1)) + ".");
String spellCheckerSuggestionSpan = "";
int start = spellCheckResults.getOffsetAt(i);
int end = start + spellCheckResults.getLengthAt(i) - 1;

for (int j = 0; j < suggestionsCount; j++) {
spellCheckerSuggestionSpan += (suggestionsInfo.getSuggestionAt(j) + "/n");
}
spellCheckerSuggestionSpan += String.valueOf(start) + ".";
spellCheckerSuggestionSpan += String.valueOf(end) + ".";

spellCheckerSuggestionSpans.add(
spellCheckerSuggestionSpan.substring(0, spellCheckerSuggestionSpan.length() - 2));
}
for (int j = 0; j < suggestionsCount; j++) {
spellCheckerSuggestionSpan += suggestionsInfo.getSuggestionAt(j) + "\n";
}

spellCheckerSuggestionSpans.add(
spellCheckerSuggestionSpan.substring(0, spellCheckerSuggestionSpan.length() - 1));
}

mSpellCheckChannel.updateSpellCheckResults(spellCheckerSuggestionSpans);
Copy link
Contributor

@justinmc justinmc Apr 20, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Similar to my previous comment, if the framework calls initiateSpellCheck multiple times before receiving updateSpellCheckResults, what guarantees does it have about the order of incoming updateSpellCheckResults calls?

Copy link
Contributor Author

@camsim99 camsim99 Apr 20, 2022

Choose a reason for hiding this comment

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

I've seen the results consistently come back in order, and like I said in the previous comment, in cases where it takes a bit longer to get spell check results back, the older calls get consumed, so it seems like the spell check service handles this already.

Copy link
Contributor

@justinmc justinmc Apr 21, 2022

Choose a reason for hiding this comment

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

This is pretty theoretical but do you think this is a valid problem?

  1. The framework calls initiateSpellCheck with string "recieve".
  2. The engine receives this and calls updateSpellCheckResults.
  3. The user has quickly pasted something new, so framework calls initiateSpellCheck with string "thier".
  4. The engine is called back on updateSpellCheckResults with the suggestions for "recieve" and sends them to the framework.
  5. The framework receives the suggestions for "recieve". The last call it made to initiateSpellCheck was for string "thier", and that's what's in the text field, so it incorrectly applies the suggestions for "recieve" to "thier".

In reality, I think this would only be a problem for a few milliseconds because the framework would then receive the correct suggestions for "thier". These kinds of problems are rare in reality but hard to eliminate because everything is asynchronous. We have similar problems with updateTextEditingState itself, and we currently don't do anything about them, so maybe this is fine here too.

One thing we could do, though, is to send the text (or a version or a hash) along with the suggestions so we could make sure we apply them to the correct text. I don't know if that is worth it here.

@LongCatIsLooong I know you've worked with these asynchronous problems before, any thoughts here? I'm probably being too nitpicky but I want a second opinion to tell me not to worry about this.

Copy link

@blasten blasten Apr 21, 2022

Choose a reason for hiding this comment

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

In reality, I think this would only be a problem for a few milliseconds because the framework would then receive the correct suggestions for "thier".

I was suggesting to send the string from the request back in the response, so the framework can handle that case. I think it would be great to address this issue.

Copy link
Contributor

@LongCatIsLooong LongCatIsLooong Apr 21, 2022

Choose a reason for hiding this comment

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

The spell checking process probably should not be initiated before the text input formatters are applied, the onChanged callback is called, and the TextEditingController listeners are notified in the framework (as we allow developers to change the text in these steps), to minimize the chance of this kind of races happening. But if the initiateSpellCheck API is exposed to the developer then I think we should provide a way to allow them to tell if the result is still relevant.

I'm ok with including the original string from the request but that could increase the payload size by a lot if the developer sends over the entire Bible for spellchecking. From the framework's perspective ideally the spell check service should take a String and return a Future<ListOfSuggestions>, with this kind of one-to-one correspondence the text field can determine whether the suggestion results are still relevant, and discards the results if the text has already changed.

Copy link
Contributor Author

@camsim99 camsim99 Apr 22, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I'm ok with including the original string from the request but that could increase the payload size by a lot if the developer sends over the entire Bible for spellchecking. From the framework's perspective ideally the spell check service should take a String and return a Future<ListOfSuggestions>, with this kind of one-to-one correspondence the text field can determine whether the suggestion results are still relevant, and discards the results if the text has already changed.

I discussed a similar approach with @blasten yesterday and I agree that this is the best way to proceed. Even if I do use the relevant text to ensure the results are still relevant, I don't have to send them to the engine and back, as I already found a way to recall the TextEditingValue that the spell check results are related to (by passing them here).

Down the line, I think exploring techniques for speeding up the entire process like only spell checking sentences that have changed versus the entirety of the inputted text could help, as well, as I suspect that the case @justinmc described would occur more frequently as the length of the text being spell checked increases. I definitely think that case should be addressed before any framework PRs land, but I believe it shouldn't block this PR since potential fixes would be in the framework.

Copy link
Contributor

@LongCatIsLooong LongCatIsLooong Apr 27, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Ah thanks for the clarification! Looking at the framework-side implementation, it seems to be expecting the spellchecking service to guarantee the requests are replied to in FIFO order, but the framework-side API actually doesn't provide the same FIFO guarantee its implementation relies on, to the API consumer (because the signature of the function is (Locale, TextEditingValue) -> Future<List<SpellCheckerSuggestionSpan>>).

For Android and iOS builtin spellchecking services it's easy to satisfy the FIFO requirement but I think this can be a bit too "strong" of a requirement in some other occasions. If someone wants to implement a spellchecking service that, say, fetches results from a web server, it could be a hassle to guarantee the FIFO ordering (and in the end Flutter developers that use the API don't even benefit from that).

Copy link
Contributor Author

@camsim99 camsim99 May 9, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I do agree it could be hassle, but I do think it could be largely addressed on the framework side. Due to the fact that the framework may attempt to build the TextSpan tree with spell check results that are not up to date, I added some logic there that could do some corrections on those results to build the TextSpanTree as accurate as it can guess. This may address that problem to a large degree, but of course, I acknowledge that it would be best if we could guarantee the FIFO ordering.

Expand Down
2 changes: 1 addition & 1 deletion shell/platform/android/test/io/flutter/plugin/editing/SpellCheckPluginTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,6 +152,6 @@ public void onGetSentenceSuggestionsProperlyRequestsUpdateSpellCheckResults() {
});

verify(fakeSpellCheckChannel)
.updateSpellCheckResults(new ArrayList<String>(Arrays.asList("7.11.world/nword/nold")));
.updateSpellCheckResults(new ArrayList<String>(Arrays.asList("7.11.world\nword\nold")));
}
}