Now that labels have been replaced by tags, we need to replace the labels preferences panel with a respective tag configuration panel. (Since tags are on the 1.8 branch, this is a 1.1a blocker!)

Do we really need this for the Alpha (note that Alpha is expected to happen with 1.8.1b2, due on Aug 15th) - or can we let this slip to Beta? Note that we really need to have it for Beta, as that also marks the L10n freeze... Karsten, how soon could you fix that?

If this is simply a "copy Thunderbirds Text/Options" for tags, I can tackle this if Karsten Cannot; though I am not sure if I can make the 1.1a date. Also imho I do think we can let this slip to beta.

> Do we really need this for the Alpha (note that Alpha is expected to happen > with 1.8.1b2, due on Aug 15th) - or can we let this slip to Beta? With the current dialog, you can't neither remove, rename, reorder nor recolor tags (you can add tags via context menus in the main mail window). We could temporarily use a dead-hacked version of the TB panel for Alpha, that shouldn't be too complicated and would allow us at least to remove tags (but still no rename/reorder/recolor). Actually, this bug depends upon bug 342065 (at least for SM). > Karsten, how soon could you fix that? Tuesday (11th) in the worst case for the TB dialog take-over.

XRef: Bug 343875 (TB pref pane)

This patch just copies over what TB has currently as its (also temporary) pref pane. This UI needs to be reowrked for beta/final!

Comment on attachment 232343 [details] [diff] [review] Band-aid patch: just copy TB's pref pane >-/* Function to restore pref values to application defaults */ >+/* Function to restore pref values to application defaults * / > function restoreColorAndDescriptionToDefaults() >+ tagService.deleteKey(itemToRemove.getAttribute("value")); Nit: itemToRemove.value should work. Do we still need this? >+ <hbox flex="1"> >+ <listbox id="tagList" flex="1" rows="10"/> rows doesn't make sense here, because the list is stretched to the box. I notice that a white tag doesn't work very well ;-) I'm torn between two ideas for UI for this: 1. use a tree, so you can easily reuse the thread pane styles 2. use a listbox of textboxes and pickers, so you can easily edit inline

