81292 - RFE: "Labels" feature to help users organize their messages

Status: VERIFIED FIXED

(SeaMonkey :: MailNews: Message Display, enhancement, P2)

Description

[jglick]

Started in Bug 74071 which was about using color to indicate priority of 
messages:

Personally I don't like using color for priority (especially the whole row) 
because:

The Sender gets to determine what is high/low priority not the recipient.
Often what is high/low priority to the sender, is not high/low priority to
the recipient, and then these messages (that may not be important to the
recipient) stand out in the user's Thread Pane and there is no way for the
recipient to change them.  Spam sent as high priority will stand out.

With just the Priority column colored, the user can at least choose to ignore 
this feature by now viewing the Priority column. If the whole row is colored, 
the user has no choice.

Going farther, I would like to see the ability to use color for messages, but I 
would like to see it used for something that is under the Recipient's control 
instead of the Sender's.  Allowing the user to use color as a way to organize 
what THEY think is important would be a much nicer feature.   Dealing with and
attempting to organize the huge quantities of mail some users get today, is
a big issue. And I would rather give the control of marking messages with
color to the recipient instead of the sender.  Similar to the "Flag"
feature, users would be able to use color to prioritize their own Inboxes.
Similar to the Mac or Eudora Mail "Labels", users would have the ability to
categorize/organized by color.

Comment 1

[jglick]

In addition, setting a color ("label") of a message header could also be the 
"Action" part of a filter.  So you could set up a filter to display all 
message headers from your boss in red if you like (for example).

Comment 2

[jglick]

Similar implementations, Eudora mail has a similar feature and Mac OS lets you 
color/label folders to help you organize/prioritize.

Comment 3

[jglick]

Attachment: Image One

Comment 4

[jglick]

Attachment: Image Two

Comment 5

[jglick]

Attachment: Image Three

Comment 6

[kmurray1115]

Should Label names be numerical? What about temperature: cold, cool, warm, hot, 
etc. Or custom?

Comment 7

[Håkan Waara]

I vote for a custom editor where you can edit the "themes". A theme is a color 
with a (custom) named attached to it which you will see in the "Labels" column.

Comment 8

[jglick]

See Image Two. Ideally, users would be able to name their labels whatever they 
want AND pick the colors they want to use for each label.  But we should have 
decent defaults for users who don't make any changes.

Comment 9

[kmurray1115]

Ah. Image Two. Those thingies there are indeed editable fields. My bad. Great 
work, as usual.

Comment 10

[Håkan Waara]

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

Comment 11

[Håkan Waara]

bug 67676 is duplicate/related.

Comment 12

[Ben Bucksch (:BenB)]

We shouldn't restrict the number of labels to 5.

Don't you think, setting the background color is nicer? What, if the user sets a
fore-/background color that is similar to the back-/foregournd color? Should we
maybe allow to user to set both colors or select the other one automatically?

Comment 13

[timeless]

Personally, i'd expect to be able to use userChrome.css to style my labels 
however i choose instead of just being limited by whatever pref interface we 
create.

Comment 14

[kmurray1115]

Let's keep the first phase of the UI implementation simple, then expand as more
folks give us feedback. 

Comment 15

[Alex Bishop]

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

Comment 16

[Adam Lock]

My thoughts on colouring from bug 84737 and expanded on a bit.

I'd like to be able to colour a message blue if it comes from the
netscape.com/mozilla.org domain, or brown for potential spam. This sounds like a
special kind of message filter, that perhaps sets a header field or property
(e.g. X-moz-colour or X-moz-label) on incoming articles that is used when it is
displayed. The viewer could look for a colour/label, e.g. "Red" and draw it that
way. 

Even more powerful yet would be if I could colour entire threads, e.g. threads
with replies from me. Much more useful that the "Watch thread" icon that appears
in 4.x

Comment 17

[Håkan Waara]

I agree with Adam that such a filter action would be neat. However, I think that
should be its own bug... This bug isn't exactly about that feature. Or is it?

Comment 18

[Adam Lock]

Bug 84737 talked about colouring but was made a dup of this one...

I do think they are similar internally though the UI would be different.
Internally, a message that I told the filter to colour blue could be labelled 
"Blue" allowing the rendering mechanism for labels and colours to be shared.

Comment 19

[Alex Bishop]

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

Comment 20

[Håkan Waara]

FYI, this is being discussed over in the newsgroup. And it seems like it's
becoming a wontfix according to the discussion over there...

Comment 21

[Adam Lock]

A lot of feature requests have been lumped into this one bug including my
"colouring" bug 84737.

What's the rationale for marking it WONTFIX since the idea is clearly popular
(and useful)?

Comment 22

[Ben Bucksch (:BenB)]

> And it seems like it's
> becoming a wontfix according to the discussion over there...

huh? Anyways, over to the newsgroup, please.

Comment 23

[scottputterman]

reassigning to ssu

Comment 24

[scottputterman]

adding nsbeta1+ and moving to 0.9.6

Comment 25

[David :Bienvenu]

adding dependency. I'll attach a patch to bug 106067 in a minute, that does most
of the backend support, and puts in rudimentary front end support for my testing
purposes.

Comment 26

[David :Bienvenu]

Just an FYI, Sean, when I made my little label column a cycler in
threadpane.xul, it no longer displayed the cell text for the label (I think the
outliner stopped asking me for the cell text). So you should check with hyatt if
it's possible to have cell text with a cycler column (all of our cycler columns
right now have images).

Comment 27

[Aleksander Adamowski]

We also need an item in the Message->Label submenu that would be at the top and
would read "none" - so we can remove labels from messages...

