Closed Bug 687319 Opened 8 years ago Closed 8 years ago

Spell check dialog shows no misspelled word and no suggestions

Categories

(Core :: Spelling checker, defect)

x86
All
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla9
Tracking Status
firefox9 - ---

People

(Reporter: Paenglab, Assigned: Bienvenu)

References

Details

(Keywords: regression, Whiteboard: [tbtrack9])

Attachments

(1 file, 2 obsolete files)

On writing a message the misspelled words are correctly underlined. When checking with F7 the spell check window opens but no misspelled word nor a suggestion is shown.

When check before sending is enabled, the spell check window opens with the same behavior but the buttons on bottom right are now 'Stop' and 'Close' instead of 'Stop' and 'Send'.

I have no error in the error console found which is related to this bug.

Last good: Mozilla/5.0 (Windows NT 6.1; rv:9.0a1) Gecko/20110914 Thunderbird/9.0a
First bad: Mozilla/5.0 (Windows NT 6.1; rv:9.0a1) Gecko/20110915 Thunderbird/9.0a

This bug is no dupe of bug 686878 because the other is about dictionary installation problems.
Keywords: regression
I tried it also under Linux with the same result.
OS: Windows 7 → All
Nothing obvious in c-c changes:

http://hg.mozilla.org/comm-central/pushloghtml?fromchange=77a6a1df6eb3&tochange=dc0820370836

m-c changes are:

http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=cfd1e3533f0f&tochange=5faaa652594f

Therefore I'd bet on a regression from bug 591780.
Blocks: 591780
Component: Message Compose Window → Spelling checker
Product: Thunderbird → Core
QA Contact: message-compose → spelling-checker
I think this needs tracking - I'm not sure if the UI for this dialog is in Firefox, but even if it isn't something has broken which could affect add-ons (and does affect other apps).
Summary: Spell check shows no misspelled word and no suggestions → Spell check dialog shows no misspelled word and no suggestions
Whiteboard: [tbtrack9]
When looking through the code, I found three bugs, but I don't know if they are related to this.

http://mxr.mozilla.org/comm-central/search?string=spellChecker.dictionary+%3D&find=&findi=&filter=^[^\0]*%24&hitlimit=&tree=comm-central
I see three places in the code using mozISpellCheckingEngine.dictionary=x, which I would assume to be wrong since bug 338427, and even more wrong with the combination of bug 338427 and bug 591780. The correct way to set the dictionary would be nsIInlineSpellChecker.spellChecker.SetCurrentDictionary(x)

Setting the current language directly on mozISpellCheckingEngine would cause strange changes in the current language whenever an editor with inline spell checking gains or looses focus.
Duplicate of this bug: 687420
Mark, can you try fixing the stuff in comment 4?  FWIW, this UI is not in Firefox, and this looks like a comm-central bug to me...
Bienvenu: see comment 6 please.
yeah, this is pretty broken on the trunk. I'll try to fix this before we go to Aurora.
Issue seems to be that mozSpellChecker has no converter, which breaks spell checking. SetCurrentDictionary used to 

mozSpellChecker::SetCurrentDictionary(const nsAString &aDictionary)
{

...

  nsXPIDLString language;
  
  nsCOMPtr<mozISpellI18NManager> serv(do_GetService("@mozilla.org/spellchecker/i18nmanager;1", &res));
  if(serv && NS_SUCCEEDED(res)){
    res = serv->GetUtil(language.get(),getter_AddRefs(mConverter));

But it no longer does that. The patch in Bug 591780 removed the code, which breaks our spell checking.
Jesper, any idea why that code was removed?

I did make the changes locally you suggested about Get/SetCurrentDictionary, which I suspect are necessary, but not sufficient.
(In reply to David :Bienvenu from comment #10)
> Jesper, any idea why that code was removed?

No, I don't remember having any intension of removing that.
Yes, indeed that seems to be a bug which should be fixed.  Sorry for not catching that in the review process.  :/
I'll attach a patch for the get/setcurrentdictionary changes as well.
Assignee: nobody → dbienvenu
Attachment #561921 - Flags: review?(ehsan)
Attached patch use get/setcurrentdictionary (obsolete) — Splinter Review
Attachment #561924 - Flags: review?(mbanner)
Comment on attachment 561921 [details] [diff] [review]
fix moz-central part of issue

>+      return serv->GetUtil(language.get(),getter_AddRefs(mConverter));

Nit: space after comma.

r=me.
Attachment #561921 - Flags: review?(ehsan) → review+
Comment on attachment 561924 [details] [diff] [review]
use get/setcurrentdictionary

That does not make sense to me. You are trying to call a method from nsIEditorSpellCheck on an object implementing mozISpellCheckingEngine.
(In reply to Jesper Kristensen from comment #16)
> Comment on attachment 561924 [details] [diff] [review] [diff] [details] [review]
> use get/setcurrentdictionary
> 
> That does not make sense to me. You are trying to call a method from
> nsIEditorSpellCheck on an object implementing mozISpellCheckingEngine.

Ah, I misunderstood your previous comment, then.
Attachment #561924 - Attachment is obsolete: true
Attachment #561924 - Flags: review?(mbanner)
Attached patch fix setting dictionary (obsolete) — Splinter Review
This is what I meant. I tested it, and it doesn't fix this bug. It however fixes another bug: When more than one dictionary is installed, you sometimes cannot switch between them for inline spell check. Maybe I should file that as a separate bug.
thx, Jesper, yes, that should be covered in a new bug.
So, David, is your m-c patch enough here?  We should land that patch before the migration day (Tuesday).
(In reply to Ehsan Akhgari [:ehsan] from comment #20)
> So, David, is your m-c patch enough here?  We should land that patch before
> the migration day (Tuesday).

Yeah, I've been trying to get set up with mozilla-inbound so I can push there. Which I have just now done:

http://hg.mozilla.org/integration/mozilla-inbound/rev/7226c37145cd
Comment on attachment 562135 [details] [diff] [review]
fix setting dictionary

(In reply to David :Bienvenu from comment #19)
filed bug 688860
Attachment #562135 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/7226c37145cd
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla9
Comment on attachment 561921 [details] [diff] [review]
fix moz-central part of issue

(In reply to Ehsan Akhgari [:ehsan] from comment #15)
> Comment on attachment 561921 [details] [diff] [review] [diff] [details] [review]
> fix moz-central part of issue
> 
> >+      return serv->GetUtil(language.get(),getter_AddRefs(mConverter));
> 
> Nit: space after comma.

You forget to address this.

Also, I don't understand what the language local is for. Surely language.get() is just null?
(In reply to Ms2ger from comment #24)

> 
> You forget to address this.
Oh, sorry, I did fix it locally, but then we I decided to use moz-inbound, I forgot to carry over that change.
> 
> Also, I don't understand what the language local is for. Surely
> language.get() is just null?

for both this, and the lack of space after the comma, I'm just restoring code that was removed before, inadvertently, from what Jesper said. In essence, backing out a small part of his patch. I have no idea why the code was the way it was before, but yeah, you're right, it would be the empty string (not null, I don't think).
Generally the spell checker interfaces contain a lot of methods, attributes and parameters "for future use". See for example the many methods in mozHunspell.cpp which return NS_ERROR_NOT_IMPLEMENTED. The first parameter to GetUtil is such an example. The value is never used, so you can pass in whatever you want. It would probably also be fine to just call GetUtil once in the constructor instead of every time the current dictionary is changed.
You need to log in before you can comment on or make changes to this bug.