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

Support spellcheck and auto-educate quotes and dashes #130

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

dgileadi
Copy link

@dgileadi dgileadi commented Nov 9, 2017

Besides what it says on the tin, this also adds menu items for enabling/disabling the above features and persists those choices between app launches.

This addresses #40.

This has only been tested on macOS 10.13, so testing on other platforms is requested.

@dgileadi
Copy link
Author

Hmm, I tried running the win64 and win32 apps built by BUILD_PACKAGE.command and ran into the error described here. As per neurosnap's comment on that issue, apparently you need to build on the target platform. Is that a blocker for this PR?

@dgileadi
Copy link
Author

Thanks to the original problem solving by Bartosz Antosik in vscode-spellright, I got building for all platforms working. I haven't tested linux, but running on win32 and win64 (built on my Mac) works for me.

@joethephish
Copy link
Member

Hey, sorry for the delay on reviewing this PR (and indeed any future delay) -- work on Heaven's Vault unfortunately always has to come ahead of any work we do on ink/inky, and there's no lack of it!

Gave this a quick test and it seems pretty great! Out of interest, when you tell it to learn a spelling, where does that get stored?

Hmm, the smart quotes, although smart indeed, are slightly more controversial. While I like the fact that you detect the current scope so that they're only used within written text and not logic, I found that if you type something like {"hello world"}, then since it hasn't yet detected your completed scope after the first {, then you still end up accidentally getting a smart quote.

Arguably this could be fixed with some better syntax parsing in the Ace grammar... but the grammar is unlikely to ever be as robust as the compiler. And, our preferred solution to smart quotes at inkle is to fix them up at runtime, along with a whole host of other text cleaning operations which are applied to the final text using a utility function. It's something that we did in previous private versions of the ink runtime engine (so for Sorcery and 80 Days) but not something that we've fully built yet for Heaven's Vault.

On the other hand, since it's a toggle-able option, perhaps there's not harm in including it if it's easy to switch off.

Anyone else have an opinion on this? @joningold?

@dgileadi
Copy link
Author

When you learn a spelling, it gets stored in the OS's dictionary. This is done by the node-spellchecker module, since it uses native libraries for spell checking. However this may not work on Linux since there it uses the Hunspell library, and I don't know if/how node-spellchecker handles the "learn" feature there.

I should make a fix to the {"hello world"} case, since I had to do the same for the -> DONE case. Like you say it probably will never be perfect, but it's worth catching common cases.

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

Successfully merging this pull request may close these issues.

None yet

2 participants