Comment 28

[jglick]

>We also need an item in the Message->Label submenu ... so we can remove labels 
>from messages.

putterman and I were just discuss that! ;-)

Sean, would you like a separate bug for that?

Comment 29

[jglick]

Attachment: Example with "None" option

Comment 30

[David :Bienvenu]

Sean, just set the label to 0 to have no label. Also, the cycler, if the
outliner would work, would allow you to get no label - i.e., it's implemented to
go to 0 after 5.

Comment 31

[Ben Ruppel]

Are there plans for making the labeling of messages more accessible?  I would
like to see a dropdown button like the File button on the toolbar.  Should I
file a new bug?

Comment 32

[jglick]

>Accessible
Clicking on the label directly should cause it to cycle through the available 
labels, 1,2,3,4,5,None,1,2, etc.

Comment 33

[Caspy7]

My suggestion would be to allow for up to 8 colors but with the last three
disabled (not showing up in the menu).  Because the number would be potentially
variable, put "None" at the top of the list in the menu so it will always be '0'.

Also, is "Sort Messages by Label" a potential option to be added as well
(putting whichever color is '1' at the top and so on)?

Why is this called "Label" instead of "Color?"  From a user's perspective,
labels are usually text labels.  Its just what I expect a label to be.  When I
want to change the color of a message I think "I want to change its color", not
"I want to change its label"  
So could we consider calling it "Color" or "Message Color" (under prefs) instead?

Comment 34

[mike lima]

Just eight colors?  I protest! :-)  I need dozens, hundreds, millions!!!!  :-)

I will suggest also, as you guys are theading this new feature, that you allow
labels on folders (inbox, outbox... and all the folders created by people).

Any name you put, don´t fotget to allow SEARCH MESSAGES BY COLOR.

Let´s crush Microshaft IE that today lauched XP (eXPloit)!

thanks  

Comment 35

[jglick]

>"Sort Messages by Label"
Clicking on the column header will sort by label. The "View:Sort by" menu should 
also have a "Label" item.

>Why is this called "Label" instead of "Color?" 

Color is only part of the feature. You can also change the "label" Names to 
categories which suit you. Some color blind users may rely more on the unique 
names of this feature instead of color.

Comment 36

[Håkan Waara]

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

Comment 37

[scottputterman]

If it can get in before 0.9.7 great, but let's figure labels will land after 0.9.6.

Comment 38

[Sean Su]

Attachment: Image of what the patch will create

Comment 39

[Sean Su]

Attachment: Image of what the patch will create

Comment 40

[Sean Su]

Attachment: labels patch attempt #1

Comment 41

[Sean Su]

This first attempt patch is currently lacking a Labels menu on the context popup
menu.  The labels menu items have a couple of problems too:
  1) the access keys do not look "right".  The way it's shown in the labels spec
is nicer.  I'm working on it.
  2) they are missing a color box next to the strings to show what color each
label corresponds to.

Comment 42

[Sean Su]

adding bhuvan to the cc: list.

Comment 43

[Alex Bishop]

Adding http://www.mozilla.org/mailnews/specs/labels/ to the URL field to make
the spec easier to find (particularly as it's not marked 'MachV' but 'MackV' -
presumably Netscape's planning a special release for Scottish people).

Comment 44

[Sean Su]

Attachment: Image of what the patch will create (highlight has colors)

This image shows the labeled messages with its highlights colored (and the text
white).

Comment 45

[Sean Su]

Attachment: Image of what the patch will create (higlight is not colored)

This is a better image one of the version that the highlight is not colored.

I personally like this one better because it's no so 'in your face'.  It looks
more professional IMHO.

Jennifer, you had mentioned that when the selected message(s) is labeled (via
the menus), the color might not immediately be reflected in the thread pane (in
this example image).  It actually does change on the fly.

Comment 46

[kmurray1115]

Not sure I agree. I like the first screen shot better (selection not so in your
face, but background color changes if message is selected). What we don't see in
the first screen shot though is something you have represented nicely in the
second: the unselected messages that are labeled, bear the color with their font
color changed on the white background, which I think looks nice, and IIRC, is in
the spec.

Comment 47

[scottputterman]

I like the second screen shot better.  I think having multiple selection colors
is confusing.

Comment 48

[kmurray1115]

I can see that - the only thing we'll have to balance then is the drowned out
effect of the dark blue selection. I think as I use this though, the
multi-colored selection will grow on me. Point is, we'll need to see it in
action instead of just looking at it. 

Comment 49

[scottputterman]

agreed, though I did get to see a demo on Sean's machine which is where my
confusing statement came from.

I think there are three choices all of which have pros and cons.

1.  Just keep regular selection color and label text color.

Pro:  Selection color is less confusing
Con:  The label color might conflict with the selection color and become unreadable.

2.  Keep regular selection color and white text color.

Pro:  Selection color is less confusing and text is always readable.
Con:  Label isn't apparent which is bad when setting a label since there's no
feedback until unselecting.

3.  Use label selection color and white text color.

Pro:  You can always read it and label color is apparent.
Con:  Very colorful (my opinion at least that this is a con).  Hard to tell
where the selection is.

My ranking is 2,1,3.  But, I have no problem checking in #3 and waiting for
feedback.

Comment 50

[timeless]

Couldn't you use darken/lighten label color for a selection option? would it be
possible to see a picture of that? (i think text for all selected would be white
in this case)

Comment 51

[scottputterman]

As a reference point, Outlook 2000 does #2 in the list in my comments above.

Comment 52

[Ben Ruppel]

I say we go with selection 1.  Selection 2 is nice, but labeling something with
a neato color isn't as gratifiying if you don't get to see it immediately or
when the message is selected.  If people make their messages the same color as
the selection color they should already know how to change the label color to
something else once they see the problem.  If not, I'd say we have a case of
social darwinism and we should let nature take its course ;)

