Closed Bug 81292 Opened 24 years ago Closed 23 years ago

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

Categories

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

Product:
SeaMonkey
Component:
MailNews: Message Display
Type:
enhancement

Tracking

(Not tracked)

Status:
VERIFIED FIXED
Milestone:
mozilla0.9.7

People

(Reporter: jglick, Assigned: ssu0262)

References

(Blocks 1 open bug,
URL
)

Details

Attachments

(13 files, 8 obsolete files)

Image One
57.47 KB, image/gif
Details
Image Two
32.80 KB, image/gif
Details
Image Three
37.85 KB, image/gif
Details
Example with "None" option
30.15 KB, image/gif
Details
Image of what the patch will create
58.13 KB, image/jpeg
Details
Image of what the patch will create (highlight has colors)
235.87 KB, image/jpeg
Details
Image of what the patch will create (higlight is not colored)
243.01 KB, image/jpeg
Details
Color labels in Entourage v.X for Mac OS X
66.55 KB, image/png
Details
colors on the folders side too, please!
96.70 KB, image/gif
Details
Labels menu image 1 - normal w/o colors for highlight or strings
75.64 KB, image/jpeg
Details
Labels menu image 2 - background is colored and strings turn white when menu item is selcted
77.46 KB, image/jpeg
Details
Labels menu image 3 - strings are colored; highlight/background are normal
77.34 KB, image/jpeg
Details
labels patch v2.6
163.06 KB, patch
sspitzer
: review+
Bienvenu
: superreview+
Details | Diff | Splinter Review
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.
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).
Similar implementations, Eudora mail has a similar feature and Mac OS lets you color/label folders to help you organize/prioritize.
Attached image Image One —
Attached image Image Two —
Attached image Image Three —
Should Label names be numerical? What about temperature: cold, cool, warm, hot, etc. Or custom?
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.
Severity: normal → enhancement
OS: Windows 98 → All
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.
Ah. Image Two. Those thingies there are indeed editable fields. My bad. Great work, as usual.
*** Bug 67817 has been marked as a duplicate of this bug. ***
bug 67676 is duplicate/related.
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?
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.
Let's keep the first phase of the UI implementation simple, then expand as more folks give us feedback.
Blocks: 34146
*** Bug 84737 has been marked as a duplicate of this bug. ***
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
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?
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.
*** Bug 96549 has been marked as a duplicate of this bug. ***
FYI, this is being discussed over in the newsgroup. And it seems like it's becoming a wontfix according to the discussion over there...
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)?
> And it seems like it's > becoming a wontfix according to the discussion over there... huh? Anyways, over to the newsgroup, please.
reassigning to ssu
Assignee: sspitzer → ssu
Blocks: 102231
Depends on: 103168
Depends on: 81085
Keywords: nsbeta1
adding nsbeta1+ and moving to 0.9.6
Keywords: nsbeta1 → nsbeta1+
Priority: -- → P2
Target Milestone: --- → mozilla0.9.6
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.
Depends on: 106067
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).
Depends on: 106187
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...
>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?
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.
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?
>Accessible Clicking on the label directly should cause it to cycle through the available labels, 1,2,3,4,5,None,1,2, etc.
Depends on: 106711
No longer depends on: 103168
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?
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
>"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.
*** Bug 107554 has been marked as a duplicate of this bug. ***
QA Contact: esther → laurel
If it can get in before 0.9.7 great, but let's figure labels will land after 0.9.6.
Target Milestone: mozilla0.9.6 → mozilla0.9.7
Attached image Image of what the patch will create (obsolete) —
Attached patch labels patch attempt #1 (obsolete) — — Splinter Review
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.
Status: NEW → ASSIGNED
adding bhuvan to the cc: list.
No longer depends on: 106711
Blocks: 106711
Blocks: 107663
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).
URL: http://www.mozilla.org/mailnews/specs...
This image shows the labeled messages with its highlights colored (and the text white).
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.
Attachment #56977 - Attachment is obsolete: true
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.
I like the second screen shot better. I think having multiple selection colors is confusing.
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.
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.
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)
As a reference point, Outlook 2000 does #2 in the list in my comments above.
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 ;)
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.
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.
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?
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.
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?
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.
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.
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.
>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.
>>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?
We take a poll within the mozilla community - didn't you see it? ;-)
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"
yes, labels are persistent, and there is a separate bug for filters : bug 106187
>"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
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.
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.
Attached patch labels patch v2.0 (obsolete) — — Splinter Review
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.
Attachment #56978 - Attachment is obsolete: true
but the term LABELS TO ORGANIZE USERS MESSAGES can be applied also to folders.
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.
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.
Excuse me but if you had 3000 mails organized thru 200 folders and subfolders you would find usefull a way to color a folder.
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.
All right guys, as has been stated before, this belongs in another bug. Please continue discussion there or in the newsgroups.
Blocks: 109514
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.
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.
Yeah...I'm not sold on the menu coloring either. #1 looks more than adequate.
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.
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.
The accesskey for the Labels menu being 'l' as opposed to 'L' is addressed in my patch to this bug.
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.
I also prefer attachment 57409 [details] (Labels menu image 1 - normal w/o colors for highlight or strings). Nice work. :-)
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
Attached patch labels patch v2.1 (obsolete) — — Splinter Review
> 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.
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.
Blocks: 107432
>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.
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.
Blake, won't that conflict with the checkbox effect?
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.
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.
Attached patch labels patch v2.2 (obsolete) — — Splinter Review
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.
Attachment #57321 - Attachment is obsolete: true
Attachment #57660 - Attachment is obsolete: true
*** Bug 111412 has been marked as a duplicate of this bug. ***
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?
> 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.
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.
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.
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
Attached patch labels patch v2.3 (obsolete) — — Splinter Review
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.
Attachment #59272 - Attachment is obsolete: true
No longer blocks: 107663
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.
Hardware: PC → All
stephend, I think that is a known bug. I believe srilatha owns it. let me go look for it.
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
Attachment #60253 - Flags: superreview+
I fixed that right after I attached the patch. Thanks for the review Seth.
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 on attachment 60253 [details] [diff] [review] labels patch v2.3 needs work, see david's comments.
Attachment #60253 - Flags: superreview+ → needs-work+
Attached patch labels patch v2.4 (obsolete) — — Splinter Review
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
Attachment #60253 - Attachment is obsolete: true
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
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.
> 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;
Attached patch labels patch v2.5 (obsolete) — — Splinter Review
This patch contains fixes for all of Seth's and David's comments.
Attachment #60594 - Attachment is obsolete: true
+ 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!
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]);
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.
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.
Attachment #60788 - Attachment is obsolete: true
Comment on attachment 60829 [details] [diff] [review] labels patch v2.6 sounds good, thanks, Sean. sr=bienvenu
Attachment #60829 - Flags: superreview+
Comment on attachment 60829 [details] [diff] [review] labels patch v2.6 r=sspitzer great work, ssu.
Attachment #60829 - Flags: review+
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?
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.
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
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.
Filed http://bugzilla.mozilla.org/show_bug.cgi?id=114503 to add keyboard shortcuts.
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
Status: RESOLVED → VERIFIED
*** Bug 114655 has been marked as a duplicate of this bug. ***
Product: Browser → Seamonkey
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: