1175908 - No dictionary selected after upgrade from TB 31 to TB 38 when xy_XY dictionary was selected before upgrade

Status: RESOLVED FIXED

(Thunderbird :: Message Compose Window, defect)

Description

[Enea]

Attachment: INLINE_BUG.pdf

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:38.0) Gecko/20100101 Firefox/38.0
Build ID: 20150525141253

Steps to reproduce:

Write new message.


Actual results:

OS: WIN7 PRO SP1 64bit
Nome 	Thunderbird
Versione 	38.0.1
User agent 	Mozilla/5.0 (Windows NT 6.1; WOW64; rv:38.0) Gecko/20100101 Thunderbird/38.0.1
ID di build dell'applicazione 	20150608103712

After update to 38.0.1 inline spell checking don't check text, even if dictionary are installed (reinstalled too). Preference are setting for italian language and there's italian dictionary. When "check on send" in enable, dictionary are not selected by default. 
Images attached for details.



Expected results:

Misspelled words with a red squiggly line under.

Comment 1

[Jorg K (CEST = GMT+2)]

Please show us what is on the drop-down menu on the "Spelling" button ("Ortografia").
That the default message is missing hints at some corruption in the installation.

Please try installing TB again into a different directory and start with a new profile, for example modify the shortcut to append "-profilemanager".

I did just that and noticed, that the Italian version is shipped without a dictionary. So I installed Dizionario Italiano 3.4.0.

Inline spell check works. BTW, I used Windows 7, 64 bit.

Comment 2

[Enea]

Attachment: spelling.jpg

Atthached contyent of drop-down menu on the "Spelling" button.
Thanks

Comment 3

[Jorg K (CEST = GMT+2)]

Sorry, you misunderstood. I repeat:
Please show us what is on the drop-down menu on the "Spelling" button ("Ortografia").
On the compose window!

Also, did you do a clean install with a new profile as requested?

Comment 4

[Enea]

Attachment: spelling2.jpg

Sorry, attached compose window details.

Comment 5

[Jorg K (CEST = GMT+2)]

Sorry, you misunderstood. I repeat again:
Please show us what is on the drop-down menu on the "Spelling" button ("Ortografia").
On the compose window!

Perhaps we are third time lucky:
COMPOSE WINDOW, the window you use to write messages. Look at your original attachment under "WRITING MAIL".

Also, did you do a clean install with a new profile as requested?

Comment 6

[Enea]

Attachment: spelling3.jpg

With fresh installation and new profile problem is not present.

Comment 7

[Jorg K (CEST = GMT+2)]

(In reply to Enea from comment #6)
> With fresh installation and new profile problem is not present.

Thanks you for the confirmation. I'm afraid we can't do anything here.

Of course the new profile doesn't contain your e-mail, so that's a problem for you.

Depending on whether you use IMAP or POP and whether you have local folders, you may need to migrate the mail to the new profile.

You could also try to "repair" the old profile by manually removing the installed addons.

Please understand that Bugzilla is Mozilla's tracker for bugs in the software. It is not a way to support people with problems related to their individual installation.

Unless you can present a *reproducible case*, like:
1) Install TB 31.7, install English US/GB? and Italian dictionaries
2) Upgrade to TB 38
3) Here's the problem
we won't be able to take the matter further.

If you need more support, please visit the Support Mozilla (SUMO) pages at
https://support.mozilla.org/en-US/products/thunderbird

Comment 8

[Enea]

I have done some more tests:
 - New installation of TB 31.7 with old profile has not problem.
 - New installation of TB 38.0.1 with old profile has the problem.
 - Old versione upgraded to TB 38.0.1 with old profile has the problem.

I have also try what you suggested:
 - New installation of TB 31.7 with a new profile has not problem.
 - Upgrade this TB 31.7 to TB 38.0 and problem is come back.

Looks like that, after update, the drop-down menu on the "Spelling" on composing window has nothing select by default.

Comment 9

[Enea]

Final test: removed English US/GB dictionary in all installation and problem was fixed for every version and for every profile (the drop-down menu on the "Spelling" on composing window has select default dictionary ).

Comment 10

[Jorg K (CEST = GMT+2)]

