728775 - Make language list consistent everywhere: Spelling toolbar button, context menu, full spell check, composition preference

Status: RESOLVED FIXED

(Thunderbird :: Message Compose Window, defect)

Description

[Jim Porter (:squib)]

Attachment: Hacky patch

The Spelling toolbar button has a menu of languages, but this list is formatted differently from the context menu. Let's make them the same, and remove some code while we're at it! Here's a patch, which I've lightly tested. I don't know if I'll have time to write tests anytime soon, so I'm not going to assign this to myself. Other people are welcome to take over, if they're so inlined.

Comment 1

[Mark Banner (:standard8)]

Comment on attachment 598762 [details] [diff] [review]
Hacky patch

Not tested it, but the idea seems reasonable - especially as InlineSpellChecker doesn't handle multiple menus in one instance.

Comment 2

[Jorg K (CEST = GMT+2)]

Unbelievable that the patch has hung around for three years!!

Comment 3

[Jim Porter (:squib)]

Well, as comment 0 says:

> Other people are welcome to take over, if they're so inlined. [sic]

Comment 4

[Jorg K (CEST = GMT+2)]

I will, once we settled bug 368915.

Comment 5

[Jorg K (CEST = GMT+2)]

Attachment: WIP: adapted Jim's patch to the year 2016.

I've finally returned to this bug. The patch has highly rotted since we now have a language indicator in the status bar. This works "a little".

There are two obvious problems:
1) The language indicator doesn't show the current language when the compose
   window is first displayed.
2) Selecting any of the menu items on the toolbar button language list
   starts the full spell check instead of just selecting another language.

This needs more work ;-)

Comment 6

[Jorg K (CEST = GMT+2)]

Attachment: WIP: adapted Jim's patch to the year 2016 (v2).

This fixes:
2) Selecting any of the menu items on the toolbar button language list
   starts the full spell check instead of just selecting another language.
   == There was a stopPropagation(); missing.

Still to fix:
1) The language indicator doesn't show the current language when the compose
   window is first displayed.

Comment 7

[Jorg K (CEST = GMT+2)]

Attachment: Proposed solution (v1).

This works now.

I'll have a look whether we can also unify the language selection on the (full) spell check panel. Oops, there is also the language setting in the composition/spelling preferences.

Perhaps these two should go to another bug ;-)

Comment 8

[Jorg K (CEST = GMT+2)]

Full spell check constructs the list here:
https://dxr.mozilla.org/comm-central/source/editor/ui/dialogs/content/EdSpellCheck.js#114

And for the options/preferences it's done here:
https://dxr.mozilla.org/comm-central/source/mail/components/preferences/compose.js#117

Reminder: What is removed in with the patch for composition used to be here:
https://dxr.mozilla.org/comm-central/source/mail/components/compose/content/MsgComposeCommands.js#3235

So I'm having second thoughts about the patch which uses the hacky approach of a second inline spell checker to get the menu constructed here:
https://dxr.mozilla.org/comm-central/source/mozilla/toolkit/modules/InlineSpellChecker.jsm#202

Maybe it would be best to unify all three functions we use and base the unified function on the InlineSpellChecker.jsm code, but not directly call this code via a hacky second inline spell checker.

Aceman, what do you think? And also, where would we place such a function?

As we see in the patch, the second hacky inline spell checker comes at a cost since it initialises late and I need to do crazy stuff like
+    let languageToSet = getValidSpellcheckerDictionary();
+    setTimeout(function() {
+      document.documentElement.setAttribute("lang", languageToSet);
+    }, 0);
to make sure it's accessed sufficiently late.

And it only addresses the inconsistency in compose, but not in full spell check and preferences.

Comment 9

[Jorg K (CEST = GMT+2)]

Attachment: Alternative solution (v1).

Here is a much simpler solution. Much less intrusive. All we do it use
  getDictionaryDisplayName()
from the M-C core code.

This can *very* easily be done the very same way here (full spell check):
https://dxr.mozilla.org/comm-central/source/editor/ui/dialogs/content/EdSpellCheck.js#114

and here (options):
https://dxr.mozilla.org/comm-central/source/mail/components/preferences/compose.js#117

Comment 10

[Jorg K (CEST = GMT+2)]

Attachment: Alternative solution (v2).

OK, here is the full solution. We now show the same dictionary names everywhere courtesy of the inline spell checker and getDictionaryDisplayName().

