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: enable builtin spellchecker #20692

Merged
merged 25 commits into from Oct 31, 2019

Conversation

@MarshallOfSound
Copy link
Member

MarshallOfSound commented Oct 22, 2019

This does all the work to get the spellchecker working on all supported platforms. Testing matrix I'm using is outlined below:

  • Windows 10
  • Windows 8.1
  • Windows 7
  • Ubuntu 18.04
  • Fedora
  • RHEL
  • macOS 10.14
  • macOS 10.15

This is behind a build flag that is now enabled by default. The addition of this introduces a few new APIs.

session.setSpellCheckerLanguages(languageCodes: string[])

This set the required pref that tells the spellchecker which languages it should use, example usage is. session.setSpellCheckLanguages(['en-US', 'fr']) to enable french and american english. This only impacts hunspell based platforms.

session.availableSpellCheckerLanguages: string[] (Readonly)

This is a read only property with an array of supported language codes that you can provide to the set method above.

session.setSpellCheckerDictionaryDownloadURL(url: string)

This is a writeable property that allows you to modify the base URL that Electron will use to download hunspell dictionaries from. By default we will fetch them from the chromium provided CDN, if you don't want your app to do that you can host these yourself and modify this URL here.

For more info on these new APIs check out the new documentation.

Releasing

There is also a new published asset hunspell_dictionaries.zip which is published once (identical across platforms) that includes all the dictionary files and their licenses that you would need to host if you wanted to override the dictionarySourceURL property.

Existing APIs?

Personally I think we should remove the webFrame.setSpellChecker API as this supersedes in every way it but I'm open to other thoughts there. For now I've added a spellcheck option in webPreferences to enable / disable the spellchecker. While we make sure this is gonna work for everyone we can keep both code paths with the intention to deprecate and remove the webFrame method at some point in the future.

Notes: Added support for the built-in spellchecker. We will use the OS spellchecker on macOS and hunspell on all other platforms.

@MarshallOfSound MarshallOfSound requested a review from electron/wg-upgrades as a code owner Oct 23, 2019
@electron-cation electron-cation bot removed the new-pr 🌱 label Oct 23, 2019
@MarshallOfSound MarshallOfSound force-pushed the spellcher-why-not branch from 2e83e4b to ce4ddc9 Oct 24, 2019
@MarshallOfSound MarshallOfSound requested a review from electron/wg-releases as a code owner Oct 25, 2019
@MarshallOfSound MarshallOfSound force-pushed the spellcher-why-not branch from a050fef to 7a704bb Oct 25, 2019
@MarshallOfSound MarshallOfSound changed the title chore: add code required to use chromes spellchecker feat: enable builtin spellchecker Oct 25, 2019
docs/api/session.md Outdated Show resolved Hide resolved
Copy link
Member

codebytere left a comment

lgtm code-wise as is :)

Copy link
Member

deepak1556 left a comment

LGTM with some style changes.

.circleci/config.yml Outdated Show resolved Hide resolved
BUILD.gn Outdated Show resolved Hide resolved
BUILD.gn Outdated Show resolved Hide resolved
BUILD.gn Outdated Show resolved Hide resolved
BUILD.gn Outdated Show resolved Hide resolved
shell/browser/api/atom_api_app.cc Show resolved Hide resolved
shell/browser/api/atom_api_session.cc Show resolved Hide resolved
shell/browser/api/atom_api_session.cc Show resolved Hide resolved
shell/app/manifests.cc Outdated Show resolved Hide resolved
@miniak

This comment has been minimized.

Copy link
Contributor

miniak commented Oct 31, 2019

@MarshallOfSound how do you do spelling suggestions in the context menu? This was handled by getting those from hunspell directly in the context-menu event before.

@MarshallOfSound

This comment has been minimized.

Copy link
Member Author

MarshallOfSound commented Oct 31, 2019

@miniak Spelling suggestions are exposed in the context-menu event. Looks like I didn't document that bit, I'll do that tomorrow 👍

https://github.com/electron/electron/pull/20692/files#diff-36e0f46c12e6e923ad35682f90b84857R142

shell/app/manifests.h Outdated Show resolved Hide resolved
BUILD.gn Outdated Show resolved Hide resolved
BUILD.gn Outdated Show resolved Hide resolved
Copy link
Member

deepak1556 left a comment

LGTM with minor corrections and once ci passes