Thank you for your persistence!

I can reproduce the problem now.

Steps to reproduce:
1) Create a new profile with TB 31 Italian version.
   Notice that no dictionaries are installed, however, there seems to be a
   phantom dictionary present, since the "Spelling" button (Ortografia) *is* enabled.
2) Install Italian and English Dictionary. Everything seems to work.
3) Switch to TB 38 on the same profile. Symptoms that the reporter described start to occur.

Workaround seems to be to reinstall the English dictionary.

I'll have to look into it further.

Comment 11

[Enea]

Thanks for you time!!!

Comment 12

[Jorg K (CEST = GMT+2)]

OK, here is what happens:

TB 31 stores the selected spellcheck language in a user preference, like so:
user_pref("spellchecker.dictionary", "it_IT"); <<<< Note the underscore.

TB 38 sets the spell check dictionary from the stored preference.

However, the new Italian dictionary is stored as
user_pref("spellchecker.dictionary", "it-IT");

So when the upgrade happens while the preference is set to it_IT, then in TB 38 no dictionary is selected for inline and explicit spellcheck as can be seen in attachment 8624678 [details] because "it-IT" does not match "it_IT".

The simple fix is the change the spell check language in
Tools > Options, Composition, Spelling
to English and back to Italian.

I don't know why the Italian dictionary has corrected its internal identifier from "it_IT" to "it-IT", but there is nothing that needs to be fixed in TB.

Comment 13

[Jorg K (CEST = GMT+2)]

I've been to quick closing this.

The situation is the following:
In TB 31 some dictionaries, for example the Italian one announce themselves as xy_XY (with an underscore), for example it_IT.

In TB 38 they announce themselves as xy-XY (with a hyphen), for example it-IT. I don't know why, TB uses a M-C core service to retrieve those dictionaries:
https://dxr.mozilla.org/comm-central/source/mail/components/compose/content/MsgComposeCommands.js#3029

TB 38 retrieves the default spell check language from the stored preference, and if it doesn't match any dictionary, there is no inline spell checking until the user selects a dictionary. But this selection doesn't stick, since we don't change the preference any more, so in the next e-mail they write, they have to repeat the dictionary selection.

The fix is to reset the preference manually, but that's not at all obvious to do.

So far, we know that all Italian users will be affected by this, and there may be other locales which display the problem. The solution is dead simple: When initialising the language from the stored preference, make sure that the stored value matches one of them, if not, select any old dictionary and fix the preference. Five to ten lines. I'll submit a patch in a few hours.

Comment 14

[Jorg K (CEST = GMT+2)]

Attachment: This ensures that the spell check preference matches a dictionary

Comment 15

[Jorg K (CEST = GMT+2)]

I hope we can land this quickly at least on trunk, since this touches the same file as bug 368915, so I'd like to un-rot bug 368915 before it gets reviewed.

Comment 16

[Jorg K (CEST = GMT+2)]

Attachment: This ensures that the spell check preference matches a dictionary (v2, more elegant)

Comment 17

[Magnus Melin [:mkmelin]]

Comment on attachment 8625030 [details] [diff] [review]
This ensures that the spell check preference matches a dictionary (v2, more elegant)

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

LGTM! r=mkmelin with the nits addressed

