335607 - Make dictionary selection persistent

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Brett Wilson]

We need to have a preference to expose at least disable + enable by default for multiline controls. We could also add (but it's probably too much) the ability to turn it on by default for all controls.

Comment 1

[Brett Wilson]

After talking to Ben, we won't be putting this in the options dialog since all commands are available via the context menu. However, we should make the language selection in this menu sticky. Right now, the selection is set only for that specific element.

Comment 2

[Brett Wilson]

Attachment: Patch

The way the inline spellchecker works is that Uninit never gets called, so that we never save the default dictionary. Therefore, I added a new function to force saving the default right now.

I also changed the way that the initial dictionary selection works. If there is no dictionary option and we can't set it to be the current locale, we just use the first dictionary and set it as default. This way, if a user with no dictionary installs one, they get it automatically set as the default.

This patch also fixes the editorspellcheck QI call.

Comment 3

[Scott MacGregor]

Comment on attachment 220039 [details] [diff] [review]
Patch

Even though these changes are all spell check related, I always like to get a moa review from glazman since the changes live in mozilla\editor.

Do we need to bump the IID on nsIEditorSpellCheck_MOZILLA_1_8_BRANCH? Or are we avoiding that because it's a new interface for the 1.8 branch...

I'll approve for the branch once Daniel gives it an moa.

Comment 4

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

Comment on attachment 220039 [details] [diff] [review]
Patch

>@@ -176,22 +178,24 @@ nsEditorSpellCheck::InitSpellChecker(nsI
> 
>   // Tell the spellchecker what dictionary to use:
> 
>   nsXPIDLString dictName;
> 
>   nsCOMPtr<nsIPrefBranch> prefBranch =
>     do_GetService(NS_PREFSERVICE_CONTRACTID, &rv);
> 
>+  PRBool hasPreference = PR_FALSE;
>   if (NS_SUCCEEDED(rv) && prefBranch) {
>     nsCOMPtr<nsISupportsString> prefString;
>     rv = prefBranch->GetComplexValue("spellchecker.dictionary",
>                                      NS_GET_IID(nsISupportsString),
>                                      getter_AddRefs(prefString));
>     if (prefString) {
>+      hasPreference = PR_TRUE;
>       prefString->ToString(getter_Copies(dictName));
>     }
>   }
> 
>   if (NS_FAILED(rv) || dictName.IsEmpty())

Should you turn that test into
   if (!hasPreference || dictName.IsEmpty())
and don't assign rv above ?

To be honest, I really dislike having an interface called nsIEditorSpellCheck_MOZILLA_1_8_BRANCH
but I am sure you dislike it too and you probably have very good reasons for that.

Comment 5

[Brett Wilson]

(In reply to comment #3)
> Do we need to bump the IID on nsIEditorSpellCheck_MOZILLA_1_8_BRANCH?

Yes, I'll do that before checking in. Thanks for catching it.

(In reply to comment #4)
> Should you turn that test into
>    if (!hasPreference || dictName.IsEmpty())
> and don't assign rv above ?

Sounds good.

> To be honest, I really dislike having an interface called
> nsIEditorSpellCheck_MOZILLA_1_8_BRANCH
> but I am sure you dislike it too and you probably have very good reasons for
> that.

Yeah, they made me do that :(

Comment 6

[Brett Wilson]

Attachment: Updated patch

Comment 7

[Brett Wilson]

Oops, I forgot to describe what this was:

I updated the UUID for the branch editor interface and did the if-statemet change like daniel suggestd

Comment 8

[Brian Ryner (not reading)]

Comment on attachment 220167 [details] [diff] [review]
Updated patch

>+  // If there was no preference 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 - it will get set as the default. If there was
>+  // a preference but we can't set it, don't do anything. If the user's selected
>+  // dictionary went missing, we don't want to set it to a random dictionary.

Can you clarify this comment a bit?  In particular, when you say "but we can't set it", it's somewhat abiguous whether you mean "set the preference" (which is how I read it at first), or "set the current dictionary to the pref value".

Code changes all look good.

Comment 9

[Brett Wilson]

Attachment: Wrong patch

Comment 10

[Brett Wilson]

On branch, leaving open for commit to trunk.

Comment 11

[Brett Wilson]

Attachment: Trunk patch

Comment 12

[Brett Wilson]

Fixed on trunk

Comment 13

[Gervase Markham [:gerv]]

Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h".

In Thunderbird 3.0b, you do that as follows:
Tools | Message Filters
Make sure the correct account is selected. Click "New"
Conditions: Body   contains   places-to-b-and-h
Change the action to "Delete Message".
Select "Manually Run" from the dropdown at the top.
Click OK.

Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter.

Gerv