Closed
Bug 687319
Opened 13 years ago
Closed 13 years ago
Spell check dialog shows no misspelled word and no suggestions
Categories
(Core :: Spelling checker, defect)
Tracking
()
RESOLVED
FIXED
mozilla9
Tracking | Status | |
---|---|---|
firefox9 | - | --- |
People
(Reporter: Paenglab, Assigned: Bienvenu)
References
Details
(Keywords: regression, Whiteboard: [tbtrack9])
Attachments
(1 file, 2 obsolete files)
1.04 KB,
patch
|
ehsan.akhgari
:
review+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Updated•13 years ago
|
Keywords: regression
Reporter | ||
Comment 1•13 years ago
|
||
I tried it also under Linux with the same result.
OS: Windows 7 → All
Comment 2•13 years ago
|
||
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
Comment 3•13 years ago
|
||
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).
tracking-firefox9:
--- → ?
Summary: Spell check shows no misspelled word and no suggestions → Spell check dialog shows no misspelled word and no suggestions
Updated•13 years ago
|
Whiteboard: [tbtrack9]
Comment 4•13 years ago
|
||
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 6•13 years ago
|
||
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...
Assignee | ||
Comment 8•13 years ago
|
||
yeah, this is pretty broken on the trunk. I'll try to fix this before we go to Aurora.
Assignee | ||
Comment 9•13 years ago
|
||
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.
Assignee | ||
Comment 10•13 years ago
|
||
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•13 years ago
|
||
(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•13 years ago
|
||
Yes, indeed that seems to be a bug which should be fixed. Sorry for not catching that in the review process. :/
Assignee | ||
Comment 13•13 years ago
|
||
I'll attach a patch for the get/setcurrentdictionary changes as well.
Assignee: nobody → dbienvenu
Attachment #561921 -
Flags: review?(ehsan)
Assignee | ||
Comment 14•13 years ago
|
||
Attachment #561924 -
Flags: review?(mbanner)
Comment 15•13 years ago
|
||
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 16•13 years ago
|
||
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.
Assignee | ||
Comment 17•13 years ago
|
||
(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.
Assignee | ||
Updated•13 years ago
|
Attachment #561924 -
Attachment is obsolete: true
Attachment #561924 -
Flags: review?(mbanner)
Comment 18•13 years ago
|
||
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.
Assignee | ||
Comment 19•13 years ago
|
||
thx, Jesper, yes, that should be covered in a new bug.
Comment 20•13 years ago
|
||
So, David, is your m-c patch enough here? We should land that patch before the migration day (Tuesday).
Assignee | ||
Comment 21•13 years ago
|
||
(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•13 years ago
|
||
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
Comment 23•13 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/7226c37145cd
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla9
Comment 24•13 years ago
|
||
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?
Assignee | ||
Comment 25•13 years ago
|
||
(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•13 years ago
|
||
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.
Updated•13 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•