Comment 53

[Brice Ruth]

Attachment: Color labels in Entourage v.X for Mac OS X

Here's what the label colors look like in Entourage.  I have some of them
highlighted, some of them not.	I noticed that there isn't really an option to
select a color that is the same as the highlight color, so the 'social
darwinism' doesn't really come into play.  I prefer changing the font color
over changing the background, but I believe the change should be immediate. 
Also, in Entourage, the colors to choose from seem to all be pastels ... this
is nice on the eyes and may be better than the primary colors shown in the
previous screen-shots.

Comment 54

[Brice Ruth]

Addendum to my previous posting: I can create a category with a custom color
that is the same as the highlight color (which is defined by Aqua in Carbon OS X
apps). I can set a message's category to this color (or close to it), but when I
select that message, instead of 'disappearing' the font becomes black - and then
reverts to its category color when not selected.  This seems to be a pretty good
tactic.  Just have a heuristic that detects if a category color is 'close' to
the highlight color - if so, make the font the 'default' and revert afterwards.

Comment 55

[kmurray1115]

I think we avoid the color conflicts by limiting the palette, although a
heuristic might be cool. I'm also not opposed to the Entourage method, but I
think we have two different selection colors as a base. 

I'm still wondering how invasive/obnoxious it would be to change the selection
clor based on the label color (#3). Can we try that to see if we like the
"different" approach?

Comment 56

[scottputterman]

Sure we can try the different approach, but my opinion is that that goes really
well with the Toy Factory theme, but not so great with modern or classic.

Comment 57

[Brice Ruth]

I agree entirely with putterman.  Keep in mind: selection should be standard
across the UI - once you start haphazardly changing the way selection 'looks' it
looses its uniformity and thereby looses its meaning, in a sense.  Based on a
previous comment, apparently the colors available for labels will be limited?
How many? What colors? Will the colors be under the control of the theme (this
could be good and bad).  I would vote for having a standard set of colors for
the labels, but then being able to add your own or change the standard set.
Everything else in Mozilla is configurable to the MAX (many times much more than
is necessary) - why should a useful feature such as this (especially for folks
who use email professionally and need to organize it as much as they use hanging
file folders for other documents) be limited?

Comment 58

[scottputterman]

Brice,

There are 2 reasons why colors are limited and may have to be limited further.

1.  While researching this feature, Sean, Seth and Hyatt decided that it would
be too much work to change the style rules dynamically so instead we're
hardcoding the colors into the stylesheet.  Since we're hardcoding the colors
we're stuck with a limited amount.

2.  Because we're hardcoding the style rules, there ends up being either 2 or 3
style rules for each color we support.  So currently with the 70 colors that
Sean is supporting there are 140-210 style rules.  This could have a performance
impact.  If it does, the choices will be limited further (I think the spec says
10 or 15).

Sean can correct me if I've explained any of this incorrectly.

Comment 59

[jglick]

I wonder how common it is to have that many messages selected at one time?

If its common, #1 (regular selection color and label text color) seems the best. 
If common, #3 (label selection color and white text color) is a bit 
overwhelming.

If its more common to select one message at a time, #3 ensures the text can be 
read, but #1 seems pretty readable with the default colors.

For reference, Eudora uses #3.

I guess i lean toward #1 with the Modern skin and default label colors. My worry 
is if users change the label colors or how the default selection color of a 
different skin might affect #1.

Comment 60

[Alex Bishop]

I note that 'MackV' has been changed to 'MachV' in the spec, presumably in an
attempt to make it look like I can't read. I suppose that if I mention it still
says 'Mojo' in the title (was this feature really once planned for Netscape 6.1?
That was a bit optimistic) then that will be changed as well.

Comment 61

[jglick]

>I note that 'MackV' has been changed to 'MachV' in the spec, presumably in an
>attempt to make it look like I can't read. 

Actually it was changed in an attempt to make it look like I can correctly type. 
:-)

>I suppose that if I mention it still
>says 'Mojo' in the title (was this feature really once planned for Netscape 
>6.1? That was a bit optimistic) then that will be changed as well.

Darn, was hoping you won't notice that one too. :-)
I re-use specs as templates instead of starting from scratch, unfortunately I 
miss a few things in the process sometimes. Will fix shortly.

Comment 62

[Alex Bishop]

>>I note that 'MackV' has been changed to 'MachV' in the spec, presumably in an
>>attempt to make it look like I can't read. 
>
> Actually it was changed in an attempt to make it look like I can correctly type. 
> :-)

Ah, I see. The document is one of the earliest I could find mentioning 'MachV'
so I thought perhaps you'd only been told the name verbally at that stage and
misheard it as 'MackV'. Who comes up with these codenames anyway?

Comment 63

[kmurray1115]

We take a poll within the mozilla community - didn't you see it? ;-)

Comment 64

[Adam Lock]

Can I suggest a minor UI improvement to the label drop down menu would be to
remove the numbers, or make them keyboard shortcuts that right align themselves
or put them before the label, i.e.

"0 None " instead of "None(0)"
"1 Red" instead of "Red(1)"
and so on.
May I also enquire whether labels are persisted? Presuming they are is this
patch a suitable base to add labelling support to the message filter dialog? e.g.

  if body matches "dear friend and future millionaire" then label "spam"

Comment 65

[David :Bienvenu]