::: mail/components/compose/content/MsgComposeCommands.js
@@ +2000,5 @@
>  function AttachmentsChanged() {
>    manageAttachmentNotification(true);
>  }
>  
> +function getValidSpellcheckerDictionary() {

please add documentation for this function, describing the use case. It will be hard to understand why we do this later otherwise.

@@ +2002,5 @@
>  }
>  
> +function getValidSpellcheckerDictionary() {
> +  let prefValue = Services.prefs.getCharPref("spellchecker.dictionary");
> +  let spellChecker = Components.classes['@mozilla.org/spellchecker/engine;1']

nit: prefer double quotes

Comment 18

[Jorg K (CEST = GMT+2)]

Attachment: This ensures that the spell check preference matches a dictionary (v3, nits fixed)

Carrying forward Magnus' r+. Please land if you're happy with the comment. I could write more, but I think it's already long enough.

The reason this is exposed in TB 38 is that in TB 38 we do not set the preference any more when the user sets the spelling language (bug 967494) in the compose window. So bad preference values don't get "auto-fixed" any more like in previous versions.

Comment 19

[Magnus Melin [:mkmelin]]

The text is fine, but preferably use documentation comments for functions. I mean like
/**
 * foo bar
 */

Comment 20

[Jorg K (CEST = GMT+2)]

Attachment: This ensures that the spell check preference matches a dictionary (v4)

Carrying forward Magnus' r+.
Changed comment style.
Please land.

Comment 21

[aleth [:aleth]]

https://hg.mozilla.org/comm-central/rev/5bccc565b812

Comment 22

[aleth [:aleth]]

Should this be uplifted?

Comment 23

[Jorg K (CEST = GMT+2)]

Yes, but I let Kent decide.

Comment 24

[Jorg K (CEST = GMT+2)]

Comment on attachment 8625088 [details] [diff] [review]
This ensures that the spell check preference matches a dictionary (v4)

[Approval Request Comment]
Regression caused by (bug #): 967494
It's not really *caused* by this bug, but this bug exposes the problem.
User impact if declined: All Italian users will have trouble since their dictionary just changed from it_IT to it-IT.
Risk to taking this patch (and alternatives if risky):
No risky. A few lines that check the stored preference against the list of installed dictionaries.

Comment 25

[Jorg K (CEST = GMT+2)]

Actually, Aleth found the cause of the problem: bug 1097550, there xy_XY was changed to xy-XY.

Comment 26

[Ian Neal]

(In reply to Jorg K from comment #25)
> Actually, Aleth found the cause of the problem: bug 1097550, there xy_XY was
> changed to xy-XY.

Is it worth checking the pref for the underscore and changing it? Do we know how many locales it impacts on?

Comment 27

[Philip Chee]

(In reply to Ian Neal from comment #26)
> (In reply to Jorg K from comment #25)
> > Actually, Aleth found the cause of the problem: bug 1097550, there xy_XY was
> > changed to xy-XY.
> 
> Is it worth checking the pref for the underscore and changing it? Do we know
> how many locales it impacts on?
Mid-aired was going to ask the same.

Comment 28

[Jorg K (CEST = GMT+2)]

(In reply to Ian Neal from comment #26)
> Is it worth checking the pref for the underscore and changing it? Do we know
> how many locales it impacts on?

My simple patch also catches the case where the dictionary has been removed and the preference has gone stale. In TB 31, the preference changed all the time if you had more than one dictionary, so the user won't mind/know/notice as long as something valid is set. If they only have one dictionary this choice will always be correct.

Personally, I'm happy with my solution ;-)

And no, I don't know how many locales are affected.

Comment 29

[Philip Chee]

Proposed code polish (not tested):

  let prefValue = Services.prefs.getCharPref("spellchecker.dictionary");
  let spellChecker = Components.classes["@mozilla.org/spellchecker/engine;1"]
                               .getService(mozISpellCheckingEngine);
  let o1 = {};
  spellChecker.getDictionaryList(o1, {});
  let dictList = o1.value;

  // If there are no dictionaries, we can't check the value, so return it.
  if (!dictList.length) {
    return prefValue;
  }

  // Make sure preference contains a valid value.
  if (dictList.indexOf(prefValue) > -1)
      return prefValue;

  prefValue = prefValue.replace(/_/, "-");
  if (dictList.indexOf(prefValue) > -1) {
    Services.prefs.setCharPref("spellchecker.dictionary", prefValue);
    return prefValue;
  }

  // Set a valid value, any value will do.
  Services.prefs.setCharPref("spellchecker.dictionary", dictList[0]);
  return dictList[0];
}

Comment 30

[aleth [:aleth]]

(In reply to Jorg K from comment #28)
> And no, I don't know how many locales are affected.

If the regression is due to bug 1097550, it's not about locales, it's Linux distributions with system dictionaries.

Comment 31

[Jorg K (CEST = GMT+2)]

Looks good, more elegant (polished). What do we do now? Do you want to land this instead?

Comment 32

[Jorg K (CEST = GMT+2)]

(In reply to aleth [:aleth] from comment #30)
> If the regression is due to bug 1097550, it's not about locales, it's Linux
> distributions with system dictionaries.

That too. But the bug was started by an Italian guy on Windows 7 whose dictionary changed from it_IT to it-IT.

Knowing little about it, I think, the Italian dictionary carries the it_IT inside it, but it's now returned as it-IT. There may be other locales like this.

Comment 33

[Jorg K (CEST = GMT+2)]

(In reply to Jorg K from comment #31)
> Looks good, more elegant (polished). What do we do now? Do you want to land
> this instead?

I heard on IRC that we can leave it, if we like it. I think it's OK, no need to change "xy_XY" to "xy-XY" since (see comment #28):
a) if they had one dictionary, we pick it anyway.
b) if they had more than one, the preference was a random value anyway, ie. the one they last used.

Comment 34

[Jorg K (CEST = GMT+2)]

Attachment: Possible change to already landed patch

This could be landed additionally to the (v4) patch already landed.

It tries the "hyphen version" of the dictionary first before picking the first one in the list. That was suggested in comment #29, so I'm presenting it as an option.

Comment 35

[Kent James (:rkent)]

Comment on attachment 8625088 [details] [diff] [review]
This ensures that the spell check preference matches a dictionary (v4)

This code does not exist in TB 39 because Bug 1168945 did not land there.

Comment 36

[Jorg K (CEST = GMT+2)]

Sorry Kent, you're 100% right, the whole new spell check stuff starting with bug 967494 was never landed on TB 39. I overzealously checked too many boxes ;-(

Comment 37

[Kent James (:rkent)]

Comment on attachment 8625088 [details] [diff] [review]
This ensures that the spell check preference matches a dictionary (v4)

https://hg.mozilla.org/releases/comm-aurora/rev/df5e2f421ce6

Comment 38

[Magnus Melin [:mkmelin]]

Comment on attachment 8625184 [details] [diff] [review]
Possible change to already landed patch

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

::: mail/components/compose/content/MsgComposeCommands.js
@@ +2033,5 @@
>  
>    // Make sure preference contains a valid value.
> +  if (dictList.indexOf(prefValue) >= 0) {
> +    return prefValue;
> +  }

I think this part is ok.

@@ +2044,1 @@
>    }

... but I don't think thes part is worth it.

Comment 39

[Jorg K (CEST = GMT+2)]

Well, do you just want to replace this
// Make sure preference contains a valid value.
for (let i = 0; i < count; i++) {
  if (dictList[i] == prefValue) {
    return prefValue;
  }
}
with
// Make sure preference contains a valid value.
if (dictList.indexOf(prefValue) >= 0) {
  return prefValue;
}

Is that worth it? I thought it was worth it in the together with the xy_XY to xy-XY replacement.

Comment 40

[Magnus Melin [:mkmelin]]

You decide.

Comment 41

[Jorg K (CEST = GMT+2)]

Attachment: Possible change to already landed patch: Add a little polish and shave off some CPU cycles (v2)

Comment 42

[Jorg K (CEST = GMT+2)]

Comment on attachment 8625337 [details] [diff] [review]
Possible change to already landed patch: Add a little polish and shave off some CPU cycles (v2)

This is part of the already approved patch.

Comment 43

[Magnus Melin [:mkmelin]]

Comment on attachment 8625337 [details] [diff] [review]
Possible change to already landed patch: Add a little polish and shave off some CPU cycles (v2)

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

No real need to uplift this

Comment 44

[Jorg K (CEST = GMT+2)]

checkin-needed for attachment 8625337 [details] [diff] [review].
Part one was already landed:
https://hg.mozilla.org/comm-central/rev/5bccc565b812 (comment #21)
https://hg.mozilla.org/releases/comm-aurora/rev/df5e2f421ce6 (comment #37)

Comment 45

[aleth [:aleth]]

https://hg.mozilla.org/comm-central/rev/4c0a2ce3eed4

Comment 46

[Kent James (:rkent)]

Comment on attachment 8625088 [details] [diff] [review]
This ensures that the spell check preference matches a dictionary (v4)

http://hg.mozilla.org/releases/comm-esr38/rev/da0718a46e71