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.

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

Merged
merged 8 commits into from Oct 18, 2018
Prev

Address review

  • Loading branch information...
nitsakh committed Oct 15, 2018
commit e6956b3b119ff1a50172f5221f03a32732c74793
@@ -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"
@@ -52,7 +52,7 @@ class SpellCheckClient::SpellcheckRequest {
}
~SpellcheckRequest() {}

base::string16& text() { return text_; }
const base::string16& text() const { return text_; }
blink::WebTextCheckingCompletion* completion() { return completion_; }
WordMap& wordmap() { return word_map_; }

@@ -146,35 +146,29 @@ void SpellCheckClient::SpellCheckText() {

SpellCheckScope scope(*this);
base::string16 word;
int word_start;
int word_length;
std::vector<base::string16> words;
auto& word_map = pending_request_param_->wordmap();

This comment has been minimized.

Copy link
@ckerr

ckerr Oct 4, 2018

Member

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

This comment has been minimized.

Copy link
@nitsakh

nitsakh Oct 14, 2018

Author Contributor

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.

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)) {
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;

// 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.
std::vector<base::string16> contraction_words;
if (!IsContraction(scope, word, &contraction_words)) {
blink::WebTextCheckingResult result;
result.location = word_start;
result.length = word_length;
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 misspelt
// 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.

This comment has been minimized.

Copy link
@ckerr

ckerr Oct 4, 2018

Member

^ This is a good idea!

for (const auto& w : contraction_words) {
blink::WebTextCheckingResult result;
result.location = word_start;
result.length = word_length;
words.push_back(w);
word_map[w].push_back(result);
}
@@ -186,15 +180,20 @@ void SpellCheckClient::SpellCheckText() {
}

void SpellCheckClient::OnSpellCheckDone(
const std::vector<base::string16>& misspelt_words) {
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();

for (const auto& word : misspelt_words) {
// 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;

This comment has been minimized.

Copy link
@ckerr

ckerr Oct 4, 2018

Member

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

This comment has been minimized.

Copy link
@nitsakh

nitsakh Oct 15, 2018

Author Contributor

Added a comment.

results.insert(results.end(), words.begin(), words.end());
words.clear();
@@ -5,7 +5,6 @@
#ifndef ATOM_RENDERER_API_ATOM_API_SPELL_CHECK_CLIENT_H_
#define ATOM_RENDERER_API_ATOM_API_SPELL_CHECK_CLIENT_H_

#include <map>
#include <memory>
#include <string>
#include <vector>
@@ -67,7 +66,7 @@ class SpellCheckClient : public blink::WebSpellCheckPanelHostClient,

// Call JavaScript to check spelling a word.
// The javascript function will callback OnSpellCheckDone
// with the results of all the misspelt words.
// with the results of all the misspelled words.
void SpellCheckWords(const SpellCheckScope& scope,
const std::vector<base::string16>& words);

@@ -79,8 +78,8 @@ class SpellCheckClient : public blink::WebSpellCheckPanelHostClient,
const base::string16& word,
std::vector<base::string16>* contraction_words);

This comment has been minimized.

Copy link
@ckerr

ckerr Oct 4, 2018

Member

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

This comment has been minimized.

Copy link
@nitsakh

nitsakh Oct 15, 2018

Author Contributor

Nope, changed.

This comment has been minimized.

Copy link
@nitsakh

nitsakh Oct 15, 2018

Author Contributor

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 misspelt words.
void OnSpellCheckDone(const std::vector<base::string16>& misspelt_words);
// 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.
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.