yes, labels are persistent, and there is a separate bug for filters : bug 106187

Comment 66

[jglick]

>"0 None " instead of "None(0)"
>"1 Red" instead of "Red(1)" and so on.

The desired behavior:
http://www.mozilla.org/mailnews/specs/labels/images/Label4.gif

Comment 67

[mike lima]

Attachment: colors on the folders side too, please!

Of course, don´t forget to allow colors on the folders side too.  It´s so or
much more important than colors on mail side.

Comment 68

[Sean Su]

labeling (and coloring) the folders in the folder pane is not on this spec/bug.
 This bug only deals with labeling/coloring messages in the threadpane.

If Jennifer updates the spec to include labeling folders in the folder pane, I'd
be willing to work it in.

I suspect it would be a new feature altogether.

Comment 69

[Sean Su]

Attachment: labels patch v2.0

This new patch will fix problems that were in the initial patch:
 * the accesskeys that Jennifer pointed out are now shown correctly.
 * add the Labels menu to the thread pane context menu
 * add the Labels menu to the message pane context menu
 * remove the css style info that colors the Priority column

This patch will show the highlighted row(s) in the colored label(s) and the
text in white.	See:
http://bugzilla.mozilla.org/attachment.cgi?id=57160&action=view

Once this is checked in, we can decide if we want to keep it in this form or go
with a different type of highlight/text color combination.

Comment 70

[mike lima]

but the term LABELS TO ORGANIZE USERS MESSAGES can be applied also to folders.

Comment 71

[Brice Ruth]

Forgive me for being blunt, but coloring folders is hardly necessary. A folder
is an organizational mechanism, a color is an organizational mechanism. A
hierarchy of organization is powerful (folders containing labeled messages),
redundant organization (colored folders) is useless and developer's time and
efforts could be spent doing far more useful things.  Maybe in some cases it
would 'look neat' (certainly not in the provided attachment), but by and large,
nothing is gained by it in terms of organization.

My $0.02.

Comment 72

[scottputterman]

Coloring/Labelling folders is interesting and might be worth discussion at a
later point, but another bug should be filed on it as an enhancement and it
won't be part of the current feature.

Comment 73

[mike lima]

Excuse me but if you had 3000 mails organized thru 200 folders and subfolders
you would find usefull a way to color a folder.

Comment 74

[Brice Ruth]

As a point of note - I have 26452 messages in over 85 folders - I have no need
to redundantly organize.  I have folders within folders within folders. If a
person's mail system doesn't support that level of organization, maybe the mail
system ought to be upgraded, don't you think?  The only useful way to apply
color to folders, in my opinion, would be if you had 10 folders each called
'work' and each with a separate color to denote different types of work. 
However, since most mail systems don't support 'colored folders' - I doubt that
they'll allow you to have multiple folders of the same name.  And if you're
going to name them differently to begin with, why do you need colors?  If you
have 50 folders and you want some of them to be 'together', i.e. the same color,
why not create a parent folders with those folders as subfolders - that's the
point I'm trying to make.  Coloring folders doesn't inherently *add* anything to
your organization, it simply replaces something that's already possible.  I'll
leave any further discussion on this topic to a new bug.

Comment 75

[Ben Ruppel]

All right guys, as has been stated before, this belongs in another bug.  Please
continue discussion there or in the newsgroups.

Comment 76

[Sean Su]

Attachment: Labels menu image 1 - normal w/o colors for highlight or strings

Comment 77

[Sean Su]

Attachment: Labels menu image 2 - background is colored and strings turn white when menu item is selcted

Comment 78

[Sean Su]

Attachment: Labels menu image 3 - strings are colored; highlight/background are normal

Comment 79

[Sean Su]

The Labels spec indicates to use a colored box next to the description of the
label item.  Such a box is not currently possible as part of a menu item.

There is the same limiation in the drop down list menu for use in the Filters
dialog part of the Labels spec.

The 3 attached Labels menu images (1, 2, 3) offers alternative options as to how
to show the labels menu items.

I will check in the plain version (image 1) for now, but we can easily change to
any of them later.

Comment 80

[scottputterman]

Great job with the choices Sean (and I notice you got the numbered mnemonics
working).  Well, based on my previous comments, it'll be no surprise that I
prefer #1, followed by #3 with a hope that #2 never gets checked in :)

#1 looks like every other menu we have and I see no reason for it to not be that
way.  

Comment 81

[kmurray1115]

Yeah...I'm not sold on the menu coloring either. #1 looks more than adequate.

Comment 82

[Caspy7]

Agreed, I don't dig the colored text or backgrounds in the menu.
Will the colored boxes in the menu ever be possible?  What holds that back?

side note: I don't enjoy very much the idea of the colored backgrounds for 
highlighted messages.  Without using it, its hard to make any definitive 
judgements, but I suspect it would make the color clash with certain themes 
especially evident, and also has the potential to distract or confuse the user.

Comment 83

[Ben Ruppel]

Just noticed this since I'm working on another bug to fix accesskeys.  If I go
to View>sort by in mailnews, the Label option has the lowercase "l" underlined,
whereas it should be the uppercase "L".  This can easily be fixed by changing
the accesskey definition in the appropriate .dtd file.  I figure it would be
easier for you guys to fix this quickly, since I'm still waiting for an r/sr for
my diff to be checked in.

Comment 84

[Sean Su]

The accesskey for the Labels menu being 'l' as opposed to 'L' is addressed in my
patch to this bug.

Comment 85

[Adam Lock]

A hack around drawing the coloured boxes in the menu may be to write some code
that dumps out a 1x1 gif/png in the user's profile, in the appropriate colour
and specify those in the menu items.

