GH-1085 Replace simple-spellchecker with electron-spellchecker #1128

keithcruz · 2019-12-11T17:29:42Z

Before submitting, please confirm you've

read and understood our Contributing Guidelines
completed Mattermost Contributor Agreement
executed npm run lint:js for proper code formatting

Please provide the following information:

Summary

This is a WIP that replaces simple-spellchecker with electron-spellchecker.

Issue link

GH-1085
Test Cases

Additional Notes
Some initial findings:

Language detection doesn’t seem to work on Mac. I have tried by setting and not setting the initial language and then proceeding to type in Spanish and it never seems to switch or catch on. I have found a few issues in the repo that indicate that this may be an ongoing issue. Some whacky macOS logic in need of ❤? electron-userland/electron-spellchecker#127 . In the current version of mattermost spellcheck you are able to switch language from the context menu. Electron-spellcheck does not have this behavior built in. Further investigation is required to determine if it would be possible to add this capability or if it would be better to just add to the settings an option to select a language if on mac.
Electron-builder and electron-rebuild seem to have issues building native node modules with the two package.json structure. It may be worthwhile to see if a single package.json structure will be better long term. What's the difference between the --which-module and --only options? electron/rebuild#253 (comment)

…directly to package.json

keithcruz · 2019-12-11T17:59:21Z

@deanwhillier Next step will be to see if adding language switching via context menu is possible or not too much of a hack 😁 . If not would like to get input on whether or not the team would like to add language selection as a setting or to keep the current implementation with simple-spellchecker. It seems being able to switch languages automatically is best, via context menu next best, and via a setting as the least desirable option but that is a personal opinion and would like feedback on whether the benefits of electron-spellcheck outweigh the slight regression.

wget · 2019-12-15T22:19:25Z

@keithcruz +1 for only one package.json data structure.

This has been discussed one year ago with yuya-oc who didn't have time to implement such a refactorisation. If you think this will help, please go for it =)

mattermod · 2019-12-26T00:00:08Z

This issue has been automatically labelled "stale" because it hasn't had recent activity.
A core team member will check in on the status of the PR to help with questions.
Thank you for your contribution!

/cc @jasonblais @hanzei

keithcruz · 2020-01-06T03:36:11Z

@deanwhillier @jasonblais apologies for the lack of activity, I will have an update on this soon. Thanks.

deanwhillier · 2020-01-09T16:39:45Z

@keithcruz, I'm open to automatic language switching, but wonder if a backup setting would be good in case someone really wants a different language than their OS. I don't think language selection from the context menu is necessary at all! Open to others thoughts on this!

@devinbinnie, @Willyfrog, can you guys please take some time over the next bit to start reviewing this PR?

Willyfrog

I see the context menu has changed, I think it probably improved, but we might want to have a PM/UX review that.

Regarding language detection we should probably try and take advantage of provideHintText if we can

I suggested to have a setting for the default language of the user (or revert to the original option of having the context menu on it), but I'm open to alternatives. while I like to have autodetection, we should also have a way for the user to select by themselves in case it fails (and maybe users might want to turn autodetect off)

Willyfrog · 2020-01-10T11:39:46Z

src/browser/components/MattermostView.jsx

@@ -154,18 +152,6 @@ export default class MattermostView extends React.Component {
        webview.blur();
        webview.focus();
      }