Comment 11

[Jorg K (CEST = GMT+2)]

Comment on attachment 8759195 [details] [diff] [review]
Alternative solution (v2).

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

::: editor/ui/dialogs/content/EdSpellCheck.js
@@ +144,5 @@
>    gDictCount = count;
>  
> +  for (let i = 0; i < count; i++) {
> +    let langId = dictList[i];
> +    let inlineSpellChecker = new InlineSpellChecker();

Better outside the loop, right? And do I need to delete this later?

::: mail/components/preferences/compose.js
@@ +142,5 @@
>      this.mDictCount = count;
>  
> +    for (let i = 0; i < count; i++) {
> +      let langId = dictList[i];
> +      let inlineSpellChecker = new InlineSpellChecker();

Same here.

Comment 12

[:aceman]

Comment on attachment 8759195 [details] [diff] [review]
Alternative solution (v2).

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

How I love merging duplicated code! Jorg+++

Is it now possible to remove the bundles from compose.xul and messengercompose.xul?
    <stringbundle id="languageBundle" src="chrome://global/locale/languageNames.properties"/>
    <stringbundle id="regionBundle" src="chrome://global/locale/regionNames.properties"/>

::: editor/ui/dialogs/content/EdSpellCheck.js
@@ +144,5 @@
>    gDictCount = count;
>  
> +  for (let i = 0; i < count; i++) {
> +    let langId = dictList[i];
> +    let inlineSpellChecker = new InlineSpellChecker();

Outside the loop seems fine, in MsgComposeComands one instance was used. It seems to me in JS you do not need to delete the object. The GC will do it when no longer needed/referenced.

@@ +156,5 @@
> +      if (a[0] < b[0])
> +        return -1;
> +      if (a[0] > b[0])
> +        return 1;
> +      return 0;

Why killing the locale-aware sorting? I think the language names can be translated. Also, can't you use InlineSpellChecker.sortDictionaryList(dictList) here?

Maybe you could actually do what addDictionaryListToMenu() does:
 var spellchecker = this.mInlineSpellChecker.spellChecker;
 var o1 = {}, o2 = {};
 spellchecker.GetDictionaryList(o1, o2);
 list = o1.value;
 var listcount = o2.value;
 try {
   curlang = spellchecker.GetCurrentDictionary();
   } catch(e) {}

 var sortedList = this.sortDictionaryList(list);

...populate the menu...

::: mail/components/compose/content/MsgComposeCommands.js
@@ +3276,4 @@
>  
>    // Remove any languages from the list.
>    while (languageMenuList.hasChildNodes())
>      languageMenuList.lastChild.remove();

languageMenuList.clearAllItems() ?

::: mail/components/preferences/compose.js
@@ +160,5 @@
>  
>      // Remove any languages from the list.
>      var languageMenuPopup = languageMenuList.firstChild;
>      while (languageMenuPopup.hasChildNodes())
>        languageMenuPopup.lastChild.remove();

languageMenuList.clearAllItems() ?

@@ +163,5 @@
>      while (languageMenuPopup.hasChildNodes())
>        languageMenuPopup.lastChild.remove();
>  
>      // append the dictionaries to the menu list...
>      for (i = 0; i < count; i++)

'i' is now undeclared.

Comment 13

[:aceman]

(In reply to Jorg K (GMT+2, PTO during summer, NI me) from comment #8)
> Maybe it would be best to unify all three functions we use and base the
> unified function on the InlineSpellChecker.jsm code, but not directly call
> this code via a hacky second inline spell checker.
> 
> Aceman, what do you think? And also, where would we place such a function?

Could we put it into EdSpellCheck.js and then link this file in compose.xul and messengercompose.xul? You can't import it into the .js files as it is not a module. Would that work?

Comment 14

[Jorg K (CEST = GMT+2)]

Attachment: Alternative solution (v3).

Like this then?

To answer your questions:
> Why killing the locale-aware sorting?
To guarantee the same order as in the context popup, we must use the same sort as M-C, here:
https://dxr.mozilla.org/comm-central/source/mozilla/toolkit/modules/InlineSpellChecker.jsm#189

> Maybe you could actually do what addDictionaryListToMenu() does: ...
Well, I'm happy with my clean-up and the result. I don't need a grand solution here ;-)

> languageMenuList.clearAllItems() ?
What's clearAllItems()?

Comment 15

[:aceman]

(In reply to Jorg K (GMT+2, PTO during summer, NI me) from comment #14)
> To answer your questions:
> > Why killing the locale-aware sorting?
> To guarantee the same order as in the context popup, we must use the same
> sort as M-C, here:
> https://dxr.mozilla.org/comm-central/source/mozilla/toolkit/modules/
> InlineSpellChecker.jsm#189

OK, understood. But their code is then buggy :)

> > Maybe you could actually do what addDictionaryListToMenu() does: ...
> Well, I'm happy with my clean-up and the result. I don't need a grand
> solution here ;-)

You would remove even more code and also NOT duplicate the sort logic.
 
> > languageMenuList.clearAllItems() ?
> What's clearAllItems()?

A typo. It is removeAllItems() :)

So a single shared function in EdSpellCheck.js didn't work?

Comment 16

[Jorg K (CEST = GMT+2)]

Attachment: Alternative solution (v4).

> It is removeAllItems() :)
That doesn't work in MsgComposeCommands.js since it's a button menu. Try it.

