1097625 - Fallback to use LANG variable as dictionary doesn't work

Status: RESOLVED FIXED

(Core :: Spelling checker, defect)

Description

[Jan Horak [:jhorak]]

Fallback to LANG variable doesn't work when LANG variable contains charset because charset is wrongly cut off.

It also doesn't work on some Linux systems where dictionary files in system has '_' separator instead of '-' and currently code is replacing the '_' by '-' before trying dictionary with underscore first.

Comment 1

[Jan Horak [:jhorak]]

Attachment: fix-lang-fallback v1

Attaching patch with cutoff fix and also trying dictionaries names with '_' (valid for some Linux distros).

Comment 2

[(no longer active)]

Comment on attachment 8531582 [details] [diff] [review]
fix-lang-fallback v1

Review of attachment 8531582 [details] [diff] [review]:
-----------------------------------------------------------------

Looks mostly good except for the below.  I'd like to look at the final version of the patch.

::: editor/composer/nsEditorSpellCheck.cpp
@@ +866,5 @@
> +          int32_t underScore = lang.FindChar('_');
> +          if (underScore != -1) {
> +            lang.Replace(underScore, 1, '-');
> +          }
> +          rv = SetCurrentDictionary(lang);

Please move this line inside the if block after Replace(), because it doesn't make sense to call SetCurrentDictionary() twice with the same lang value.

@@ +872,5 @@
>        }
>        if (NS_FAILED(rv)) {
>          rv = SetCurrentDictionary(NS_LITERAL_STRING("en-US"));
>          if (NS_FAILED(rv)) {
> +          rv = SetCurrentDictionary(NS_LITERAL_STRING("en_US"));

Nit: please add a comment stating that some Linux distros use _ here as well.

Comment 3

[Jan Horak [:jhorak]]

Attachment: fix-lang-fallback v2

Adding fixed version.

Comment 4

[Jan Horak [:jhorak]]

Thanks for the review.

Comment 5

[Ryan VanderMeulen [:RyanVM]]

Is there a Try link handy for this patch? :)

Comment 6

[Jan Horak [:jhorak]]

(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #5)
> Is there a Try link handy for this patch? :)

Sure, there it is:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=d99325a230ea

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cb926aa1e69d

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/cb926aa1e69d