User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.0.2) Gecko/20060405 SeaMonkey/1.0.1 Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.0.2) Gecko/20060405 SeaMonkey/1.0.1 Hello, Thunderbird has a little dropdown menu next to the spellcheck button on the compose new message window. This is missing in SeaMonkey 1.0.1, instead you have to click on the spellcheck button, and select your language from a new dialog that pops up. It would be nice to pick from all installed spellcheck languages from a dropdown menu on the main composer window, possibly by clicking on a current region-lang (en_US, es_ES) indicator on the bottom-right hand of window. Reproducible: Always

Ian: You have added the inline spellchecking, IIRC - could you add this small but helpful selection (perhaps port it from Thunderbird)? IMO, we want this for 1.1 as well.

Yes, I was looking to put something in both the menu but also the prefs pane and I was looking to free upspace in prefs to put it. I'm sure there is a bug already open on one or both of these issues. Bug 327152 is one, that I can see on a quick scan.

This patch: * Adds a language selector drop-down to the spell button in the compose window. * Re-arranges the Mail & Newsgroups Composition pref panel. * Adds a language selector to the Mail & Newsgroups Composition pref panel. * Updates help to reflect the changes to the pref panel.

Comment on attachment 221519 [details] [diff] [review] Spellcheck selector patch v0.1 >+ <menupopup id="LanguageMenulist" onpopupshowing="onShowDictionaryMenu();"> >+ <!-- this popup gets dynamically generated --> >+ </menupopup> Please move the comment out so that you can use <menupopup .../> > var nsIMsgCompDeliverMode = Components.interfaces.nsIMsgCompDeliverMode; > var nsIMsgCompSendFormat = Components.interfaces.nsIMsgCompSendFormat; > var nsIMsgCompConvertible = Components.interfaces.nsIMsgCompConvertible; > var nsIMsgCompType = Components.interfaces.nsIMsgCompType; > var nsIMsgCompFormat = Components.interfaces.nsIMsgCompFormat; > var nsIAbPreferMailFormat = Components.interfaces.nsIAbPreferMailFormat; > var nsIPlaintextEditorMail = Components.interfaces.nsIPlaintextEditor; >+const nsISupportsString = Components.interfaces.nsISupportsString; var (or change them all to const) for consistency? >+ var languageMenuList = document.getElementById('LanguageMenulist'); >+ if (!languageMenuList) return; Nit: return on its own line >+ // we really need to add locale-aware JS collation Actually this is now possible in JS :-) but what we really need is RDF ;-) >+ var item = document.createElement("menuitem"); >+ item.setAttribute("label", dictList[i][0]); >+ item.setAttribute("value", dictList[i][1]); >+ item.setAttribute("type", "radio"); >+ item.setAttribute("oncommand", "changeLanguage(event)"); Nit: command events bubble, so put the handler on the menupopup in the XUL. >+function onShowDictionaryMenu() >+{ >+ var curLang = sPrefs.getComplexValue("spellchecker.dictionary", nsISupportsString).data; >+ var languageMenuList = document.getElementById("LanguageMenulist"); You could pass event.target to avoid having to do this. >+ var languages = languageMenuList.getElementsByAttribute("value", curLang); >+ languages[0].setAttribute("checked", true); Heh, for some reason I had my dictionary set to a missing language. You might want to use .item(0) and some null-testing. >+ sPrefs.setComplexValue("spellchecker.dictionary", nsISupportsString, str); It's very confusing how many different places you might have to set the dictionary to get anything done :-/ I do wonder though whether we should be setting the pref here - after all changing the dictionary in preferences won't update the compose window. Of course that would mean adjusting or removing the code that shows the current dictionary. >+ // now check the document over again with the new dictionary >+ InlineSpellChecker.checkDocument(window.content.document); I wonder why ToggleInlineSpellChecker duplicates the enabled check... >+ event.stopPropagation(); Is this necessary? The spelling button uses command= rather than oncommand= >+ var aSpellChecker = Components.classes['@mozilla.org/spellchecker/myspell;1'] Nit: a (!) erroneous prefix here. >+ // we really need to add loacel-aware JS collation, see bug XXXXX Interestingly this is misspelled ;-) and bug XXXXX is no point either >+ } else >+ languageMenuList.selectedIndex = 0; >+ Is this necessary? It should default to the first item. Nit: blank line.