> So a single shared function in EdSpellCheck.js didn't work?
I haven't tried. The shared code is minimal and there are also subtle differences.
MsgComposeCommands.js already has an inline spell checker and uses it, the other two allocate one. Sure, you can pass it in. Also, why would be create dependencies just to save a few lines?

Comment 17

[:aceman]

(In reply to Jorg K (GMT+2, PTO during summer, NI me) from comment #16)
> > It is removeAllItems() :)
> That doesn't work in MsgComposeCommands.js since it's a button menu. Try it.

Ok, I just expected a *Menulist variable to represent a menulist element, not something else :)

> > So a single shared function in EdSpellCheck.js didn't work?
> I haven't tried. The shared code is minimal and there are also subtle
> differences.
> MsgComposeCommands.js already has an inline spell checker and uses it, the
> other two allocate one. Sure, you can pass it in. Also, why would be create
> dependencies just to save a few lines?

You asked where to put the shared function. Yes, to save a few lines. But if it is too few lines then it isn't worth it.

But why I you still rolling your own code and sorting when using the block from addDictionaryListToMenu would be shorter and less duplicative? Why not using the grand solution when it is free? Is there some catch I do not see?

Comment 18

[Richard Marti (:Paenglab)]

Looks good. But when I have disabled spell check as you type and the default spell language is English, the indicator is correctly on English. When I choose now German in the spelling toolbar button menu, the spelling dialog opens, but still with English selected. Also the indicator stays on English. In the button menu "German" is selected -> not in sync with the indicator and the spelling dialog.

Comment 19

[Jorg K (CEST = GMT+2)]

Sure. Try with TB 45 or any other version, then raise a new bug ;-)

Comment 20

[Jorg K (CEST = GMT+2)]

This patch changes the menu display only, nothing else.

Comment 21

[Jorg K (CEST = GMT+2)]

Attachment: Alternative solution (v5).

> Why not using the grand solution when it is free?
Happy?

Comment 22

[:aceman]

No, it is not what I wanted. I think we agreed not to make the shared function as it is too small.

But you didn't answer why you do not use the same code as addDictionaryListToMenu() but unnecessarily roll your own.

Comment 23

[Jorg K (CEST = GMT+2)]

Richard, I reported your observation here: Bug 1277658.
The "looks good" from comment #18 is an ui-r+, right?

Comment 24

[Richard Marti (:Paenglab)]

Comment on attachment 8759292 [details] [diff] [review]
Alternative solution (v5).

Yes, it's a ui-r+

Comment 25

[Jorg K (CEST = GMT+2)]

Aceman, well, there has been a misunderstanding then. You said in comment #15:
> You would remove even more code and also NOT duplicate the sort logic.
And that's a good thing, since maybe one day we want to change the sort.

And in comment #16:
> So a single shared function in EdSpellCheck.js didn't work?
That's what I've done.

> But you didn't answer why you do not use the same code as addDictionaryListToMenu()
> but unnecessarily(??) roll your own.
Luckily I saved a previous draft reply as a text file, so here goes:

