Closed Bug 378795 Opened 14 years ago Closed 14 years ago
Editor Spell Check is not automatically picking and setting a dictionary when spellchecker .dictionary is empty
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.
fix the broken logic, only act as if we had a preferred dictionary to use if spellchecker.dictionary actually has a non empty value.
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 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); ?
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 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.
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...
(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...
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?
Ok, I think this does what we want. It removes hasPreference altogether.
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
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 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+
Updated fix that I'll be checking in based on Neil's feedback.
Attachment #262948 - Attachment is obsolete: true
fixed on the trunk.
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
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: approval220.127.116.11?
Comment on attachment 263037 [details] [diff] [review] fix as checked in approved for 18.104.22.168, a=dveditz for release-drivers if you land today
Attachment #263037 - Flags: approval22.214.171.124? → approval126.96.36.199+
verified on the 1.8 branch using version 188.8.131.52 (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.
You need to log in before you can comment on or make changes to this bug.