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

Spell checker #213

Merged
merged 5 commits into from Jan 14, 2016

Conversation

Projects
None yet
8 participants
@bbowyersmyth
Copy link
Contributor

commented Dec 22, 2015

Initial Windows spell check support to address #130

Uses the Windows provided checker for on-demand and real-time spelling. Auto correct is not part of this PR.

There were a lot of file paths being passed around which the Windows spell API does not require. They have mostly been removed. In the future when the spell API becomes pluggable/cross-plat those instances can probably just be more opinionated about their storage locations.

LCID has been replaced with the culture name that this API requires.
Options for ignore word with numbers and uppercase have been removed. They are on by default and are not set at an application level.

Anything else that needs to be addressed? Interface changes OK?
Note: Only tested with English so far

@dnfclas

This comment has been minimized.

Copy link

commented Dec 22, 2015

Hi @bbowyersmyth, I'm your friendly neighborhood .NET Foundation Pull Request Bot (You can call me DNFBOT). Thanks for your contribution!
You've already signed the contribution license agreement. Thanks!

The agreement was validated by .NET Foundation and real humans are currently evaluating your PR.

TTYL, DNFBOT;

@ScottIsAFool

This comment has been minimized.

Copy link
Member

commented Dec 22, 2015

Is there anything in place for if the user is running this on Windows 7? I haven't looked through the code yet, so if there is, forgive me :)

@bbowyersmyth

This comment has been minimized.

Copy link
Contributor Author

commented Dec 22, 2015

No there isn't. This is PR is to get the spell check back up and running using the Windows spell check API (Win8 and later). Win7 will need to be addressed separately with an API that has it's own dictionaries.

@ScottIsAFool

This comment has been minimized.

Copy link
Member

commented Dec 22, 2015

Sorry, I should have been clearer. What happens if this is run on Windows 7 with your changes in place. I know spell check won't be there, I'm just making sure it doesn't crash or anything :)

@bbowyersmyth

This comment has been minimized.

Copy link
Contributor Author

commented Dec 22, 2015

Oh right. The spelling will be disabled/unavailable like it is currently. There are IsPlatformSupported checks in the initialization.

@ScottIsAFool

This comment has been minimized.

Copy link
Member

commented Dec 22, 2015

Perfect, thanks.

@ScottIsAFool

This comment has been minimized.

Copy link
Member

commented Dec 22, 2015

Quick testing of this looks good so far. You have this marked as WIP, what else is remaining as part of this?

@martinwoodward

This comment has been minimized.

Copy link
Member

commented Dec 22, 2015

This is awesome! The interface changes are just fine. Incredibly impressive piece of work!

Agreed with @ScottIsAFool - let us know what you want to get done before this is merged. So far looking good to :shipit: in terms of the functionality being delivered.

You rock @bbowyersmyth!

@bbowyersmyth bbowyersmyth changed the title [WIP] Spell checker Spell checker Dec 22, 2015

@bbowyersmyth

This comment has been minimized.

Copy link
Contributor Author

commented Dec 22, 2015

Being unfamiliar with the code base I was not sure I had found all the commented out pieces of code. It looks to be ok though.
Also it would be good for someone to test with a language other than English.

But this is probably a good enough starting point that others can now jump in if they find something. Removing the WIP.

@RobStand

This comment has been minimized.

Copy link
Member

commented Dec 23, 2015

LGTM. Impressive work!

@martinwoodward

This comment has been minimized.

Copy link
Member

commented Dec 23, 2015

Be good to add a pointer to https://github.com/bbowyersmyth/spellcheck in AboutForm.cs to give credit to your wrapper NuGet package.

I think the best way to get additional language testing on this is probably going to be to merge in and create a new signed build and then see what bugs roll in. @willduff - any thoughts on this?

@ScottIsAFool

This comment has been minimized.

Copy link
Member

commented Dec 23, 2015

@martinwoodward sounds good to me.

@bbowyersmyth

This comment has been minimized.

Copy link
Contributor Author

commented Dec 23, 2015

Thanks everyone. AboutForm has been appended.

@martinwoodward

This comment has been minimized.

Copy link
Member

commented Dec 23, 2015

Looking good thanks!

👍 from me now but as it's a big change want to leave a little bit longer to give others the chance to code review over the holiday period in many countries.

Spellcheck - what an excellent Christmas present. Thanks again @bbowyersmyth!

@StephenCWLL

This comment has been minimized.

Copy link

commented Dec 30, 2015

Is this thread saying spelling has been fixed in latest build 0.1.5.4? It's still greyed out for me :/

@ScottIsAFool