Comment 86

[jglick]

I also prefer attachment 57409 [details] (Labels menu image 1 - normal w/o colors for
highlight or strings). Nice work. :-)

Comment 87

[racham]

Sean,

Sory for the delay. Fix looks good. Here are couple of small things I have
noticed (patch 57321) & some questions I have :

1. I see only modern skin changes. You may need to add classic skin changes too..
2. Can you mention the default color names after the color codes like "#FF0000"
// default : red ?
3. Can you add some comments explaining (nsMsgViewCommandType::label1 - 1) + label ?
4. Call ApplyToProperty(labelPrefColor, properties); in
nsMsgDBView::SetLabelProperties can be moved into if(labelPrefColor) below the
switch statement.
5. SetLabelProperties(msgHdr, properties); in nsMsgDBView::GetRowProperties -
why are we setting label properties when getting the row properties. The named
of the routine looked pretty generic. Isn't there another function where we
apply all row properties (then, we can move SetlabelProperties call there).
6. We can get rid of 'nsMsgDBView::' in nsMsgDBView.h on 2 of the new functions
added.

bhuvan

Comment 88

[Sean Su]

Attachment: labels patch v2.1

> 1. I see only modern skin changes. You may need to add classic skin changes
too..

done.  files added to classic win and classic mac

> 2. Can you mention the default color names after the color codes like
"#FF0000"
// default : red ?

comments added

> 3. Can you add some comments explaining (nsMsgViewCommandType::label1 - 1) +
label ?

I changed it to '(nsMsgViewCommandType::label0) + label'.  The value of label
can be between 0 and 5 inclusive.  I rather compare label against the
predefined values of the labels (such as nsMsgViewCommandType::label0) in case
they change.

> 4. Call ApplyToProperty(labelPrefColor, properties); in
nsMsgDBView::SetLabelProperties can be moved into if(labelPrefColor) below the
switch statement.

done

> 5. SetLabelProperties(msgHdr, properties); in nsMsgDBView::GetRowProperties -

why are we setting label properties when getting the row properties. The named
of the routine looked pretty generic. Isn't there another function where we
apply all row properties (then, we can move SetlabelProperties call there).

The SetLabelProperties is dependent on the 'properties' parameter.  In the case
of it being called in GetRowProperties, the 'properties' parameter relates to
the row being processed, and only the row properties are applied.  This is the
same for when being called from GetCellProperties().

I'm not aware of any other function that processes/applies all row properties.
David, is there another (or better) place to apply row properties?

> 6. We can get rid of 'nsMsgDBView::' in nsMsgDBView.h on 2 of the new
functions
added.

yeah, I caught that right after I attached the patch.  fixed  now.

Comment 89

[scottputterman]

Jennifer, if you think it would be ideal if the color were to appear before the
menu name, can you file a bug on that?  I don't want to forget that idea since
that wouldn't be a blocker for getting this feature in and probably won't happen
in the near future.

Comment 90

[jglick]

>Jennifer, if you think it would be ideal if the color were to appear before the
>menu name, can you file a bug on that?  

bug 110099 filed.

Comment 91

[Blake Ross]

Note that we do support menuitems with colored boxes next to the labels right 
now, just make tiny square images with those colors as the background and use 
them as the menuitems' icons.  I suppose you could also make a binding to 
associate a color with a menuitem, but that would rarely be used.

Comment 92

[[e-mail]]

Blake, won't that conflict with the checkbox effect?

Comment 93

[(not reading, please use [e-mail] instead)]

some quick comments on the last patch:

1)  don't re-get the label color prefs on every row paint.  cache them, and make
nsMsgDBView an nsIObserver on those five prefs.  when the prefs change, update
the local values, and invalidate the outliner.

2) don't re-get the label text values (which goes through the i18n code) every
row paint.  cache them as well, and observe on those 5 prefs.  (that's 10 in all)
when they change, update the local cached values, and invalidate.

3)  you've got 70 rules for all 15 possible colors.  how does that affect
performance?

4)  your css rules are of the form "label-color-XXXXXX".  hyatt would know (or
we could debug and evaluate) if we could get more performance by doing
"XXXXXX-label-color" (as the XXXXXX part is the part that changes).  this might
not really matter, I don;t know how the style caching coding works.

5)  style nit, instead of returning strings in some of your helper functions,
return nsresult, and return the strings via an out param.

Comment 94

[(not reading, please use [e-mail] instead)]

you've got some duplicated rules.

+outlinerbody:-moz-outliner-cell-text(label-color-X, selected) {
+  color: #000000
+}
+outlinerbody:-moz-outliner-cell-text(label-color-Y, selected) {
+  color: #FFFFFF
+}

instead, maybe do:

+outlinerbody:-moz-outliner-cell-text(label-color-black, selected) {
+  color: #000000
+}
+outlinerbody:-moz-outliner-cell-text(label-color-white, selected) {
+  color: #FFFFFF
+}

is repeated.  if you've got 70 colors, you only need two rules for selected text.
that gets rid of 68 rules.  you'll have to fix the code that returns cell
properties to turn the pref color into "label-color-white" and
"label-color-black" given the pref color.

I think that should be better than having an extra 68 rules, but I haven't
quantified to prove it.

Comment 95

[Sean Su]

Attachment: labels patch v2.2

This new patch deals with Seth's comments.

The color and text values for the labels' prefs are now cached and uses
nsIObserver to get notification on pref changes.

The css rule labels have been changed in case it helps with performance issues.
 I'm not sure how much speed up we would gain, but the rule labels are shorter
