Last Comment Bug 687319 - Spell check dialog shows no misspelled word and no suggestions
: Spell check dialog shows no misspelled word and no suggestions
Status: RESOLVED FIXED
[tbtrack9]
: regression
Product: Core
Classification: Components
Component: Spelling checker (show other bugs)
: Trunk
: x86 All
: -- normal (vote)
: mozilla9
Assigned To: David :Bienvenu
:
:
Mentors:
: 687420 (view as bug list)
Depends on:
Blocks: 591780
  Show dependency treegraph
 
Reported: 2011-09-18 01:22 PDT by Richard Marti (:Paenglab)
Modified: 2013-12-27 14:23 PST (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
-


Attachments
fix moz-central part of issue (1.04 KB, patch)
2011-09-22 16:35 PDT, David :Bienvenu
ehsan: review+
Details | Diff | Splinter Review
use get/setcurrentdictionary (3.42 KB, patch)
2011-09-22 16:40 PDT, David :Bienvenu
no flags Details | Diff | Splinter Review
fix setting dictionary (4.71 KB, patch)
2011-09-23 12:39 PDT, Jesper Kristensen
no flags Details | Diff | Splinter Review

Description Richard Marti (:Paenglab) 2011-09-18 01:22:11 PDT
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.
Comment 1 Richard Marti (:Paenglab) 2011-09-18 03:04:37 PDT
I tried it also under Linux with the same result.
Comment 2 Mark Banner (:standard8) 2011-09-19 02:47:54 PDT
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.
Comment 3 Mark Banner (:standard8) 2011-09-19 02:50:25 PDT
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).
Comment 4 Jesper Kristensen 2011-09-19 10:07:48 PDT
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.
Comment 5 Ian Neal 2011-09-19 13:08:17 PDT
*** Bug 687420 has been marked as a duplicate of this bug. ***
Comment 6 :Ehsan Akhgari 2011-09-19 13:29:13 PDT
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...
Comment 7 :Ehsan Akhgari 2011-09-21 14:11:27 PDT
Bienvenu: see comment 6 please.
Comment 8 David :Bienvenu 2011-09-22 08:18:06 PDT
yeah, this is pretty broken on the trunk. I'll try to fix this before we go to Aurora.
Comment 9 David :Bienvenu 2011-09-22 14:05:27 PDT
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.
Comment 10 David :Bienvenu 2011-09-22 14:07:47 PDT
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.
Comment 11 Jesper Kristensen 2011-09-22 14:18:25 PDT
(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.
Comment 12 :Ehsan Akhgari 2011-09-22 14:58:04 PDT
Yes, indeed that seems to be a bug which should be fixed.  Sorry for not catching that in the review process.  :/
Comment 13 David :Bienvenu 2011-09-22 16:35:01 PDT
Created attachment 561921 [details] [diff] [review]
fix moz-central part of issue

I'll attach a patch for the get/setcurrentdictionary changes as well.
Comment 14 David :Bienvenu 2011-09-22 16:40:45 PDT
Created attachment 561924 [details] [diff] [review]
use get/setcurrentdictionary
Comment 15 :Ehsan Akhgari 2011-09-22 19:06:49 PDT
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.
Comment 16 Jesper Kristensen 2011-09-23 07:28:24 PDT
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.
Comment 17 David :Bienvenu 2011-09-23 07:30:12 PDT
(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.
Comment 18 Jesper Kristensen 2011-09-23 12:39:33 PDT
Created attachment 562135 [details] [diff] [review]
fix setting dictionary

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.
Comment 19 David :Bienvenu 2011-09-23 12:46:06 PDT
thx, Jesper, yes, that should be covered in a new bug.
Comment 20 :Ehsan Akhgari 2011-09-23 12:49:08 PDT
So, David, is your m-c patch enough here?  We should land that patch before the migration day (Tuesday).
Comment 21 David :Bienvenu 2011-09-23 12:53:04 PDT
(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 22 Jesper Kristensen 2011-09-23 13:39:41 PDT
Comment on attachment 562135 [details] [diff] [review]
fix setting dictionary

(In reply to David :Bienvenu from comment #19)
filed bug 688860
Comment 23 Ed Morley [:emorley] 2011-09-23 20:48:43 PDT
https://hg.mozilla.org/mozilla-central/rev/7226c37145cd
Comment 24 :Ms2ger (⌚ UTC+1/+2) 2011-09-24 04:29:00 PDT
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?
Comment 25 David :Bienvenu 2011-09-24 06:11:10 PDT
(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).
Comment 26 Jesper Kristensen 2011-09-24 06:29:18 PDT
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.

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