Spellchecking added

[thecyberd3m0n]

Tested on Linux and Windows.

---

Fixes #2661 ~ Travis

[anoadragon453]

Interesting this is done without native node modules!

[thecyberd3m0n]

I'll sit resolving comments in ~5hours

[turt2live]

Please also use spaces instead of tabs, per the code style. We'll also need signoff before we can merge this.

[thecyberd3m0n]

code style fixed due to eslint

[thecyberd3m0n]

please wait because installing dictionaries not works yet.

[thecyberd3m0n]

ok, if someone could test on Mac, while I'll configure my Mac VM for future contributions?:) I can't build for Mac without Mac.

[anoadragon453]

This one too? :)

[thecyberd3m0n]

enableRemoteModule: false leads to errors and breaks the feature in my case. Can I leave it?

[thecyberd3m0n]

[thecyberd3m0n]

@anoadragon453 ?

[anoadragon453]

Sorry, I meant just moving it back to where it was originally in the list, not changing the value.

[thecyberd3m0n]

fixed

[turt2live]

We can't take copyright for this. It looks almost identical to Rocket.Chat's implementation (https://github.com/RocketChat/Rocket.Chat.Electron/blob/7ad28f3f24f7b32b22ea75ed4e35ed2b173b933c/src/preload/spellchecking.js) which is under MIT. We need to give them credit, and use their license for this file, in order to include it.

[thecyberd3m0n]

we could change something inside. For example their implementation allow Multilanguage, our not ;) I can try to restructure the code - please suggest something, what should we do?

[turt2live]

I'm not a lawyer, but I believe we need to have the file under MIT and their copyright. Then when changes are made the applicable party's copyright is added underneath the Rocket.Chat copyright.

[thecyberd3m0n]

Please take a look at new version

[dbkr]

I haven't exhaustively gone through this yet, but a few general points:

1. See @turt2live 's comment about copyright

2. Your indenting is all over the place: 4 space indents please (https://github.com/matrix-org/matrix-react-sdk/blob/develop/code_style.md)

3. There isn't a single comment in this PR and there's a lot of code that needs explanation. For example...

4. ...it looks like you're installing new dictionary files to the app directory? Won't these be wiped away when the app updates and/or the user won't have permission to write there?

5. I really, really, really dislike having dictionary files in the riot codebase. Where did they come from? How would we update them? Is whatever getAvailableDictionaries() has by default not enough?

6. You're enabling the remote module which is probably okay security-wise, but it looks like it's mostly used to show context menus from the renderer process. Why not just do it from the main process?

7. You're adding a lot of dependencies (eg. jetpack, mem, os (is this one even used?)) This might be ok but it's not really clear that they're necessarily justifying their presence.

8. and mostly: the spellchecker module is a native module. We're still looking at the implications of adding native modules to Riot but basically we don't yet have enough infrastructure to build them ourselves and for a security-sensitive encrypted messenger, pulling in more precompiled binaries from more not-necessarily-trusted sources is somewhat nonideal.

Thanks for looking at this, it is a somewhat complicated one though I'm afraid.

[thecyberd3m0n]

I have inspired by working Rocket.Chat implementation, so similarities can be found.

1. I'll change the implementation, by removing dependencies and replacing it by Node.js vanilla implementations (also 7 - that will be fixed). Changes that I'll describe below should resolve that problem.
2. I used eslint with your configs, I'll recheck it. I would like to add this to eslintrc, if you don't mind
3. I'll add some in new code
4. Yes, like Rocket Chat did.
5. Node.js simple-spellchecker also bundles some dictionaries inside https://github.com/jfmdev/simple-spellchecker/tree/master/dict. and unpacks it on start (https://github.com/jfmdev/simple-spellchecker/blob/master/test.js). How do you want to solve it instead?
6. You know I tried several hours but without success. It was a workaround. I can retry because of new conditions.
7. Will be removed for sure according to default dictionaries that comes from simple-spellchecker
8. I'll try to replace it with https://www.npmjs.com/package/simple-spellchecker. It has no native bindings

I'm not giving up easily, so please wait for new commits ;)

[thecyberd3m0n]

@dbkr
I resolved most of issues. In details:

1. I think it's not the case according to new version
2. I've added rule to eslint and fixed automatically using IDE
3. there was one place where it was reasonable
4. 5. simple-spellchecker also comes with dictionaries out of box. Please give some ideas how we can adddress this issue. How we should install dictionaries, from where app should get them? Please help I'm running out of ideas
5. now I've reused existing context menu in main process
6. not the case anymore, that part was left untouched
7. now just simple-spellchecker should be new which is non-native ;)

[thecyberd3m0n]

did some diff cleanup. please check again. Perhaps I trust automated tools for code-style too much

[dbkr]

1. I think it's not the case according to new version

The copyright? Rocketchat is MIT licensed, so it's fine to use their code, but we must attribute it appropriately. Code you write is copyright yourself (ie. not New Vector).

2. I've added rule to eslint and fixed automatically using IDE

See inline comment, but you've also introduced a lot of unrelated formatting changes.

4. 1. simple-spellchecker also comes with dictionaries out of box. Please give some ideas how we can adddress this issue. How we should install dictionaries, from where app should get them? Please help I'm running out of ideas

Honestly I'm not sure - I'd have to go and look at what the various parts of the spellchecking infrastructure come with. Unfortunately this is one of the reasons this PR is tricky, ie. it's not just a question of writing the code.

Thanks again for the work on this, although I think there's still a lot to be done: I'm not really sure I understand how this code works and there still aren't really many comments to help me. As an example, what do we gain/lose from simple-spellcheck over what you were using before? Presumably it's no longer using the OS spellchecking so words the user added to their system's dictionary won't be included?

You've certainly picked a very complex issue to fix: we'd normally expect people to start with a simpler one for their first PR - you could maybe start with some issues with the 'easy' tag?

[thecyberd3m0n]

maybe. But if we've got time we can investigate issues here, and build to the end. If not you can close PR, I'll focus on other issues :)

1. I've meant that after changes code doesn't look similar anymore.
2. I'll try to minize the diff, sorry for that ;)

simple-spellcheck is found by me replacement over native module, that comes with node-spellchecker and electron-spellchecker (https://www.npmjs.com/package/simple-spellchecker). Unfortunately - it bundles dictionaries inside. It also allows to install dictionaries.
Simple-spellcheck should not require any major infrastructure upgrade, as it uses Javascript code https://github.com/jfmdev/simple-spellchecker.

[jryans]

The PR author mentioned in the issue that the PR can be closed. Thanks for investigating this issue, and I'm sure we'll find a fix down the road.