This comment has been minimized.

Copy link
Member

commented Dec 30, 2015

No, we have a fix for it but it's not in the released builds yet.

@StephenCWLL

This comment has been minimized.

Copy link

commented Dec 30, 2015

Ok, I'm not going crazy then :)

@@ -150,13 +150,12 @@ private static void LoadPreferencesPanels()
preferencesPanelTypeTable["accounts"] = type;
types.Add(type);

//ToDo: OLW Spell Checker
//if (SpellingSettings.CanSpellCheck)

This comment has been minimized.

Copy link
@willduff

willduff Jan 13, 2016

Member

Could uncomment or delete this check for if (SpellingSettings.CanSpellCheck), I could go either way.

if (spellerStatus == null)
{
offset = -1;
length = -1;

This comment has been minimized.

Copy link
@willduff

willduff Jan 13, 2016

Member

I'd recommend setting offset = 0; and length = word.length; instead of -1. Or fixing up the handling in SpellCheckerForm.ContinueSpellCheck, because the SpellCheckerForm.ContinueSpellCheck() method will end up passing this length to String.Substring which will throw an ArgumentOutOfRangeException if its less than zero.

@willduff

This comment has been minimized.

Copy link
Member

commented Jan 13, 2016

This is awesome work @bbowyersmyth, really appreciate it. I'm going to do some quick testing on the build in the morning. I'll plan to merge this in shortly after.

@bbowyersmyth

This comment has been minimized.

Copy link
Contributor Author

commented Jan 13, 2016

Thanks @willduff. Made those changes

@willduff

This comment has been minimized.

Copy link
Member

commented Jan 13, 2016

I went to check the continuous integration build for this change (Scroll down this PR to ' All checks have passed' and click on 'Show all checks', then click the 'Details' link next to 'AppVeyor build succeeded'. Then you can see the output of the build, assuming it succeeded, click on 'Artifacts' and you'll see an OpenLiveWriterSetup.exe). When I attempt to launch I get

System.IO.FileNotFoundException: Could not load file or assembly 'OpenLiveWriter.SpellChecker, Version=0.5.1.4, Culture=neutral, PublicKeyToken=null' or one of its dependencies. The system cannot find the file specified.
File name: 'OpenLiveWriter.SpellChecker, Version=0.5.1.4, Culture=neutral, PublicKeyToken=null'

We need to update OpenLiveWriter.nuspec to include OpenLiveWriter.SpellChecker.dll.

@willduff

This comment has been minimized.

Copy link
Member

commented Jan 13, 2016

And we need to include PlatformSpellcheck.dll too.

@kathweaver

This comment has been minimized.

Copy link
Contributor

commented Jan 13, 2016

I pulled the build from the last check and got an unexpected error. Here's the error file.
ErrorReport.txt

@bbowyersmyth

This comment has been minimized.

Copy link
Contributor Author

commented Jan 13, 2016

Added those two files to the nuspec. Looks like the CI is picking them up now.

@kathweaver

This comment has been minimized.

Copy link
Contributor

commented Jan 13, 2016

Still having problems.
SquirrelSetup.txt
Different error this time.

@willduff

This comment has been minimized.

Copy link
Member

commented Jan 14, 2016

@kathweaver Your latest error looks like it wasn't able to even finish installation. I'm guessing you may have already had Open Live Writer running, which caused System.UnauthorizedAccessException: Access to the path 'C:\Users\Kathleen\AppData\Local\OpenLiveWriter\app-0.5.1.4\Google.Apis.dll' is denied.

@bbowyersmyth Your latest changes look good to me, so I'll merge this in now. I installed the build and did some manual testing and everything is working well. The only follow up we should do is open a new issue to track getting autocorrect working, which used to depend on an mso.acl file that Windows Live Writer used. The mso.acl file was from the Microsoft Office team and was a simple binary file of common misspellings paired with a recommended auto correction. Looks like you left a // TODO at src\managed\OpenLiveWriter.PostEditor\ContentEditor\ContentEditor.cs:2435 for this, thanks!

willduff added a commit that referenced this pull request Jan 14, 2016

Merge pull request #213 from bbowyersmyth/SpellChecker
Issue #130 - Add spell check feature back in OLW

@willduff willduff merged commit ab5a5e6 into OpenLiveWriter:master Jan 14, 2016

1 check passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@bbowyersmyth bbowyersmyth deleted the bbowyersmyth:SpellChecker branch Jan 14, 2016

@bbowyersmyth

This comment has been minimized.

Copy link
Contributor Author

commented Jan 14, 2016

Thanks everyone. Glad I could help out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.