> >-/* Function to restore pref values to application defaults */ > >+/* Function to restore pref values to application defaults * / > > function restoreColorAndDescriptionToDefaults() Oops. Yes, this function should go away. > >+ tagService.deleteKey(itemToRemove.getAttribute("value")); > Nit: itemToRemove.value should work. It does. > rows doesn't make sense here, because the list is stretched to the box. Oops. Yes. > I notice that a white tag doesn't work very well ;-) Heh. :) > I'm torn between two ideas for UI for this: > 1. use a tree, so you can easily reuse the thread pane styles > 2. use a listbox of textboxes and pickers, so you can easily edit inline With (part of) my patch in bug 342576, the thread pane styles are accessible as CSS classes also, so that would be less of an issue.

Fixed Neil's comments.

(In reply to comment #7) >>I'm torn between two ideas for UI for this: >>1. use a tree, so you can easily reuse the thread pane styles >>2. use a listbox of textboxes and pickers, so you can easily edit inline >With (part of) my patch in bug 342576, the thread pane styles are accessible as >CSS classes also, so that would be less of an issue. Nice, but don't forget to take care that you will always be able to see which tag is selected - you wouldn't want to delete the wrong tag by mistake ;-)

Comment on attachment 232606 [details] [diff] [review] Band-aid patch2 >Index: resources/content/pref-labels.xul >=================================================================== >+ <description>&displayText.label;</description> >+ <hbox flex="1"> >+ <listbox id="tagList" flex="1"/> >+ <vbox> >+ <button label="&addTagButton.label;" >+ accesskey="&addTagButton.accesskey;" >+ oncommand="AddTag();"/> >+ <button label="&removeTagButton.label;" >+ accesskey="&removeTagButton.accesskey;" >+ oncommand="RemoveTag();"/> >+ </vbox> >+ </hbox> The entities should be deleteTag* rather than removeTag* similarly for the function. Would be good to have a "Restore Defaults" button still here, if not for 1.1a then for the non-band aid solution. >Index: resources/locale/en-US/mailPrefsOverlay.dtd >=================================================================== >-<!ENTITY labels.label "Labels"> >+<!ENTITY labels.label "Tags"> Entity should be really be tags.label so that anyone localising 1.1a picks the change up more easily. >Index: resources/locale/en-US/pref-labels.dtd >=================================================================== >+<!ENTITY pref.labels.title "Tags"> >+<!ENTITY labelsSettings.caption "Customize Tags"> >+<!ENTITY displayText.label "Tags can be used to categorize and prioritize your messages."> Again entities should be changed to help anyone localising 1.1a. >+<!ENTITY addTagButton.label "Add"> >+<!ENTITY addTagButton.accesskey "A"> >+<!ENTITY removeTagButton.label "Delete"> >+<!ENTITY removeTagButton.accesskey "D"> deleteTag* instead of removeTag* In any non-band aid solution should also have the ability to disable the Add/Delete/Restore buttons by pref setting - see http://lxr.mozilla.org/seamonkey/search?string=disable_button

Addressed Ian's comments.

Comment on attachment 232621 [details] [diff] [review] Band-aid patch 3 >-<!ENTITY pref.labels.title "Labels"> Note that this might be referenced in help, so be sure to change it to pref.tags.title there as well if that's the case. a=me for 1.1a given its gets r+

> >-<!ENTITY pref.labels.title "Labels"> > > Note that this might be referenced in help, I don't see why it would and lxr claims it isn't. We do need help to reflect the UI changes, though; I'll do that tonight.

Comment on attachment 232621 [details] [diff] [review] Band-aid patch 3 Restoring the default tags via addTag does not create the correct keys, I need to create the pref.js entries directly. IanN, please suppose that instead of the "tagService.addTag(tag, color);" call in RestoreDefaults. ++

(In reply to comment #13) > > >-<!ENTITY pref.labels.title "Labels"> > > > > Note that this might be referenced in help, > > I don't see why it would and lxr claims it isn't. We do need help to reflect > the UI changes, though; I'll do that tonight. I was thinking about the original intent behind bug 289444 (referencing the pref panel titles from help docs), but it seems that has never been actually done, so no need to worry, that's true. Thanks for looking at the help docs as well, though :)

Restore default tags with correct keys. Would be taking over a+ if I could, so imagine it's there. ;-)

Help patch for tags in general, thus slightly ahead of the ban-aid patch.

Comment on attachment 232801 [details] [diff] [review] help patch a=me for landing the appropriate changes in 1.1a code (the suite/locales/ part doesn't apply there, and pref-help.js probably still lives in xpfe/ there)

Comment on attachment 232800 [details] [diff] [review] Band-aid patch 4 r/a=me

Comment on attachment 232801 [details] [diff] [review] help patch help-index1.rdf files are listed alphabetically so you need to move "Tags" into the correct place in the file rather than just replacing in current position for both /suite and /xpfe versions. >Index: extensions/help/resources/locale/en-US/mail_help.xhtml >=================================================================== There are changes in this file that do not seem relevant possibly bit rotted or other patches in your tree? >Index: suite/common/pref/pref-help.js >=================================================================== As Kairo said need the xpfe version for 1.1a >Index: suite/locales/en-US/chrome/common/help/mail_help.xhtml >=================================================================== There are changes in this file that do not seem relevant possibly bit rotted or other patches in your tree? >Index: suite/locales/en-US/chrome/common/help/suite-toc.rdf >=================================================================== There are changes in this file that do not seem relevant possibly bit rotted or other patches in your tree?

Corrected index order, against trunk.

Same textual content as attachment 233149 [details] [diff] [review], but against 1.8 branch (i.e. different pref-help.js location).

Comment on attachment 233150 [details] [diff] [review] help patch, v2branch a=me for 1.1a given it gets r+

Comment on attachment 232800 [details] [diff] [review] Band-aid patch 4 Checked in into MOZILLA_1_8_BRANCH for SM 1.1a and trunk (to keep it in sync).

Comment on attachment 233149 [details] [diff] [review] help patch, v2trunk >Index: extensions/help/resources/locale/en-US/mail_help.xhtml >=================================================================== >@@ -4749,37 +4753,40 @@ to filter unwanted mail.</p> >+ <li><strong>Customize Tags</strong>: Specifies the tag text and the color >+ for each tag. You can edit or replace the default tag text with your >+ own text (up to 32 characters). To change the tag color, click the color >+ chip next to that tag and select a new color. Use the Move Up and Move Down >+ buttons to reorder your tags. Top tags have higher priority when coloring >+ tagged messages.</li> This last bit needs rewording to make the meaning clearer. >Index: suite/locales/en-US/chrome/common/help/mail_help.xhtml >=================================================================== >@@ -4749,37 +4753,40 @@ to filter unwanted mail.</p> >+ <li><strong>Customize Tags</strong>: Specifies the tag text and the color >+ for each tag. You can edit or replace the default tag text with your >+ own text (up to 32 characters). To change the tag color, click the color >+ chip next to that tag and select a new color. Use the Move Up and Move Down >+ buttons to reorder your tags. Top tags have higher priority when coloring >+ tagged messages.</li> As above. r=me with that change.

Comment on attachment 233150 [details] [diff] [review] help patch, v2branch >Index: extensions/help/resources/locale/en-US/mail_help.xhtml >=================================================================== >@@ -4749,37 +4754,39 @@ to filter unwanted mail.</p> >+ <li><strong>Customize Tags</strong>: Specifies the tag text and the color >+ for each tag. You can edit or replace the default tag text with your >+ own text (up to 32 characters). To change the tag color, click the color >+ chip next to that tag and select a new color. Use the Move Up and Move Down >+ buttons to reorder your tags. Top tags have higher priority when coloring >+ tagged messages.</li> As per trunk patch, r=me with that change.

Checked in help changes into trunk and MOZILLA_1_8_BRANCH. We may need to change some wording when the real pref panel is there, though; e.g. Neil suggested using "more important"/"less important" for the tag order buttons...

If it's possible to add a option in the prefs panel to change the text decoration (bold, underline, cross out) of the tags? IMHO it's to little that I could only change the text color of tags.

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

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

New tag pref panel for SM, including tag ordering. Further changes: - call pref panel from tag context menu - help update

Comment on attachment 238834 [details] [diff] [review] Tag pref panel, v1 >- <treeitem> >+ <treeitem id="mailtagspref"> Hmm? >+// <listbox> have drawbacks which make them seem inappropriate here. What's the drawback of a listbox, the fact that you need allowevents="true" to be able to embed arbitrary xul or something else? >+// init global stuff before the wsm is used Any particular reason why; is SetFields called before Startup or something? >+ // Creating a colorpicker dynamically in an onload handler is really sucky. >+ // You MUST first set its type attribute, then add the element to the DOM >+ // and finally set the color property(!) afterwards. Try in any other order >+ // and fail... :-( Would it help to make the colorpicker constructor read the color attribute? >+function OnFocus(aTarget) >+{ >+ // alas, the textbox doesn't catch this event, but its html:input element If this is still applicable with a listbox conversion, you could add a capturing focus event listener to the box, then just walk up until you get to the right element. > function AddTag() I don't like this. I'd prefer for it to immediately add a row to the list.

(In reply to comment #34) > (From update of attachment 238834 [details] [diff] [review] [edit]) > >- <treeitem> > >+ <treeitem id="mailtagspref"> > Hmm? nsPrefWindow needs this when opening the tag pref panel from a context menu. > >+// <listbox> have drawbacks which make them seem inappropriate here. > What's the drawback of a listbox The overflow-y scrollbars on listboxes overlap the content. And if the rightmost column consists of eg. colorpickers, these (on Win2k at least) are drawn incomplete! (This seems to be fixed on trunk, but not on the 1.8 branch.) > , the fact that you need allowevents="true" to > be able to embed arbitrary xul or something else? Okay, that'd work with trunk and 1.8 branch. Nevertheless, I actually don't see the point of a listbox here... > >+// init global stuff before the wsm is used > Any particular reason why; > is SetFields called before Startup Yes, it is. >>+ // Creating a colorpicker dynamically in an onload handler is really sucky. >>+ // You MUST first set its type attribute, then add the element to the DOM >>+ // and finally set the color property(!) afterwards. Try in any other order >>+ // and fail... :-( > Would it help to make the colorpicker constructor read the color attribute? Yes, adding this.color = this.getAttribute("color"); to the colorpicker-button constructor seems to fix this problem. > > function AddTag() > I don't like this. I'd prefer for it to immediately add a row to the list. Good idea.

(In reply to comment #35) >>What's the drawback of a listbox >The overflow-y scrollbars on listboxes overlap the content. And if the >rightmost column consists of eg. colorpickers, these (on Win2k at least) are >drawn incomplete! (This seems to be fixed on trunk, but not on the 1.8 branch.) Ah yes, there was a similar problem with the filter/search editor, it seems that the workaround there was an amount of padding. Putting the colourpickers on the left also works. >Nevertheless, I actually don't see the point of a listbox here... I thought that a listbox looked more obviously like a list, not only from a visual point of view but it might also improve accessibility. >>Would it help to make the colorpicker constructor read the color attribute? >Yes, adding > this.color = this.getAttribute("color"); >to the colorpicker-button constructor seems to fix this problem. Feel free to file that in a separate bug if you think it would help.

Listbox-based version, including dropping of the newTag dialog.

Comment on attachment 239402 [details] [diff] [review] Tag pref panel, v2 This didn't work on a new profile which had no tags. In such a case I suggest you "Restore Defaults". >- <listbox id="tagList" flex="1"/> >+ <listbox id="tagList"> >+ <listcols> >+ <listcol/> This didn't work properly for me. In one build, the listbox was narrow, and could have been wider, while in the other it was too wide. I suggest flex="1" on the listbox, the first listcol and each textbox that you create. >-</page> >+</page> >\ No newline at end of file Oops :-P >+ // Listitems we append to the "end" of the listbox and would be rendered >+ // outside the clipping area don't get their text and color set! >+ // So we stuff them in bottom-up... :-| For textboxes you can fix that by setting attributes on the items. (Filed that bug on colourpickers yet?) >+ textbox.setAttribute('onfocus', 'OnFocus(this);'); I still don't like this being done through attributes. >+ while (aTag + '#' + suffix in aTagList) >+ ++suffix; >+ aTag += '#' + suffix; By comparison the Filter Editor uses "Untitled Filter 2" etc. >+<!ENTITY raiseTagButton.label "Raise Importance"> >+<!ENTITY raiseTagButton.accesskey "+"> >+<!ENTITY lowerTagButton.label "Lower Importance"> >+<!ENTITY lowerTagButton.accesskey "-"> I'm not convinced of these strings: I was thinking of "&More Important" and "&Less Important". >+/* ::::: Tags ::::: */ >+ >+#tagList listcell >+{ >+ padding-left: 16px; >+ padding-right: 16px; >+} >+ >+#tagList .listheader-label >+{ >+ text-align: center; >+} Descendent selectors considered harmful. See http://developer.mozilla.org/en/docs/Writing_Efficient_CSS#Avoid_the_descendant_selector.21 Another option which you may want to consider is to set flex="2" on the first listcol and flex="1" on the second listcol, and then sprinkle pack="center" on listcells and listheads. Anyway, nearly there, I think.