===
I don't get what your saying, maybe because your English derailed ;-)
(referring to the last paragraph in comment #17):

This is the code from addDictionaryListToMenu:

  sortDictionaryList: function(list) {
    var sortedList = [];
    for (var i = 0; i < list.length; i ++) {
      sortedList.push({"id": list[i],
                       "label": this.getDictionaryDisplayName(list[i])});
    }
    sortedList.sort(function(a, b) {
      if (a.label < b.label)
        return -1;
      if (a.label > b.label)
        return 1;
      return 0;
    });

We can't use that since it doesn't deliver the data structure we need. So we need to do it ourselves:

function PrepareLanguageMenuList(aInlineSpellChecker, aDictList)
{
  for (let i = 0; i < aDictList.length; i++) {
    aDictList[i] = [aInlineSpellChecker.getDictionaryDisplayName(aDictList[i]),
                    aDictList[i]];
  }

  // We need to use a simple sort instead of localeCompare since we need to be
  // consistent with toolkit/modules/InlineSpellChecker.jsm.
  aDictList.sort(function(a, b) {
    if (a[0] < b[0])
      return -1;
    if (a[0] > b[0])
      return 1;
    return 0;
  });
}

===

What I've done is the minimal solution that removes a maximum of duplicated code. Only other option would be to use sortDictionaryList() and change our menu precessing, so instead of using dictList[i][0], use dictList[i].id and instead of dictList[i][1] use dictList[i].label. Do you want that?

And on addDictionaryListToMenu():

addDictionaryListToMenu() can certainly *not* be used since it's doing a whole heap of stuff we don't need.

Also, please look at the two different use cases:
MsgComposeCommands.js:
  var spellChecker = Components.classes['@mozilla.org/spellchecker/engine;1']
                               .getService(mozISpellCheckingEngine);
  spellChecker.getDictionaryList(o1, o2);

EdSpellCheck.js:
gSpellChecker = Components.classes['@mozilla.org/editor/editorspellchecker;1'].createInstance(Components.interfaces.nsIEditorSpellCheck);
gSpellChecker.GetDictionaryList(o1, o2);
This is a different class, it's not like the other two.

compose.js:
    this.mSpellChecker = Components.classes['@mozilla.org/spellchecker/engine;1'].getService(Components.interfaces.mozISpellCheckingEngine);
    this.mSpellChecker.getDictionaryList(o1, o2);

So I can't use addDictionaryListToMenu() and I also can't move the retrieval of the dictionary list into a common function.

Comment 26

[Jorg K (CEST = GMT+2)]

I also have the feeling that if M-C ever change this code, they are more likely to change sortDictionaryList() than getDictionaryDisplayName() since the latter has a clear interface: Short name is, long label returned.

Comment 27

[Jorg K (CEST = GMT+2)]

Sorry: Short name in, long label returned.

Comment 28

[:aceman]

(In reply to Jorg K (GMT+2, PTO during summer, NI me) from comment #25)
> Aceman, well, there has been a misunderstanding then. You said in comment
> #15:
> > You would remove even more code and also NOT duplicate the sort logic.
> And that's a good thing, since maybe one day we want to change the sort.

But today you advocate to change the sort to the strange one in m-c. Therefore I say why do we not just call their sort. You want them to be same to have same menus. Then safest option is to call their sort.

> I don't get what your saying, maybe because your English derailed ;-)
> (referring to the last paragraph in comment #17):
> 
> This is the code from addDictionaryListToMenu:
> 
>   sortDictionaryList: function(list) {
>     var sortedList = [];
>     for (var i = 0; i < list.length; i ++) {
>       sortedList.push({"id": list[i],
>                        "label": this.getDictionaryDisplayName(list[i])});
>     }
>     sortedList.sort(function(a, b) {
>       if (a.label < b.label)
>         return -1;
>       if (a.label > b.label)
>         return 1;
>       return 0;
>     });
> 
> We can't use that since it doesn't deliver the data structure we need. So we
> need to do it ourselves:

Do we really need our different datastructure? I don't see a need. The items contain the same data, just as object properties, not array members. We can populate the menulists from their structure too and it is semantically better.

> What I've done is the minimal solution that removes a maximum of duplicated
> code.
I don't think this is the minimal version as you duplicate the sort that I think can be avoided.

> Only other option would be to use sortDictionaryList() and change our
> menu precessing, so instead of using dictList[i][0], use dictList[i].id and
> instead of dictList[i][1] use dictList[i].label. Do you want that?

Yes, that was exactly my idea.

> And on addDictionaryListToMenu():
> 
> addDictionaryListToMenu() can certainly *not* be used since it's doing a
> whole heap of stuff we don't need.

I said and even pasted just a block of that function that we could copy instead of our duplicated and slightly different code.

> Also, please look at the two different use cases:
> MsgComposeCommands.js:
>   var spellChecker = Components.classes['@mozilla.org/spellchecker/engine;1']
>                                .getService(mozISpellCheckingEngine);
>   spellChecker.getDictionaryList(o1, o2);
> 
> EdSpellCheck.js:
> gSpellChecker =
> Components.classes['@mozilla.org/editor/editorspellchecker;1'].
> createInstance(Components.interfaces.nsIEditorSpellCheck);
> gSpellChecker.GetDictionaryList(o1, o2);
> This is a different class, it's not like the other two.
> 
> compose.js:
>     this.mSpellChecker =
> Components.classes['@mozilla.org/spellchecker/engine;1'].
> getService(Components.interfaces.mozISpellCheckingEngine);
>     this.mSpellChecker.getDictionaryList(o1, o2);
> 
> So I can't use addDictionaryListToMenu() and I also can't move the retrieval
> of the dictionary list into a common function.

You didn't want to make the common function and I accepted that (it would be ~5 lines now). So if you fetch the lists differently in the 3 places and then paste the 5-line block I pasted in comment 12 to those 3 places, what is wrong with that? I think it is still shorter than your version, even if we change to dictList[i].label and .id (which is 1-2 line change).

Comment 29

[Jorg K (CEST = GMT+2)]

Attachment: Alternative solution (v6).

So this is what you want. Sorry, I've been misunderstanding you since comment #12. Now you should be happy, finally ;-)

Comment 30

[Jorg K (CEST = GMT+2)]

Comment on attachment 8759372 [details] [diff] [review]
Alternative solution (v6).

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

One last question, Sir Aceman.

::: mail/components/preferences/compose.js
@@ -194,5 @@
>  
>      // Remove any languages from the list.
> -    var languageMenuPopup = languageMenuList.firstChild;
> -    while (languageMenuPopup.hasChildNodes())
> -      languageMenuPopup.lastChild.remove();

Any idea what this was about?
popup = menulist.firstChild?
And then we remove from it?
But below we append to menulist.
Isn't that strange?

Comment 31

[:aceman]

(In reply to Jorg K (GMT+2, PTO during summer, NI me) from comment #30)
> >      // Remove any languages from the list.
> > -    var languageMenuPopup = languageMenuList.firstChild;
> > -    while (languageMenuPopup.hasChildNodes())
> > -      languageMenuPopup.lastChild.remove();
> 
> Any idea what this was about?
> popup = menulist.firstChild?
> And then we remove from it?
> But below we append to menulist.
> Isn't that strange?

Yes it is strange and I do not like it, that is why we fix it :) It uses internal knowledge of menulist children and removes them as elements. Then later we add new items using XUL method (that internally knows what elements to produce). It usually works I just do not like the inconsistency (why not always let the XUL methods do what is needed).

Comment 32

[:aceman]

Comment on attachment 8759372 [details] [diff] [review]
Alternative solution (v6).

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

Wow, so it is less than even the 5 lines I guessed, it reduced to 1 line calling .sortDictionaryList() :)

I hope you like the elegance of this solution now :)

Comment 33

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/comm-central/rev/d39b521c1bb7

Actually, I like the fact that I see the same dictionary name everywhere now ;-)

Just wait until M-C remove sortDictionaryList() ;-)

Comment 34

[Jorg K (CEST = GMT+2)]

Comment on attachment 8759372 [details] [diff] [review]
Alternative solution (v6).

[Triage Comment]
This can go into TB 48 currently in Aurora. This has been inconsistent for years, so I'd like to ship this in the upcoming TB 48 beta.

Comment 35

[Jorg K (CEST = GMT+2)]

Aurora (TB 48):
https://hg.mozilla.org/releases/comm-aurora/rev/c5eb848ac878

Comment 36

[Jorg K (CEST = GMT+2)]

rsx11m, you might be interested in this bug. You could port this to SM:
https://dxr.mozilla.org/comm-central/source/suite/mailnews/compose/MsgComposeCommands.js#1747
https://dxr.mozilla.org/comm-central/source/suite/common/pref/pref-spelling.js#41

Also see bug 1277658 which you could port to
https://dxr.mozilla.org/comm-central/source/suite/mailnews/compose/MsgComposeCommands.js#1818