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.
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
feat: Spellchecker Async Implementation #14032
Changes from all commits
42caab2
ebe1cbc
048735f
b3d5331
927bb2e
20df488
c46860a
e6956b3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,7 +4,7 @@ | |
|
||
#include "atom/renderer/api/atom_api_spell_check_client.h" | ||
|
||
#include <algorithm> | ||
#include <map> | ||
#include <vector> | ||
|
||
#include "atom/common/native_mate_converters/string16_converter.h" | ||
|
@@ -13,6 +13,7 @@ | |
#include "chrome/renderer/spellchecker/spellcheck_worditerator.h" | ||
#include "native_mate/converter.h" | ||
#include "native_mate/dictionary.h" | ||
#include "native_mate/function_template.h" | ||
#include "third_party/blink/public/web/web_text_checking_completion.h" | ||
#include "third_party/blink/public/web/web_text_checking_result.h" | ||
#include "third_party/icu/source/common/unicode/uscript.h" | ||
|
@@ -40,30 +41,35 @@ bool HasWordCharacters(const base::string16& text, int index) { | |
|
||
class SpellCheckClient::SpellcheckRequest { | ||
public: | ||
// Map of individual words to list of occurrences in text | ||
using WordMap = | ||
std::map<base::string16, std::vector<blink::WebTextCheckingResult>>; | ||
|
||
SpellcheckRequest(const base::string16& text, | ||
blink::WebTextCheckingCompletion* completion) | ||
: text_(text), completion_(completion) { | ||
DCHECK(completion); | ||
} | ||
~SpellcheckRequest() {} | ||
|
||
base::string16 text() { return text_; } | ||
const base::string16& text() const { return text_; } | ||
blink::WebTextCheckingCompletion* completion() { return completion_; } | ||
WordMap& wordmap() { return word_map_; } | ||
|
||
private: | ||
base::string16 text_; // Text to be checked in this task. | ||
|
||
WordMap word_map_; // WordMap to hold distinct words in text | ||
// The interface to send the misspelled ranges to WebKit. | ||
blink::WebTextCheckingCompletion* completion_; | ||
|
||
DISALLOW_COPY_AND_ASSIGN(SpellcheckRequest); | ||
}; | ||
|
||
SpellCheckClient::SpellCheckClient(const std::string& language, | ||
bool auto_spell_correct_turned_on, | ||
v8::Isolate* isolate, | ||
v8::Local<v8::Object> provider) | ||
: isolate_(isolate), | ||
: pending_request_param_(nullptr), | ||
isolate_(isolate), | ||
context_(isolate, isolate->GetCurrentContext()), | ||
provider_(isolate, provider) { | ||
DCHECK(!context_.IsEmpty()); | ||
|
@@ -79,19 +85,6 @@ SpellCheckClient::~SpellCheckClient() { | |
context_.Reset(); | ||
} | ||
|
||
void SpellCheckClient::CheckSpelling( | ||
const blink::WebString& text, | ||
int& misspelling_start, | ||
int& misspelling_len, | ||
blink::WebVector<blink::WebString>* optional_suggestions) { | ||
std::vector<blink::WebTextCheckingResult> results; | ||
SpellCheckText(text.Utf16(), true, &results); | ||
if (results.size() == 1) { | ||
misspelling_start = results[0].location; | ||
misspelling_len = results[0].length; | ||
} | ||
} | ||
|
||
void SpellCheckClient::RequestCheckingOfText( | ||
const blink::WebString& textToCheck, | ||
blink::WebTextCheckingCompletion* completionCallback) { | ||
|
@@ -103,16 +96,15 @@ void SpellCheckClient::RequestCheckingOfText( | |
} | ||
|
||
// Clean up the previous request before starting a new request. | ||
if (pending_request_param_.get()) { | ||
if (pending_request_param_) { | ||
pending_request_param_->completion()->DidCancelCheckingText(); | ||
} | ||
|
||
pending_request_param_.reset(new SpellcheckRequest(text, completionCallback)); | ||
|
||
base::ThreadTaskRunnerHandle::Get()->PostTask( | ||
FROM_HERE, | ||
base::BindOnce(&SpellCheckClient::PerformSpellCheck, AsWeakPtr(), | ||
base::Owned(pending_request_param_.release()))); | ||
base::BindOnce(&SpellCheckClient::SpellCheckText, AsWeakPtr())); | ||
} | ||
|
||
bool SpellCheckClient::IsSpellCheckingEnabled() const { | ||
|
@@ -128,12 +120,13 @@ bool SpellCheckClient::IsShowingSpellingUI() { | |
void SpellCheckClient::UpdateSpellingUIWithMisspelledWord( | ||
const blink::WebString& word) {} | ||
|
||
void SpellCheckClient::SpellCheckText( | ||
const base::string16& text, | ||
bool stop_at_first_result, | ||
std::vector<blink::WebTextCheckingResult>* results) { | ||
if (text.empty() || spell_check_.IsEmpty()) | ||
void SpellCheckClient::SpellCheckText() { | ||
const auto& text = pending_request_param_->text(); | ||
if (text.empty() || spell_check_.IsEmpty()) { | ||
pending_request_param_->completion()->DidCancelCheckingText(); | ||
pending_request_param_ = nullptr; | ||
return; | ||
} | ||
|
||
if (!text_iterator_.IsInitialized() && | ||
!text_iterator_.Initialize(&character_attributes_, true)) { | ||
|
@@ -153,64 +146,94 @@ void SpellCheckClient::SpellCheckText( | |
|
||
SpellCheckScope scope(*this); | ||
base::string16 word; | ||
int word_start; | ||
int word_length; | ||
for (auto status = | ||
text_iterator_.GetNextWord(&word, &word_start, &word_length); | ||
status != SpellcheckWordIterator::IS_END_OF_TEXT; | ||
status = text_iterator_.GetNextWord(&word, &word_start, &word_length)) { | ||
std::vector<base::string16> words; | ||
auto& word_map = pending_request_param_->wordmap(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Will this every be populated from a previous run -- do we need to I think the answer is "no" but am not positive There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nope, we don't need to clear it. Ideally, we shouldn't receive another request until the first one is complete, i.e. we call |
||
blink::WebTextCheckingResult result; | ||
for (;;) { // Run until end of text | ||
const auto status = | ||
text_iterator_.GetNextWord(&word, &result.location, &result.length); | ||
if (status == SpellcheckWordIterator::IS_END_OF_TEXT) | ||
break; | ||
if (status == SpellcheckWordIterator::IS_SKIPPABLE) | ||
continue; | ||
|
||
// Found a word (or a contraction) that the spellchecker can check the | ||
// spelling of. | ||
if (SpellCheckWord(scope, word)) | ||
continue; | ||
|
||
// If the given word is a concatenated word of two or more valid words | ||
// (e.g. "hello:hello"), we should treat it as a valid word. | ||
if (IsValidContraction(scope, word)) | ||
continue; | ||
std::vector<base::string16> contraction_words; | ||
if (!IsContraction(scope, word, &contraction_words)) { | ||
words.push_back(word); | ||
word_map[word].push_back(result); | ||
} else { | ||
// For a contraction, we want check the spellings of each individual | ||
// part, but mark the entire word incorrect if any part is misspelled | ||
// Hence, we use the same word_start and word_length values for every | ||
// part of the contraction. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ^ This is a good idea! |
||
for (const auto& w : contraction_words) { | ||
words.push_back(w); | ||
word_map[w].push_back(result); | ||
} | ||
} | ||
} | ||
|
||
blink::WebTextCheckingResult result; | ||
result.location = word_start; | ||
result.length = word_length; | ||
results->push_back(result); | ||
// Send out all the words data to the spellchecker to check | ||
SpellCheckWords(scope, words); | ||
} | ||
|
||
if (stop_at_first_result) | ||
return; | ||
void SpellCheckClient::OnSpellCheckDone( | ||
const std::vector<base::string16>& misspelled_words) { | ||
std::vector<blink::WebTextCheckingResult> results; | ||
auto* const completion_handler = pending_request_param_->completion(); | ||
|
||
auto& word_map = pending_request_param_->wordmap(); | ||
|
||
// Take each word from the list of misspelled words received, find their | ||
// corresponding WebTextCheckingResult that's stored in the map and pass | ||
// all the results to blink through the completion callback. | ||
for (const auto& word : misspelled_words) { | ||
auto iter = word_map.find(word); | ||
if (iter != word_map.end()) { | ||
// Word found in map, now gather all the occurrences of the word | ||
// from the map value | ||
auto& words = iter->second; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (minor) 'words' is a confusing variable name here because they're ranges / results. I know 'results' is already taken but could something else be used here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added a comment. |
||
results.insert(results.end(), words.begin(), words.end()); | ||
words.clear(); | ||
} | ||
} | ||
completion_handler->DidFinishCheckingText(results); | ||
pending_request_param_ = nullptr; | ||
} | ||
|
||
bool SpellCheckClient::SpellCheckWord( | ||
void SpellCheckClient::SpellCheckWords( | ||
const SpellCheckScope& scope, | ||
const base::string16& word_to_check) const { | ||
const std::vector<base::string16>& words) { | ||
DCHECK(!scope.spell_check_.IsEmpty()); | ||
|
||
v8::Local<v8::Value> word = mate::ConvertToV8(isolate_, word_to_check); | ||
v8::Local<v8::Value> result = | ||
scope.spell_check_->Call(scope.provider_, 1, &word); | ||
v8::Local<v8::FunctionTemplate> templ = mate::CreateFunctionTemplate( | ||
isolate_, base::Bind(&SpellCheckClient::OnSpellCheckDone, AsWeakPtr())); | ||
|
||
if (!result.IsEmpty() && result->IsBoolean()) | ||
return result->BooleanValue(); | ||
else | ||
return true; | ||
v8::Local<v8::Value> args[] = {mate::ConvertToV8(isolate_, words), | ||
templ->GetFunction()}; | ||
// Call javascript with the words and the callback function | ||
scope.spell_check_->Call(scope.provider_, 2, args); | ||
} | ||
|
||
// Returns whether or not the given string is a valid contraction. | ||
// Returns whether or not the given string is a contraction. | ||
// This function is a fall-back when the SpellcheckWordIterator class | ||
// returns a concatenated word which is not in the selected dictionary | ||
// (e.g. "in'n'out") but each word is valid. | ||
bool SpellCheckClient::IsValidContraction(const SpellCheckScope& scope, | ||
const base::string16& contraction) { | ||
// Output variable contraction_words will contain individual | ||
// words in the contraction. | ||
bool SpellCheckClient::IsContraction( | ||
const SpellCheckScope& scope, | ||
const base::string16& contraction, | ||
std::vector<base::string16>* contraction_words) { | ||
DCHECK(contraction_iterator_.IsInitialized()); | ||
|
||
contraction_iterator_.SetText(contraction.c_str(), contraction.length()); | ||
|
||
base::string16 word; | ||
int word_start; | ||
int word_length; | ||
|
||
for (auto status = | ||
contraction_iterator_.GetNextWord(&word, &word_start, &word_length); | ||
status != SpellcheckWordIterator::IS_END_OF_TEXT; | ||
|
@@ -219,18 +242,9 @@ bool SpellCheckClient::IsValidContraction(const SpellCheckScope& scope, | |
if (status == SpellcheckWordIterator::IS_SKIPPABLE) | ||
continue; | ||
|
||
if (!SpellCheckWord(scope, word)) | ||
return false; | ||
contraction_words->push_back(word); | ||
} | ||
return true; | ||
} | ||
|
||
void SpellCheckClient::PerformSpellCheck(SpellcheckRequest* param) { | ||
DCHECK(param); | ||
|
||
std::vector<blink::WebTextCheckingResult> results; | ||
SpellCheckText(param->text(), false, &results); | ||
param->completion()->DidFinishCheckingText(results); | ||
return contraction_words->size() > 1; | ||
} | ||
|
||
SpellCheckClient::SpellCheckScope::SpellCheckScope( | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -31,19 +31,13 @@ class SpellCheckClient : public blink::WebSpellCheckPanelHostClient, | |
public base::SupportsWeakPtr<SpellCheckClient> { | ||
public: | ||
SpellCheckClient(const std::string& language, | ||
bool auto_spell_correct_turned_on, | ||
v8::Isolate* isolate, | ||
v8::Local<v8::Object> provider); | ||
~SpellCheckClient() override; | ||
|
||
private: | ||
class SpellcheckRequest; | ||
// blink::WebTextCheckClient: | ||
void CheckSpelling( | ||
const blink::WebString& text, | ||
int& misspelledOffset, | ||
int& misspelledLength, | ||
blink::WebVector<blink::WebString>* optionalSuggestions) override; | ||
void RequestCheckingOfText( | ||
const blink::WebString& textToCheck, | ||
blink::WebTextCheckingCompletion* completionCallback) override; | ||
|
@@ -65,22 +59,27 @@ class SpellCheckClient : public blink::WebSpellCheckPanelHostClient, | |
~SpellCheckScope(); | ||
}; | ||
|
||
// Check the spelling of text. | ||
void SpellCheckText(const base::string16& text, | ||
bool stop_at_first_result, | ||
std::vector<blink::WebTextCheckingResult>* results); | ||
// Run through the word iterator and send out requests | ||
// to the JS API for checking spellings of words in the current | ||
// request. | ||
void SpellCheckText(); | ||
|
||
// Call JavaScript to check spelling a word. | ||
bool SpellCheckWord(const SpellCheckScope& scope, | ||
const base::string16& word_to_check) const; | ||
// The javascript function will callback OnSpellCheckDone | ||
// with the results of all the misspelled words. | ||
void SpellCheckWords(const SpellCheckScope& scope, | ||
const std::vector<base::string16>& words); | ||
|
||
// Returns whether or not the given word is a contraction of valid words | ||
// (e.g. "word:word"). | ||
bool IsValidContraction(const SpellCheckScope& scope, | ||
const base::string16& word); | ||
|
||
// Performs spell checking from the request queue. | ||
void PerformSpellCheck(SpellcheckRequest* param); | ||
// Output variable contraction_words will contain individual | ||
// words in the contraction. | ||
bool IsContraction(const SpellCheckScope& scope, | ||
const base::string16& word, | ||
std::vector<base::string16>* contraction_words); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Any reason There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nope, changed. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ohh, just saw. The linter tells to make this a pointer. |
||
|
||
// Callback for the JS API which returns the list of misspelled words. | ||
void OnSpellCheckDone(const std::vector<base::string16>& misspelled_words); | ||
|
||
// Represents character attributes used for filtering out characters which | ||
// are not supported by this SpellCheck object. | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -215,15 +215,14 @@ int WebFrame::GetWebFrameId(v8::Local<v8::Value> content_window) { | |
|
||
void WebFrame::SetSpellCheckProvider(mate::Arguments* args, | ||
const std::string& language, | ||
bool auto_spell_correct_turned_on, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I found that we weren't doing anything with this parameter in the API, so getting rid of it and also removing from the API. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this a breaking change? Our app currently has this code which seems like it will now fail:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just confirmed that it is, see electron-userland/electron-spellchecker#144 this should be noted as breaking in the release notes, it's currently a "Feature" There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Never mind, I see this has been raised elsewhere #17915 |
||
v8::Local<v8::Object> provider) { | ||
if (!provider->Has(mate::StringToV8(args->isolate(), "spellCheck"))) { | ||
args->ThrowError("\"spellCheck\" has to be defined"); | ||
return; | ||
} | ||
|
||
auto client = std::make_unique<SpellCheckClient>( | ||
language, auto_spell_correct_turned_on, args->isolate(), provider); | ||
auto client = | ||
std::make_unique<SpellCheckClient>(language, args->isolate(), provider); | ||
// Set spellchecker for all live frames in the same process or | ||
// in the sandbox mode for all live sub frames to this WebFrame. | ||
FrameSpellChecker spell_checker( | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -57,26 +57,34 @@ Sets the maximum and minimum pinch-to-zoom level. | |
|
||
Sets the maximum and minimum layout-based (i.e. non-visual) zoom level. | ||
|
||
### `webFrame.setSpellCheckProvider(language, autoCorrectWord, provider)` | ||
### `webFrame.setSpellCheckProvider(language, provider)` | ||
|
||
* `language` String | ||
* `autoCorrectWord` Boolean | ||
* `provider` Object | ||
* `spellCheck` Function - Returns `Boolean`. | ||
* `text` String | ||
* `spellCheck` Function. | ||
* `words` String[] | ||
* `callback` Function | ||
* `misspeltWords` String[] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. curious if callback accepts
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Currently it doesn't, but I can make changes to change the API to accept There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (just my 5c) it makes js callback doesn't need to maintain list of words to be returned but just apply fn then return it directly - i.e I could do similar like below in provider: // this is fn signature runs spellcheck in async and returns result
const isMisspelled: async (word) => boolean;
spellcheck: (words, callback) =>
Observable.from(words)
.mergeMap((w) => isMisspelled(w))
.toArray().subscribe(callback); when provider required to return array of misspelled words, provider fn need to re-map from result of spellcheck to construct list of misspelled words to be returned. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also sort of alignment (still it's breaking change), previously provider returns boolean for single words, now returns array of boolean for corresponding words. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That sounds reasonable. However, to do that, we will have to maintain the list of all words on the c++ side and then map the returned array to those to get the locations in the text to return back to blink. I was just trying to avoid running through all the words by using a map. So, it's definitely doable but I'm not sure how important it is. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. /cc @juturu |
||
|
||
Sets a provider for spell checking in input fields and text areas. | ||
|
||
The `provider` must be an object that has a `spellCheck` method that returns | ||
whether the word passed is correctly spelled. | ||
The `provider` must be an object that has a `spellCheck` method that accepts | ||
an array of individual words for spellchecking. | ||
The `spellCheck` function runs asynchronously and calls the `callback` function | ||
with an array of misspelt words when complete. | ||
|
||
An example of using [node-spellchecker][spellchecker] as provider: | ||
|
||
```javascript | ||
const { webFrame } = require('electron') | ||
webFrame.setSpellCheckProvider('en-US', true, { | ||
spellCheck (text) { | ||
return !(require('spellchecker').isMisspelled(text)) | ||
const spellChecker = require('spellchecker') | ||
webFrame.setSpellCheckProvider('en-US', { | ||
spellCheck (words, callback) { | ||
setTimeout(() => { | ||
const spellchecker = require('spellchecker') | ||
const misspelled = words.filter(x => spellchecker.isMisspelled(x)) | ||
callback(misspelled) | ||
}, 0) | ||
} | ||
}) | ||
``` | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is only called by blink when creating the context menu. Clients use electron APIs to create context menus, so we don't need to override this function.