343299 - Spell-check context menu missing Ignore and Learn Spelling entries

Status: RESOLVED FIXED

(Camino Graveyard :: OS Integration, defect, P3)

Description

[[e-mail]]

The spell-check context menu don't include the usual "Ignore Spelling" and "Learn Spelling" entries, like the ones in TextEdit, Mail, etc.
Which mean that we are not able to add new words to the spell check database from Camino.

Comment 1

[philippe (part-time)]

Additional: there should be an option to allow to switch dictionaries for people who use multiple languages.

Comment 2

[froodian (Ian Leue)]

Also, we should make a disabled "No Guesses Found" item when there aren't any guesses (especially since right now we just display the menu separator on its lonesome, which looks weird)

Comment 3

[Samuel Sidler (old account; do not CC)]

And... there should be an option near the bottom which has a Spelling submenu (this option/submenu should also appear in the main Edit menu).

Comment 4

[froodian (Ian Leue)]

Attachment: A Start

Most of this bug is way beyond me, but this implements comment 2.  Whomever fixes this should feel free to roll this code into their patch.

BTW, see "changes to camino v3.1" for how the rest of this bug was once implemented.

Comment 5

[Jasper]

It might be a new bug, but what I miss most is the ability to change the language. I want to be able to determine the language the spellcheck is set on. 

A lot of people run their native language system but write text in english, it's important for them to be able to change the language as they go.

Comment 6

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

I don't disagree that we need the option (or access to the spell-check dialogue), but how is it that my Camino is checking multi-lingually?  It's caught French misspellings when I'm bumbling along in French (and doesn't mark every French word as misspelled, as an English-only spell-checker would).

Comment 7

[froodian (Ian Leue)]

