Last Comment Bug 378795 - nsEditorSpellCheck is not automatically picking and setting a dictionary when spellchecker.dictionary is empty
: nsEditorSpellCheck is not automatically picking and setting a dictionary when...
Status: RESOLVED FIXED
: verified1.8.1.4
Product: Core
Classification: Components
Component: Editor (show other bugs)
: Trunk
: x86 Windows Vista
: -- normal (vote)
: ---
Assigned To: Scott MacGregor
:
:
Mentors:
Depends on:
Blocks: 367533
  Show dependency treegraph
 
Reported: 2007-04-25 14:59 PDT by Scott MacGregor
Modified: 2007-06-11 12:12 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
fix the broken logic (1.05 KB, patch)
2007-04-25 15:01 PDT, Scott MacGregor
no flags Details | Diff | Splinter Review
remove hasPrefernce (2.01 KB, patch)
2007-04-26 15:03 PDT, Scott MacGregor
neil: superreview+
Details | Diff | Splinter Review
fix as checked in (2.13 KB, patch)
2007-04-27 11:39 PDT, Scott MacGregor
dveditz: approval1.8.1.4+
Details | Diff | Splinter Review

Description Scott MacGregor 2007-04-25 14:59:33 PDT
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 2007-04-25 15:01:42 PDT
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.
Comment 2 neil@parkwaycc.co.uk 2007-04-25 15:52:13 PDT
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 2007-04-25 15:57:00 PDT
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 neil@parkwaycc.co.uk 2007-04-25 16:57:35 PDT
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 2007-04-25 18:25:11 PDT
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 neil@parkwaycc.co.uk 2007-04-26 08:45:42 PDT
(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 neil@parkwaycc.co.uk 2007-04-26 09:01:23 PDT
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 2007-04-26 15:03:25 PDT
Created attachment 262948 [details] [diff] [review]
remove hasPrefernce

Ok, I think this does what we want. It removes hasPreference altogether.
Comment 9 neil@parkwaycc.co.uk 2007-04-27 01:57:18 PDT
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 2007-04-27 11:39:24 PDT
Created attachment 263037 [details] [diff] [review]
fix as checked in

Updated fix that I'll be checking in based on Neil's feedback.
Comment 11 Scott MacGregor 2007-04-27 11:42:28 PDT
fixed on the trunk. 
Comment 12 Scott MacGregor 2007-04-27 11:48:35 PDT
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] 2007-04-30 10:23:36 PDT
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 - use ni] 2007-06-11 12:12:49 PDT
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.

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