528893 - Dictionaries are not human-readable in compose window

Status: VERIFIED FIXED

(Thunderbird :: OS Integration, defect)

Description

[Cristian KLEIN]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; fr; rv:1.9.1.5) Gecko/20091114 Ubuntu/9.10 (karmic) Firefox/3.5.5
Build Identifier: version 2.0.0.23 (20091115)

Linux stores dictionaries in /usr/share/myspell/dicts in file-names like "en_US". Thunderbird only recognises "-" as a valid locale / region separator. As a result, the spell-checker displays dictionaries like: en_US, en_GB, ro_RO instead of English (US), English (GB), Romanian (Romania).

Reproducible: Always

Steps to Reproduce:
1. Open a compose window.
2. Click on the body of the message to be composed.
3. Click on Spelling button.
Actual Results:  
The menu shows:
- en_US
- en_GB
- ro_RO



Expected Results:  
The menu should be:
- English (US)
- English (GB)
- Romanian (RO)

Some Linux distributions, for some dictionaries, work this problem around by including symlinks (e.g. en-US -> en_US). This is horrible as the user sees both human-readable and non-human-readable version of the same dictionaries and is very bad for UI experience.

This bug is similar to a recently fixed bug in Firefox: https://bugzilla.mozilla.org/show_bug.cgi?id=514151

Comment 1

[Cristian KLEIN]

Attachment: Patch against the source code of Thunderbird 2.0.0.23.

Comment 2

[Cristian KLEIN]

Note: this problem persists in Thunderbird trunk:

--- cut here ---
$ hg clone http://hg.mozilla.org/comm-central/
$ grep -R 'split("-")' *
calendar/base/content/calendar-base-view.xml:            let typelist = this.id.split("-");
editor/ui/dialogs/content/EdSpellCheck.js:      isoStrArray = dictList[i].split("-");
mail/components/compose/content/MsgComposeCommands.js:      isoStrArray = dictList[i].split("-");
mail/components/preferences/compose.js:        isoStrArray = dictList[i].split("-");
suite/browser/metadata.js:    var tokens = abbr.split("-");
suite/mailnews/compose/prefs/pref-composing_messages.js:      isoStrArray = dictList[i].split("-");
suite/mailnews/compose/MsgComposeCommands.js:      isoStrArray = dictList[i].split("-");
suite/common/bookmarks/bm-props.js:          var hours = values[1].split("-");
cristi@hades:~/dev/comm-central$ grep -R 'split("-")' *
calendar/base/content/calendar-base-view.xml:            let typelist = this.id.split("-");
editor/ui/dialogs/content/EdSpellCheck.js:      isoStrArray = dictList[i].split("-");
mail/components/compose/content/MsgComposeCommands.js:      isoStrArray = dictList[i].split("-");
mail/components/preferences/compose.js:        isoStrArray = dictList[i].split("-");
suite/browser/metadata.js:    var tokens = abbr.split("-");
suite/mailnews/compose/prefs/pref-composing_messages.js:      isoStrArray = dictList[i].split("-");
suite/mailnews/compose/MsgComposeCommands.js:      isoStrArray = dictList[i].split("-");
suite/common/bookmarks/bm-props.js:          var hours = values[1].split("-");
--- end here ---

The "isoStrArray" lines should be replaced with something like:
  isoStrArray = dictList[i].split(/[-_]/);
similarly to the previously mentioned Firefox bug.

Comment 3

[Reed Loden [:reed]]