for quicker string comparisons.

Fixed the duplicate style rules.

Fixed the code style nit that Seth noticed.

Comment 96

[laurel]

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

Comment 97

[(not reading, please use [e-mail] instead)]

1)

your comment is incorrect.

+/* There are 10x7 color definitions (size of the color picker used)
+   times 3 (3 style definition for each color).
+   The color definition can be in the following formats:
+     color: red;
+     color: #FF0000;
+     color: rgb(128, 0, 0); 
+*/

2)  priority, why are you removing the priority colors?

we are still appending the priority atoms.  (the color should still show up, or
it text only now?)
if text, we should remove the code that appends the priority atoms.

3)  remove dump statements, like  dump("initing message labels\n");

4)  in InitMessageLabel(),  last label doesn't need to be a var.

just do:

for (var i=0; i<=5; i++)

5)  remove old comment:

/* this code should probably get the label strings and change the menu labels */

6) <outlinercol id="labelCol" class="outlinercol-header outlinercol-inset-header
sortDirectionIndicator" persist="hidden ordinal width" flex="1"
label="&labelColumn.label;" hidden="true"/>

isn't right.  if you do that, users can show, but next time it will be hidden.
handling the label column correctly is going to be a spin off bug.

7)

you should add a localization noted explaining how these two match up

+labelMenuItemFormat0=0 %S

<!ENTITY labelCmd0.accesskey "0">

the access key matches the "0", right?

8)

you're using contractid, instead of CID

so there is no need to add this

+static NS_DEFINE_CID(kPrefServiceCID, NS_PREF_CID);

9)

_GetPrefString and GetPrefString
_GetPrefLocalizedString and GetPrefLocalizedString

you don't need to do all that.  

just have GetPrefString and GetPrefLocalizedString.

have the callers get and pass in the pref branch.

so in InitLabelPrefs() and nsMsgDBView::Observe(), get the branch and pass it
through to GetPrefString and GetPrefLocalizedString.

10)

when a label or color changes, you need to invalidate the outliner.

11)

converting every time, we can avoid it.

+nsresult nsMsgDBView::ApplyToProperty(const PRUnichar *aLabelPrefColor,
nsISupportsArray *aProperties)
+{
+  nsCOMPtr<nsIAtom> labelColorAtom;
+  nsCAutoString prefColorOutliner(LABEL_COLOR_STRING);
+
+  if(aLabelPrefColor)
+  {
+    prefColorOutliner.AppendWithConversion(aLabelPrefColor + 1);
+    labelColorAtom = NS_NewAtom(prefColorOutliner.get());
+    NS_ENSURE_TRUE(labelColorAtom, NS_ERROR_FAILURE);
+    aProperties->AppendElement(labelColorAtom);
+  }
+  return NS_OK;
+}


12)

+  NS_ENSURE_TRUE(aMsgHdr, NS_ERROR_FAILURE);
+  NS_ENSURE_TRUE(aProperties, NS_ERROR_FAILURE);


you want NS_ENSURE_ARG_POINTER() here.

13)

instead of doing those switch statements, do this:

if (label == nsMsgViewCommandType::label0)
  return NS_OK;

PRInt32 index = label - nsMsgViewCommandType::label0;

if(!mLabelPrefColors[index].IsEmpty()) 
  ApplyToProperty(mLabelPrefColors[].get(), aProperties);

same idea, to nsMsgDBView::SetLabelCommonProperties and nsMsgDBView::FetchLabel

14)

not checking rv of SetLabelProperties() or ApplyToProperty() or
SetLabelCommonProperties()

15)

instead of:

+      case nsMsgViewCommandType::label0:
       case nsMsgViewCommandType::label1:
       case nsMsgViewCommandType::label2:
       case nsMsgViewCommandType::label3:
       case nsMsgViewCommandType::label4:
       case nsMsgViewCommandType::label5:
         rv = SetLabelByIndex(indices[i], (command -
nsMsgViewCommandType::label1 + 1));

do:

rv = SetLabelByIndex(indices[i], (command - nsMsgViewCommandType::label0));

16)

don't call RemoveLabelPrefObservers() from the dtor.  do it on Close()  and,
check the rv.

before you check in, verify that all these new rules don't slow us down.
also, make sure it works right in quick search and advanced message search.

also, do we need a spin off bug for filtering and search by label, or is that
already done?

Comment 98

[Alex Bishop]

> also, do we need a spin off bug for filtering and search by label, or is that
> already done?

I don't think it's intended for users to be able to filter based on the label of
a message, but setting a label as a filter action is bug 106187.

Comment 99

[John Morrison]

Saw this float by in bugmail:

> 6) <outlinercol id="labelCol" class="outlinercol-header 
> outlinercol-inset-header sortDirectionIndicator" persist="hidden ordinal 
> width" flex="1"> label="&labelColumn.label;" hidden="true"/>

The classes 'outlinercol-header' and 'outlinercol-inset-header' don't exist,
although they were copy-paste propagated to all outliners at some point.
Just remove them. See bug 96154.

Comment 100

[Daniel Veditz [:dveditz]]

Alex, I think that was meant to be read as "a bug for filtering and [another
bug] for search by label". It would reduce the usefulness of labels if you
couldn't search on that field, although you could at least still sort on it.

Comment 101

[jglick]

The plan is to eventually make labels available for:

Manually apply Labels
Label as a Filter action
Use a Label as a Filter criteria
Search based on a Label

Comment 102

[Sean Su]

Attachment: labels patch v2.3

this new patch addresses Seth's comments:
1) updated
2) I talked to Scott and have removed the priority color code altogether.
3) fixed
4) fixed
5) fixed
6) filed bug 113355
7) added localization note
8) fixed
9) fixed
10) fixed
11) we already talked about it.  leaving as is.
12) fixed
13) fixed
14) fixed
15) fixed
16) fixed

verified that Quick Search and Advanced Message Search still works correctly
with this labels patch.

The classes 'outlinercol-header' and 'outlinercol-inset-header' have been
removed too.

Setting labels via filters is bug 106187.

Comment 103

[[e-mail] (gone - use [e-mail] instead)]

I hope this is just a bad merge of files or something, but I'm using the Labels
build and I get:

Error: parent.hPrefWindow has no properties
Source File: chrome://messenger-mapi/content/pref-mailnewsOverlay.js
Line: 23

when trying to open the Edit | Preferences window the 2nd and each successive time.

Comment 104

[(not reading, please use [e-mail] instead)]

stephend, I think that is a known bug.  I believe srilatha owns it.

let me go look for it.

Comment 105

[(not reading, please use [e-mail] instead)]

Comment on attachment 60253 [details] [diff] [review]
labels patch v2.3

in nsMsgDBView::Observe(), you could remove all the duplicate calls to 

NS_ENSURE_SUCCESS(rv,rv);
mOutliner->Invalidate();

and just do it at the end of that method, once.

other than that, looks great.  sr=sspitzer

Comment 106

[Sean Su]

I fixed that right after I attached the patch.  Thanks for the review Seth.

Comment 107

[(not reading, please use [e-mail] instead)]

I couldn't find that bug, so I logged it on srilatha.

http://bugzilla.mozilla.org/show_bug.cgi?id=113548

Comment 108

[David :Bienvenu]

It looks to me like we're creating a new atom every time we paint a cell:

+    labelColorAtom = NS_NewAtom(prefColorOutliner.get());
+    NS_ENSURE_TRUE(labelColorAtom, NS_ERROR_FAILURE);
+    aProperties->AppendElement(labelColorAtom);
+  }

Can't we cache these atoms? There should only be five of them at any given time.

In here, nsMsgDBView::Observe

aren't all the pref strings identical up to the last character? Can't we just
check for the first n-1 characters, and then the last character, and then use
that as an index? This code could be much more compact, I'm sure. Then you'd
only need one call to Invalidate, for example.

Have we measured the performance affect if you don't have any labels defined? I
would hope that it's pretty much nil.

Comment 109

[(not reading, please use [e-mail] instead)]

Comment on attachment 60253 [details] [diff] [review]
labels patch v2.3

needs work, see david's comments.

Comment 110

[Sean Su]

Attachment: labels patch v2.4

This patch addresses David's comments:
 * color atoms are now cached.	there are now only 5 at any given time.
 * nsMsgDBView::Observe, nsMsgDBView::InitLabelPrefs,
nsMsgDBView::AddLabelPrefObservers, and nsMsgDBView::RemoveLabelPrefObservers
have all been simplified.
 * Jennifer's UI concerns:
     -changed default label descriptions
     -reduced the width of the label text field in the prefs
     -updated labels pref dialog description

Comment 111

[(not reading, please use [e-mail] instead)]

1)

style="width:100px"

don't do inline style.  see http://www.mozilla.org/xpfe/skins.html

2)  

restoreColorAndDescriptionToDefaults() is that supposed to restore to the
application defaults, or to the users last values?

"Restore Defaults" is the label text, and I'm not sure.

3)

+    index.Assign(nsPrintfCString("%d", i + 1));
+    prefString.Append(index);

why not use index.AppendInt(i+1)?


4)

+    PRInt32 irv;
+    indexInt = indexStr.ToInteger(&irv);
+    NS_ENSURE_SUCCESS(irv,irv);


is the out param to ToInteger() a nsresult?  if so, you should define irv as an
nsresult.
if not, you shouldn't use NS_ENSURE_SUCCESS(), you should probably do

NS_ASSERTION(!irv, "ToInteger() failed");
if (irv)
  return NS_ERROR_FAILURE;
 

5)  if(prefName.Find(PREF_LABELS_DESCRIPTION, true, 0, 1) != -1)


use kNotFound instead of -1.

6)

GetPrefString() should be renamed to GetLabelPrefStringAndAtom() or something,
as now there are two out params.

there are some comments that need to be updated, as well.

like 

+// helper function used to fetch strings from the prefs

Comment 112

[David :Bienvenu]

Sean, other than Seth's comments, this looks a lot better. I have one more nit:

+    *aLabelString = nsCRT::strdup(mLabelPrefDescriptions[label - 1].get());
+    NS_ENSURE_ARG_POINTER(*aLabelString);

that's not really the correct use of NS_ENSURE_ARG_POINTER, imo, since it's
meant for verifying input params to a method. Seth, what do you think?

Also, I don't think you need to init mLabelPrefColorAtoms to nsnull's, since the
compiler/linker will do that.

Comment 113

[(not reading, please use [e-mail] instead)]

> that's not really the correct use of NS_ENSURE_ARG_POINTER

right, here's a sample of how to use those macros.

nsresult nsMsgDBView::FetchLabel(nsIMsgHdr *aHdr, PRUnichar ** aLabelString)
{
  NS_ENSURE_ARG_POINTER(aHdr);  
  NS_ENSURE_ARG_POINTER(aLabelString);  

  *aLabelString = nsCRT::strdup("foo");
#if 1
  // return, don't assert
  if (!*aLabelString)
    return NS_ERROR_OUT_OF_MEMORY;
#else
  // return and assert
  NS_ENSURE_TRUE(*aLabelString, NS_ERROR_OUT_OF_MEMORY);
#endif

for this specific bit of your patch, I'd just do:

  if (!*aLabelString)
    return NS_ERROR_OUT_OF_MEMORY;

Comment 114

[Sean Su]

Attachment: labels patch v2.5

This patch contains fixes for all of Seth's and David's comments.

Comment 115

[David :Bienvenu]

+  rv = RemoveLabelPrefObservers();
+  NS_ENSURE_SUCCESS(rv,rv);

please ignore this error code - we still want to do the rest of the cleanup in
close. (NS_ENSURE_SUCCESS will return immediately in failure case). I don't need
another patch for that; just do it.

One last question: What does SetLabelCommonProperties do? It seems to flip black
and white somehow, but neither the name of the routine or the function comment
really tell me what's going on here. thanks!

Comment 116

[(not reading, please use [e-mail] instead)]

1)

your PREF_LABELS_MAX could be used here:

+nsIAtom * nsMsgDBView::mLabelPrefColorAtoms[5] = {nsnull, nsnull, nsnull, 
nsnull, nsnull};

and here:

+    // we don't care if label is not between 1 and 5 inclusive.
+    if ((label < 1) || (label > 5))

2)

kLabelColorWhite and kLabelColorBlack are text colors, right?
you might want to change the atom names to make that more clear.

Instead of having a seperate SetLabelCommonProperties(), which goes and re-gets 
the label value from the msg hdr, why not move the code into SetLabelProperties
() so that you only have to get the label value once and check for < 1 and > 5 
once?

+    // We need to subtract 1 because mLabelPrefColors is 0 based.
+    if(mLabelPrefColors[label - 1].Equals(NS_LITERAL_STRING
(LABEL_COLOR_WHITE_STRING)))
+      aProperties->AppendElement(kLabelColorBlack);  
+    else
+      aProperties->AppendElement(kLabelColorWhite);  

could be put right after this:

+      NS_ENSURE_TRUE(mLabelPrefColorAtoms[label - 1], NS_ERROR_FAILURE);
+      aProperties->AppendElement(mLabelPrefColorAtoms[label - 1]);

Comment 117

[(not reading, please use [e-mail] instead)]

ssu explained to me over aim why my suggestion is no good.

"the reason why I didn't have the SetLabelCommonProperties() combined into 
SetLabelProperties() was because SetLabelProperties() was also being called 
from GetRowProrperties() (as well as GetCellTextProperties())."

keeping it as a seperate method makes sense, he's working on a better name now.

Comment 118

[Sean Su]

Attachment: labels patch v2.6

We used to have a white (or black) text color style definition for each of the
colors defined in the style sheet:

  outlinerbody:-moz-outliner-cell-text(lc-663366) {
    color: #663366
  }
> outlinerbody:-moz-outliner-cell-text(lc-lc-663366, selected) {
>   color: #FFFFFF
> }
  outlinerbody:-moz-outliner-row(lc-663366, selected) {
    background-color: #663366;
  }

  outlinerbody:-moz-outliner-cell-text(lc-330033) {
    color: #330033
  }
> outlinerbody:-moz-outliner-cell-text(lc-330033, selected) {
>   color: #FFFFFF
> }
  outlinerbody:-moz-outliner-row(lc-330033, selected) {
    background-color: #330033;
  }

Seth suggested that they be combined and defined on its own to minimize code
duplication.

SetLabelCommonProperties() is used to set this common text color for when the
message is selected.  The only logic in this function is to check to see if the
text color is already white (we don't care if it's black).  If it is white,
then set the text color to black so that it'll be readable on the white
highlighted background.

I'll add more comments and change the function name to something like
SetSelectedTextColorProperties().

I've fixed Seth's and David's new comments and also moved the call:
  msgHdr->GetLabel(&label)
to outside of both AppendLabelProperties() and AppendSelectedTextProperties()
for optimization purposes.

Comment 119

[David :Bienvenu]

Comment on attachment 60829 [details] [diff] [review]
labels patch v2.6

sounds good, thanks, Sean. sr=bienvenu

Comment 120

[(not reading, please use [e-mail] instead)]

Comment on attachment 60829 [details] [diff] [review]
labels patch v2.6

r=sspitzer

great work, ssu.

Comment 121

[[e-mail] (gone - use [e-mail] instead)]

Is there time to request one last build from the build team, that I can test
well over the weekend?  Or should I just proceed with the one I already have?

Comment 122

[Sean Su]

woo hoo! patch finally checked in!

ps. the patch has been verified to build and run on the mac too.

stephend, I'll talk to the build team about a new build.  I'll request just
win32, unless I hear otherwise.

Comment 123

[Sean Su]

QA - here's something that you might want to test:

  1) with the browser closed, edit a mbox file, say "Inbox", using a text editor
     in either your Local Folders or any POP account dir.
  2) search for the first occurrence of:
       X-Mozilla-Status2: 00000000
  3) change that to:
       X-Mozilla-Status2: 0c000000
     0c000000 represents a label value of 6, which is out of the supported labels
     range of 1-5.
  4) run mailnews and scroll to the message changed.  It should not have any
     label/color associated with that message or even crash the app.

Comment 124

[kmurray1115]

Filed http://bugzilla.mozilla.org/show_bug.cgi?id=114503 to add keyboard shortcuts.

Comment 125

[laurel]

Marking this bug verified. 
Labels in and basically functioning.
Any specific issues found with the labels feature will be logged separately.

OK using dec13 commercial trunk builds: win98, mac OS X, linux rh6.2

Comment 126

[Sean Su]

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