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

RESOLVED FIXED in Thunderbird 41.0

Status

Thunderbird
Message Compose Window
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: Enea, Assigned: Jorg K (GMT+2))

Tracking

({regression})

38 Branch
Thunderbird 41.0
regression

Thunderbird Tracking Flags

(thunderbird38 affected, thunderbird39 wontfix, thunderbird40 fixed, thunderbird41 fixed, thunderbird_esr3839+ fixed)

Details

(Whiteboard: [regression:TB37?])

Attachments

(4 attachments, 6 obsolete attachments)

(Reporter)

Description

2 years ago
Created attachment 8624221 [details]
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.
(Assignee)

Comment 1

2 years ago
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.
Status: UNCONFIRMED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → WORKSFORME
(Reporter)

Comment 2

2 years ago
Created attachment 8624663 [details]
spelling.jpg

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

Comment 3

2 years ago
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?
(Reporter)

Comment 4

2 years ago
Created attachment 8624668 [details]
spelling2.jpg

Sorry, attached compose window details.
Attachment #8624663 - Attachment is obsolete: true
(Assignee)

Comment 5

2 years ago
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?
(Reporter)

Comment 6

2 years ago
Created attachment 8624678 [details]
spelling3.jpg

With fresh installation and new profile problem is not present.
Attachment #8624668 - Attachment is obsolete: true
(Assignee)

Comment 7

2 years ago
(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
(Reporter)

Comment 8

2 years ago
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.
(Reporter)

Comment 9

2 years ago
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 ).
(Assignee)

Comment 10

2 years ago
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.
Status: RESOLVED → REOPENED
Ever confirmed: true
Resolution: WORKSFORME → ---
(Reporter)

Comment 11

2 years ago
Thanks for you time!!!
(Assignee)

Comment 12

2 years ago
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.
Status: REOPENED → RESOLVED
Last Resolved: 2 years ago2 years ago
Resolution: --- → WONTFIX
Summary: Inline spell checking fail after 38.0.1 update → No dictionary selected after upgrade from TB 31 to TB 38 when it_IT dictionary was selected before upgrade
(Assignee)

Comment 13

2 years ago
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.
Assignee: nobody → mozilla
Status: RESOLVED → REOPENED
status-thunderbird38: --- → affected
status-thunderbird39: --- → affected
status-thunderbird40: --- → affected
status-thunderbird41: --- → affected
tracking-thunderbird38: --- → ?
Keywords: regression
Resolution: WONTFIX → ---
Summary: No dictionary selected after upgrade from TB 31 to TB 38 when it_IT dictionary was selected before upgrade → No dictionary selected after upgrade from TB 31 to TB 38 when xy_XY dictionary was selected before upgrade
(Assignee)

Updated

2 years ago
Status: REOPENED → ASSIGNED
(Assignee)

Comment 14

2 years ago
Created attachment 8624865 [details] [diff] [review]
This ensures that the spell check preference matches a dictionary
Attachment #8624865 - Flags: review?(mkmelin+mozilla)
(Assignee)

Comment 15

2 years ago
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.
(Assignee)

Comment 16

2 years ago
Created attachment 8625030 [details] [diff] [review]
This ensures that the spell check preference matches a dictionary (v2, more elegant)
Attachment #8624865 - Attachment is obsolete: true
Attachment #8624865 - Flags: review?(mkmelin+mozilla)
Attachment #8625030 - Flags: review?(mkmelin+mozilla)

Comment 17

2 years ago
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
Attachment #8625030 - Flags: review?(mkmelin+mozilla) → review+

Updated

2 years ago
tracking-thunderbird38: ? → ---
tracking-thunderbird_esr38: --- → +
(Assignee)

Comment 18

2 years ago
Created attachment 8625087 [details] [diff] [review]
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.
Attachment #8625030 - Attachment is obsolete: true
Attachment #8625087 - Flags: review+

Comment 19

2 years ago
The text is fine, but preferably use documentation comments for functions. I mean like
/**
 * foo bar
 */
