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

electron-update fix spellcheck #266

Merged

Conversation

ZedTheYeti
Copy link

@ZedTheYeti ZedTheYeti commented Dec 14, 2016

@RocketChat/core

  • Re-enable previously broken spellchecking code for electron-update
  • Fix Mac OS X not having spellchecker module, and thus not working, by adding explicit spellchecker v3.3.1 dependency
  • Fixed no default language being selected on Windows when spellchecker loads the available dictionaries (en-US as opposed to en_US)

Tested on so far:

  • Windows 10 x64
  • Mac OS X Siera (10.12)
  • Ubuntu 16.04.1 LTS
  • Fedora 25.1

ZedTheYeti added 3 commits December 13, 2016 22:26
Spellchecking was working on Windows and Linux but not on Mac OS X. For some reason, in my tests, Mac OS X just wasn't loading the spellchecker module, adding an explicit dependency fixed it.
 The code for spellchecking looks for en_US instead of en-US. Since checker.getAvailableDictionaries() was returning en-US and not getting normalized, no default language was being selected on Windows. Now that it's normalized, a default language is automatically selected.
@CLAassistant
Copy link

CLAassistant commented Dec 14, 2016

CLA assistant check
All committers have signed the CLA.

@ZedTheYeti ZedTheYeti changed the title Electron update spellcheck electron-update fix spellcheck Dec 14, 2016
@graywolf336
Copy link
Contributor

Thanks for the pull request, have you tested this on Linux any?

@ZedTheYeti
Copy link
Author

ZedTheYeti commented Dec 14, 2016

I have not. It was mentioned in #242 that spell check was working in Ubuntu 16.04 when re-enabled. I'll go ahead and see if I can't spin up RHEL and Debian setups sometime today to test on and get back to you.

@engelgabriel
Copy link
Member

@ZedTheYeti
Copy link
Author

ZedTheYeti commented Dec 15, 2016

@graywolf336 I did testing on Ubuntu 16.04.1 LTS tonight, spellchecking is working. I'll likely test on a RPM-based distro tomorrow

@engelgabriel That definitely is an interesting and informative write up. I'm glad to see Slack contributing to the Electron community, it looks like there might be one or two things that could be brought into Rocket.Chat.Electron someday.

@rodrigok
Copy link
Member

@ZedTheYeti I'm getting a black screen when running your branch. Any ideias?

Running with npm start
node 4.6.2

@ZedTheYeti
Copy link
Author

ZedTheYeti commented Dec 16, 2016

@rodrigok Hmm I've been testing on Node.js 6.9.2. Does the Rocket.Chat/Rocket.Chat.Electron electron-update branch run for you? Whoops silly question, you're the one who bumped the Electron version in the electron-update branch.

What OS?

@ZedTheYeti
Copy link
Author

Tested Fedora 25, spellcheck is working.

@geekgonecrazy
Copy link
Member

@rodrigok which platform did you get blackscreen on? on OSX I haven't seen the issue so far running on @ZedTheYeti 's branch.

@xenithorb
Copy link
Contributor

xenithorb commented Dec 23, 2016

I tested this in several ways:

  1. The usual npm install fight with node-gyp a bit to figure out what you're missing and then npm start - worked fine, I had used n to acquire version 4.6.2 and learned about some additional build dependencies that I was missing, once I did that it worked fine

  2. Next up was npm run release and I installed the RPM and verified that all things were fine and worked the same

  3. Built it for F25 using mock-build on my local system while I was working the kinks out of the basic SPEC file. It's nothing special in terms of packaging it basically just does the above and gathers the artifact files before the build script generates deb/rpms (I turned that off via sed)

  4. Finally, submit the spec to my rocketchat-dev copr repository: dnf copr enable xenithorb/rocketchat-dev

All five scenarios worked eventually, and they all have working spellcheck! Finally! Thanks @ZedTheYeti

@geekgonecrazy
Copy link
Member

@xenithorb that's great news! 😁

@rodrigok
Copy link
Member

rodrigok commented Dec 26, 2016

@ZedTheYeti I'm using MacOS

@geekgonecrazy
Copy link
Member

@rodrigok how long did it take before the black screen came?

In my test I made sure to purge both sets of node_modules. Then fresh npm install. (Mainly because of that spell checker package that seems to be culprit for black screen). I ran it for 2-3 hours with no issues

@engelgabriel engelgabriel merged commit 9f128f7 into RocketChat:electron-update Dec 26, 2016
@rodrigok
Copy link
Member

@geekgonecrazy I took 0 seconds to show the black screen :(

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

7 participants