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

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect)

Description

[Scott MacGregor]

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.

Comment 1

[Scott MacGregor]

Attachment: 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.

Comment 2

[[e-mail]]

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); ?

Comment 3

[Scott MacGregor]

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

[[e-mail]]

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.

Comment 5

[Scott MacGregor]

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

[[e-mail]]

(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

[[e-mail]]

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?

Comment 8

[Scott MacGregor]

Attachment: remove hasPrefernce

Ok, I think this does what we want. It removes hasPreference altogether.

Comment 9

[[e-mail]]

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 :-)

Comment 10

[Scott MacGregor]

Attachment: fix as checked in

Updated fix that I'll be checking in based on Neil's feedback.

Comment 11

[Scott MacGregor]

fixed on the trunk. 

Comment 12

[Scott MacGregor]

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.

Comment 13

[Daniel Veditz [:dveditz]]

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

Comment 14

[Marcia Knous [:marcia]]

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.