Skip to content
This repository has been archived by the owner on Dec 11, 2019. It is now read-only.

Using Chromium's spellchecker #9951

Merged
merged 3 commits into from
Jul 14, 2017
Merged

Using Chromium's spellchecker #9951

merged 3 commits into from
Jul 14, 2017

Conversation

darkdh
Copy link
Member

@darkdh darkdh commented Jul 11, 2017

Test Plan:

a. Suggestions

  1. Go to about:styles
  2. Type braave in textbox
  3. There should be redline under braave
  4. Open context menu on braave
  5. You can see bravo, brave, Brava
  6. Click on brave
  7. The text should be replaced with brave with no redline

b. Learn Spelling

  1. Go to about:styles
  2. Type braave in textbox
  3. There should be redline under braave
  4. Open context menu on braave
  5. Click Learn Spelling
  6. braave will no longer be redlined
  7. Next time you type braave, you will not see redline

c. Forget Learned Spelling

  1. Go to about:styles
  2. Type braave in textbox
  3. There should be redline under braave
  4. Open context menu on braave
  5. Click Learn Spelling
  6. braave will no longer be redlined
  7. Open context menu on braave
  8. Click Forget Learned Spelling
  9. Next time you type braave, you will see redline under it

d. Legacy data migration

  1. Use older Brave to Learn Spelling for braave and Ignore Spelling for braaave
  2. Update to the version with chromium spellchecker
  3. There will be no dictionary structure in session store
  4. You can see those two words will be added to Custom Dictionary.txt under user folder
  5. And try to type braave and braaave in about:styles
  6. They should be valid words

Auditors: @bridiver, @bbondy, @bsclifton

fixes #9880

requires brave/muon#244

Submitter Checklist:

  • Submitted a ticket for my issue if one did not already exist.
  • Used Github auto-closing keywords in the commit message.
  • Added/updated tests for this change (for new code or code which already has tests).
  • Ran git rebase -i to squash commits (if needed).
  • Tagged reviewers and labelled the pull request as needed.

Reviewer Checklist:

Tests

  • Adequate test coverage exists to prevent regressions
  • Tests should be independent and work correctly when run individually or as a suite ref
  • New files have MPL2 license header

@darkdh
Copy link
Member Author

darkdh commented Jul 11, 2017

working on migrating old custom dictionary data

