Language display in the status bar doesn't react when dictionaries are removed.

RESOLVED FIXED in Thunderbird 45.0

Status

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

People

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

Tracking

Trunk
Thunderbird 45.0

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

2 years ago
The language display in the status bar introduced in bug 368915 doesn't react when dictionaries are removed.

M-C will introduce a SPELLCHECK_DICTIONARY_REMOVE_NOTIFICATION in bug 697981, so we should listen to that to fix the document language and hence the status bar.
(Assignee)

Updated

2 years ago
Assignee: nobody → mozilla
Status: NEW → ASSIGNED
(Assignee)

Comment 1

2 years ago
Created attachment 8663371 [details] [diff] [review]
Untested: Should be something like this.
(Assignee)

Comment 2

2 years ago
Created attachment 8665018 [details] [diff] [review]
Proposed solution (v1).

Debug dump will be removed for the final version.

Note that the "spellcheck-dictionary-remove" notification was introduced in M-C bug 697981 which landed on mozilla44. (There are still some issues being fixed up to do with contenteditables, which is what we use in the mail composition window, in bug 1205983. But they don't affect this bug.)

Steps for testing:
Case 1: removing a dictionary which is not the default.
- install three dictionaries: d1, d2, d3.
- set spelling preference to, say, d3.
- open three composition windows in three different languages,
  observe the language indicator.
- disable dictionary d1.
- check that the windows with d2 and d3 are unaffected,
  check that the language in the window which had d1 now has d3.
  
Case 2: removing a dictionary which is the default.
- install three dictionaries: d1, d2, d3.
- set spelling preference to, say, d3.
- open three composition windows in three different languages,
  observe the language indicator.
- disable dictionary d3.
- check that the windows with d1 and d2 are unaffected,
  check that a new preference has been set,
  check that the language in the window which had d3 now has the new preference.
  
Check the language in using the "Spelling" button, I noticed that the right click (context) menu sometimes doesn't show the right information. In fact, you need to left click first, then right click. If you only right click, you get the information from the previous window or none. See bug 1207713.
Attachment #8663371 - Attachment is obsolete: true
Attachment #8665018 - Flags: review?(acelists)
(Assignee)

Comment 3

2 years ago
Comment on attachment 8665018 [details] [diff] [review]
Proposed solution (v1).

Looks like I can't nominate Aceman. OK, ask Magnus.
Attachment #8665018 - Flags: review?(mkmelin+mozilla)

Comment 4

2 years ago
Maybe you can, I just didn't yet get to testing it out as it needs special preparational steps (like 3 dictionaries) :)
(Assignee)

Comment 5

2 years ago
How hard can this be? I suppose you already have two dictionaries, your mother tongue and English.
Just get two more:
https://addons.mozilla.org/en-us/firefox/addon/spanish-spain-dictionary/
Type: estoy en una ciudad bonita.
https://addons.mozilla.org/en-us/firefox/addon/german-dictionary-de_de-for-sp/
Type: Heute ist ein guter Tag.

Comment 6

2 years ago
Yes, just do not assume there is the same state in my production profile as in the development profiles (under different user, TB trunk, etc.) with its mess of patches and testing deviations from a clean trunk.

Actually I am not even sure I have those 2 dictionaries installed as I write in both languages randomly and do not care to switch the dictionary. I do not think it works automatically.

Comment 7

2 years ago
Comment on attachment 8665018 [details] [diff] [review]
Proposed solution (v1).

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

::: mail/components/compose/content/MsgComposeCommands.js
@@ +2080,5 @@
> +    spellChecker.getDictionaryList(o1, o2);
> +    let dictList = o1.value;
> +    let count    = o2.value;
> +
> +    if (count > 0 && dictList.indexOf(language) >= 0) {

use dictList.includes instead

@@ +2088,5 @@
> +    }
> +
> +    // Set a valid language from the preference.
> +    let prefValue = Services.prefs.getCharPref("spellchecker.dictionary");
> +    if (count == 0 || dictList.indexOf(prefValue) >= 0) {

here too

@@ +2095,5 @@
> +      language = dictList[0];
> +      // Fix the preference while we're here. We know it's invalid.
> +      Services.prefs.setCharPref("spellchecker.dictionary", language);
> +    }
> +    dump("***** setting new language " + language + "\n");

please remove the dumps
(Assignee)

Comment 8

2 years ago
Thanks. I'll submit a new patch once M-C bug 1205983 lands, should be in the next few hours, landed on inbound this morning (and didn't get backed out ;-)).

BTW, Ehsan made me add #ifdef MOZ_THUNDERBIRD here:
#ifdef MOZ_THUNDERBIRD
 nsCOMPtr<nsIObserverService> obs = mozilla::services::GetObserverService();
 if (obs) {
   obs->NotifyObservers(nullptr, SPELLCHECK_DICTIONARY_REMOVE_NOTIFICATION, ...

So I hope that that is defined for a TB compile.
(Assignee)

Comment 9

2 years ago
Created attachment 8668314 [details] [diff] [review]
Proposed solution (v1a).

Review issues fixed.

I also tested it, so the conditional compilation works ;-)
http://mxr.mozilla.org/mozilla-central/source/extensions/spellcheck/hunspell/glue/mozHunspell.cpp#595
Attachment #8665018 - Attachment is obsolete: true
Attachment #8665018 - Flags: review?(mkmelin+mozilla)
Attachment #8665018 - Flags: review?(acelists)
Attachment #8668314 - Flags: review?(mkmelin+mozilla)

Comment 10

2 years ago
Comment on attachment 8668314 [details] [diff] [review]
Proposed solution (v1a).

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

Thx! r=mkmelin
Attachment #8668314 - Flags: review?(mkmelin+mozilla) → review+
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 11

2 years ago
https://hg.mozilla.org/comm-central/rev/2435da3ea8b4e7e16b47bcfecc7dec83cc9d276c
Bug 1203957 - Listen to spellcheck-dictionary-remove and fix language attribute. r=mkmelin a=aleth CLOSED TREE

Updated

2 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 45.0
You need to log in before you can comment on or make changes to this bug.