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

feat: Spellchecker Async Implementation #14032

Merged
merged 8 commits into from Oct 18, 2018
Merged

feat: Spellchecker Async Implementation #14032

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
42caab2
feat:Spellchecker Async Implementation
nitsakh Aug 12, 2018
ebe1cbc
Adhere to chromium style
nitsakh Sep 20, 2018
048735f
Updating dependency to use gh branch
nitsakh Sep 20, 2018
b3d5331
Update docs and electron-typescript-definitions module
nitsakh Sep 23, 2018
927bb2e
Fix lint
nitsakh Sep 23, 2018
20df488
Update electron typescript definitions version
nitsakh Sep 24, 2018
c46860a
Update spec
nitsakh Oct 3, 2018
e6956b3
Address review
nitsakh Oct 15, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
152 changes: 83 additions & 69 deletions atom/renderer/api/atom_api_spell_check_client.cc
View file
Open in desktop
Expand Up @@ -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"
Expand All @@ -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"
Expand Down Expand Up @@ -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());
Expand All @@ -79,19 +85,6 @@ SpellCheckClient::~SpellCheckClient() {
context_.Reset();
}

void SpellCheckClient::CheckSpelling(
const blink::WebString& text,
Copy link
Contributor Author

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.

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) {
Expand All @@ -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 {
Expand All @@ -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)) {
Expand All @@ -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();
Copy link
Member

Choose a reason for hiding this comment

The 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 .clear() this out?

I think the answer is "no" but am not positive

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 DidFinishCheckingText on blink, which happens at the end of this function.

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.
Copy link
Member

Choose a reason for hiding this comment

The 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;
Copy link
Member

Choose a reason for hiding this comment

The 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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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;
Expand All @@ -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(
Expand Down
33 changes: 16 additions & 17 deletions atom/renderer/api/atom_api_spell_check_client.h
View file
Open in desktop
Expand Up @@ -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;
Expand All @@ -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);
Copy link
Member

Choose a reason for hiding this comment

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

Any reason contraction_words is a pointer instead of a reference here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope, changed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ohh, just saw. The linter tells to make this a pointer.
Is this a non-const reference? If so, make const or use a pointer: std::vector<base::string16>& contraction_words [runtime/references] [2]


// 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.
Expand Down
5 changes: 2 additions & 3 deletions atom/renderer/api/atom_api_web_frame.cc
View file
Open in desktop
Expand Up @@ -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,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

The 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:

webFrame.setSpellCheckProvider(locale, true, provider);

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

The 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(
Expand Down
1 change: 0 additions & 1 deletion atom/renderer/api/atom_api_web_frame.h
View file
Open in desktop
Expand Up @@ -58,7 +58,6 @@ class WebFrame : public mate::Wrappable<WebFrame> {
// Set the provider that will be used by SpellCheckClient for spell check.
void SetSpellCheckProvider(mate::Arguments* args,
const std::string& language,
bool auto_spell_correct_turned_on,
v8::Local<v8::Object> provider);

void RegisterURLSchemeAsBypassingCSP(const std::string& scheme);
Expand Down
26 changes: 17 additions & 9 deletions docs/api/web-frame.md
View file
Open in desktop
Expand Up @@ -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[]
Copy link
Contributor

Choose a reason for hiding this comment

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

curious if callback accepts Array<boolean> instead of returning word itself. i.e,

spellcheck: (words, callback) => {
 // do some async
 callback(words.map(isMisspelled));
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 Array<boolean> if we want.
What would be the benefit of doing that?

Copy link
Contributor

@kwonoj kwonoj Aug 12, 2018 •
edited

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.
Also, APIwise returning misspelt words seems better than an array of booleans. But that's just me. 😃
I'd like to see if others have any opinions about this. I'm okay going either way!

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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)
}
})
```
Expand Down