The default bug view has changed. See this FAQ.

nsEditorSpellCheck is not automatically picking and setting a dictionary when spellchecker.dictionary is empty

RESOLVED FIXED

Status

()

Core
Editor
RESOLVED FIXED
10 years ago
10 years ago

People

(Reporter: Scott MacGregor, Assigned: Scott MacGregor)

Tracking

({verified1.8.1.4})

Trunk
x86
Windows Vista
verified1.8.1.4
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

10 years ago
If you download a version of Thunderbird or seamonkey that doesn't come bundled with a dictionary, and you then install a dictionary, the inline spell checker does not select that dictionary by default when you bring up a compose window.

See:

http://mxr.mozilla.org/seamonkey/source/editor/composer/src/nsEditorSpellCheck.cpp#197

When initializing the inline spell checker, nsEditorSpellCheck is supposed to:

1) Use the dictionary specified by spellchecker.dictionary if it has a value.
2) If that fails, try a dictionary that maps to the current language
3) If that fails, pick the first dictionary in the list.

There's a logic bug in the code that doesn't account for spellchecker.dictionary having an empty string as a value. As a result, it keeps us from picking the first dictionary in the list and automatically using that as the default dictionary.

This makes for a poor first run experience after installing the first dictionary into your profile since you have to manually select the dictionary before it gets used. 

Fix coming up.
(Assignee)

Comment 1

10 years ago
Created attachment 262806 [details] [diff] [review]
fix the broken logic

fix the broken logic, only act as if we had a preferred dictionary to use if spellchecker.dictionary actually has a non empty value.
Attachment #262806 - Flags: superreview?(neil)
(Assignee)

Updated

10 years ago
Summary: nsEditorSpellCheck for picking a dictionary is faulty when spellchecker.dictionary is empty → nsEditorSpellCheck logic for picking a dictionary is faulty when spellchecker.dictionary is empty

Comment 2

10 years ago
Comment on attachment 262806 [details] [diff] [review]
fix the broken logic

Out of interest, do you know who's saving an empty value for that preference?

>       prefString->ToString(getter_Copies(dictName));
While you're here would this be better as prefString->GetData(dictName); ?
(Assignee)

Comment 3

10 years ago
In CVS I see:

suite/browser-prefs.js
and
all-thunderbird.js

For en-US, all-l10n.js overrides the empty string with en-US so they don't notice. But many other thunderbird locales don't override this pref, probably because they don't ship with a dictionary by default.

Comment 4

10 years ago
Comment on attachment 262806 [details] [diff] [review]
fix the broken logic

So, the current logic is this:
1. Try to set the dictionary to the preference, or if it is empty then the locale.
2. If there is no dictionary for the preference, give up.
3. If there is no dictionary for the locale, then pick a dictionary and save it as the preference.
Except 2. always gives up, which is what this patch fixes.
Should we pick a dictionary if we weren't able to use the preference (or locale if the preference is blank)? Basically that would just mean eliminating hasPreference altogether.
(Assignee)

Comment 5

10 years ago
I wouldn't object to removing hasPreference altogether. 

Or we could remove the pref all together from suite/browser-prefs.js and all-thunderbird.js and make sure call sites that get the pref can handle the pref not existing...

Comment 6

10 years ago
(In reply to comment #5)
>Or we could remove the pref all together from suite/browser-prefs.js and
>all-thunderbird.js and make sure call sites that get the pref can handle the
>pref not existing...
I'm not sure how the preference widgets will cope with that...

Comment 7

10 years ago
So, do we a) want to pick a dictionary if the pref specifies an invalid dictionary b) want to save the picked dictionary at all times?
(Assignee)

Comment 8

10 years ago
Created attachment 262948 [details] [diff] [review]
remove hasPrefernce

Ok, I think this does what we want. It removes hasPreference altogether.
Attachment #262806 - Attachment is obsolete: true
Attachment #262948 - Flags: superreview?(neil)
Attachment #262806 - Flags: superreview?(neil)
(Assignee)

Updated

10 years ago
Summary: nsEditorSpellCheck logic for picking a dictionary is faulty when spellchecker.dictionary is empty → nsEditorSpellCheck is not automatically pickingand setting a dictionary when spellchecker.dictionary is empty
(Assignee)

Updated

10 years ago
Summary: nsEditorSpellCheck is not automatically pickingand setting a dictionary when spellchecker.dictionary is empty → nsEditorSpellCheck is not automatically picking and setting a dictionary when spellchecker.dictionary is empty

Comment 9

10 years ago
Comment on attachment 262948 [details] [diff] [review]
remove hasPrefernce

>+    if (NS_SUCCEEDED(rv) && prefString) 
>       prefString->ToString(getter_Copies(dictName));
I'd still prefer if you could change this to use GetData ;-)

>+  // If there was no dictionary specified by spellchecker.dictionary and setting it to the locale dictionary didn't
>   // work, try to use the first dictionary we find. This helps when the first
>+  // dictionary is installed 
I hope you rewrap this comment before checking it in :-)
Attachment #262948 - Flags: superreview?(neil) → superreview+
(Assignee)

Comment 10

10 years ago
Created attachment 263037 [details] [diff] [review]
fix as checked in

Updated fix that I'll be checking in based on Neil's feedback.
Attachment #262948 - Attachment is obsolete: true
(Assignee)

Comment 11

10 years ago
fixed on the trunk. 
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
(Assignee)

Comment 12

10 years ago
Comment on attachment 263037 [details] [diff] [review]
fix as checked in

We don't ship a dictionary for many locales. The first time the user installs a dictionary, we currently aren't selecting the dictionary for use with the inline spell checker. The user must manually choose the dictionary before nsEditorSpellCheck would use it. 

The risk to Firefox is very low as only thunderbird and seamonkey are setting spellchecker.dictionary to an empty string in their default pref files for non en-US locales. This is a regression in moz 1.8.1 over moz 1.8 due to changes made for firefox to support inline spell check.
Attachment #263037 - Flags: approval1.8.1.4?
Comment on attachment 263037 [details] [diff] [review]
fix as checked in

approved for 1.8.1.4, a=dveditz for release-drivers if you land today
Attachment #263037 - Flags: approval1.8.1.4? → approval1.8.1.4+
(Assignee)

Updated

10 years ago
Keywords: fixed1.8.1.4
verified on the 1.8 branch using version 2.0.0.4 (20070604). I downloaded the en-GB build out of the FTP candidate directory (did not have a dictionary installed by default). I then installed the en_GB dictionary and opened a compose window, and it was invoked by default. Bravo! Adding verified keyword.
Keywords: fixed1.8.1.4 → verified1.8.1.4
You need to log in before you can comment on or make changes to this bug.