(In reply to comment #2)
> Note: this problem persists in Thunderbird trunk:

If you would, please make your patch against trunk (comm-central).

Comment 4

[Cristian KLEIN]

Attachment: Patch against mercurial repository of comm-central.

Please backport this patch to Thunderbird 2 too.

Comment 5

[Cristian KLEIN]

I'm sorry, but I don't get it. How can this bug be assigned to me, since I don't have any commit privilege?

Comment 6

[Reed Loden [:reed]]

(In reply to comment #5)
> I'm sorry, but I don't get it. How can this bug be assigned to me, since I
> don't have any commit privilege?

In Mozilla's Bugzilla (bugzilla.mozilla.org), the assignee is always the patch author, even if he/she doesn't have commit access.

Comment 7

[Ludovic Hirlimann [:Usul]]

(In reply to comment #4)
> Created an attachment (id=412560) [details]
> Patch against mercurial repository of comm-central.

Don't forget to request reviews if you want your patch to get in .... 

see https://developer.mozilla.org/en/comm-central#Requirements to figure out who to ask them to.

Comment 8

[era+mozilla]

Cristian KLEIN: are you able to request reviews for these patches as suggested in comment #7 or would you like for someone else to take care of this chore?

Comment 9

[Cristian KLEIN]

(In reply to comment #8)
> Cristian KLEIN: are you able to request reviews for these patches as suggested
> in comment #7 or would you like for someone else to take care of this chore?

Sorry, I didn't have the time to read the procedures yet. I would really appreciate if anybody took the time to do this on my behalf.

Comment 10

[era+mozilla]

Seems I don't have the privileges to add the necessary flags.

I was planning to ask for a review from bienvenu, with dmose as my planned fallback.  You can do this by viewing the "details" link for the patch and selecting "review > ? > bienvenu".  Maybe you should send email as well.  See also https://developer.mozilla.org/en/Mailnews_and_Mail_code_review_requirements

As per the FAQ, as this is a fairly trivial change, I would like to suggest rubberstamping this ("requesting 'rs= '").

Sorry if I'm doing it wrong, this is my first time.

Comment 11

[Ludovic Hirlimann [:Usul]]

Sr is needed for mailnews code and maybe for the editor code that needs to be changed (ain't sure on that one).

Sending an email is not required.

Comment 12

[David :Bienvenu]

Comment on attachment 412560 [details] [diff] [review]
Patch against mercurial repository of comm-central.

switching review to standard8 - I think he has linux available.

Comment 13

[Mark Banner (:standard8)]

Comment on attachment 412560 [details] [diff] [review]
Patch against mercurial repository of comm-central.

Sorry, my review queue is overloaded at the moment, I'm redirecting this to some others who are hopefully less loaded.

Comment 14

[Magnus Melin [:mkmelin]]

I think the code looks ok, but what do i have to build with for thunderbird to pick up those spell checking libraries? My normal builds only pick up what's shipped with tb.

Comment 15

[Micah Gersten]

(In reply to comment #14)
> I think the code looks ok, but what do i have to build with for thunderbird to
> pick up those spell checking libraries? My normal builds only pick up what's
> shipped with tb.

Ubuntu symlinks the dictionaries directory to /usr/share/myspell/dicts after install.

Comment 16

[Magnus Melin [:mkmelin]]

Comment on attachment 412560 [details] [diff] [review]
Patch against mercurial repository of comm-central.

Ah, yeah works fine, thx! r=mkmelin

Comment 17

[[e-mail]]

Comment on attachment 412560 [details] [diff] [review]
Patch against mercurial repository of comm-central.

>diff -r e00dce0f3aaf suite/browser/metadata.js
>--- a/suite/browser/metadata.js	Mon Nov 16 11:04:18 2009 +0100
>+++ b/suite/browser/metadata.js	Mon Nov 16 13:27:07 2009 +0100
>@@ -462,7 +462,7 @@
>     if (!abbr) return "";
>     var result;
>     var region = "";
>-    var tokens = abbr.split("-");
>+    var tokens = abbr.split(/[-_]/);
RFC5646 seems to suggest that only "-" is legal, or am I overlooking something?
[Note that this question does not apply to the rest of the patch, as that apparently exists to work around Linux distros using non-RFC language codes.]

Comment 18

[Reed Loden [:reed]]

(In reply to comment #17)
> >-    var tokens = abbr.split("-");
> >+    var tokens = abbr.split(/[-_]/);
> RFC5646 seems to suggest that only "-" is legal, or am I overlooking something?

Yeah, that part of the patch is just wrong. Should only be changing the parts that have to do with finding spelling dictionaries.

Comment 19

[[e-mail]]

Comment on attachment 412560 [details] [diff] [review]
Patch against mercurial repository of comm-central.

In that case, sr=me excluding the metadata.js changes.

Comment 20

[Mark Banner (:standard8)]

Cristian: to get your patches checked in once reviewed, add checkin-needed to the keywords. I've added it now (I would check it in, but parts of the tree are closed).

Comment 21

[Justin Wood (:Callek)]

Christian,

Thank You VERY much for this patch. I just checked it in:

pushed as: http://hg.mozilla.org/comm-central/rev/efbc27606f4e

If you want this backported we can probably do that, just please provide a patch to do just that, there is the 1.9.1 branch (for TB3.0) I doubt we'll backport to TB2.0 though. (To get a patch backported it will need a new review if any changes are different then this last patch, and would need approval from a thunderbird driver)

Comment 22

[Cristian KLEIN]

(In reply to comment #21)
> Christian,
> 
> Thank You VERY much for this patch. I just checked it in:
> 
> pushed as: http://hg.mozilla.org/comm-central/rev/efbc27606f4e
> 
> If you want this backported we can probably do that, just please provide a
> patch to do just that, there is the 1.9.1 branch (for TB3.0) I doubt we'll
> backport to TB2.0 though. (To get a patch backported it will need a new review
> if any changes are different then this last patch, and would need approval from
> a thunderbird driver)

Hello Justin,

Thanks a deal for checking this in. This patch (i.e. http://hg.mozilla.org/comm-central/raw-rev/efbc27606f4e) already applies cleanly to comm-1.9.1. What do I need to do in this case?

Comment 23

[Justin Wood (:Callek)]

Comment on attachment 412560 [details] [diff] [review]
Patch against mercurial repository of comm-central.

in that case, requesting approval is enough, I just did so.

Comment 24

[Mark Banner (:standard8)]

(In reply to comment #21)
> Thank You VERY much for this patch. I just checked it in:
> 
> pushed as: http://hg.mozilla.org/comm-central/rev/efbc27606f4e

If a bug is fixed on trunk, please ensure that it is marked as such. Branch patches are tracked in a different status field as described here:

https://wiki.mozilla.org/Thunderbird/Thunderbird3_Security_And_Stability_Releases#General_Process_for_landing_bugs_on_Thunderbird_3.0.x

Comment 25

[Justin Wood (:Callek)]

(In reply to comment #24)
> (In reply to comment #21)
> > Thank You VERY much for this patch. I just checked it in:
> > 
> > pushed as: http://hg.mozilla.org/comm-central/rev/efbc27606f4e
> 
> If a bug is fixed on trunk, please ensure that it is marked as such. Branch
> patches are tracked in a different status field as described here:
> 
> https://wiki.mozilla.org/Thunderbird/Thunderbird3_Security_And_Stability_Releases#General_Process_for_landing_bugs_on_Thunderbird_3.0.x

I think in this case I only left open as I was not sure if this patch fixed the entire bug or if it was a partial; but thank you for pointing out the doc again. I of course should have error-ed on my gut feeling here.

Comment 26

[Mark Banner (:standard8)]

Comment on attachment 412560 [details] [diff] [review]
Patch against mercurial repository of comm-central.

Note: the a=Standard8 is also for the SeaMonkey part of the patch here.

Comment 27

[Mark Banner (:standard8)]

Checked into 1.9.1: http://hg.mozilla.org/releases/comm-1.9.1/rev/43c76e41ab32