(Assignee)

Comment 20

2 years ago
Created attachment 8625088 [details] [diff] [review]
This ensures that the spell check preference matches a dictionary (v4)

Carrying forward Magnus' r+.
Changed comment style.
Please land.
Attachment #8625087 - Attachment is obsolete: true
Attachment #8625088 - Flags: review+
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 21

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

Updated

2 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago2 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 41.0

Comment 22

2 years ago
Should this be uplifted?
(Assignee)

Comment 23

2 years ago
Yes, but I let Kent decide.
(Assignee)

Comment 24

2 years ago
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.
Attachment #8625088 - Flags: approval-comm-esr38?
Attachment #8625088 - Flags: approval-comm-beta?
Attachment #8625088 - Flags: approval-comm-aurora?
(Assignee)

Comment 25

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

Comment 26

2 years ago
(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?
Flags: needinfo?(mozilla)

Comment 27

2 years ago
(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.
Depends on: 1097550

Updated

2 years ago
See Also: → bug 1176602
(Assignee)

Comment 28

2 years ago
(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.
Flags: needinfo?(mozilla)

Comment 29

2 years ago
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

2 years ago
(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.
(Assignee)

Comment 31

2 years ago
Looks good, more elegant (polished). What do we do now? Do you want to land this instead?
Flags: needinfo?(philip.chee)
(Assignee)

Comment 32

2 years ago
(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.
(Assignee)

Comment 33

2 years ago
(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.
Flags: needinfo?(philip.chee)
(Assignee)

Comment 34

2 years ago
Created attachment 8625184 [details] [diff] [review]
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.
Attachment #8625184 - Flags: review?(mkmelin+mozilla)

Comment 35

2 years ago
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.
Attachment #8625088 - Flags: approval-comm-beta? → approval-comm-beta-
(Assignee)

Comment 36

2 years ago
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

2 years ago
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
Attachment #8625088 - Flags: approval-comm-aurora? → approval-comm-aurora+

Updated

2 years ago
status-thunderbird39: affected → wontfix
status-thunderbird40: affected → fixed

Comment 38

2 years ago
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.
Attachment #8625184 - Flags: review?(mkmelin+mozilla)
(Assignee)

Comment 39

2 years ago
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.
Flags: needinfo?(mkmelin+mozilla)

Comment 40

2 years ago
You decide.
Flags: needinfo?(mkmelin+mozilla)
(Assignee)

Comment 41

2 years ago
Created attachment 8625337 [details] [diff] [review]
Possible change to already landed patch: Add a little polish and shave off some CPU cycles (v2)
Attachment #8625184 - Attachment is obsolete: true
Attachment #8625337 - Flags: review?(mkmelin+mozilla)

Updated

2 years ago
Attachment #8625337 - Flags: review?(mkmelin+mozilla) → review+
(Assignee)

Updated

2 years ago
Keywords: checkin-needed
(Assignee)

Comment 42

2 years ago
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.
Attachment #8625337 - Flags: approval-comm-esr38?
Attachment #8625337 - Flags: approval-comm-aurora?

Comment 43

2 years ago
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
Attachment #8625337 - Flags: approval-comm-esr38?
Attachment #8625337 - Flags: approval-comm-esr38-
Attachment #8625337 - Flags: approval-comm-aurora?
Attachment #8625337 - Flags: approval-comm-aurora-
(Assignee)

Comment 44

2 years ago
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

2 years ago
https://hg.mozilla.org/comm-central/rev/4c0a2ce3eed4
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 46

2 years ago
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
Attachment #8625088 - Flags: approval-comm-esr38? → approval-comm-esr38+

Updated

2 years ago
status-thunderbird_esr38: --- → fixed
tracking-thunderbird_esr38: + → 39+
(Assignee)

Updated

2 years ago
status-thunderbird41: affected → fixed
(Assignee)

Updated

2 years ago
Duplicate of this bug: 1181712
Whiteboard: [regression:TB37?]
You need to log in before you can comment on or make changes to this bug.