(In reply to comment #4)
> BTW, see "changes to camino v3.1" for how the rest of this bug was once
> implemented.
> 

I must have been in crack when I commented.  That attachment is on bug 151040.

https://bugzilla.mozilla.org/attachment.cgi?id=211692

Comment 8

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

(In reply to comment #4)
> Created an attachment (id=230302) [edit]
> A Start
> 
> Most of this bug is way beyond me, but this implements comment 2.  Whomever
> fixes this should feel free to roll this code into their patch.

If it's clean and works, why don't we go ahead and get it reviewed and land it now?  No point in letting fixes for discreet issues bitrot waiting for more complex related issues....

Won't that need to be a localizable.string, though?

Comment 9

[froodian (Ian Leue)]

Attachment: Uses localized string

Um, yes. :p

Comment 10

[Nick Kreeger]

Comment on attachment 230982 [details] [diff] [review]
Uses localized string

Looks fine to me, but i didn't test it

Comment 11

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Comment on attachment 230982 [details] [diff] [review]
Uses localized string

> Looks fine to me, but i didn't test it 

But I did  ;) It works, and there are no errors in the Console, so r=me

Comment 12

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 230982 [details] [diff] [review]
Uses localized string

+      NSMenuItem* item = [[NSMenuItem alloc] initWithTitle:NSLocalizedString(@"No Guesses Found", nil) action:NULL keyEquivalent:@""];

this needs to be added to the localized.strings  file so l10ns know to localize it.

also, just autorelease it at init time so avoid someone returning early and leaking.

sr=pink with those.

Comment 13

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

The string probably needs to go in a new "section" at the bottom, something like

/* Spelling context menu */
"No Guesses Found" = "No Guesses Found";

Comment 14

[froodian (Ian Leue)]

Attachment: sr=pinkerton patch (landed on branch and trunk)

Comment 15

[Nick Kreeger]

Fixed trunk and branch.

Comment 16

[Nick Kreeger]

Apparently this is not a complete fix.

Comment 17

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Where is the checkin for localizable.strings (comment 13) for the patch that landed?

Comment 18

[Nick Kreeger]

Im used to seeing a Localizable.strings posted here. The updated string is checked in.

Comment 19

[Stuart Morgan]

We can do Ignore, but I haven't been able to find any API for Learn. I suppose we could try editing /Library/Spelling/<current spelling language> directly, but that strikes me deeply sketchy.

Comment 20

[Sean Murphy]

I have a working patch for adding "Ignore Spelling."  I'm updating my source tree and building again to ensure it also works with the latest trunk changes.

As for "Learn Spelling," looking over nsIInlineSpellChecker.h reveals:

NS_IMETHOD AddWordToDictionary(const nsAString & aWord) = 0;

Now, presuming that header actually corresponds to the spell checker we're using, wouldn't that work?  The ability to ignore a word is handled correctly by the same inline spell checker object.

Comment 21

[Stuart Morgan]

Yeah, I hadn't looked into how it was wrapped, and was just looking at NSSpellChecker APIs; pink's wrapper handles it by handing the learn functionality off to the base moz stuff, meaning that it won't actually add it to the user's system dictionary. If I'm reading it correctly those words also aren't used by the suggestion system in the wrapper, so it may be that "Learn" and "Ignore" would act pretty much the same.

I guess even if that's true we could hook up the front-end now and worry about changing the implementation of learn later if need be.

Comment 22

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Just to keep this bug sane from here on out, I spun comment 1 (language/dictionary selection) off into bug 364550.

Comment 23

[Sean Murphy]

Attachment: learn and ignore patch

Ignore/Learn Spelling functionality added along with the corresponding context menu items.  They are inserted into the context menu in a location consistent with the rest of OS X, which is under the suggestions with a separater in-between.

I'm aware that the [inMenu insertItem:...] methods are kinda lengthy.  What's our indentation policy in this case?

I determine the index inside the context menu of which to insert the new items with the expression: (numSuggestions + insertBase + x) and x is incremented in code for each successive item.  Is there a better way that I couldn't think of, aside from using NSMenu's indexOfItem… methods?

Also: is mispelled actually "misspelled?"  I didn't change my new methods since the previous ones in BWC.mm and all mozilla code uses the first spelling.

We'll have some new strings in need of localization if this lands, which are (not surprisingly): "Learn Spelling" and "Ignore Spelling."

Comment 24

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

(In reply to comment #23)
> Also: is mispelled actually "misspelled?"  I didn't change my new methods since
> the previous ones in BWC.mm and all mozilla code uses the first spelling.

See bug 357465 :p

Comment 25

[Stuart Morgan]

(In reply to comment #23)
> I'm aware that the [inMenu insertItem:...] methods are kinda lengthy.  What's
> our indentation policy in this case?

Standard Cocoa style; new line after each argument, aligned on the colons. So:
    [inMenu insertItemWithTitle:NSLocalizedString(@"Learn Spelling", nil)
                         action:@selector(learnMispelledWord:)
                  keyEquivalent:@""
                        atIndex:numSuggestions + insertBase + 2];

> I determine the index inside the context menu of which to insert the new items
> with the expression: (numSuggestions + insertBase + x) and x is incremented in
> code for each successive item.  Is there a better way that I couldn't think of,
> aside from using NSMenu's indexOfItem… methods?

Generally we do things like this by tagging a separator that marks the insertion location, and using indexOfItemWithTag:

If you could make your methods just ignoreWord: and learnWord: to prevent more of the mispelled/misspelled stuff, that would make me happy; as the bug Smokey referenced suggests, I'm not thrilled with the current situation

Comment 26

[Sean Murphy]

An implementation question, again concerning the insertion of Learn/Add items into the input context menu:

Following the same technique by which the spelling suggestions are added goes like this...

[A separator item was added in IB with the appropriate tag]

int dictionaryItemsInsertBase = [inMenu indexOfItemWithTag:kSpellingDictionaryItemsTag];

[inMenu insertItemWithTitle:NSLocalizedString(@"Learn Spelling", nil) 
                     action:@selector(learnWord:) 
              keyEquivalent:@"" 
                    atIndex:dictionaryItemsInsertBase];
                    
[inMenu insertItemWithTitle:NSLocalizedString(@"Ignore Spelling", nil) 
                     action:@selector(ignoreWord:) 
              keyEquivalent:@"" 
                    atIndex:dictionaryItemsInsertBase];

...but also requires some additional code to remove that extra separator later on if it's not needed (just as is done for the suggestions separator item):

if (!showSpellingItems) {
  // word spelled correctly or not applicable, remove all traces of spelling items
  NSMenuItem* selectionItem;
  while ((selectionItem = [result itemWithTag:kSpellingRelatedItemsTag]) != nil)
    [result removeItem:selectionItem];
  while ((selectionItem = [result itemWithTag:kSpellingDictionaryItemsTag]) != nil)
    [result removeItem:selectionItem];
}

These additional steps required to remove the item could be kinda fragile here and make future changes difficult, since we're dealing with two locations in the code for one feature (not to mention a third location, which is IB).  Should I go back to inserting a separator dynamically and only when needed?  If so, is it better to calculate the index similar to the way my previous patch does, or find the index of the separator and base insertions upon it?

Put more generally, should IB only be used to create menu items which are always present (and create others dynamically only when needed, instead of removing them in cases which they're unneeded)?

I am torn, since tagging simplifies the insertion index calculation.  I'd appreciate anyone's opinion on this.

Comment 27

[Sean Murphy]

Attachment: learn and ignore patch v2

Well, here's a patch anyway ;)

Changed in this version are the method names, which are now ignoreWord: and learnWord:, as suggested.

Also modified a bit is the way in which the context menu items are inserted.  To accomplish this, I chose not to add another tagged separator in IB, since there was work required to remove it in cases where spell checking is not performed and the removal had to take place in a method outside of the area in which the dictionary-related items were inserted.  (See my above comment for more explanation.)

Comment 28

[Stuart Morgan]

Comment on attachment 249885 [details] [diff] [review]
learn and ignore patch v2

>+  // Uses the inline spell checker to obtain the currently mispelled word.
>+  nsCOMPtr<nsIEditor> editor;
>+  [self currentEditor:getter_AddRefs(editor)];
>+  ENSURE_TRUE(editor);
>+  
>+  nsCOMPtr<nsIInlineSpellChecker> inlineChecker;
>+  editor->GetInlineSpellChecker(PR_TRUE, getter_AddRefs(inlineChecker));
>+  ENSURE_TRUE(inlineChecker);
>+  
>+  nsCOMPtr<nsIDOMNode> anchorNode;
>+  PRInt32 anchorOffset = 0;
>+  GeckoUtils::GetAnchorNodeFromSelection(editor, getter_AddRefs(anchorNode), &anchorOffset);
>+  nsCOMPtr<nsIDOMRange> mispelledRange;
>+  inlineChecker->GetMispelledWord(anchorNode, (long)anchorOffset, getter_AddRefs(mispelledRange));
>+  ENSURE_TRUE(mispelledRange);
>+  
>+  nsString mispelledWord;
>+  mispelledRange->ToString(mispelledWord);

Rather than have essentially the same code three times (twice in your patch plus the original in prepareSpellingSuggestionMenu), make a helper function that returns the misspelled word for the current selection (or at least the range, if passing around a string pointer is a pain) or NULL if there isn't one, and use that in all three places.

Comment 29

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Comment on attachment 249885 [details] [diff] [review]
learn and ignore patch v2

>+// -ignoreWord:
>+//
>+// Context menu action for ignoring a mispelled word in a text field.

[...]

>+  
>+  // Uses the inline spell checker to obtain the currently mispelled word.

IMO, just because the Gecko spelling folks can't spell doesn't mean our comments have to be misspelled, too ;)

Comment 30

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Comment on attachment 249885 [details] [diff] [review]
learn and ignore patch v2

Eh, what the?

Bugzilla overwrote smorgan's flag change without even telling me, and made me a new requestee :/

Setting this back to r-

Comment 31

[Sean Murphy]

Attachment: learn and ignore patch v3

Alright, this update alleviates much of that duplicate code.  Sorry I didn't notice it myself and thanks for pointing out the problem.  There is still some degree of code repeatedly occurring, but most of the editor/checker/range determination has been segregated and future changes upon that area can happen in only one place now.

I couldn't think of any obvious way to combine any more behavior.  I thought about letting a single method accomplish the spell checking operations, but after some experimentation with that idea, this seemed more coherent anyway.

Here's -learnWord: after moving duplicate stuff into a helper method:

+  nsCOMPtr<nsIInlineSpellChecker> inlineChecker;
+  nsCOMPtr<nsIDOMRange> misspelledRange;
+  [self getMisspelledWordRange:getter_AddRefs(misspelledRange) inlineSpellChecker:getter_AddRefs(inlineChecker)];
+  
+  if (!misspelledRange || !inlineChecker)
+    return;
+  
+  nsString misspelledWord;
+  misspelledRange->ToString(misspelledWord);
+  inlineChecker->AddWordToDictionary(misspelledWord);

Other than variable declarations and error checking, there's about three lines of actual code now.

I chose to have the helper method return a range since replacing the word needs access to that range and not the entire string.  I'm not sure if you really meant to use a function to implement this, but I had to go the method route since access to the 'self' variable was necessary.

> IMO, just because the Gecko spelling folks can't spell doesn't mean our
> comments have to be misspelled, too ;)

Darn it, thanks for noticing that!  I'm pretty sure I fixed all occurrences of that troublesome word now.  I grepped out the comments and performed a spell check on them.

Comment 32

[Stuart Morgan]

Comment on attachment 249975 [details] [diff] [review]
learn and ignore patch v3

(In reply to comment #31)
> There is still some degree of code repeatedly occurring, but most of the
> editor/checker/range determination has been segregated and future changes
> upon that area can happen in only one place now.

Could prepareSpellingSuggestionMenu:tag: be changed to use your new helper method so there's only one copy of this code instead of two?

>+    // index of inTag has changed, so get the new location.
>+    int dictionaryItemsInsertBase = [inMenu indexOfItemWithTag:inTag];
>+    
>+    // insert dictionary-related menu items successively after the tagged item.
>+    [inMenu insertItemWithTitle:NSLocalizedString(@"Ignore Spelling", nil) 
>+                         action:@selector(ignoreWord:) 
>+                  keyEquivalent:@"" 
>+                        atIndex:(dictionaryItemsInsertBase + 1)];
>+                        
>+    [inMenu insertItemWithTitle:NSLocalizedString(@"Learn Spelling", nil) 
>+                         action:@selector(learnWord:) 
>+                  keyEquivalent:@"" 
>+                        atIndex:(dictionaryItemsInsertBase + 2)];
>+                        
>+    [inMenu insertItem:[NSMenuItem separatorItem] atIndex:(dictionaryItemsInsertBase + 3)];

The method doc says that items will be inserted before the tag, so this should be:

[inMenu insertItem:[NSMenuItem separatorItem] atIndex:dictionaryItemsInsertBase];
[inMenu insertItemWithTitle:... atIndex:(dictionaryItemsInsertBase + 1)];
[inMenu insertItemWithTitle:... atIndex:(dictionaryItemsInsertBase + 2)];


>+// Upon return, |outRange| contains the range of the currently misspelled word and 
>+// |outInlineChecker| contains the inline spell checker to allow for further action.

Could you add a comment that both will be AddRef'd?

>+- (void)getMisspelledWordRange:(nsIDOMRange**)outRange inlineSpellChecker:(nsIInlineSpellChecker**)outInlineChecker
...
>+  ENSURE_TRUE(outRange);

Remove this, since it doesn't do anything useful (it's going to immediately return anyway).

>-  // it's unfortunate that we have to re-fetch this stuff since we just did it
>-  // when buliding the context menu, but we don't really have any convenient place
>-  // to stash it where we can guarantee it will get cleaned up when the menu goes
>-  // away.

Don't remove this comment.


Since I'm not going to have time to test this before I head out for the rest of the week and I don't want to hold this up, r=me with those changes from a code standpoint, and someone else should do the behavioral testing part of the review.

Comment 33

[froodian (Ian Leue)]

I tested this, and it does comment 21.  That is, ignore and learn both just make the word not be underlined (no suggestions for "learned" words), and neither affect the system-wide dictionary (ie changes aren't carried over to other apps).  If that's what we're expecting, r=me behaviorally.

Comment 34

[Stuart Morgan]

We should preemptively file a bug on the fact that learn doesn't really work. We need to try to find a way to either persist back to the system dictionary or use the dictionary we are learning into at the moment for suggestions.

Comment 35

[Sean Murphy]

Thanks for the reviews.  I'm fixing up the patch now but have one additional question:

Should I formally declare my helper method in either the header or private category?  It's working without a compiler warning right now simply because it is listed sequentially before anything that calls it in the file.  Similar methods such as currentEditor:, focusController:, and the like are not explicitly protoytyped either at the moment.

I'm also following the r/sr status of bug 343296 and will support that hack in the helper method by adding "++anchorOffset."

Comment 36

[Nick Kreeger]

> Should I formally declare my helper method in either the header or private
> category?  

Judging by the patch from your last attachment, |getMisspelledWordRange:| is only called my internal methods, therefore it doesn't have any need to be exposed publicly, so stick it in the Private category. 

Two nits from the patch:

+  #define ENSURE_TRUE(x) if (!x) return;

Can't we define this at least at the top of the source file, I think I've seen this code repeated recently, it would be nice to find a place to globally declare macros.

+  (*outInlineChecker)->GetMispelledWord(anchorNode, (long)anchorOffset, outRange);

Is there any reason other than style that you added parens around |*outInlineChecker|?

Comment 37

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

(In reply to comment #34)
> We should preemptively file a bug on the fact that learn doesn't really work.

Bug 365883

Comment 38

[Sean Murphy]

I posted this over on the new bug 365883, but since there's no CCs there yet, I'll mention it here as well:

(In reply to comment #33)
> I tested this, and it does comment 21.  That is, ignore and learn both just
> make the word not be underlined (no suggestions for "learned" words), and
> neither affect the system-wide dictionary (ie changes aren't carried over to
> other apps).  If that's what we're expecting, r=me behaviorally.

Learned words do actually persist, but the problem is they do so in whatever
dictionary the mozilla API is using, rather than the standard OS X system
dictionary.  Ending an active editing session (reloading the page, or quitting
Camino) will reset ignored words, that's a temporary action, but learned
entries will continue to persist.  They do so only in Camino, however, and
that's what we have to fix.  Interestingly, they are not added to Firefox's
dictionary either.

Unless we change the way our spell checking is performed, a learned word must
still be added into the dictionary we use.  We would have to add a method which
would also inform the Cocoa spell check service to learn that same word.

NSSpellChecker does not provide any documented way of accomplishing this,
though.  I did find an undocumented -learnWord: method, which doesn't have the
normal underscore prefix Apple uses to explicitly indicate privacy, but it's
not declared in the header and probably not indented for public use.  I'm
almost always opposed to using private methods, since we can be sure exactly
"how" they work, and furthermore we'd have to ensure Apple never changes them
in a future OS release.  Having said that, I did test learnWord: by handing it
an NSString and it worked flawlessly, the added word was recognized correctly
throughout all Cocoa apps.

Comment 39

[Sean Murphy]

(In reply to comment #36)
> Judging by the patch from your last attachment, |getMisspelledWordRange:| is
> only called my internal methods, therefore it doesn't have any need to be
> exposed publicly, so stick it in the Private category. 

I will do that, thanks.  I figured that's what you guys would want, but wanted to double-check first before going ahead and doing it.

> 
> Two nits from the patch:
> 
> +  #define ENSURE_TRUE(x) if (!x) return;
> 
> Can't we define this at least at the top of the source file, I think I've seen
> this code repeated recently, it would be nice to find a place to globally
> declare macros.

Declaring it locally keeps looking at this code from having to jump all the way to the top of the file and figure out what exactly that macro does although an obvious and well-named macro like this one isn't too bad in that regard.  The other problem is that it's used in various methods, some of which have a return type other than void, which this one assumes.

> +  (*outInlineChecker)->GetMispelledWord(anchorNode, (long)anchorOffset,
> outRange);
> 
> Is there any reason other than style that you added parens around
> |*outInlineChecker|?

outInlineChecker is passed into the method as a double pointer (or, a pointer to a pointer).  The C infix operator (->) will take care of de-referencing the pointer once when referring to a struct, but I needed to de-reference it once more.  This would be the same as doing:

(**outInlineChecker).getMispelledWord()

Structure field dereference (".") takes precedence over the standard pointer dereference operator ("*"), so parentheses are required.

Additionally, I think it's much more practical to avoid requiring someone reading your code to have to remember any of the non-obvious precedence rules of a particular language.

Comment 40

[Sean Murphy]

s/"keeps"/"keeps someone"

Sorry for the bugmail ;)

Comment 41

[Sean Murphy]

Just wanted to let you guys know that I have an updated patch just about ready, in case this is destined for a2 tomorrow.

It's complete now, but I'm taking some more time to look it over myself first, though, because I ended up making quite a few changes - and modified the helper method in a way that made it re-usable to all spelling operations (prepareSpellingSuggestionMenu:tag: included).

After I make sure I didn't mess up anything obvious, and testing to ensure it works in all situations, I'll submit early tonight.

Comment 42

[Nick Kreeger]

> Declaring it locally keeps looking at this code from having to jump all the way
> to the top of the file and figure out what exactly that macro does although an
> obvious and well-named macro like this one isn't too bad in that regard.  The
> other problem is that it's used in various methods, some of which have a return
> type other than void, which this one assumes.


Yes, but this macro is just a simplification of the |NS_ENSURE_SUCESS| macro seen through out gecko code. The |NS_ENSURE_SUCCESS| macro takes two params (nsresult or pointer) and the return value. So there is no need to redeclare the macro for return types. It makes no sense to #define and #undef a macro in various functions/methods.

> outInlineChecker is passed into the method as a double pointer (or, a pointer
> to a pointer).  The C infix operator (->) will take care of de-referencing the
> pointer once when referring to a struct, but I needed to de-reference it once
> more.  This would be the same as doing:
> 
> (**outInlineChecker).getMispelledWord()
> 
> Structure field dereference (".") takes precedence over the standard pointer
> dereference operator ("*"), so parentheses are required.
> 
> Additionally, I think it's much more practical to avoid requiring someone
> reading your code to have to remember any of the non-obvious precedence rules
> of a particular language.
> 

Yes I'm aware of what double pointers do, I was suggesting removing the extra parens around the pointer as a style suggestion. I.e:

*outLineChecker->GetMispelledWord()

Comment 43

[Sean Murphy]

(In reply to comment #42)

> Yes, but this macro is just a simplification of the |NS_ENSURE_SUCESS| macro
> seen through out gecko code. The |NS_ENSURE_SUCCESS| macro takes two params
> (nsresult or pointer) and the return value. So there is no need to redeclare
> the macro for return types. It makes no sense to #define and #undef a macro in
> various functions/methods.

Even though I was kinda defending this macro in my last comment, I definitely agree with you.  It was already in place when I started working on this area of the code and I didn't want to just remove it.  As a slight improvement, my latest patch only #defines it for use in one method, not all over the place as was done previously.

> Yes I'm aware of what double pointers do, I was suggesting removing the extra
> parens around the pointer as a style suggestion. I.e:
> 
> *outLineChecker->GetMispelledWord()

outInlineChecker** needs to be dereferenced twice before calling GetMispelledWord() on the aggregate type it fundamentally points at.  Due to the precedence of operators here (the primary expression '->' comes before the unary '*'), removing the parens would attempt to call GetMispelledWord() on a pointer to nsIInlineSpellChecker, instead of the object itself, and then dereference the entire result.

So *outInlineChecker->GetMispelledWord() and (*outInlineChecker)->GetMispelledWord() do not exhibit the same behavior.  Each attempts to call GetMispelledWord(), but the first does so on a pointer which knows nothing about it, and the latter does so on an actual object containing that method.

My goal here isn't to sound like some kinda know-it-all, but mainly to just explain why I coded it that way.  Also, the more my patches are reviewed, the more I end up learning.  Anyway, thanks Nick for the suggestions.

Comment 44

[Sean Murphy]

Attachment: learn and ignore patch v3.1

Updated patch, with corrections.

> The method doc says that items will be inserted before the tag, so this should
> be:
> 
> [inMenu insertItem:[NSMenuItem separatorItem]
> atIndex:dictionaryItemsInsertBase];
> [inMenu insertItemWithTitle:... atIndex:(dictionaryItemsInsertBase + 1)];
> [inMenu insertItemWithTitle:... atIndex:(dictionaryItemsInsertBase + 2)];

I tried using these new indices, but with them Learn and Ignore were located directly under the suggested terms, without a separator.  Standard Cocoa text editing inserts a separator, ignore, and then learn after the suggested words.  Based upon that, I left these alone for now.

Comment 45

[Stuart Morgan]

How could it not have a separator between the suggested terms and Learn/Ignore when the first thing that block of code does is insert a separator, then insert Learn and Ignore after it?

If my math is wrong, adjust as necessary, but my point is that nothing should be inserted after the tagged separator.

Comment 46

[Sean Murphy]

Attachment: learn and ignore patch (modified menu indexing)

Sorry for the confusion.  I was wrong about the indexing and the mistake there was on my part.  Menu items are now added in the manner you suggested.

In this patch I went ahead and modified the flow of the prepareSpellingSuggestionMenu:tag: method slightly.  Since this was a matter of style, and is thus prone to opinion, I'm attaching two patches so the bug isn't held up if the previous flow-style is preferred.

Comment 47

[Sean Murphy]

Attachment: same as above, with new code flow when building sugg. menu

Just some style changes, no problem if we don't use it.

The diff'ed patch is kinda hard to read and isolate what style changes I made, so here's the method:
(basically removed the showSuggestions variable and took the menu building out of a nested if statement; the purpose of the return type is now commented since I removed the returned variable)

...
// Returns a BOOL indicating whether suggestions are shown.
- (BOOL)prepareSpellingSuggestionMenu:(NSMenu*)inMenu tag:(int)inTag
{
  nsCOMPtr<nsIDOMRange> misspelledRange;
  nsCOMPtr<nsIInlineSpellChecker> inlineChecker;
  [self getMisspelledWordRange:getter_AddRefs(misspelledRange) inlineSpellChecker:getter_AddRefs(inlineChecker)];
  if (!misspelledRange || !inlineChecker)
    return NO;
    
  nsString currentWord;
  misspelledRange->ToString(currentWord);
  
  nsCOMPtr<nsIEditorSpellCheck> spellCheck;
  inlineChecker->GetSpellChecker(getter_AddRefs(spellCheck));
  if (!spellCheck)
    return NO;
  
  // ask the spellchecker to check the misspelled word, which seems redundant
  // but is also used to generate the suggestions list. 
  PRBool isIncorrect = NO;
  spellCheck->CheckCurrentWord(currentWord.get(), &isIncorrect);
  
  // the word must be spelled correctly; bail now without building a suggestions menu.
  if (!isIncorrect)
    return NO;

  [...build spelling menu here...]

Comment 48

[Sean Murphy]

Sorry for the excessive commenting on this bug, but…

After running Camino in gdb, I realized that the isIncorrect variable isn't technically needed.  If the word is spelled correctly, nothing will be returned for misspelledRange and the entire method would have bailed before even reaching the check for isIncorrect.

Despite that, we might have to leave the code alone there.  A call to CheckCurrentWord() is what triggers the suggested words to actually be computed.  So, GetSuggestedWord() depends on CheckCurrentWord() having been previously called even though we already know the word to be spelled incorrectly by this point.

Additionally, we can't use NULL for &isIncorrect.  So, we might as well leave if (!isIncorrect) in the code for precautionary reasons, especially since it will look pretty strange to have a value declared and returned for isIncorrect and leave it unused.

Comment 49

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

We talked about this at today's meeting, and the only thing that murph's patch (iiuc) doesn't do yet that we want to have it do for 1.1 is to patch our private learned words list back into our suggestions list.

[12:19pm] pinkerton: if we can get "learn" to learn and suggest for just camino, i'm ok with that for 1.1

Comment 50

[Sean Murphy]

I've been continually examining the behavior of the mozilla spell checking API we're using, specifically, why learned words did in fact persist despite not being inserted into the standard NSSpellChecker user additions file (and therefore not available to other Cocoa applications).

We call inlineChecker->AddWordToDictionary(misspelledWord) inside of the learnWord: IBAction.  This fundamentally hands off learning behavior all the way down to an instance of mozIPersonalDictionary.  The personal dictionary object, in turn, saves user additions into a file entitled "persdict.dat" inside of Camino's Application Support folder.  (Note that additions to the persdict file are cached and written upon termination of the application).

So, without changing any of the underlying moz API, it's necessary to accompany AddWordToDictionary() with a message to the shared NSSpellChecker informing it learn the word as well.  Bug 365883 documents all of the details we've discovered about calling the private method learnWord: on NSSpellChecker.

I'm fairly certain that the only area of the mozilla API that is catered to OS X and interacts with NSSpellChecker is pink's mozOSXSpell.  An instance of this object is never informed about or is capable of adding words to a dictionary.

Comment 51

[Sean Murphy]

Just about ready with a patch here which will allow our Learn and Ignore spelling menu items to exhibit behavior identical to and consistent with any standard Cocoa text control.

There's just one final hitch which again concerns word learning.  Since we have to call learnWord: on NSSpellChecker, this should be the only learning action which needs to be invoked, meaning the mozilla API technique inlineChecker->AddWordToDictionary could then be removed.  The focused browser editor's inline spell checker does become aware of words learned in this manner, but the misspelled underline indication does not properly disappear.  When we only used the mozilla AddWordToDictionary method, the underline would be taken away properly.

Simply augmenting the message to NSSpellChecker's -learnWord: method with the AddWordToDictionary technique to handle removing the visual misspelled indication is not a good approach.  As I mentioned in my previous comment, AddWordToDictionary keeps track of learned words on its own, which means words forgotten (unlearned) elsewhere in the system would still be considered correctly spelled by Camino.

So basically, one more hurdle to clear and this functionality can be ready to get in place.  I just wanted to let everyone know where I was at with this stuff.  I'll keep working and get something in very soon.

Comment 52

[Sean Murphy]

Attachment: learn + ignore fully implemented

This patch completes functionality for learning and ignoring words.  The behavior demonstrated by our inline checker in regard to these two actions is now completely consistent with the parallel spelling features of the Cocoa text system.

(In reply to comment #49)
> We talked about this at today's meeting, and the only thing that murph's patch
> (iiuc) doesn't do yet that we want to have it do for 1.1 is to patch our
> private learned words list back into our suggestions list.
> 
> [12:19pm] pinkerton: if we can get "learn" to learn and suggest for just
> camino, i'm ok with that for 1.1

As it turns out, even words learned "the normal way" using the Cocoa text system itself are never added into the suggestion pool.  We thought it just wasn't working for Camino.  Our inline checker ultimately gets its suggestions from Cocoa's NSSpellChecker and words are now learned through this object as well.  If this was possible, we'd get that behavior (and if Apple introduces such functionality, we'll get it automatically and for free).  Learned words are now done so system-wide though, and available to all Cocoa applications.

Also note that we're using a currently undocumented |learnWord:| method on NSSpellChecker.  We've tested this method to work perfectly 10.3.9+ (10.3.0 support is not yet known).  You could consider it a further justification that the WebKit project seems to use this technique as well.  This patch ensures the NSSpellChecker instance responds to such a method before calling it, meaning it should fail gracefully if the undocumented API breaks in the future.

Some more discussion and further explanation about those two issues (learned words as suggestions and using learnWord:) can be found over on bug 365883.

Stuart, I'm going to target you for another review on this, and I appreciate the multiple looks you've now given to the patches I posted for this bug.

Comment 53

[Stuart Morgan]

Why is this patching around the underlying AddWordToDictionary behavior in the Camino layer instead of fixing mozOSXSpell?

Comment 54

[Sean Murphy]

(In reply to comment #53)
> Why is this patching around the underlying AddWordToDictionary behavior in the
> Camino layer instead of fixing mozOSXSpell?

mozOSXSpell is never called upon when using AddWordToDictionary.  mozOSXSpell implements the mozISpellCheckingEngine interface, which does not declare any mechanism for handling word actions.

Fixing this below the Camino layer would require substituting a new implementation of mozIPersonalDictionary.  I'm unsure of the best way to go about substituting the new dictionary at runtime, however.  mozOSXSpell returns a dictionary in GetPersonalDictionary, but I'm not aware of this method being called at all.  Another approach would be to override the appropriate contract id (as mozOSXSpell does to replace mySpell).

I'm pushing my C++ knowledge here but I will look into it some more and see if I'm capable of writing this code.  I was unable to find any previously filed Core bugs more appropriate for submitting work at that level (bug 301451, bug 343535, bug 151040, and bug 86886 are sort-of related).  Furthermore, pink submitted mozOSXSpell under Camino.  If you agree a new mozIPersonalDictionary implementation is the correct approach, should such work be submitted here or under a new Core bug?

Comment 55

[Stuart Morgan]

Okay, I hadn't realized how the interfaces were laid out. Subbing in a different personal dictionary implementation probably wouldn't make sense here, as the fact that NSSpellChecker doesn't seem to provide the concept of a separate user dictionary through the API would make implementing GetWordList problematic.

I'll take a look at the posted patch in the next few days.

Comment 57

[Stuart Morgan]

Comment on attachment 252982 [details] [diff] [review]
learn + ignore fully implemented

>+// NSSpellChecker's API does not currently expose word learning publicly,
>+// however an undocumented method |learnWord:| will accomplish the task.
>+// Declare it here since it's not present in NSSpellChecker.h.

The method name is self explanatory; just comment this as:
// Declare private NSSpellChecker method.
This whole thing should also be wrapped in a:
#if MAC_OS_X_VERSION_MAX_ALLOWED <= MAC_OS_X_VERSION_10_4

>+// Both parameters should be |AddRef|ed when calling this method.

This comment doesn't make any sense. It should say that it AddRefs the out paramaters.

>+- (void)getMisspelledWordRange:(nsIDOMRange**)outRange
...
>+  // it's unfortunate that we have to re-fetch this stuff again after buliding the
>+  // context menu, but we don't really have any convenient place to stash it where
>+  // we can guarantee it will get cleaned up when the menu goes away.

This comment doesn't belong in this method.

> // -replaceMispelledWord:
...
>-  GeckoUtils::GetAnchorNodeFromSelection(editor, getter_AddRefs(anchorNode), &anchorOffset);
>-  // once again, we shift the anchor off the beginning of the word to make the spelling system happy
>-  ++anchorOffset;
>-  
>+  misspelledRange->GetStartContainer(getter_AddRefs(anchorNode));
>+  misspelledRange->GetEndOffset(&anchorOffset);

That's the purpose of this change? And doesn't this give a completely different value for anchorOffset?

>+//
>+// -learnWord:
>+//
>+// Context menu action for adding a word to the spell checking dictionary.
>+//
>+// nsIInlineSpellChecker's AddWordToDictionary does not insert the learned
>+// word into the shared system dictionary and instead remembers it using a
>+// file named "persdict.dat" inside Camino's Application Support folder.

s/a file named .../its own personal dictionary/. Implementation details of a completely different component don't belong here.

>+// This technique recognizes 
>+// the word across the entire system as well as within our own spell checker.

Remove this, as it's redundant with the explanation preceding it.

>+  if ([[NSSpellChecker sharedSpellChecker] respondsToSelector:@selector(learnWord:)])

This can be removed, since you checked back into 10.3, and it's not going away.

Comment 58

[Sean Murphy]

(In reply to comment #57)

I have some follow-up below, but all other review comments have been addressed. Thanks.

> > // -replaceMispelledWord:
> ...
> >-  GeckoUtils::GetAnchorNodeFromSelection(editor, getter_AddRefs(anchorNode), &anchorOffset);
> >-  // once again, we shift the anchor off the beginning of the word to make the spelling system happy
> >-  ++anchorOffset;
> >-  
> >+  misspelledRange->GetStartContainer(getter_AddRefs(anchorNode));
> >+  misspelledRange->GetEndOffset(&anchorOffset);
> 
> That's the purpose of this change? And doesn't this give a completely different
> value for anchorOffset?

I was trying to utilize the helper method here.  The node and offset are obtained from the range object since it would go unused otherwise.  I might have to modify the getMispelledWordRange:inlineChecker: helper method if I want to rely on it here though, since obtaining a misspelledRange is currently required.  The reason for misspelledRange->GetEndOffset instead of GetStartOffset eliminates the need to do another ++anchorOffset (the spell checker recognizes the misspelled word when the cursor is at the end of the word, but not before it).

I'll look over this in detail tonight when I have more time.  At minimum, replaceMispelledWord: should use misspelledRange->GetEndContainer if the code you highlighted above stays, to concern itself in both cases with where the range ends.  If modifying the helper method or breaking its functionality up makes more sense, I'll consider that as well.

Elsewhere in the patch -

Instead of using GetAnchorNodeFromSelection in the helper method, we can just obtain an endNode and endOffset from the nsISelection directly.  Doing so eliminates the need to increment anchorOffset entirely:

nsCOMPtr<nsISelection> selection;
editor->GetSelection(getter_AddRefs(selection));

nsCOMPtr<nsIDOMNode> endNode;
PRInt32 endOffset = 0;

selection->GetFocusNode(getter_AddRefs(endNode));
selection->GetFocusOffset(&endOffset);

Comment 59

[Sean Murphy]

Attachment: review fixes

The four spelling-related methods (prepareSpellingSuggestionMenu:, replaceMispelledWord:, ignoreWord:, learnWord:) all differ just slightly enough in regard to the objects they require to perform their necessary behavior.  Such differences force a compromise when making a general-use helper function to segregate away common behavior amongst them.  Because of this, I didn't change around the helper method's implementation.

This patch contains all review corrections, plus the following differences from the previous patch:

-- (IBAction)replaceMispelledWord:(id)inSender
+- (IBAction)replaceWord:(id)inSender

I was tired of purposely spelling "misspelled" incorrectly, so I changed the method name and the menu item selector.  I'm not sure if there was a reason we didn't do that before, but this way the word should only be used incorrectly when dealing with the Core.

+  nsCOMPtr<nsISelection> selection;
+  editor->GetSelection(getter_AddRefs(selection));
+  ENSURE_TRUE(selection);
+
+  nsCOMPtr<nsIDOMNode> selectionEndNode;
+  PRInt32 selectionEndOffset = 0;
+  selection->GetFocusNode(getter_AddRefs(selectionEndNode));
+  selection->GetFocusOffset(&selectionEndOffset);

getMisspelledWordRange:inlineChecker: uses the ending node/offset for the current selection to determine which misspelled word range to return.  This eliminates a need to increment the offset when it was equal to the beginning of the selection instead.

+  misspelledRange->GetEndContainer(getter_AddRefs(endNode));
+  misspelledRange->GetEndOffset(&endOffset);

I knew there was another reason I avoided using GeckoUtils::GetAnchorNodeFromSelection in replaceWord: - it required a reference to the current editor, which was no longer local and had been moved into the helper method.  While obtaining a misspelledRange in replaceWord: is somewhat unnecessary, any other technique requires either modifying the helper method to accommodate one specific requirement (making it less general) or not utilizing the helper method and duplicating some code and the error checking that goes along with it.  The only change here is that the end of the misspelledRange is used to obtain both a node and offset this time.

Comment 60

[Stuart Morgan]

Comment on attachment 254544 [details] [diff] [review]
review fixes

r=me.

Comment 61

[Stuart Morgan]

Oh, one small nit: I'd rather see the string keys as @"LearnSpelling" and @"IgnoreSpelling", since not having the correct string as the key helps us not accidently forget strings in the strings file and mess up the localizers. That can just be fixed at checkin if mento doesn't have any changes.

Comment 62

[froodian (Ian Leue)]

Checked in with comment 61 on trunk and MOZILLA_1_8_BRANCH.