-
Notifications
You must be signed in to change notification settings - Fork 156
Allow users to add custom words to dictionary #244
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First of all I apologize for my very late reply. I'm really busy in this time with my job and I couldn't answer sooner.
Thank you very much for this PR which looks good!
Here are my answers to you questions:
Is the reloading custom words in setDictionary required? I think it's good to do so for safety's sake either way.
I don't know. Actually node-spellchecker
's setDictionary
method is very poorly documented. I assume it may work without reloading because setDictionary
obviously keeps the same instance of Spellchecker
: https://github.com/atom/node-spellchecker/blob/25b517e5db3814da7d09de1168727c7bc8defc91/lib/spellchecker.js#L35
But I'm not 100% sure so we can keep it like this if you believe it is safer.
Can the config file be reloaded while running Abricotine? If so, with the running list of custom words being stored in this.words, is there anywhere we'll need to re-load the words list from disk?
Unfortunately there is no way to do such a thing right now. This means added words won't be shared between windows until the app is restarted. This is one of Abricotine design issues 😕
If you want to avoid duplicates in config.json, we can add a test addSpelledWord (see details in my review).
Is there a better-supported way to reload the spellchecker highlighting than to run abr-spellcheck-off and then abr-spellcheck-on?
I don't think so. If you want it to be more readable, maybe you can create an AbrDocument.refreshSpelling()
method apart.
Do we need a way to allow users to see the custom word list and remove from/add to it from a menu in Abricotine, or can we just tell them to edit the config file directly to do so?
Right now there is no UI for editing the config. I think it is better to work on this feature (= a form to edit config) for the whole config rather than doing a separate interface for the spellchecker. I'm already working on something like this in a separate repo: https://github.com/brrd/electron-settings-gui (no idea when I will be able to finish this work, though).
Is there a way to only display the "Add to Dictionary" context menu item when right-clicking on misspelled words? Right now it displays all the time.
No. But we can work on it later: #245
When the OS dictionary is in use (on at least macOS, maybe others), the custom words are stored in the macOS custom dictionary.
Same on Windows.
This means we don't need to store or refresh the custom words list every load,
Then you can maybe avoid this part with a condition on process.platform
(because I don't think there is another way to do this, as explained here: atom/node-spellchecker#47). But I'm not sure it is necessary.
and they won't be able to remove the custom words by just removing them from the Abricotine config file.
Yes indeed. This is maybe related to #134.
Thanks again for the awesome software, and hopefully this change looks decent!
It does, definitely 👍
Thank you!
words.forEach(function (word) { | ||
// just for safety | ||
if (word != "") { | ||
spellchecker.add(word) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Please beware of missing semicolon
;
- What do you think about trimming
word
? Like:
word = word.trim();
if (word) {
spellchecker.add(word);
}
(not sure this is useful though)
if (words != null) { | ||
that.words = words; | ||
words.forEach(function (word) { | ||
// just for safety |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO you can remove this comment.
@@ -773,6 +788,15 @@ AbrDocument.prototype = { | |||
setDictionary: function (lang, path) { | |||
if (lang) { | |||
spellchecker.setDictionary(lang, path); | |||
|
|||
// reload custom words | |||
this.words.forEach(function (word) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same remarks here.
Actually you can maybe use a function here to avoid code duplication. I let you decide.
@@ -789,6 +813,18 @@ AbrDocument.prototype = { | |||
return spellchecker.isMisspelled; | |||
}, | |||
|
|||
// Adds the given word to our custom spellcheck-ok words | |||
addSpelledWord: function (word) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To avoid duplicates in config (in case of multiple windows), maybe we can add this here:
this.words = this.getConfig("spellchecker:custom-words");
// add to config file | ||
if (this.words.indexOf(word) == -1) { | ||
this.words.push(word) | ||
this.setConfig("spellchecker:custom-words", this.words) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing two ;
Heya, thanks for the feedback! No problem, I know how things can get sometimes :) Hmm, yeah I'm not overly worried about how/whether we have to allow users to remove custom words from the dictionary once they've already been added. All else fails we can approach it later without an issue. I'm interested to see how this goes for #245, for sure! Happy to work on #83 as well after some of the groundwork for that issue's done (I'd look at #245 and all right now myself, but not familiar enough with how the whole system works to put in a change like that right now). |
This PR adds the ability to right-click on a misspelled word and select "Add to Dictionary" to mark it as fine (fixes #243). It stores the custom words in the config file in this way:
The main questions I have with this PR as it exists are:
setDictionary
required? I think it's good to do so for safety's sake either way.this.words
, is there anywhere we'll need to re-load the words list from disk?abr-spellcheck-off
and thenabr-spellcheck-on
?SpellChecker.add
note here for more info).Thanks again for the awesome software, and hopefully this change looks decent! Feel free to rip me apart over it if it's not right – I'm pretty new to Node dev overall so happy to learn 😄