@MarshallOfSound MarshallOfSound merged commit 6bcf67e into master Oct 31, 2019
10 of 15 checks passed
10 of 15 checks passed
appveyor: win-ia32-testing Waiting for AppVeyor build to complete
Details
appveyor: win-woa-testing Waiting for AppVeyor build to complete
Details
appveyor: win-x64-testing Waiting for AppVeyor build to complete
Details
Artifact Comparison Changes Detected
Details
Backportable? - 8-x-y Backport Failed
Details
Semantic Pull Request ready to be squashed
Details
WIP Ready for review
Details
appveyor: win-ia32-testing-pr AppVeyor build succeeded
Details
appveyor: win-x64-testing-pr AppVeyor build succeeded
Details
build-linux Workflow: build-linux
Details
build-mac Workflow: build-mac
Details
electron-arm-testing Build #20191031.55 succeeded
Details
electron-arm64-testing Build #20191031.55 succeeded
Details
lint Workflow: lint
Details
release-notes Release notes found
@release-clerk

This comment has been minimized.

Copy link

release-clerk bot commented Oct 31, 2019

Release Notes Persisted

Added support for the built-in spellchecker. We will use the OS spellchecker on macOS and hunspell on all other platforms.

@MarshallOfSound MarshallOfSound deleted the spellcher-why-not branch Oct 31, 2019
@trop

This comment has been minimized.

Copy link
Contributor

trop bot commented Oct 31, 2019

I was unable to backport this PR to "8-x-y" cleanly;
you will need to perform this backport manually.

@trop trop bot added needs-manual-bp/8-x-y and removed target/8-x-y labels Oct 31, 2019
MarshallOfSound added a commit that referenced this pull request Oct 31, 2019
* chore: add code required to use chromes spellchecker

* chore: fix linting

* chore: manifests needs buildflags now

* chore: add dictionarySuggestions to the context menu event when the spellchecker is active

* chore: enable by default for windows builds

* chore: add patch to remove incognito usage in the spellchecker

* chore: add dependencies on spellcheck common and flags

* chore: conditionally include spell check panel impl

* chore: fix deps for spellcheck feature flags

* chore: add patch for electron resources

* chore: add dependency on //components/language/core/browser

* chore: patches to make hunspell work on windows

* build: collect hunspell dictionaries into a zip file and publish

* chore: clean up patches

* chore: add docs and set spell checker url method

* chore: fix error handling

* chore: fix hash logic

* build: update hunspell filename generator

* fix: default spellchecker list to the current system locale if we can

* docs: document the language getter

* chore: patch IDS_ resources for linux builds

* feat: add spellcheck webpref flag to disable the builtin spellchecker

* chore: fix docs typo

* chore: clean up spellchecker impl as per feedback

* remove unneeded deps
@trop

This comment has been minimized.

Copy link
Contributor

trop bot commented Oct 31, 2019

@MarshallOfSound has manually backported this PR to "8-x-y", please check out #20897

MarshallOfSound added a commit that referenced this pull request Oct 31, 2019
* feat: enable builtin spellchecker (#20692)

* chore: add code required to use chromes spellchecker

* chore: fix linting

* chore: manifests needs buildflags now

* chore: add dictionarySuggestions to the context menu event when the spellchecker is active

* chore: enable by default for windows builds

* chore: add patch to remove incognito usage in the spellchecker

* chore: add dependencies on spellcheck common and flags

* chore: conditionally include spell check panel impl

* chore: fix deps for spellcheck feature flags

* chore: add patch for electron resources

* chore: add dependency on //components/language/core/browser

* chore: patches to make hunspell work on windows

* build: collect hunspell dictionaries into a zip file and publish

* chore: clean up patches

* chore: add docs and set spell checker url method

* chore: fix error handling

* chore: fix hash logic

* build: update hunspell filename generator

* fix: default spellchecker list to the current system locale if we can

* docs: document the language getter

* chore: patch IDS_ resources for linux builds

* feat: add spellcheck webpref flag to disable the builtin spellchecker

* chore: fix docs typo

* chore: clean up spellchecker impl as per feedback

* remove unneeded deps

* chore: disable spellcheck by default in web prefs
@trop trop bot added merged/8-x-y and removed in-flight/8-x-y labels Oct 31, 2019
@astoilkov

This comment has been minimized.

Copy link
Contributor

astoilkov commented Nov 1, 2019

I was very happily surprised to see Electron doing significant improvements to the spellchecker. Congrats on the work!

@MarshallOfSound I have a quick question related to the future of spellchecking in Electron. Do you see a path to deprecating the need for node-spellchecker?

We are building a text editor where we can't use the spellchecking of HTML elements because we need a more advanced implementation. We need methods to manually check if a text is misspelled.

@miniak miniak referenced this pull request Nov 3, 2019
2 of 6 tasks complete
@CvX CvX referenced this pull request Nov 3, 2019
@sofianguy sofianguy added this to Fixed in 8.0.0-beta.2 in 8.0.x Nov 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
8.0.x
Fixed in 8.0.0-beta.2
6 participants
You can’t perform that action at this time.