Last Comment Bug 816281 - "Avant-Garde" Not Spellchecked Properly
: "Avant-Garde" Not Spellchecked Properly
Status: RESOLVED FIXED
[mentor=ehsan]
:
Product: Core
Classification: Components
Component: Spelling checker (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla22
Assigned To: Joshua Gaines
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-11-28 14:42 PST by draco18s
Modified: 2013-02-26 18:18 PST (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch1 (771 bytes, patch)
2013-01-27 19:22 PST, Joshua Gaines
no flags Details | Diff | Splinter Review
Patch2 (146.75 KB, patch)
2013-01-27 19:27 PST, Joshua Gaines
no flags Details | Diff | Splinter Review
FullPatch (147.50 KB, patch)
2013-02-02 05:49 PST, Joshua Gaines
no flags Details | Diff | Splinter Review
Capitalized form of avant-garde removed (147.49 KB, patch)
2013-02-03 06:06 PST, Joshua Gaines
no flags Details | Diff | Splinter Review
Capitialized form of Avant-garde removed (147.28 KB, patch)
2013-02-19 05:11 PST, Joshua Gaines
no flags Details | Diff | Splinter Review
Clean Tree Patch (8.50 KB, patch)
2013-02-26 06:40 PST, Joshua Gaines
no flags Details | Diff | Splinter Review
Fixed? Final Patch? (9.06 KB, patch)
2013-02-26 17:31 PST, Joshua Gaines
ehsan: review+
Details | Diff | Splinter Review

Description draco18s 2012-11-28 14:42:25 PST
User Agent: Mozilla/5.0 (Windows NT 6.0; WOW64; rv:16.0) Gecko/20100101 Firefox/16.0
Build ID: 20121024073032

Steps to reproduce:

Type Avant-Gaurd
R-click to correct the spelling


Actual results:

Avant-Gaurd was corrected to Avaunt-Gaurd.  Which Firefox indicates is also spelled incorrectly.  R-clicking again, it suggests Avaunt-Gaurd.  Which is also incorrect, and is underlined.  Which corrects to Avaunt-Guard.  Which is also not the correct spelling.


Expected results:

It should correctly spellcheck to "Avant-Garde"
Comment 1 Mihaela Velimiroviciu (:mihaelav) 2012-12-04 06:12:58 PST
Mozilla/5.0 (X11; Linux i686; rv:20.0) Gecko/20121204 Firefox/20.0

Confirming spell-checking issue.
Comment 2 :Ehsan Akhgari (away Aug 1-5) 2012-12-04 07:44:09 PST
See <https://developer.mozilla.org/en-US/docs/Adding_a_new_word_to_the_en-US_dictionary> for how to fix this bug.
Comment 3 draco18s 2012-12-04 07:49:57 PST
(In reply to Ehsan Akhgari [:ehsan] from comment #2)
> See
> <https://developer.mozilla.org/en-US/docs/Adding_a_new_word_to_the_en-
> US_dictionary> for how to fix this bug.

I know how to add new words to the dictionary, that wasn't actually the problem.  The problem is that it suggested a spelling and then said that that new spelling was ALSO WRONG.  The fact that the suggestions were incorrectly spelled was a side note.
Comment 4 :Ehsan Akhgari (away Aug 1-5) 2012-12-04 07:54:45 PST
Well that's because we don't have avant-garde in our en-US dictionary, which makes it impossible for the spell checker to show the correct suggestion.
Comment 5 draco18s 2012-12-04 07:58:51 PST
(In reply to Ehsan Akhgari [:ehsan] from comment #4)
> Well that's because we don't have avant-garde in our en-US dictionary, which
> makes it impossible for the spell checker to show the correct suggestion.

Did you actually read the initial report?

Avant-Gaurd -> Avaunt-Gaurd -> Avaunt-Gaurd -> Avaunt-Guard

Each of the first three are underlined in red and using the spellcheck feature it suggests the next one over.  If "Avaunt-Gaurd" is a suggested spelling, meaning that it's in the dictionary, why is the spellchecker saying it's wrong?

Also, if "avant-garde" isn't in the dictionary, because it's not en-US, then why are "Avaunt-Gaurd", "Avaunt-Gaurd", and "Avaunt-Guard" in the dictionary (and SPELLED WRONG)?
Comment 6 :Ehsan Akhgari (away Aug 1-5) 2012-12-04 10:21:23 PST
(In reply to comment #5)
> (In reply to Ehsan Akhgari [:ehsan] from comment #4)
> > Well that's because we don't have avant-garde in our en-US dictionary, which
> > makes it impossible for the spell checker to show the correct suggestion.
> 
> Did you actually read the initial report?
> 
> Avant-Gaurd -> Avaunt-Gaurd -> Avaunt-Gaurd -> Avaunt-Guard
> 
> Each of the first three are underlined in red and using the spellcheck feature
> it suggests the next one over.  If "Avaunt-Gaurd" is a suggested spelling,
> meaning that it's in the dictionary, why is the spellchecker saying it's wrong?
> 
> Also, if "avant-garde" isn't in the dictionary, because it's not en-US, then
> why are "Avaunt-Gaurd", "Avaunt-Gaurd", and "Avaunt-Guard" in the dictionary
> (and SPELLED WRONG)?

None of these words are in the dictionary.  This is just the spell checker getting mislead because of the hyphen in this word.  Please see <http://mxr.mozilla.org/mozilla-central/source/extensions/spellcheck/locales/en-US/hunspell/en-US.dic> for the dictionary source that I'm talking about.
Comment 7 draco18s 2012-12-04 10:40:37 PST
(In reply to Ehsan Akhgari [:ehsan] from comment #6)
> This is just the spell checker
> getting mislead because of the hyphen

Sounds like a bug!
Comment 8 Joshua Gaines 2013-01-21 16:03:36 PST
I would like to take a stab at the this bug.
Comment 9 :Ehsan Akhgari (away Aug 1-5) 2013-01-21 16:10:16 PST
Great, thanks!
Comment 10 Joshua Gaines 2013-01-27 19:16:22 PST
Ok I have come up with a fix for this bug. It was simple, I just added "Avant-garde" and "avant-garde" to the US-EN.dic file. Now when you type the incorrect spelling "avant-gaurd", then "avant-garde" is now suggested as a correction. I am attaching two patch files. Patch1.txt is where I altered the extensions\spellcheck\locales\en-US\hunspell\dictionary-sourceshunspell-en_US-20081205.dic to contain the new words. Patch2.txt is just the diff of the upstream dictionary file where the lower lever dictionaries are merged together into one main dictionary.

Now, I am just waiting on how to proceed since this is my first bug fix. Do I need to create some sort of automated test for this fix? I have obvisouly built firefox with my changes and manually tested it.
Comment 11 Joshua Gaines 2013-01-27 19:22:55 PST
Created attachment 706935 [details] [diff] [review]
Patch1

This patch is just the diff of where I added the word to the lower level hunspell-en_US-20081205.dic found in \extensions\spellcheck\locales\en-US\hunspell\dictionary-sources
Comment 12 Joshua Gaines 2013-01-27 19:27:45 PST
Created attachment 706936 [details] [diff] [review]
Patch2

This patch is just a diff of the final dictionary file after the make merges the lower dictionary files together.
Comment 13 :Ehsan Akhgari (away Aug 1-5) 2013-01-28 08:17:19 PST
Comment on attachment 706935 [details] [diff] [review]
Patch1

Review of attachment 706935 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks a lot for your patch, Joshua!

Please make a patch which contains all of the changes to the repository, not one patch per file.  See <https://developer.mozilla.org/en-US/docs/Creating_a_patch> for an explanation on how to do that.

Also, although we usually require automated tests alongside patches, I don't think there's much point in an automated test for this, so you don't need to add one here.

Waiting to see your next patch! :-)

::: extensions/spellcheck/locales/en-US/hunspell/en-US.dic
@@ +1265,5 @@
>  Autumn/M
>  Av/M
>  Ava/M
>  Avalon
> +Avant-garde

You shouldn't need to add the upper-case version.
Comment 14 Joshua Gaines 2013-02-02 05:49:44 PST
Created attachment 709372 [details] [diff] [review]
FullPatch

This is a full diff of the altered directory.
Comment 15 :Ehsan Akhgari (away Aug 1-5) 2013-02-02 11:12:04 PST
Comment on attachment 709372 [details] [diff] [review]
FullPatch

Review of attachment 709372 [details] [diff] [review]:
-----------------------------------------------------------------

::: extensions/spellcheck/locales/en-US/hunspell/en-US.dic
@@ +1265,5 @@
>  Autumn/M
>  Av/M
>  Ava/M
>  Avalon
> +Avant-garde

As I said before, you don't need the upper-case version.
Comment 16 Joshua Gaines 2013-02-03 06:06:49 PST
Created attachment 709465 [details] [diff] [review]
Capitalized form of avant-garde removed

Lets try this one more time
Comment 17 :Ehsan Akhgari (away Aug 1-5) 2013-02-04 10:08:37 PST
Comment on attachment 709465 [details] [diff] [review]
Capitalized form of avant-garde removed

Review of attachment 709465 [details] [diff] [review]:
-----------------------------------------------------------------

::: extensions/spellcheck/locales/en-US/hunspell/en-US.dic
@@ +1265,5 @@
>  Autumn/M
>  Av/M
>  Ava/M
>  Avalon
> +Avant-garde

Still here!  Am I missing something?
Comment 18 Joshua Gaines 2013-02-19 05:11:07 PST
Created attachment 715459 [details] [diff] [review]
Capitialized form of Avant-garde removed

Okay I apologize for my previous patch submissions. Turns out that removing the word from the lower stream dictionaries does not remove it from the upper level merged dictionary. I was thinking that each time I would build the upper level dictionary was reconstructed from the lower level dictionaries. Anyways, I hope this is the correct patch needed.
Comment 19 :Ehsan Akhgari (away Aug 1-5) 2013-02-20 10:27:13 PST
Comment on attachment 715459 [details] [diff] [review]
Capitialized form of Avant-garde removed

Review of attachment 715459 [details] [diff] [review]:
-----------------------------------------------------------------

::: extensions/spellcheck/locales/en-US/hunspell/en-US.dic
@@ +1,1 @@
> +57444

Hmm, this count is wrong.  I'd expect attempting to load this dictionary to crash, etc.  :(

I suggest that you start over on a clean tree, and don't use the previous changes made to the dictionaries...  Sorry that this proved to be this much of a hassle.  :-)
Comment 20 Joshua Gaines 2013-02-26 06:40:10 PST
Created attachment 718415 [details] [diff] [review]
Clean Tree Patch

Hopefully I got it write this time. I pulled a clean tree and added only the lower case word to the dictionary. Sorry I didn't knock this simple fix out quickly.
Comment 21 :Ehsan Akhgari (away Aug 1-5) 2013-02-26 16:38:51 PST
Comment on attachment 718415 [details] [diff] [review]
Clean Tree Patch

Review of attachment 718415 [details] [diff] [review]:
-----------------------------------------------------------------

The en-US.dic file seems to be missing from this patch...
Comment 22 Joshua Gaines 2013-02-26 17:31:58 PST
Created attachment 718776 [details] [diff] [review]
Fixed? Final Patch?

So I think I am going for the record of patch attempts for a trivial fix. I am very sorry if I cause any inconvenience. I forgot to merge the dictionaries and move the new en-US.dic to the top directory. I am hoping this I have go it correct this time.
Comment 23 :Ehsan Akhgari (away Aug 1-5) 2013-02-26 18:16:52 PST
Comment on attachment 718776 [details] [diff] [review]
Fixed? Final Patch?

Review of attachment 718776 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great, thanks a lot!  And no worries about the number of iterations, it was astonishing to see you stick around this whole time and persevere until everything is right!  Thanks for doing that!  :-)
Comment 24 :Ehsan Akhgari (away Aug 1-5) 2013-02-26 18:18:29 PST
https://hg.mozilla.org/mozilla-central/rev/eb562a97db7c

Note You need to log in before you can comment on or make changes to this bug.