Comment on attachment 239402 [details] [diff] [review] Tag pref panel, v2 >Index: mailnews/base/prefs/resources/content/pref-labels.js >=================================================================== >+function GetFields(aPageData) > { >+ // collect the tag definitions from the UI and store them in the wsm >+ var tags = []; >+ for (var entry = gTagList.firstChild; entry; entry = entry.nextSibling) You could use here (and is more readable): for each (var entry in gTagList.childNodes) >+function SetFields(aPageData) > { >+ var tagService = Components.classes["@mozilla.org/messenger/tagservice;1"] >+ .getService(Components.interfaces.nsIMsgTagService); >+ var tagArray = tagService.getAllTags({}); >+ tags = aPageData[ACTIVE_TAGS_ID] = []; >+ for (i = 0; i < tagArray.length; ++i) You could use here: for each (var t in tagArray) > function AddTag() >+{ >+ // Add a new tag to the UI here. It will be only be written to the >+ // preference system only if the OKHandler is executed! >+ >+ // create unique tag name >+ var dupeList = {}; // indexed by tag >+ for (var entry = gTagList.firstChild; entry; entry = entry.nextSibling) As before, could use (and more readable): for each (var entry in gTagList.childNodes) >+function RaiseTag() > { >+ // Move the selected tag one position up in the tagList's child order. >+ // This reordering may require changing ordinal strings, which will happen >+ // when we write tag data to the preferences system in the OKHandler. >+ var entry = gTagList.selectedItem; >+ entry.parentNode.insertBefore(entry, gTagList.getPreviousItem(entry, 1)); >+ UpdateTagEntry(entry.taginfo, entry); >+ FocusTagEntry(entry); >+} >+ >+function LowerTag() >+{ >+ // Move the selected tag one position down in the tagList's child order. >+ // This reordering may require changing ordinal strings, which will happen >+ // when we write tag data to the preferences system in the OKHandler. >+ var entry = gTagList.selectedItem; >+ entry.parentNode.insertBefore(entry, gTagList.getNextItem(entry, 2)); >+ UpdateTagEntry(entry.taginfo, entry); >+ FocusTagEntry(entry); > } Is it worth looking at combining RaiseTag and LowerTag into one function with an argument? > >+function OnOK() >+{ >+ var i; >+ var tagService = Components.classes["@mozilla.org/messenger/tagservice;1"] >+ .getService(Components.interfaces.nsIMsgTagService); >+ // we may be called in another page's context, so get the stored data from the >+ // wsm the hard way >+ var pageData = parent.hPrefWindow.wsm.dataManager.pageData[TAGPANEL_URI]; >+ var activeTags = pageData[ACTIVE_TAGS_ID]; >+ var deletedTags = pageData[DELETED_TAGS_ID]; >+ >+ // remove all deleted tags from the preferences system >+ for (var key in deletedTags) >+ tagService.deleteKey(key); >+ >+ // count dupes so that we can eliminate them later >+ var dupeCounts = {}; // indexed by tag >+ for (i = 0; i < activeTags.length; ++i) >+ { >+ var tag = activeTags[i].tag; You could use: for each (var tagInfo in activeTags) { var tag = tagInfo.tag; >+ // Now write tags to the preferences system, create keys and ordinal strings. >+ // Manually set ordinal strings are NOT retained! >+ var lastTagInfo = null; >+ for (i = 0; i < activeTags.length; ++i) >+ { >+ var tagInfo = activeTags[i]; Again could use: for each (var tagInfo in activeTags) >Index: mailnews/base/prefs/resources/locale/en-US/pref-labels.dtd >=================================================================== >@@ -31,17 +31,24 @@ > - use your version of this file under the terms of the MPL, indicate your > - decision by deleting the provisions above and replace them with the notice > - and other provisions required by the LGPL or the GPL. If you do not delete > - the provisions above, a recipient may use your version of this file under > - the terms of any one of the MPL, the GPL or the LGPL. > - > - ***** END LICENSE BLOCK ***** --> > >-<!ENTITY pref.tags.title "Tags"> >-<!ENTITY pref.tags.caption "Customize Tags"> >-<!ENTITY pref.tags.description "Tags can be used to categorize and prioritize your messages."> >-<!ENTITY addTagButton.label "Add"> >-<!ENTITY addTagButton.accesskey "A"> >-<!ENTITY deleteTagButton.label "Delete"> >-<!ENTITY deleteTagButton.accesskey "D"> >-<!ENTITY restoreDefaults.label "Restore Defaults"> >-<!ENTITY restoreDefaults.accesskey "R"> >+<!ENTITY pref.tags.title "Tags"> >+<!ENTITY pref.tags.caption "Customize Tags"> >+<!ENTITY pref.tags.description "Tags can be used to categorize and prioritize your messages. Modify the appearance and importance of tags using the settings below. Tags near the top are more important than those further down."> >+<!ENTITY tagColumn.label "Tag"> >+<!ENTITY colorColumn.label "Color"> >+<!ENTITY defaultTagName.label "New Tag"> >+<!ENTITY addTagButton.label "Add"> >+<!ENTITY addTagButton.accesskey "A"> >+<!ENTITY deleteTagButton.label "Delete"> >+<!ENTITY deleteTagButton.accesskey "D"> >+<!ENTITY raiseTagButton.label "Raise Importance"> >+<!ENTITY raiseTagButton.accesskey "+"> >+<!ENTITY lowerTagButton.label "Lower Importance"> >+<!ENTITY lowerTagButton.accesskey "-"> >+<!ENTITY restoreDefaultsButton.label "Restore Defaults"> >+<!ENTITY restoreDefaultsButton.accesskey "R"> Why the extra space between the entity name and entity value? >Index: mailnews/base/resources/content/mailWindowOverlay.js >=================================================================== >@@ -610,41 +610,16 @@ function ToggleMessageTag(key, addKey) > prevHdrFolder = msgHdr.folder; > } > messages.AppendElement(msgHdr); > } > if (prevHdrFolder) > prevHdrFolder[toggler](messages, key); > } > >-function AddTag() >-{ >- var args = {result: "", okCallback: AddTagCallback}; >- var dialog = window.openDialog("chrome://messenger/content/newTagDialog.xul", >- "", >- "chrome,titlebar,modal", >- args); >-} If we are no longer calling newTagDialog.xul it should no longer be packaged in mailnews/jar.mn

Using for each on arrays is dubious because of the possibility of extensions polluting the array prototype. It could even happen for a NodeList too.

Most comments addressed, some notes on those I didn't: (In reply to comment #38) > This didn't work on a new profile which had no tags. In such a case I suggest > you "Restore Defaults". This is bug 354381. > >+ // Listitems we append to the "end" of the listbox and would be rendered > >+ // outside the clipping area don't get their text and color set! > >+ // So we stuff them in bottom-up... :-| > For textboxes you can fix that by setting attributes on the items. > (Filed that bug on colourpickers yet?) Yes, I filed 354065 (against toolkit, since our respective syncing bug is open anyway). But without that colorpicker fix there's no point in setting the textbox' _attribute_ either. > >+<!ENTITY raiseTagButton.label "Raise Importance"> > >+<!ENTITY raiseTagButton.accesskey "+"> > >+<!ENTITY lowerTagButton.label "Lower Importance"> > >+<!ENTITY lowerTagButton.accesskey "-"> > I'm not convinced of these strings: I was thinking of "&More Important" and > "&Less Important". Buttons should describe actions (i.e. use verbs) and all other buttons on this panel do. (In reply to comment #39) > >+ for (var entry = gTagList.firstChild; entry; entry = entry.nextSibling) > You could use here (and is more readable): > for each (var entry in gTagList.childNodes) No, I can't. ECMA-357 explicitly states that the order of element traversal is implementation-dependent, and I do need to rely on strict ordering here. Carrying over sr+.

Comment on attachment 240350 [details] [diff] [review] Tag pref panel, v3 Checked in on trunk.

Landed on MOZILLA_1_8_BRANCH. Mind that bug 354381 is not yet there!

Comment on attachment 246536 [details] [diff] [review] Small optimization Or I could use event.currentTarget if you prefer.

Comment on attachment 246536 [details] [diff] [review] Small optimization Nice. :) We should also kill the misleading onfocus attributes in the comment in ll. 40 + 43.