@darkdh darkdh changed the title Using Chromium's spellchecker [WIP] Using Chromium's spellchecker Jul 11, 2017
@@ -266,6 +266,7 @@ const createViewSubmenu = () => {
label: locale.translation('toggleDeveloperTools'),
accelerator: isDarwin ? 'Cmd+Alt+I' : 'Ctrl+Shift+I',
click: function () {
const appStore = require('../../js/stores/appStore')
Copy link
Contributor

Choose a reason for hiding this comment

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

This was already fixed, so if you rebase to the master, you can remove this code 😉

Copy link
Member Author

Choose a reason for hiding this comment

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

ah, I forgot to remove the workaround. thanks

@darkdh darkdh force-pushed the cr-spellchecker branch 2 times, most recently from e394659 to 309e703 Compare July 11, 2017 05:53
@@ -77,7 +77,6 @@ const messages = {
RELOAD: _,
DETACH: _,
IS_MISSPELLED: _, /** @arg {string} word, the word to check */
Copy link
Collaborator

Choose a reason for hiding this comment

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

this should also be removed

@@ -35,13 +35,6 @@ module.exports.init = () => {
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

doesn't this whole file go away?

Copy link
Member Author

Choose a reason for hiding this comment

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

😅 yeah, I forgot to remove

webviewActions.replace(selection)
click: (item, focusedWindow) => {
if (focusedWindow) {
focusedWindow.webContents.addWord(selection)
Copy link
Collaborator

Choose a reason for hiding this comment

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

focusedWindow should be deprecated and context menu items should use actions by default. I would use something like appActions.learnSpelling and appActions.forgetLearnedSpelling and pass the tabId (always set in muon HandleContextMenu and retrieved from nodeProps.properties)

@darkdh darkdh force-pushed the cr-spellchecker branch from f233f7c to 0e8f45a Compare July 11, 2017 23:09
@darkdh darkdh added this to the 0.19.x (Developer Channel) milestone Jul 12, 2017
@darkdh darkdh changed the title [WIP] Using Chromium's spellchecker Using Chromium's spellchecker Jul 13, 2017
@darkdh darkdh requested review from bridiver, bbondy and bsclifton July 13, 2017 00:25
const {getWebContents} = require('../webContentsCache')
const spellChecker = require('../../spellChecker')

const migrate = (state) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

the other data migrations are in session store. We don't want it to get too bloated, but if we don't remove dictionary from the saved state this migration will run on every restart

@bsclifton
Copy link
Member

Will check this out once Muon 4.4.0 prebuilt is ready to go 😄

Copy link
Collaborator

@bridiver bridiver left a comment

Choose a reason for hiding this comment

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

see comment

@darkdh darkdh requested a review from bridiver July 13, 2017 20:26
Copy link
Collaborator

@bridiver bridiver left a comment

Choose a reason for hiding this comment

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

++

@darkdh darkdh force-pushed the cr-spellchecker branch from e5b7aed to 04b1288 Compare July 14, 2017 16:48
Copy link
Member

@bsclifton bsclifton left a comment

Choose a reason for hiding this comment

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

I'm having issues with this; I built locally (macOS), ensured I am using Muon 4.4.0... the wrong spelling (ex: braave) does get underlined red. However, picking from the drop down does not work (both correct spelling and learn spelling)

@darkdh darkdh force-pushed the cr-spellchecker branch from 04b1288 to 4bcde36 Compare July 14, 2017 22:30
darkdh and others added 3 commits July 14, 2017 16:04
fixes #9880

Auditors: @bridiver, @bbondy, @bsclifton

Test plan:
a. Suggestions

1. Go to `about:styles`
2. Type `braave` in textbox
3. There should be redline under `braave`
4. Open context menu on `braave`
5. You can see `bravo`, `brave`, `Brava`
6. Click on `brave`
7. The text should be replaced with `brave` with no redline

b. Learn Spelling
1. Go to `about:styles`
2. Type `braave` in textbox
3. There should be redline under `braave`
4. Open context menu on `braave`
5. Click `Learn Spelling`
6. `braave` will no longer be redlined
7.  Next time you type `braave`, you will not see redline

c. Forget Learned Spelling
1. Go to `about:styles`
2. Type `braave` in textbox
3. There should be redline under `braave`
4. Open context menu on `braave`
5. Click `Learn Spelling`
6. `braave` will no longer be redlined
7. Open context menu on `braave`
8. Click `Forget Learned Spelling`
9.  Next time you type `braave`, you will see redline under it

d. Legacy data migration
1. Use older Brave to `Learn Spelling` for `braave` and `Ignore
Spelling` for `braaave`
2. Update to the version with chromium spellchecker
3. There will be no `dictionary` structure in session store
4. You can see those two words will be added to `Custom Dictionary.txt`
under user folder
5. And try to type `braave` and `braaave` in `about:styles`
6. They should be valid words
Auditors: @darkdh

Test Plan: `npm run unittest -- --grep="spellCheckerReducer unit tests"`
Copy link
Member

@bsclifton bsclifton left a comment

Choose a reason for hiding this comment

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

Found issues with immutable that we fixed together 😄 I added unit tests for the spellCheckerReducer to help prevent that in the future. Also, I found/fixed a context menu test which failed because the code goes down a previous branch than before

Copy link
Member Author

@darkdh darkdh left a comment

Choose a reason for hiding this comment

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

++

// are incorrectly marked as spelling errors. This lets people get away with
// incorrectly spelled contracted words, but it's the best we can do for now.
const contractions = [
"ain't", "aren't", "can't", "could've", "couldn't", "couldn't've", "didn't", "doesn't", "don't", "hadn't",
Copy link
Member

Choose a reason for hiding this comment

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

pfft new spell checker ain't got these words, y'all

@bsclifton bsclifton merged commit 8ccbbdb into master Jul 14, 2017
@bsclifton bsclifton deleted the cr-spellchecker branch July 14, 2017 23:24
bsclifton added a commit that referenced this pull request Jul 14, 2017
Using Chromium's spellchecker
@bsclifton bsclifton modified the milestones: 0.18.x (Beta Channel), 0.19.x (Developer Channel) Jul 23, 2017
bsclifton added a commit that referenced this pull request Jul 23, 2017
Using Chromium's spellchecker
@Jacalz
Copy link
Contributor

Jacalz commented Jul 23, 2017

This fixed #7967 thank you guys so so much 👍

@luixxiul
Copy link
Contributor

@darkdh does this also close #2183 and #4697?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use Chromium spell checker
6 participants