Closed Bug 728775 Opened 13 years ago Closed 9 years ago

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

Categories

(Thunderbird :: Message Compose Window, defect)

defect
Not set
normal

Tracking

(thunderbird46 wontfix, thunderbird47 wontfix, thunderbird48 fixed, thunderbird49 fixed, thunderbird_esr45 wontfix)

RESOLVED FIXED
Thunderbird 49.0
Tracking Status
thunderbird46 --- wontfix
thunderbird47 --- wontfix
thunderbird48 --- fixed
thunderbird49 --- fixed
thunderbird_esr45 --- wontfix

People

(Reporter: squib, Assigned: jorgk-bmo)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 9 obsolete files)

Attached patch Hacky patch (obsolete) (deleted) — Splinter Review
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.
Attachment #598762 - Flags: feedback?(mbanner)
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.
Attachment #598762 - Flags: feedback?(mbanner) → feedback+
Blocks: 408209
Unbelievable that the patch has hung around for three years!!
Well, as comment 0 says: > Other people are welcome to take over, if they're so inlined. [sic]
I will, once we settled bug 368915.
Attached patch WIP: adapted Jim's patch to the year 2016. (obsolete) (deleted) — Splinter Review
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 ;-)
Attachment #598762 - Attachment is obsolete: true
Attached patch WIP: adapted Jim's patch to the year 2016 (v2). (obsolete) (deleted) — Splinter Review
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.
Assignee: nobody → mozilla
Attachment #8758921 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attached patch Proposed solution (v1). (obsolete) (deleted) — Splinter Review
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 ;-)
Attachment #8759075 - Attachment is obsolete: true
Attachment #8759153 - Flags: ui-review?(richard.marti)
Attachment #8759153 - Flags: feedback?(acelists)
OS: Linux → All
Hardware: x86_64 → All
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.
Attached patch Alternative solution (v1). (obsolete) (deleted) — Splinter Review
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
Attachment #8759178 - Flags: ui-review?(richard.marti)
Attachment #8759178 - Flags: feedback?(acelists)
Attached patch Alternative solution (v2). (obsolete) (deleted) — Splinter Review
OK, here is the full solution. We now show the same dictionary names everywhere courtesy of the inline spell checker and getDictionaryDisplayName().
Attachment #8759153 - Attachment is obsolete: true
Attachment #8759178 - Attachment is obsolete: true
Attachment #8759153 - Flags: ui-review?(richard.marti)
Attachment #8759153 - Flags: feedback?(acelists)
Attachment #8759178 - Flags: ui-review?(richard.marti)
Attachment #8759178 - Flags: feedback?(acelists)
Attachment #8759195 - Flags: ui-review?(richard.marti)
Attachment #8759195 - Flags: review?(acelists)
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 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.
Attachment #8759195 - Flags: review?(acelists) → feedback+
(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?
Attached patch Alternative solution (v3). (obsolete) (deleted) — Splinter Review
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()?
Attachment #8759195 - Attachment is obsolete: true
Attachment #8759195 - Flags: ui-review?(richard.marti)
Attachment #8759228 - Flags: review?(acelists)
(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?
Attached patch Alternative solution (v4). (obsolete) (deleted) — Splinter Review
> 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?
Attachment #8759228 - Attachment is obsolete: true
Attachment #8759228 - Flags: review?(acelists)
Attachment #8759258 - Flags: ui-review?(richard.marti)
Attachment #8759258 - Flags: review?(acelists)
(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?
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.
Sure. Try with TB 45 or any other version, then raise a new bug ;-)
This patch changes the menu display only, nothing else.
Attached patch Alternative solution (v5). (obsolete) (deleted) — Splinter Review
> Why not using the grand solution when it is free? Happy?
Attachment #8759258 - Attachment is obsolete: true
Attachment #8759258 - Flags: ui-review?(richard.marti)
Attachment #8759258 - Flags: review?(acelists)
Attachment #8759292 - Flags: ui-review?(richard.marti)
Attachment #8759292 - Flags: review?(acelists)
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.
Blocks: 1277658
Richard, I reported your observation here: Bug 1277658. The "looks good" from comment #18 is an ui-r+, right?
Comment on attachment 8759292 [details] [diff] [review] Alternative solution (v5). Yes, it's a ui-r+
Attachment #8759292 - Flags: ui-review?(richard.marti) → ui-review+
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.
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.
Sorry: Short name in, long label returned.
(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).
Attached patch Alternative solution (v6). (deleted) — Splinter Review
So this is what you want. Sorry, I've been misunderstanding you since comment #12. Now you should be happy, finally ;-)
Attachment #8759292 - Attachment is obsolete: true
Attachment #8759292 - Flags: review?(acelists)
Attachment #8759372 - Flags: ui-review+
Attachment #8759372 - Flags: review?(acelists)
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?
(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 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 :)
Attachment #8759372 - Flags: review?(acelists) → review+
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() ;-)
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Summary: Spelling toolbar button's language list should match the context menu's → Make language list consistent everywhere: Spelling toolbar button, context menu, full spell check, composition preference
Target Milestone: --- → Thunderbird 49.0
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.
Attachment #8759372 - Flags: approval-comm-aurora+
Blocks: 1279055
Flags: needinfo?(rsx11m.pub)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: