Need preference panel for new tag feature

RESOLVED FIXED

Status

SeaMonkey
MailNews: Message Display
--
major
RESOLVED FIXED
11 years ago
10 years ago

People

(Reporter: Karsten Düsterloh, Assigned: Karsten Düsterloh)

Tracking

(4 keywords)

unspecified
fixed-seamonkey1.1a, fixed-seamonkey1.1b, fixed1.8.1, relnote
Dependency tree / graph
Bug Flags:
blocking-seamonkey1.1a +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments, 8 obsolete attachments)

17.01 KB, patch
Ian Neal
: review+
Karsten Düsterloh
: superreview+
Details | Diff | Splinter Review
49.35 KB, patch
Details | Diff | Splinter Review
25.65 KB, patch
Details | Diff | Splinter Review
37.74 KB, patch
Ian Neal
: review+
Karsten Düsterloh
: superreview+
Details | Diff | Splinter Review
633 bytes, patch
Karsten Düsterloh
: review+
Details | Diff | Splinter Review
(Assignee)

Description

11 years ago
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!)

Updated

11 years ago
Flags: blocking-seamonkey1.1a+

Comment 1

11 years ago
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?

Comment 2

11 years ago
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.
(Assignee)

Comment 3

11 years ago
> 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.
Depends on: 342065
(Assignee)

Comment 4

11 years ago
XRef: Bug 343875 (TB pref pane)
(Assignee)

Comment 5

11 years ago
Created attachment 232343 [details] [diff] [review]
Band-aid patch: just copy TB's 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!
Attachment #232343 - Flags: superreview?(neil)
Attachment #232343 - Flags: review?(iann_bugzilla)

Comment 6

11 years ago
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
Attachment #232343 - Flags: superreview?(neil) → superreview+
(Assignee)

Comment 7

11 years ago
> >-/* 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.
(Assignee)

Comment 8

11 years ago
Created attachment 232606 [details] [diff] [review]
Band-aid patch2

Fixed Neil's comments.
Attachment #232343 - Attachment is obsolete: true
Attachment #232606 - Flags: superreview+
Attachment #232606 - Flags: review?
Attachment #232343 - Flags: review?(iann_bugzilla)
(Assignee)

Updated

11 years ago
Attachment #232606 - Flags: review? → review?(iann_bugzilla)

Comment 9

11 years ago
(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 10

11 years ago
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
Attachment #232606 - Flags: review?(iann_bugzilla) → review-
(Assignee)

Comment 11

11 years ago
Created attachment 232621 [details] [diff] [review]
Band-aid patch 3

Addressed Ian's comments.
Attachment #232606 - Attachment is obsolete: true
Attachment #232621 - Flags: superreview+
Attachment #232621 - Flags: review?(iann_bugzilla)
Attachment #232621 - Flags: approval-seamonkey1.1a?

Comment 12

11 years ago
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+
Attachment #232621 - Flags: approval-seamonkey1.1a? → approval-seamonkey1.1a+
(Assignee)

Comment 13

11 years ago
> >-<!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.
(Assignee)

Comment 14

11 years ago
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.

++

Comment 15

11 years ago
(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 :)
(Assignee)

Comment 16

11 years ago
Created attachment 232800 [details] [diff] [review]
Band-aid patch 4

Restore default tags with correct keys. Would be taking over a+ if I could, so imagine it's there. ;-)
Attachment #232621 - Attachment is obsolete: true
Attachment #232800 - Flags: superreview+
Attachment #232800 - Flags: review?(iann_bugzilla)
Attachment #232621 - Flags: review?(iann_bugzilla)
(Assignee)

Comment 17

11 years ago
Created attachment 232801 [details] [diff] [review]
help patch

Help patch for tags in general, thus slightly ahead of the ban-aid patch.
Attachment #232801 - Flags: review?(iann_bugzilla)
Attachment #232801 - Flags: approval-seamonkey1.1a?

Comment 18

11 years ago
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)
Attachment #232801 - Flags: approval-seamonkey1.1a? → approval-seamonkey1.1a+

Comment 19

11 years ago
Comment on attachment 232800 [details] [diff] [review]
Band-aid patch 4

r/a=me
Attachment #232800 - Flags: review?(iann_bugzilla)
Attachment #232800 - Flags: review+
Attachment #232800 - Flags: approval-seamonkey1.1a+

Comment 20

11 years ago
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?
Attachment #232801 - Flags: review?(iann_bugzilla) → review-
(Assignee)

Comment 21

11 years ago
Created attachment 233149 [details] [diff] [review]
help patch, v2trunk

Corrected index order, against trunk.
Attachment #232801 - Attachment is obsolete: true
Attachment #233149 - Flags: review?(iann_bugzilla)
(Assignee)

Comment 22

11 years ago
Created attachment 233150 [details] [diff] [review]
help patch, v2branch

Same textual content as attachment 233149 [details] [diff] [review], but against 1.8 branch (i.e. different pref-help.js location).
Attachment #233150 - Flags: review?(iann_bugzilla)
Attachment #233150 - Flags: approval-seamonkey1.1a?

Comment 23

11 years ago
Comment on attachment 233150 [details] [diff] [review]
help patch, v2branch

a=me for 1.1a given it gets r+
Attachment #233150 - Flags: approval-seamonkey1.1a? → approval-seamonkey1.1a+
(Assignee)

Comment 24

11 years ago
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 25

11 years ago
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.
Attachment #233149 - Flags: review?(iann_bugzilla) → review+

Comment 26

11 years ago
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.
Attachment #233150 - Flags: review?(iann_bugzilla) → review+
(Assignee)

Comment 27

11 years ago
Created attachment 233408 [details] [diff] [review]
help patch, v3trunk (checked-in version)
Attachment #233149 - Attachment is obsolete: true
(Assignee)

Comment 28

11 years ago
Created attachment 233409 [details] [diff] [review]
help patch, v3branch (checked-in version)
Attachment #233150 - Attachment is obsolete: true
(Assignee)

Comment 29

11 years ago
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...
(Assignee)

Updated

11 years ago
Keywords: fixed-seamonkey1.1a
Keywords: relnote

Comment 30

11 years ago
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.

Comment 31

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

Comment 32

11 years ago
*** Bug 352332 has been marked as a duplicate of this bug. ***
(Assignee)

Comment 33

11 years ago
Created attachment 238834 [details] [diff] [review]
Tag pref panel, v1

New tag pref panel for SM, including tag ordering.
Further changes:
- call pref panel from tag context menu
- help update
Attachment #238834 - Flags: superreview?(neil)
Attachment #238834 - Flags: review?(iann_bugzilla)

Comment 34

11 years ago
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.
(Assignee)

Comment 35

11 years ago
(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. 

Comment 36

11 years ago
(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.
(Assignee)

Comment 37

11 years ago
Created attachment 239402 [details] [diff] [review]
Tag pref panel, v2

Listbox-based version, including dropping of the newTag dialog.
Attachment #238834 - Attachment is obsolete: true
Attachment #239402 - Flags: superreview?(neil)
Attachment #239402 - Flags: review?(iann_bugzilla)
Attachment #238834 - Flags: superreview?(neil)
Attachment #238834 - Flags: review?(iann_bugzilla)

Comment 38

11 years ago
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.
Attachment #239402 - Flags: superreview?(neil) → superreview+

Comment 39

11 years ago
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
Attachment #239402 - Flags: review?(iann_bugzilla) → review-

Comment 40

11 years ago
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.
(Assignee)

Comment 41

11 years ago
Created attachment 240350 [details] [diff] [review]
Tag pref panel, v3

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+.
Attachment #240350 - Flags: superreview+
Attachment #240350 - Flags: review?(iann_bugzilla)
(Assignee)

Updated

11 years ago
Attachment #239402 - Attachment is obsolete: true

Updated

11 years ago
Attachment #240350 - Flags: review?(iann_bugzilla) → review+
(Assignee)

Updated

11 years ago
Attachment #240350 - Flags: approval-seamonkey1.1b?
(Assignee)

Comment 42

11 years ago
Comment on attachment 240350 [details] [diff] [review]
Tag pref panel, v3

Checked in on trunk.

Updated

11 years ago
Attachment #240350 - Flags: approval-seamonkey1.1b? → approval-seamonkey1.1b+
(Assignee)

Comment 43

11 years ago
Landed on MOZILLA_1_8_BRANCH. 
Mind that bug 354381 is not yet there!
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Keywords: fixed-seamonkey1.1b, fixed1.8.1
Resolution: --- → FIXED

Updated

11 years ago
Depends on: 361694

Comment 44

11 years ago
Created attachment 246536 [details] [diff] [review]
Small optimization
Attachment #246536 - Flags: review?(mnyromyr)

Comment 45

11 years ago
Comment on attachment 246536 [details] [diff] [review]
Small optimization

Or I could use event.currentTarget if you prefer.
(Assignee)

Comment 46

11 years ago
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.
Attachment #246536 - Flags: review?(mnyromyr) → review+
You need to log in before you can comment on or make changes to this bug.