Comment on attachment 330112 [details] [diff] [review] pref-formatting patch v0.1 >+var gObject; ... >+ gObject = new Object(); >+ gObject["html"] = new Object(); >+ gObject["plaintext"] = new Object(); >+ gObject["error"] = new Object(); Could be written like this: var gObject = { html: {}, plaintext: {}, error: {} }; >+ gObject["html"].title = domainDlg.getAttribute("htmldlg_title"); These can all be written gObject.html.title = etc. Or, you could write gObject = { html: { title: domainDlg.getAttribute("htmldlg_title"), msg: etc. >+ gPromptService = Components.classes["@mozilla.org/embedcomp/prompt-service;1"] >+ .getService() >+ .QueryInterface(Components.interfaces.nsIPromptService); .getService(Components.interfaces.nsIPromptService); >+ document.getElementById("formatting_pane").userChangedValue(listbox); I take it listbox.doCommand(); didn't work? >+ // Create a listitem for the new Domain >+ var item = document.createElement("listitem"); >+ item.setAttribute("label", aDomain); > >+ // Add it to the active languages listbox >+ aListbox.appendChild(item); aListbox.appendItem(item); probably works. >+function DomainAlreadyPresent(aType, aDomain, aDupe) > { > var found = false; >+ var arrayOfPrefs = gObject[aType].pref.value.split(","); I wonder whether it would be easier to find a listitem with that value. >+ var matching = aListbox.getElementsByAttribute("label", str); Like this, in fact :-) >+ var domains = aListbox.getElementsByAttribute("label", "*"); > >+ var str = ""; >+ for (var i = 0; i < domains.length; i++) >+ { >+ if (i > 0) >+ str = str + "," + domains[i].label; >+ else >+ str = domains[i].label; >+ } >+ return str; Hmm, I wonder whether it would be possible to use Array.map to convert domains to an array of labels which you can then join() together with commas. >+ <data id="domaindlg" >+ htmldlg_title="&add.htmltitle;" >+ htmldlg_msg="&add.htmldomain;" >+ plaintextdlg_title="&add.plaintexttitle;" >+ plaintextdlg_msg="&add.plaintextdomain;" >+ domainerrdlg_title="&domainnameError.title;" >+ duperr="&duplicationError.label;" >+ dualerr="&dualEntryError.label;"/> There should be a bug on converting this to a stringbundle ;-) >+ <observes element="plaintext" >+ attribute="disabled"/> Sneaky :-) Are there other places we should do this?

(In reply to comment #2) >(From update of attachment 330112 [details] [diff] [review]) >>+ // Create a listitem for the new Domain >>+ var item = document.createElement("listitem"); >>+ item.setAttribute("label", aDomain); >> >>+ // Add it to the active languages listbox >>+ aListbox.appendChild(item); >aListbox.appendItem(item); probably works. That should be aListbox.appendItem(aDomain);

Changes since v0.1: * Reformatted how gObject was defined as per Neil's comment * Switched to using doCommand() rather than calling userChangedValue directly * Made use of appendItem as per Neil's comment - removed AddListItem function * Switched to matching using getElementsByAttribute in DomainAlreadyPresent function as per Neil's comment * Switched to using generic Array.map for WriteDomain function as per Neil's comment * Switched to using stringbundle rather than data element attributes as per Neil's comment * Made AddDomain function able to cope with a comma separated list of domains - perhaps I need to rename the function to AddDomains and pluralise the dialog strings? * Made ReadDomains work properly with instant apply - now adds or removes items where relevant instead of just adding.

Comment on attachment 330400 [details] [diff] [review] pref-formatting patch v0.2 >+var gObject = { html: {}, plaintext: {}, error: {} }; Sorry for not being clear, but you can leave this empty now that you went for my third suggestion. >+ var errorMsg = aDupe ? gObject.error.duperr : gObject.error.dualerr; Possibly pass in the error string as the third parameter? >+ errorMsg = errorMsg.replace(/@string@/, aDomain); Can we change this to use %S which is more usual for .properties files? >+ var nextNode = aNode.nextSibling; >+ if (!nextNode) >+ nextNode = aNode.previousSibling; Could use aNode.nextSibling || aNode.previousSibling >+ while (listbox.selectedItems.length > 0) >+ nextNode = RemoveAndNextNode(listbox, listbox.selectedItems[0]); Could use listbox.selectedItem (both lines) >+ if (arrayOfPrefs.indexOf(domain.label) < 0) >+ nextNode = RemoveAndNextNode(aListbox, domain); >+ else >+ i -= 1; You could splice out the value from the array of prefs here so that you don't need to double-check later on. >+ if (nextNode) >+ aListbox.selectItem(nextNode); I don't think we should be changing the selection on a pref change (so you don't need to call RemoveAndNextNode either). >+ <vbox id="html_box" flex="1"> ... >+ <observes element="html" >+ attribute="disabled"/> If it fits in 80 characters, no point wrapping it. >+ onkeypress="if (event.keyCode == 8 || >+ event.keyCode == 46) Might be worth using KeyEvent.DOM_VK_BACK_SPACE/DELETE?

Changes since v0.2 (as per Neil's comments) * Error message passed as 3rd argument into DomainAlreadyPresent function * Replaced @string@ with %S in properties file * Removed use of RemoveAndNextNode function from ReadDomains and inlined code into RemoveDomains along with using next || previous sibling * Switched to using selectedItem instead of selectedItems[0] and selectItems.length * Used array splicing in ReadDomains function allowing removal of the now unnecessary matching code further on * Removed the change of selection from ReadDomains function * Unwrapped observes elements * Switched to using KeyEvent.DOM_VK_BACK_SPACE/DOM_VK_DELETE for event.keyCode Carrying forward sr= and re-requesting r=

Comment on attachment 330431 [details] [diff] [review] pref-formatting patch v0.2a Three really minor nits I just noticed: >+ var domains = aListbox.getElementsByAttribute("label", "*"); >+ if (domains) I don't think domains can ever be null here, only zero-length. >+ i = domains.length - 1; >+ while (i >= 0 && domains) >+ { ... >+ i -= 1; >+ } I don't think that domains is suddenly going to become null during the loop ;-) I think this loop would be a tiny more readable if you wrote it like this: for (i = donains.length; --i >= 0; ) { ... }

Comment on attachment 330431 [details] [diff] [review] pref-formatting patch v0.2a >Index: mailnews/compose/prefs/resources/content/pref-formatting.xul >=================================================================== >+ <vbox id="html_box" flex="1"> >+ <listbox id="html" Huh, these ids are pretty generic and thus far too likely to clash sometimes (same goes for their plaintext relatives below). >+ onsyncfrompreference="return document.getElementById('formatting_pane').ReadDomains(this);" >+ onsynctopreference="return document.getElementById('formatting_pane').WriteDomains(this);" You could store a reference to the prefpane on the listbox in Startup and just do onsyncfrompreference="return this.prefpane.ReadDomains(this);" etc here. >+ onkeypress="if (event.keyCode == KeyEvent.DOM_VK_BACK_SPACE || >+ event.keyCode == KeyEvent.DOM_VK_DELETE) >+ RemoveDomains('html');"/> Is this optimization really significant? You could do the check inside the function and keep the XUL as free of code as possible. The same comments hold for the plaintext part as well. >Index: mailnews/compose/prefs/resources/content/pref-formatting.js >=================================================================== >+var gObject; Huh, couldn't we have a more meaningful name here? But this object and its usage are an atrocity anyway and should go away - you don't need it. >+ //Get the values of the Add Domain Dlg boxes and store it in the global object Missing space behind //, no need to abbr. anything ;-) and sentences starting with an uppercase letter should end in a dot. >+ var bundle = document.getElementById("bundle_formatting"); >+ gObject = >+ { There's no need for this object, especially not if we aren't in the global namespace. >+ html: >+ { >+ title: bundle.getString("htmlTitle"), >+ msg: bundle.getString("htmlDomain"), >+ pref: document.getElementById("mailnews.html_domains"), >+ listbox: document.getElementById("html") >+ }, You can store these references on the listbox object itself and just define globals for the listboxes. Furthermore, you could store a reference to the prefpane on the listboxes here: gHTMLDomainsListbox.prefpane = this; and use it inside the pseudo event handlers. >+function AddDomain(aType) > { >+ var domains; Better set it to null explicitly, avoiding the undefined value check in "if (domains)". >+ if (gPromptService) > { > var result = {value:null}; Need a space behind the ':'. >+ if (gPromptService.prompt(window, gObject[aType].title, >+ gObject[aType].msg, result, null, {value:0})) Here, too. >+ domains = result.value.replace(/ /g,"").split(","); You should also make sure that those are in fact domain names, on a very basic level: - If an array element contains an @, only take the part after the last @. - Each array element should then contain at least one '.'. - Domain names should all be lower case. >+ if (!DomainAlreadyPresent(aType, domainName, gObject.error.duperr)) This is one of the most useless dialogs ever. If the domain is already in the list, that's fine - don't harrass anyone without a dialog! >+ if (!DomainAlreadyPresent(other, domainName, gObject.error.dualerr)) This is only slightly better. I think we should just delete the domain name in the other list without asking. (Neil, what do you think?) If we agree to not ask those stupid questions, you should put the remaining 2 x 2 texts into attributes of the respective listbox and get rid of the .properties file entirely. >+function DomainAlreadyPresent(aType, aDomain, aErrorMsg) Useless, see above. >+function ReadDomains(aListbox) > { ... >+ i = domains.length - 1; >+ while (i >= 0 && domains) >+ { ... >+ i -= 1; >+ } Woo, that's truely awful. See Neil's comment #7. >+ for (i = 0; i < arrayOfPrefs.length; i++) >+ { > var str = arrayOfPrefs[i].replace(/ /g,""); Probably worth doing basic checking here also (see above). >+function LabelFromElement(aElement) > { >+ return aElement.label; >+} You're sure you need this? >+function WriteDomains(aListbox) >+{ >+ var domains = aListbox.getElementsByAttribute("label", "*"); >+ return Array.map(domains, LabelFromElement).join(","); Just use an anonymous function as the parameter.

Changes since v0.2a: * Made ids more unique * Now store reference to prefpane on each listbox * Moved keycode checking into js * Removed stringbundle and .properties file * Now use globals for listboxes and preferences * Does validity check of domains * Only gives error on domain in other type now * Uses anonymous function in map

Comment on attachment 335446 [details] [diff] [review] pref-formatting patch v0.2b >+ document.getElementById("html_domains").pane = this; >+ document.getElementById("plaintext_domains").pane = this; Can't you use gListbox.html and gListbox.plaintext? >+ var start = aDomain.lastIndexOf("@"); >+ if (start > -1) >+ aDomain = aDomain.slice(start + 1); aDomain = aDomain.replace(/.*@/, ""); perhaps? >+ if (aDomain.indexOf(".") == -1) >+ return null; /\./.test(aDomain) perhaps? >+ var found = (matches.length > 0); Don't need the ()s. >+ var arrayOfPrefs = gPref[aListbox.id].value.split(","); This needs the .replace(/ /g, "") here instead of below. >+ <vbox id="html_box" flex="1"> >+ <vbox id="plaintext_box" flex="1"> Actually we probably don't need these ids at all, do we?

Changes since v0.2a (as per review comments): * Use gListbox.html and gListbox.plaintext for setting of .pane * Do replace and test in TidyDomainName * Removed unneeded ()s * Moved where space removal is done in ReadDomains * Removed unused ids from vbox elements Carrying forward sr=

Comment on attachment 335621 [details] [diff] [review] pref-formatting patch v0.2c >+++ b/mailnews/compose/prefs/resources/content/pref-formatting.js >+ var result = {value: null}; >+ if (gPromptService.prompt(window, gListbox[aType].getAttribute("title"), >+ gListbox[aType].getAttribute("msg"), result, >+ null, {value: 0})) >+ domains = result.value.replace(/ /g,"").split(","); Need a space before the "". >+ if (!DomainAlreadyPresent(aType, domainName, false)) >+ if (!DomainAlreadyPresent(other, domainName, true)) So I slept over the warning dialogs... ;-) I still don't think that the "already in other list" warning is useful. If the user enters the domain to be in list y, then this final decision should rule until he changes it again. No need for a disruptive warning, just remove(!) the domain name from list x and add(!) it into y. OTOH, we now drop invalid domain names - and even do so silently, which is rather confusing. Even I wondered at first if something is broken, despite knowing the code... Here we should have warning along 'The domain name "foobar" is invalid and will be ignored. Valid domain names include at least one . and no @ characters.' (Too bad we don't have the space for telling that before.) r- solely for the warning dialogs.

Changes since v0.2c: * When a duplicate domain name is entered it is removed from the original listbox and added to the new one instead of popping up an error message. * When an invalid domain name is entered an error message is popped up explaining what is valid.

Changes since v0.2d (as per mnyromyr's suggestions): * Only warns on user entry, not when reading from prefs * Slight tweak to the invalid domain name message

Comment on attachment 337159 [details] [diff] [review] pref-formatting patch v0.2e >+function DomainAlreadyPresent(aType, aDomain, aRemove) >+{ >+ var listbox = gListbox[aType]; >+ var matches = listbox.getElementsByAttribute("label", aDomain); >+ var found = matches.length > 0; >+ if (found && aRemove) >+ { >+ // Duplicate has been found so remove from other listbox. >+ listbox.removeChild(matches[0]); >+ listbox.doCommand(); >+ } >+ return found && !aRemove; >+} This function is already quite short and half of it is only used half the time - perhaps some of it should be inlined, something like this: var other = aType == "html" ? gListbox.plaintext : gListbox.html; var matches = listbox.getElementsByAttribute("label", aDomain); if (!matches.length) { matches = other.getElementsByAttribute("label", aDomain); if (matches.length) { other.removeChild(matches[0]); removed = true; } listbox.appendItem(domainName); added = true; } if (removed) other.doCommand(); if (added) listbox.doCommand();

Comment on attachment 337159 [details] [diff] [review] pref-formatting patch v0.2e >+ var other = (aType == "html") ? "plaintext" : "html"; Nit: unnecessary ()s

Or perhaps use a helper method: function DomainAlreadyPresent(aListbox, aDomain) { return aListbox.getElementsByAttribute("label", aDomain).item(0); }

Changes since v0.2e: * Revised AddDomain and helper functions as per neil's comments

Comment on attachment 337282 [details] [diff] [review] pref-formatting patch v0.2f (Checkin: Comment 20) >+function ReadDomains(aListbox) >+{ >+ var arrayOfPrefs = gPref[aListbox.id].value.replace(/ /g, "").split(","); >+ >+ if (arrayOfPrefs) Please remove that empty line on checkin. :)

Comment on attachment 337282 [details] [diff] [review] pref-formatting patch v0.2f (Checkin: Comment 20) Pushed with empty line removed. http://hg.mozilla.org/comm-central/rev/98c885b745e4