feat: Spellchecker Async Implementation #14032

nitsakh · 2018-08-12T01:30:41Z

This PR makes the spellchecker provider function async. This enables the client to process the incoming spellcheck words and respond through a callback once the checking is done.

The API, which initially accepted only one word at a time, now takes in an array of words to be checked. This reduces the cross language calls for spellchecking the text and behaves in a true asynchronous way, that blink expects.

More comments inline.

/cc @kwonoj

Checklist

PR description included and stakeholders cc'd
npm test passes
tests are changed or added
relevant documentation is changed or added
PR title follows semantic commit guidelines

Notes: Updated SpellCheck API to support asynchronous results.

nitsakh · 2018-08-12T02:18:41Z

atom/renderer/api/atom_api_spell_check_client.cc

@@ -79,19 +85,6 @@ SpellCheckClient::~SpellCheckClient() {
  context_.Reset();
 }

-void SpellCheckClient::CheckSpelling(
-    const blink::WebString& text,


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.

nitsakh · 2018-08-12T02:20:13Z

atom/renderer/api/atom_api_web_frame.cc

@@ -188,15 +188,14 @@ void WebFrame::DetachGuest(int id) {

 void WebFrame::SetSpellCheckProvider(mate::Arguments* args,
                                     const std::string& language,
-                                     bool auto_spell_correct_turned_on,


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.

Is this a breaking change? Our app currently has this code which seems like it will now fail:

webFrame.setSpellCheckProvider(locale, true, provider);

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"

Never mind, I see this has been raised elsewhere #17915

nitsakh · 2018-08-12T02:22:04Z

The linter step is failing to create the typescript definitions. Looks like I'll have to update https://github.com/electron/electron-typescript-definitions/blob/eaa478d08a30cf7c721360f478aedf81c7dd2843/test-smoke/electron/test/renderer.ts#L63.
What's the best way to do that without breaking builds for others?

MarshallOfSound · 2018-08-12T03:59:01Z

@nitsakh Breaking changes are hard on that generator. Best bet would be to update those smoke tests on a branch, then make the reference to the module in our package.json point at that branch.

Then when we merge this PR we'll do a major release of the generator

kwonoj · 2018-08-12T06:48:45Z

docs/api/web-frame.md

+  * `spellCheck` Function.
+    * `words` String[]
+    * `callback` Function
+      * `misspeltWords` String[]


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

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

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?

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

Also sort of alignment (still it's breaking change), previously provider returns boolean for single words, now returns array of boolean for corresponding words.

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!

/cc @juturu

ckerr · 2018-10-04T15:43:13Z

atom/renderer/api/atom_api_spell_check_client.cc

  SpellcheckRequest(const base::string16& text,
                    blink::WebTextCheckingCompletion* completion)
      : text_(text), completion_(completion) {
    DCHECK(completion);
  }
  ~SpellcheckRequest() {}

-  base::string16 text() { return text_; }
+  base::string16& text() { return text_; }


If we're returning a reference, should be a const reference.

Also (not new to this PR) the method should be const.

So const base::string16& text() const { return text_; }

ckerr · 2018-10-04T15:47:18Z

atom/renderer/api/atom_api_spell_check_client.cc

+      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


(opinion) let's use 'misspelled' everywhere instead of 'misspelt'. The former is what the existing API uses and has about 25x more Google hits than the latter does

ckerr · 2018-10-04T15:57:05Z

atom/renderer/api/atom_api_spell_check_client.cc

@@ -155,62 +148,93 @@ void SpellCheckClient::SpellCheckText(
  base::string16 word;
  int word_start;
  int word_length;
+  std::vector<base::string16> words;
+  auto& word_map = pending_request_param_->wordmap();
  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)) {


We could replace word_start and word_length with a blink::WebTextCheckingResult that we can use below pre-populated:

blink::WebTextCheckingResult result; std::vector<base::string16> words; ... GetNextWord(&word, &result.location, &result.length)

ckerr · 2018-10-04T16:10:17Z

atom/renderer/api/atom_api_spell_check_client.cc

@@ -155,62 +148,93 @@ void SpellCheckClient::SpellCheckText(
  base::string16 word;
  int word_start;
  int word_length;
+  std::vector<base::string16> words;
+  auto& word_map = pending_request_param_->wordmap();
  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)) {


(opinion)

Those two calls to GetNextWord() in the loop structure are kind of cumbersome. This would be less repetitive:

for (;;) { const auto status = text_iterator_.GetNextWord(...); if (status == SpellcheckWordIterator::IS_END_OF_TEXT) break; if (status == SpellcheckWordIterator::IS_SKIPPABLE) continue;

ckerr · 2018-10-04T16:10:53Z

atom/renderer/api/atom_api_spell_check_client.cc

+      // For a contraction, we want check the spellings of each individual
+      // part, but mark the entire word incorrect if any part is misspelt
+      // Hence, we use the same word_start and word_length values for every
+      // part of the contraction.


^ This is a good idea!

ckerr · 2018-10-04T16:12:17Z

atom/renderer/api/atom_api_spell_check_client.cc

@@ -155,62 +148,93 @@ void SpellCheckClient::SpellCheckText(
  base::string16 word;
  int word_start;
  int word_length;
+  std::vector<base::string16> words;
+  auto& word_map = pending_request_param_->wordmap();


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

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.

ckerr · 2018-10-04T16:22:19Z

atom/renderer/api/atom_api_spell_check_client.h

@@ -5,6 +5,7 @@
 #ifndef ATOM_RENDERER_API_ATOM_API_SPELL_CHECK_CLIENT_H_
 #define ATOM_RENDERER_API_ATOM_API_SPELL_CHECK_CLIENT_H_

+#include <map>


There's no std::map in this header, so this shouldn't be #included here

Updated, was leftover from my previous test implementation.

ckerr · 2018-10-04T16:25:09Z

atom/renderer/api/atom_api_spell_check_client.h

+  // words in the contraction.
+  bool IsContraction(const SpellCheckScope& scope,
+                     const base::string16& word,
+                     std::vector<base::string16>* contraction_words);


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

Nope, changed.

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]

ckerr · 2018-10-04T16:35:55Z

atom/renderer/api/atom_api_spell_check_client.cc

+  for (const auto& word : misspelt_words) {
+    auto iter = word_map.find(word);
+    if (iter != word_map.end()) {
+      auto& words = iter->second;


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

Added a comment.

codebytere · 2018-10-14T18:21:22Z

@nitsakh there's a bit more review changes to be done here but then i think we can finally get this in 🎉

nitsakh · 2018-10-15T15:34:02Z

Thanks a lot for the review comments @ckerr ! 🙇

codebytere

lgtm!

ckerr

Looks good!

release-clerk · 2018-10-18T16:11:56Z

Release Notes Persisted

Updated SpellCheck API to support asynchronous results.

Starting with Electron 5.0.0, the function `webFrame.setSpellCheckProvider` has a different signature, in order to support asynchronous spell checkers. For more information on this change in Electron, see electron/electron#14032. This diff updates `SpellCheckHandler` to use the new interface if running on Electron 5.0.0 and above. However, it still checks the spelling synchronously like before. If running on Electron versions below 5.0.0, the `SpellCheckHandler` still works. Closes electron-userland#144.

nitsakh requested review from a team August 12, 2018 01:30

nitsakh force-pushed the spellcheck-async branch from 0d5849d to f492da3 Compare August 12, 2018 01:34

nitsakh commented Aug 12, 2018

View reviewed changes

nitsakh requested a review from kwonoj August 12, 2018 02:26

kwonoj reviewed Aug 12, 2018

View reviewed changes

juturu requested a review from john-hern August 13, 2018 22:19

nitsakh mentioned this pull request Sep 20, 2018

fix: Update spellcheck API in smoketest electron/typescript-definitions#116

Merged

nitsakh force-pushed the spellcheck-async branch 2 times, most recently from 734f310 to 2692ec1 Compare September 23, 2018 22:59

nitsakh force-pushed the spellcheck-async branch 2 times, most recently from 36927d8 to fe4edc9 Compare October 3, 2018 15:22

ckerr reviewed Oct 4, 2018

View reviewed changes

nitsakh added 8 commits October 14, 2018 16:33

feat:Spellchecker Async Implementation

42caab2

Adhere to chromium style

ebe1cbc

Updating dependency to use gh branch

048735f

Update docs and electron-typescript-definitions module

b3d5331

Fix lint

927bb2e

Update electron typescript definitions version

20df488

Update spec

c46860a

Address review

e6956b3

nitsakh force-pushed the spellcheck-async branch from fe4edc9 to e6956b3 Compare October 15, 2018 01:08

codebytere approved these changes Oct 18, 2018

View reviewed changes

ckerr approved these changes Oct 18, 2018

View reviewed changes

ckerr changed the title ~~feat:Spellchecker Async Implementation~~ feat: Spellchecker Async Implementation Oct 18, 2018

ckerr merged commit a9ca152 into master Oct 18, 2018

ckerr deleted the spellcheck-async branch October 18, 2018 16:20

kwonoj mentioned this pull request Oct 19, 2018

Asynchronous spellchecker support kwonoj/electron-hunspell#218

Closed

chrismohr mentioned this pull request Jan 2, 2019

New setSpellCheckProvider api #16237

Closed

magne4000 mentioned this pull request Apr 23, 2019

docs: Update breaking changes on webFrame.setSpellCheckProvider #17915

Merged

4 tasks

mlalkaka mentioned this pull request Jun 11, 2019

Add support for Electron 5 and above electron-userland/electron-spellchecker#149

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Spellchecker Async Implementation #14032

feat: Spellchecker Async Implementation #14032

nitsakh commented Aug 12, 2018 •

edited

Loading

nitsakh Aug 12, 2018

nitsakh Aug 12, 2018

jwheare May 8, 2019

jwheare May 8, 2019

jwheare May 8, 2019

nitsakh commented Aug 12, 2018

MarshallOfSound commented Aug 12, 2018

kwonoj Aug 12, 2018

nitsakh Aug 12, 2018

kwonoj Aug 12, 2018 •

edited

Loading

kwonoj Aug 12, 2018

nitsakh Aug 13, 2018

nitsakh Aug 13, 2018

ckerr Oct 4, 2018

ckerr Oct 4, 2018

ckerr Oct 4, 2018

ckerr Oct 4, 2018

ckerr Oct 4, 2018

ckerr Oct 4, 2018

nitsakh Oct 14, 2018

ckerr Oct 4, 2018

nitsakh Oct 15, 2018 •

edited

Loading

ckerr Oct 4, 2018

nitsakh Oct 15, 2018

nitsakh Oct 15, 2018

ckerr Oct 4, 2018

nitsakh Oct 15, 2018

codebytere commented Oct 14, 2018

nitsakh commented Oct 15, 2018

codebytere left a comment

ckerr left a comment

release-clerk bot commented Oct 18, 2018

feat: Spellchecker Async Implementation #14032

feat: Spellchecker Async Implementation #14032

Conversation

nitsakh commented Aug 12, 2018 • edited Loading

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nitsakh commented Aug 12, 2018

MarshallOfSound commented Aug 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kwonoj Aug 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nitsakh Oct 15, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codebytere commented Oct 14, 2018

nitsakh commented Oct 15, 2018

codebytere left a comment

Choose a reason for hiding this comment

ckerr left a comment

Choose a reason for hiding this comment

release-clerk bot commented Oct 18, 2018

nitsakh commented Aug 12, 2018 •

edited

Loading

kwonoj Aug 12, 2018 •

edited

Loading

nitsakh Oct 15, 2018 •

edited

Loading