Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.
Merged
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
Modify result response protocol
  • Loading branch information
camsim99 committed May 6, 2022
commit c2a6550c7780f7d3240d50c6e6b2894292c828b4
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@
import io.flutter.plugin.common.JSONMethodCodec;
import io.flutter.plugin.common.MethodCall;
import io.flutter.plugin.common.MethodChannel;
import java.util.ArrayList;
import org.json.JSONArray;
import org.json.JSONException;

Expand Down Expand Up @@ -44,9 +43,6 @@ public class SpellCheckChannel {
public final MethodChannel channel;
private SpellCheckMethodHandler spellCheckMethodHandler;

private String textAwaitingResponse;
private MethodChannel.Result resultAwaitingResponse;

@NonNull
final MethodChannel.MethodCallHandler parsingMethodHandler =
new MethodChannel.MethodCallHandler() {
Expand All @@ -67,10 +63,8 @@ public void onMethodCall(@NonNull MethodCall call, @NonNull MethodChannel.Result
final JSONArray argumentList = (JSONArray) args;
String locale = argumentList.getString(0);
String text = argumentList.getString(1);
textAwaitingResponse = text;
resultAwaitingResponse = result;

spellCheckMethodHandler.initiateSpellCheck(locale, text);
spellCheckMethodHandler.initiateSpellCheck(locale, text, result);
} catch (JSONException exception) {
result.error("error", exception.getMessage(), null);
}
Expand All @@ -87,18 +81,6 @@ public SpellCheckChannel(@NonNull DartExecutor dartExecutor) {
channel.setMethodCallHandler(parsingMethodHandler);
}

/** Responsible for sending spell check results and corresponding text through this channel. */
public void updateSpellCheckResults(ArrayList<String> spellCheckResults) {
spellCheckResults.add(0, textAwaitingResponse);

if (resultAwaitingResponse != null && textAwaitingResponse != null) {
resultAwaitingResponse.success(spellCheckResults);
}

resultAwaitingResponse = null;
textAwaitingResponse = null;
}

/**
* Sets the {@link SpellCheckMethodHandler} which receives all requests to spell check the
* specified text sent through this channel.
Expand All @@ -110,11 +92,11 @@ public void setSpellCheckMethodHandler(

public interface SpellCheckMethodHandler {
/**
* Requests that spell check is initiated for the specified text, which will automatically
* result in a call to {@code
* SpellCheckChannel#setSpellCheckMethodHandler(SpellCheckMethodHandler)} once spell check
* results are received from the native spell check service.
* Requests that spell check is initiated for the specified text, which will respond to the
* {@code result} with either success if spell check results are received or error if the
* request is skipped.
*/
void initiateSpellCheck(@NonNull String locale, @NonNull String text);
void initiateSpellCheck(
@NonNull String locale, @NonNull String text, @NonNull MethodChannel.Result result);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,10 @@
import android.view.textservice.TextServicesManager;
import androidx.annotation.NonNull;
import io.flutter.embedding.engine.systemchannels.SpellCheckChannel;
import io.flutter.plugin.common.MethodChannel;
import io.flutter.plugin.localization.LocalizationPlugin;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Locale;

/**
Expand All @@ -32,9 +34,11 @@ public class SpellCheckPlugin
private final TextServicesManager mTextServicesManager;
private SpellCheckerSession mSpellCheckerSession;

private MethodChannel.Result pendingResult;
private String pendingResultText;

// The maximum number of suggestions that the Android spell check service is allowed to provide
// per word.
// Same number that is used by default for Android's TextViews.
// per word. Same number that is used by default for Android's TextViews.
private static final int MAX_SPELL_CHECK_SUGGESTIONS = 5;

public SpellCheckPlugin(
Expand Down Expand Up @@ -62,8 +66,21 @@ public void destroy() {
}
}

/**
* Initiates call to native spell checker to spell check specified text if there is no result
* awaiting a response.
*/
@Override
public void initiateSpellCheck(@NonNull String locale, @NonNull String text) {
public void initiateSpellCheck(
@NonNull String locale, @NonNull String text, @NonNull MethodChannel.Result result) {
if (pendingResult != null) {
result.error("error", "Previous spell check request still pending.", null);
return;
}

pendingResult = result;
pendingResultText = text;

performSpellCheck(locale, text);
Copy link

Choose a reason for hiding this comment

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

Handle case when there's a pending initiateSpellCheck. Maybe, call result.error or result.success with an empty list. The general idea is that while the framework is waiting on a spellcheck request, a new request is issued, so the older one needs to be dropped.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The approach of dropping older requests has a similar fault to the approach I pursued previously of saving the result and text awaiting a response to that result in the SpellCheckChannel class, which is that we cannot cancel requests to the Android spell checker once they have been submitted.

We already knew that, but our assumption was that if the SpellCheckPlugin has detected a result awaiting a response when another request to initiate spell check has come in, then that request will not reach the onGetSentenceSuggestions(...) callback, meaning that the Android spell checker consumed the request. The consequence of assuming such is that sometimes the plugin will attempt to respond to a new request with old spell check results. Depending on how much the text has been modified between the time those old results come in and the time that the new request to spell check was submitted, those old results could lead to some hard-to-handle errors on the framework side.

Given all of that, I decided to have the plugin only handle one request at a time by dropping new requests when an old one is still pending. This approach may lead to results lagging slightly behind, but (i) the spell check results in any given response will correspond to the text in that response, (ii) the spell check results will be returned in chronological order, and (iii) any impacts of these results lagging behind can be handled by the logic on the framework side. This is all because with this approach, we do not have to battle the unpredictable behavior of the Android spell checker. I believe also that in further iterations, other speedup techniques can be more easily explored given these guarantees.

Copy link
Contributor

Choose a reason for hiding this comment

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

Does this approach seem to work with decent performance in practice? Maybe this is the most solid approach in the real world even though it's not the fastest theoretically.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, is there ever a risk of the spell checker never responding and locking up or is that another thing that we shouldn't worry about in practice?

Copy link
Contributor Author

@camsim99 camsim99 May 10, 2022

Choose a reason for hiding this comment

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

Does this approach seem to work with decent performance in practice? Maybe this is the most solid approach in the real world even though it's not the fastest theoretically.

Yes, exactly. From playing around with it (paired with making the corrections on the framework side if the process of returning results lag behind), I don't see any difference. If you are open to playing around with it, too, that'd be great!

Also, is there ever a risk of the spell checker never responding and locking up or is that another thing that we shouldn't worry about in practice?

I think we can rely on Android for this. Android seems to have some handling for issues like that (see here).

}

Expand Down Expand Up @@ -96,13 +113,13 @@ public void performSpellCheck(@NonNull String locale, @NonNull String text) {
*/
@Override
public void onGetSentenceSuggestions(SentenceSuggestionsInfo[] results) {
ArrayList<String> spellCheckerSuggestionSpans = new ArrayList<String>();

if (results.length == 0) {
mSpellCheckChannel.updateSpellCheckResults(spellCheckerSuggestionSpans);
pendingResult.success(new ArrayList<>(Arrays.asList(pendingResultText, "")));
pendingResult = null;
return;
}

ArrayList<String> spellCheckerSuggestionSpans = new ArrayList<String>();
SentenceSuggestionsInfo spellCheckResults = results[0];

for (int i = 0; i < spellCheckResults.getSuggestionsCount(); i++) {
Expand All @@ -128,7 +145,9 @@ public void onGetSentenceSuggestions(SentenceSuggestionsInfo[] results) {
spellCheckerSuggestionSpan.substring(0, spellCheckerSuggestionSpan.length() - 1));
}

mSpellCheckChannel.updateSpellCheckResults(spellCheckerSuggestionSpans);
spellCheckerSuggestionSpans.add(0, pendingResultText);
pendingResult.success(spellCheckerSuggestionSpans);
pendingResult = null;
}

@Override
Expand Down