-      if (!this.state.isContextMenuAdded) {


removing this would remove any menu associated?

Willyfrog · 2020-01-10T11:55:31Z

src/browser/webview/mattermost.js

+  window.spellCheckHandler = new SpellCheckHandler();
+  window.spellCheckHandler.attachToInput();
+
+  window.spellCheckHandler.switchLanguage('en-US');


I'm guessing this is not final, right?

Willyfrog · 2020-01-10T11:56:33Z

src/browser/webview/mattermost.js

+  window.spellCheckHandler.switchLanguage('en-US');
+
+  const contextMenuBuilder = new ContextMenuBuilder(window.spellCheckHandler);
+  /* eslint-disable-next-line no-unused-vars */


is there a reason to create a listener and not attaching it somewhere?

Willyfrog · 2020-01-10T11:57:37Z

src/package.json

    "auto-launch": "^5.0.5",
    "bootstrap": "^3.3.7",
+    "cld": "^2.5.1",
    "electron-context-menu": "^0.15.0",


if we are not using this anymore, it should be removed too.

Willyfrog · 2020-01-10T12:48:30Z

src/browser/webview/mattermost.js

@@ -45,6 +46,17 @@ window.addEventListener('load', () => {
  watchReactAppUntilInitialized(() => {
    ipcRenderer.sendToHost('onGuestInitialized', window.basename);
  });
+
+  window.spellCheckHandler = new SpellCheckHandler();
+  window.spellCheckHandler.attachToInput();


we should consider if config.useSpellChecker is set

Willyfrog · 2020-01-10T12:49:26Z

src/browser/webview/mattermost.js

+
+  const contextMenuBuilder = new ContextMenuBuilder(window.spellCheckHandler);
+  /* eslint-disable-next-line no-unused-vars */
+  const contextMenuListener = new ContextMenuListener((info) => {


are we going to use this? we shouldn't disable eslint warnings/errors unless we have a very good reason to do so.

Willyfrog · 2020-01-10T12:50:10Z

src/browser/webview/mattermost.js

+  window.spellCheckHandler = new SpellCheckHandler();
+  window.spellCheckHandler.attachToInput();
+
+  window.spellCheckHandler.switchLanguage('en-US');


we should add a configurable default on the settings page and use that as the base

mattermod · 2020-01-21T00:00:06Z

This issue has been automatically labelled "stale" because it hasn't had recent activity.
A core team member will check in on the status of the PR to help with questions.
Thank you for your contribution!

/cc @jasonblais @hanzei

Willyfrog · 2020-01-21T09:53:18Z

@keithcruz let me know if you need clarification or any help with this

deanwhillier · 2020-07-23T17:11:04Z

@Willyfrog, given we are looking into the new Spellchecker that comes with Electron v8 (or v7, can't remember exactly off hand), should we wait to determine if it will work as we need before abandoning this approach?

Willyfrog · 2020-07-23T17:13:35Z

definitely, my initial tests shows that it works as basic spellchecker, but I have yet to test the "add new spelling" feature, which requires using browserview, as it is difficult to add to a webview tag.

deanwhillier · 2020-08-13T14:27:12Z

Closing as we are moving towards using the integrated spell checker in Electron 8.

keithcruz added 3 commits December 9, 2019 21:26

initial removal of simple-spellchecker

8c6517a

get electron-spellchecker to work by adding some of its dependencies …

91e4f5f

…directly to package.json

disable rule for electron-spellcheck setup

09df2af

deanwhillier added the Work In Progress Not yet ready for review label Dec 11, 2019

keithcruz added 2 commits December 13, 2019 14:35

fix wine-chrome build

4ff379b

fix the rest of the builds

f107a87

mattermod added the Lifecycle/1:stale label Dec 26, 2019

jasonblais requested a review from deanwhillier December 26, 2019 19:40

jasonblais added Lifecycle/frozen and removed Lifecycle/1:stale labels Dec 26, 2019

deanwhillier requested review from Willyfrog and devinbinnie January 9, 2020 16:35

deanwhillier removed the Lifecycle/frozen label Jan 9, 2020

Willyfrog requested changes Jan 10, 2020

View reviewed changes

mattermod added the Lifecycle/1:stale label Jan 21, 2020

deanwhillier closed this Aug 13, 2020

mattermod removed the Work In Progress Not yet ready for review label Aug 13, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GH-1085 Replace simple-spellchecker with electron-spellchecker #1128

GH-1085 Replace simple-spellchecker with electron-spellchecker #1128

keithcruz commented Dec 11, 2019 •

edited

Loading

keithcruz commented Dec 11, 2019

wget commented Dec 15, 2019

mattermod commented Dec 26, 2019

keithcruz commented Jan 6, 2020

deanwhillier commented Jan 9, 2020

Willyfrog left a comment

Willyfrog Jan 10, 2020

Willyfrog Jan 10, 2020

Willyfrog Jan 10, 2020

Willyfrog Jan 10, 2020

Willyfrog Jan 10, 2020

Willyfrog Jan 10, 2020

Willyfrog Jan 10, 2020

mattermod commented Jan 21, 2020

Willyfrog commented Jan 21, 2020

deanwhillier commented Jul 23, 2020

Willyfrog commented Jul 23, 2020

deanwhillier commented Aug 13, 2020

GH-1085 Replace simple-spellchecker with electron-spellchecker #1128

GH-1085 Replace simple-spellchecker with electron-spellchecker #1128

Conversation

keithcruz commented Dec 11, 2019 • edited Loading

keithcruz commented Dec 11, 2019

wget commented Dec 15, 2019

mattermod commented Dec 26, 2019

keithcruz commented Jan 6, 2020

deanwhillier commented Jan 9, 2020

Willyfrog left a comment

Choose a reason for hiding this comment

Willyfrog Jan 10, 2020

Choose a reason for hiding this comment

Willyfrog Jan 10, 2020

Choose a reason for hiding this comment

Willyfrog Jan 10, 2020

Choose a reason for hiding this comment

Willyfrog Jan 10, 2020

Choose a reason for hiding this comment

Willyfrog Jan 10, 2020

Choose a reason for hiding this comment

Willyfrog Jan 10, 2020

Choose a reason for hiding this comment

Willyfrog Jan 10, 2020

Choose a reason for hiding this comment

mattermod commented Jan 21, 2020

Willyfrog commented Jan 21, 2020

deanwhillier commented Jul 23, 2020

Willyfrog commented Jul 23, 2020

deanwhillier commented Aug 13, 2020

keithcruz commented Dec 11, 2019 •

edited

Loading