Changes since v0.1: * Moved comment out so <menupopup.../> can be used. * Switched vars to consts so consistent. * Used locale-aware JS collation. * Moved changeLanguage oncommand to menupopup. * Now pass event.target into onShowDictionaryMenu. * Test length of array of languages retrieved so does not try to use empty array. * Removed getting and setting of dictionary pref in compose window, just uses currently set spellchecker dictionary. * Checks for inline spellchecking being enabled before rechecking message with new dictionary. * Removed unneeded selectedIndex = 0 * Fixed a few other nits. I left event.stopPropagation(); in as otherwise Check Spelling dialog comes up when clicking on dictionary. Carrying forward sr=

*** Bug 292217 has been marked as a duplicate of this bug. ***

Comment on attachment 221645 [details] [diff] [review] const no pref save patch v0.1a Changing the dict on the menubutton has no effect on the dict shown in the spell checking dialog or in the preferences, although the latter two are in sync. This is quite confusing. If we want to allow the user to override the pref default temporarily, at least menubutton and spell check dialog should show the same setting. Furthermore, the "[x] Spellcheck as you type" options item is not in sync with the pref dialog. In both cases, if we allow this temporary override, it should be explained somewhere, eg. in the prefs ("You can override this setting temporarily during composition" or something like that). >Index: mailnews/compose/resources/content/MsgComposeCommands.js >=================================================================== >+const nsIMsgCompDeliverMode = Components.interfaces.nsIMsgCompDeliverMode; >+const nsIMsgCompSendFormat = Components.interfaces.nsIMsgCompSendFormat; >+const nsIMsgCompConvertible = Components.interfaces.nsIMsgCompConvertible; >+const nsIMsgCompType = Components.interfaces.nsIMsgCompType; >+const nsIMsgCompFormat = Components.interfaces.nsIMsgCompFormat; >+const nsIAbPreferMailFormat = Components.interfaces.nsIAbPreferMailFormat; >+const nsIPlaintextEditorMail = Components.interfaces.nsIPlaintextEditor; >+const nsISupportsString = Components.interfaces.nsISupportsString; Constants should either be completely UPPERCASED or prefixed with k. Furthermore, you're using >+ var spellChecker = Components.classes["@mozilla.org/spellchecker/myspell;1"] >+ .getService(Components.interfaces.mozISpellCheckingEngine); several times below, so the interface/the service should probably global, too. >@@ -1339,16 +1340,17 @@ function ComposeStartup(recycled, aParam > document.getElementById("FormatToolbar").setAttribute("hidden", true); > document.getElementById("formatMenu").setAttribute("hidden", true); > document.getElementById("insertMenu").setAttribute("hidden", true); > document.getElementById("menu_showFormatToolbar").setAttribute("hidden", true); > } > > // Do setup common to Message Composer and Web Composer > EditorSharedStartup(); >+ initLanguageMenu(); I didn't do a survey ;-), but the majority of functions in this file start with uppercase letters and so should you, too. (This holds for all your functions in this file, I won't reply that record every time.) >+ // Load the string bundles that will help us map >+ // RFC 1766 strings to UI strings. >+ >+ // Load the language string bundle. You don't _load_ the bundle here, and it's only one... >+ var isoStrArray; >+ var langId; Even though it doesn't matter in JS, it's good custom to declare variables as "close" to their use as possible. >+ var i; Like you did here... >+ try >+ { >+ langId = dictList[i]; >+ isoStrArray = dictList[i].split("-"); >+ >+ dictList[i] = new Array(2); // first subarray element - pretty name >+ dictList[i][1] = langId; // second subarray element - language ID >+ >+ if (languageBundle && isoStrArray[0]) >+ dictList[i][0] = languageBundle.getString(isoStrArray[0].toLowerCase()); >+ >+ // the user needs to be able to distinguish between the UK English dictionary >+ // and say the United States English Dictionary. If we have a isoStr value then >+ // wrap it in parentheses and append it to the menu item string. i.e. >+ // English (US) and English (UK) >+ if (!dictList[i][0]) >+ dictList[i][0] = dictList[i][1]; >+ else if (isoStrArray.length > 1 && isoStrArray[1]) // if we have a language ID like US or UK, append it to the menu item >+ dictList[i][0] += ' (' + isoStrArray[1] + ')'; Instead of doing strange things with two dimensional arrays, you could just use two variables 'label' and 'value' and do a dictList[i] = [label, value]; in the end. >+ // GetString throws an exception when a key is not found in the The function is named getString. >+ // now select the dictionary we are currently using This comment is just wrong wrt the following code: >+ for (i = 0; i < dictList.length; i++) >+ { >+ var item = document.createElement("menuitem"); >+ item.setAttribute("label", dictList[i][0]); >+ item.setAttribute("value", dictList[i][1]); >+ item.setAttribute("type", "radio"); >+ languageMenuList.appendChild(item); >+ } >Index: mailnews/compose/prefs/resources/content/pref-composing_messages.xul >=================================================================== >+ function Startup() { There'll be quite some code in this .xul file now, so probably a .js file would be useful. >+ function initSpellCheck() { This function is similar enough to the one in MsgComposeCommands.js, that (most of) the comments there still hold here. I'll just add some nits: >+ var languageBundle = document.getElementById("languageBundle"); >+ var regionBundle; You should init that. >+ <menupopup> >+ <menuitem value="more-cmd" label="&moreDictionaries.label;"/> This should be at the end of the dropdown, after the separator. >Index: mailnews/compose/prefs/resources/locale/en-US/pref-composing_messages.dtd >=================================================================== >+<!ENTITY moreDictionaries.label "Download More Dictionaries"> Word 2 + 3 shouldn't be uppercased and the string should end in an ellipsis, since it opens another (browser) window.

Changes since v0.1a: * Added entity to pref pane explaining ability to override prefs in each compose window. * Added line in help about the above. * Fixed EdSpellCheck.js so that overridden language setting is persisted through manual spell checking. * Created const for Components.interfaces.mozISpellCheckingEngine (the naming convention for interface constants seems to be the exception to the k rule and just the name of the interface). * Tweaked functions for dictList as suggested. * Matched dictList code changes in EdSpellCheck.js to those elsewhere. * Uppercased first letter of functions. * Split JS out of pref-composing_messages.xul as suggested. * Moved the "Download more dictionaries..." to the end of the drop down as suggested. * Moved declaration of variables closer to where they are used as suggested. * Corrected some comments as suggested.

Comment on attachment 223635 [details] [diff] [review] persist lang patch v0.1b You don't like jar.mn files, eh? The diff for mailnews/jar.mn is missing... ;-) >+++ mailnews/compose/prefs/resources/content/pref-composing_messages.js ... >+ // sort by locale-aware collation >+ dictList.sort( >+ function compareFn(a, b) { >+ return a[0].localeCompare(b[0]); >+ } >+ ); Hm, this doesn't seem to work correctly: I installed en-US, de-DE and ru (sic!) and got Russian German/Germany English/Unitzed Stated in the dropdown... >Index: editor/ui/dialogs/content/EdSpellCheck.js >=================================================================== ... >-function CancelSpellCheck() >+function UninitSpellChecker() "Uninit"? Ugh. Maybe better Finalize... or Exit...? The behaviour/connection of the three language lists is still very confusing: I went to the pref window and set my default spelling language to Russian, then I opened a mail editor window and entered some text, making the toolbarbutton active. I clicked the button and opened the spell check dialog and changed the language there to English and closed it. (The language drop down here has a different download text and it's at the beginning - that should probably change also.) The toolbarbutton's dropdown now says English, so I set to German. Entering the spell check dialog again still says English - and my pref setting of Russian is gone also, now set to English! We really should get some consistency into this mess... - pref panel: set the global setting here, which will be the prefilled setting when opening a mail composition window - toolbarbutton dropdown: change temporary setting here - spell check dialog: *also* change the temporary setting here, *not* the global one!

I think we are going have to go down the same route as TB and have any dictionary change saved to the pref as the inline spellchecker is shared between all compose windows. Unless someone has thoughts on how to tackle this? Changing how the inline spellchecker behaves is outside the scope of this bug and I've created bug 341960 for that enhancement.

Changes since v0.1b: * Goes back to saving the dictionary to preferences when it is changed in the spell check dialog or in the spell check button dropdown. * If a new dictionary is downloaded from either preferences or spell check dialog then all the drop downs should include the new dictionary without having to close and reopen windows/dialogs or switch pref panes. * Switching to a new dictionary in spell check dialog as well as setting the inline dictionary correctly (as in 0.1b) also rechecks the document with the inline spellchecker.

Comment on attachment 226237 [details] [diff] [review] Back to setting global pref patch v0.1c Some of changes appear several times; I only commented on one of the occurences, but they usually apply to the other instances as well... >Index: mailnews/compose/resources/content/MsgComposeCommands.js >=================================================================== >+function InitLanguageMenu(startingUp) Argument parameters should have the 'a' prefix (like you did in other functions). >+ var languageMenuList = document.getElementById("LanguageMenulist"); IDs with uppercased first letter aren't common practice, please change that. And probably equalize the casing between ID and variable... ;-) >Index: mailnews/compose/prefs/resources/content/pref-composing_messages.js >=================================================================== >+function Startup() { ... >+function enableTextbox(checkbox, textbox, startingUp) { While you're introducing this file, make use of the occasion and make all function names start with uppercase letters and IDs with lowercase... (Change to using the argument prefix 'a' for bonus points, if you're bored. ;-) ) >Index: mailnews/compose/prefs/resources/locale/en-US/pref-composing_messages.dtd >=================================================================== >+<!ENTITY spellingDesc.label "These preferences can be overridden temporarily in each mail composition window."> No, they can't. >Index: editor/ui/dialogs/content/EdSpellCheck.xul >=================================================================== > <menulist id="LanguageMenulist" oncommand="SelectLanguage()"> That'd explain where you got that ID - it's a shared dialog. :-(( (But TB uses an own EdSpellCheck.js.) So keep it here, as an exception. >Index: editor/ui/dialogs/content/EdSpellCheck.js >=================================================================== >+ if (window.opener.InlineSpellChecker.inlineSpellChecker) >+ if (window.opener.InlineSpellChecker.inlineSpellChecker.enableRealTimeSpell) What are you checking here? If window.opener.InlineSpellChecker.inlineSpellChecker is non-null or if it exists as a member in window.opener.InlineSpellChecker at all? If the latter, use ("inlineSpellChecker" in window.opener.InlineSpellChecker). r=me with theses nits fixed.

Changes since v0.1c: * Changed function parameters to have an 'a' prefix * Made all function names start with uppercase letters * Made all IDs start with lowercase letters * Removed unneeded spellingDesc.label entity * Now check for "inlineSpellChecker" in window.opener.InlineSpellChecker * Added equivalent enhancements to TB where missing Carrying forward r= for mailnews parts

Comment on attachment 227314 [details] [diff] [review] Combined mailnews / tb patch v0.1d Requesting review for TB parts of patch (latter half)

Comment on attachment 227314 [details] [diff] [review] Combined mailnews / tb patch v0.1d thanks Ian!

Comment on attachment 227314 [details] [diff] [review] Combined mailnews / tb patch v0.1d > // Do setup common to Message Composer and Web Composer > EditorSharedStartup(); >+ InitLanguageMenu(true); This isn't necessary; the language menu is only used when it is opened. [Unlike menulists, which need to display the currently selected items] >+function InitSpellCheck(aStartingUp) { >+ if (!("@mozilla.org/spellchecker;1" in Components.classes)) { >+ document.getElementById("spellingGroup").hidden = true; >+ return; >+ } This belongs in Startup(), and then you can rename the method back to InitLanguageMenu to match the other versions. Also consider something like: function Startup() { try { InitLanguageMenu(); } catch (e) { document.getElementById("spellingGroup").hidden = true; } >+ // If we're not just starting up and dictionary count >+ // hasn't changed then no need to update the menu. >+ if (!aStartingUp && (gDictCount == count)) >+ return; If you initialise gDictCount to zero then you don't need the startup flag. [The amount of time saved by avoiding removing zero menuitems is minimal] sr=me with these nits fixed.

Changes since v0.1d: * Removed aStartingUp and initialised gDictCount as zero instead. * Moved to using sDictCount instead of gDictCount in MsgComposeCommands.js to match style within file. * Removed unneeded call to InitLanguageMenu from ComposeStartup. * Renamed InitSpellCheck to InitLanguageMenu and did other suggested revisions in pref_composing-messages. * Moved to using mDictCount instead of gDictCount in TB's compose.js. Carrying forward r/sr.

Changes since v0.1e: * Removed null-checks after GetService and used "in" tests instead. * Removed some extra whitespaces. Carrying forward r/sr.

Comment on attachment 227628 [details] [diff] [review] Tweaked mailnews/tb patch v0.1f Checking in (trunk) mailnews/jar.mn; new revision: 1.112; previous revision: 1.111 mailnews/compose/resources/content/messengercompose.xul; new revision: 1.285; previous revision: 1.284 mailnews/compose/resources/content/MsgComposeCommands.js; new revision: 1.379; previous revision: 1.378 mailnews/compose/prefs/resources/content/pref-composing_messages.xul; new revision: 1.49; previous revision: 1.48 mailnews/compose/prefs/resources/content/pref-composing_messages.js; initial revision: 1.1 mailnews/compose/prefs/resources/locale/en-US/pref-composing_messages.dtd; new revision: 1.26; previous revision: 1.25 editor/ui/dialogs/content/EdSpellCheck.xul; new revision: 1.62; previous revision: 1.61 editor/ui/dialogs/content/EdSpellCheck.js; new revision: 1.58; previous revision: 1.57 editor/ui/locales/en-US/chrome/dialogs/EditorSpellCheck.dtd; new revision: 1.2; previous revision: 1.1 extensions/help/resources/locale/en-US/mail_help.xhtml; new revision: 1.72; previous revision: 1.71 mail/components/compose/content/messengercompose.xul; new revision: 1.88; previous revision: 1.87 mail/components/compose/content/MsgComposeCommands.js; new revision: 1.94; previous revision: 1.93 mail/components/compose/content/EdSpellCheck.js; new revision: 1.5; previous revision: 1.4 mail/components/preferences/compose.xul; new revision: 1.7; previous revision: 1.6 mail/components/preferences/compose.js; new revision: 1.9; previous revision: 1.8 done

Comment on attachment 227628 [details] [diff] [review] Tweaked mailnews/tb patch v0.1f Requesting approval for 1.8.1 branch / SM1.1a

Comment on attachment 227628 [details] [diff] [review] Tweaked mailnews/tb patch v0.1f a=me for SeaMonkey 1.1 given it works well on trunk.

Is this SM only or does it affect TBird? If the later MScott can you weigh in?

(In reply to comment #22) > Is this SM only or does it affect TBird? If the later MScott can you weigh in? > Yes it effects Thunderbird too, but we're fine with the change. I'll approve the fix.

(In reply to comment #23) > (In reply to comment #22) > > Is this SM only or does it affect TBird? If the later MScott can you weigh in? > > > > Yes it effects Thunderbird too, but we're fine with the change. I'll approve > the fix. > I forgot, I no longer have privileges to approve 1.8.1 bugs, so schrep will have to do it.

Is it possible to get approval on this for 1.8.1, schrep?

Comment on attachment 227628 [details] [diff] [review] Tweaked mailnews/tb patch v0.1f Yep - sorry for the delay!

Comment on attachment 227628 [details] [diff] [review] Tweaked mailnews/tb patch v0.1f Checking in (1.8 branch) mailnews/jar.mn; new revision: 1.99.2.9; previous revision: 1.99.2.8 mailnews/compose/resources/content/messengercompose.xul; new revision: 1.282.2.1; previous revision: 1.282 mailnews/compose/resources/content/MsgComposeCommands.js; new revision: 1.369.2.4; previous revision: 1.369.2.3 mailnews/compose/prefs/resources/content/pref-composing_messages.xul; new revision: 1.46.4.3; previous revision: 1.46.4.2 mailnews/compose/prefs/resources/content/pref-composing_messages.js; new revision: 1.1.2.1; previous revision: 1.1 mailnews/compose/prefs/resources/locale/en-US/pref-composing_messages.dtd; new revision: 1.23.4.3; previous revision: 1.23.4.2 editor/ui/dialogs/content/EdSpellCheck.xul; new revision: 1.61.28.1; previous revision: 1.61 editor/ui/dialogs/content/EdSpellCheck.js; new revision: 1.57.28.1; previous revision: 1.57 editor/ui/locales/en-US/chrome/dialogs/EditorSpellCheck.dtd; new revision: 1.1.8.1; previous revision: 1.1 extensions/help/resources/locale/en-US/mail_help.xhtml; new revision: 1.62.2.9; previous revision: 1.62.2.8 mail/components/compose/content/messengercompose.xul; new revision: 1.79.2.5; previous revision: 1.79.2.4 mail/components/compose/content/MsgComposeCommands.js; new revision: 1.72.2.16; previous revision: 1.72.2.15 mail/components/compose/content/EdSpellCheck.js; new revision: 1.4.4.1; previous revision: 1.4 mail/components/preferences/compose.xul; new revision: 1.3.2.4; previous revision: 1.3.2.3 mail/components/preferences/compose.js; new revision: 1.2.4.6; previous revision: 1.2.4.5 done

Is the check-in to EditorSpellCheck.dtd a semantic change? I would think so. Then it should change the entity name so that localizations are actually notified to pick that change up. And I would think that '...' should be &hellip;.

I would not see it as a real L10n bug if a localiser leaves it as "Download More" in their L10n. At the moment &hellip; does not seem to be being used anywhere according to bonsai.

*** Bug 260533 has been marked as a duplicate of this bug. ***

*** Bug 327152 has been marked as a duplicate of this bug. ***

(In reply to comment #30) > *** Bug 260533 has been marked as a duplicate of this bug. *** > I disagree. Bug 260533 is not a dup of this one. Bug 260533 is an enhancement request that is about automatically switching the spell check language if a recipient's language is recognized, for example, using domain matching (eg., *.de -> German) or using a setting in the address book entry. AFAIK this functionality has not been added with this patch, hasn't it?