Closed
Bug 309057
Opened 19 years ago
Closed 16 years ago
Port |Bug 240138 – Show just the name and not the email address in the message pane| to SeaMonkey; then bug 381555 and bug 243631
Categories
(SeaMonkey :: MailNews: Message Display, defect)
SeaMonkey
MailNews: Message Display
Tracking
(Not tracked)
RESOLVED
FIXED
seamonkey2.0a1
People
(Reporter: iannbugzilla, Assigned: sgautherie)
References
(Blocks 2 open bugs)
Details
Attachments
(5 files, 15 obsolete files)
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
sgautherie
:
review+
neil
:
superreview+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
iannbugzilla
:
review+
neil
:
superreview+
iannbugzilla
:
approval-seamonkey2.0a1+
|
Details | Diff | Splinter Review |
If the email address is in one of your address books, we should just show the
user's name instead of the full name + email address in the message pane.
Outlook and mail.app both do this and it's a great way to reduce space in the
message pane for known addresses. And it lets folks know if the message is from
someone in one of their ABs.
This is the equivalent of TB bug 240138.
Cannot work on this until patch for bug 290881 has landed though.
Comment 1•19 years ago
|
||
NO! I vote for WONTFIX or at least for disabling this by default. Please read
the comments on bug 240138 carefully.
Comment 2•19 years ago
|
||
While it may be nice to have the option to manipulate, this for sure has no
priority. And the default should be "show both".
Just because it will the equivalent of does not mean it will be identical to bug
240138. I will be taking on board the comments from that bug when I start work
on this one which means it might end up looking completely different in
implementation.
Assignee | ||
Comment 4•18 years ago
|
||
Bug 240138 was fixed a few days ago.
Do we want to port what was done there, or do something different ?
A starting point is to begin with porting but then adapting it to best suit SMs needs.
Assignee | ||
Updated•16 years ago
|
Blocks: TB2SM
Severity: enhancement → trivial
Summary: Show just the name and not the email address in the message pane → Port |Bug 240138 – Show just the name and not the email address in the message pane| to SeaMonkey
Target Milestone: --- → seamonkey2.0alpha
Assignee | ||
Comment 6•16 years ago
|
||
[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.0.2pre) Gecko/2008071402 SeaMonkey/2.0a1pre] (nightly) (W2Ksp4)
Bug 240138, ported with a few rewrites.
Assignee: mail → sgautherie.bz
Status: NEW → ASSIGNED
Attachment #329571 -
Flags: superreview?
Attachment #329571 -
Flags: review?(iann_bugzilla)
Assignee | ||
Updated•16 years ago
|
Attachment #329571 -
Flags: superreview? → superreview?(jag)
Assignee | ||
Comment 7•16 years ago
|
||
[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.0.2pre) Gecko/2008071402 SeaMonkey/2.0a1pre] (nightly) (W2Ksp4)
Bug 240138, ported to current code,
plus an accesskey.
Trouble is that this makes the panel too high to fit in the window :-<
I need a "ui-review" (= hint/help) on this.
Attachment #329572 -
Flags: superreview?(jag)
Attachment #329572 -
Flags: review?(iann_bugzilla)
Assignee | ||
Comment 8•16 years ago
|
||
Attachment #329578 -
Flags: superreview?(jag)
Attachment #329578 -
Flags: review?(jag)
Assignee | ||
Comment 9•16 years ago
|
||
Attachment #329579 -
Flags: review?(bienvenu)
Comment 10•16 years ago
|
||
Comment on attachment 329578 [details] [diff] [review]
(Cv1) Add shared default prefence value
>+// Show the friendly display names for people I know.
>+pref("mail.showCondensedAddresses", true);
Nope. This is not going to be the default.
Attachment #329578 -
Flags: superreview?(jag)
Attachment #329578 -
Flags: superreview-
Attachment #329578 -
Flags: review?(jag)
Attachment #329578 -
Flags: review-
Updated•16 years ago
|
Attachment #329579 -
Flags: review?(bienvenu) → review-
Updated•16 years ago
|
Attachment #329572 -
Flags: superreview?(jag) → superreview+
Comment 11•16 years ago
|
||
(In reply to comment #7)
> Trouble is that this makes the panel too high to fit in the window :-<
> I need a "ui-review" (= hint/help) on this.
Heh, it fits in mine. I must've resized my pref window at some point. I don't see an easy solution, other than "make the pref window bigger by default." Maybe Neil can help?
And personally I much prefer seeing both the name and the e-mail address. It helps me figure out much more quickly what's spam and what's not.
Assignee | ||
Comment 12•16 years ago
|
||
Cv1, with comment 10 suggestion(s).
Attachment #329578 -
Attachment is obsolete: true
Attachment #329656 -
Flags: superreview?(bienvenu)
Attachment #329656 -
Flags: review?(mnyromyr)
Assignee | ||
Updated•16 years ago
|
Attachment #329579 -
Flags: review?(bienvenu)
Assignee | ||
Comment 13•16 years ago
|
||
[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.0.2pre) Gecko/2008071402 SeaMonkey/2.0a1pre] (nightly) (W2Ksp4)
I would need (for example) to remove the |<separator class="thin"/>| before and after to get the panel to fit again...
Assignee | ||
Comment 14•16 years ago
|
||
(In reply to comment #11)
> (In reply to comment #7)
> > Trouble is that this makes the panel too high to fit in the window :-<
> > I need a "ui-review" (= hint/help) on this.
>
> Heh, it fits in mine. I must've resized my pref window at some point. I don't
Mine is at its default size (confirmed with a dumb profile).
> see an easy solution, other than "make the pref window bigger by default."
> Maybe Neil can help?
Comment 15•16 years ago
|
||
Ah, I see bug 297534 used up some of our valuable space!
I guess we can't afford to put both of its radiobuttons on one line.
We could remove the thin separators from that groupbox, that looks as if it should give us enough room.
Assignee | ||
Comment 16•16 years ago
|
||
(In reply to comment #15)
> I guess we can't afford to put both of its radiobuttons on one line.
The radiobuttons which seem they could fit on their previous line are the "Fixed + Variable" ones.
> We could remove the thin separators from that groupbox, that looks as if it
> should give us enough room.
As noted in comment 13, removal of 2 separators would be needed.
Assignee | ||
Comment 17•16 years ago
|
||
Or we could create a new panel and see how to reorganize the preferences we have...
Comment 18•16 years ago
|
||
(In reply to comment #16)
> (In reply to comment #15)
> > I guess we can't afford to put both of its radiobuttons on one line.
> The radiobuttons which seem they could fit on their previous line are the
> "Fixed + Variable" ones.
That's not the same thing. Those two buttons are already on the same line.
> > We could remove the thin separators from that groupbox, that looks as if it
> > should give us enough room.
> As noted in comment 13, removal of 2 separators would be needed.
Sure, but there happen to be two inside that groupbox.
Reporter | ||
Comment 19•16 years ago
|
||
(In reply to comment #18)
> (In reply to comment #16)
> > (In reply to comment #15)
> > > I guess we can't afford to put both of its radiobuttons on one line.
> > The radiobuttons which seem they could fit on their previous line are the
> > "Fixed + Variable" ones.
> That's not the same thing. Those two buttons are already on the same line.
I think he means moving them up to the same line as "Use the following font width:", which is possible though might have to shorten the label and/or change the radio buttons into a menulist (similar to HTML message composition).
Assignee | ||
Comment 20•16 years ago
|
||
(In reply to comment #18)
> (In reply to comment #16)
> > As noted in comment 13, removal of 2 separators would be needed.
> Sure, but there happen to be two inside that groupbox.
Yes, that's what I tested (successfully)...
While there, should I removed the 2 separators from the other <groupbox> too ?
(In reply to comment #19)
> I think he means moving them up to the same line as "Use the following font
> width:", which is possible though might have to shorten the label and/or change
> the radio buttons into a menulist (similar to HTML message composition).
Yes, as in the unmigrated "M & N > Composition > General > Forward messages".
Just tell me which solution you prefer.
Assignee | ||
Comment 21•16 years ago
|
||
Neil,
IanN: (IIRC)
I'm happier with either changing the label to "Font width:" followed by the two radio buttons on the same line or changing the label to "Font:" followed by a menulist with items "Fixed Width" and "Variable Width"
Me:
Reducing the text to simply "Font width:" and trivially moving the 2 radios on that line too would please both of us.
(Being identical as the other pane is fine too, thought not "needed" as we wouldn't have the font list in the dropdown.)
Final choice between the various solutions is yours...
Comment 22•16 years ago
|
||
(In reply to comment #21)
I'm happy with IanN's second suggestion, as this then resembles the font menulist in the composition pane.
Assignee | ||
Comment 23•16 years ago
|
||
[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.0.2pre) Gecko/2008071502 SeaMonkey/2.0a1pre] (nightly) (W2Ksp4)
Bv1, with comment 22 suggestion(s).
NB: Although TB (currently) has it with the radios on the same line.
Attachment #329572 -
Attachment is obsolete: true
Attachment #329658 -
Attachment is obsolete: true
Attachment #329775 -
Flags: superreview?(neil)
Attachment #329775 -
Flags: review?(iann_bugzilla)
Attachment #329572 -
Flags: review?(iann_bugzilla)
Comment 24•16 years ago
|
||
Comment on attachment 329775 [details] [diff] [review]
(Bv1a) Preference UI
>+ <menuitem value="false" label="&fontVarWidth.label;"/>
>+ <menuitem value="true" label="&fontFixedWidth.label;"/>
IMHO fixed should come first.
>-<!ENTITY plainTextFont.label "Use the following font width:">
>+<!ENTITY plainTextFont.label "Font:">
>+<!ENTITY plainTextFont.accesskey "F">
This string change might unfortunately need an entity change, maybe to fontPlainText.label/accesskey? (I'd ask KaiRo but he's not around right now.)
Assignee | ||
Comment 25•16 years ago
|
||
Bv1a, with comment 24 suggestion(s).
Attachment #329775 -
Attachment is obsolete: true
Attachment #329847 -
Flags: superreview?(neil)
Attachment #329847 -
Flags: review?(iann_bugzilla)
Attachment #329775 -
Flags: superreview?(neil)
Attachment #329775 -
Flags: review?(iann_bugzilla)
Assignee | ||
Comment 26•16 years ago
|
||
(In reply to comment #24)
> >+ <menuitem value="false" label="&fontVarWidth.label;"/>
> >+ <menuitem value="true" label="&fontFixedWidth.label;"/>
> IMHO fixed should come first.
Should I swap the ones in </editor/ui/composer/content/editorOverlay.xul> too ?
Comment 27•16 years ago
|
||
Comment on attachment 329847 [details] [diff] [review]
(Bv1b) Preference UI
No, because variable is the default there, but fixed is the default here.
Attachment #329847 -
Flags: superreview?(neil) → superreview+
Comment 28•16 years ago
|
||
Comment on attachment 329656 [details] [diff] [review]
(Cv2) Add shared default prefence value
Basically okay, just two nits.
>Index: mailnews.js
>===================================================================
>+// Show the friendly display names for people I know.
>+pref("mail.showCondensedAddresses", false);
>+#else
>+// Show the friendly display names for people I know.
>+pref("mail.showCondensedAddresses", true);
> #endif
Drop the false advertisement (at least in the SeaMonkey part) - change the comment to something neutral like "Show both name and address for people in my addressbook."
You can wrap the TB pref override (patch D) into this one as well, since one is useless without the other and you have David looking at it anyway. ;-)
Attachment #329656 -
Flags: review?(mnyromyr) → review-
Assignee | ||
Comment 29•16 years ago
|
||
Av1, with a small optimization.
Attachment #329571 -
Attachment is obsolete: true
Attachment #329932 -
Flags: superreview?(jag)
Attachment #329932 -
Flags: review?(iann_bugzilla)
Attachment #329571 -
Flags: superreview?(jag)
Attachment #329571 -
Flags: review?(iann_bugzilla)
Assignee | ||
Comment 30•16 years ago
|
||
Cv2 and Dv1-TB, with comment 28 suggestion(s).
Attachment #329579 -
Attachment is obsolete: true
Attachment #329656 -
Attachment is obsolete: true
Attachment #329933 -
Flags: superreview?(bienvenu)
Attachment #329933 -
Flags: review?(mnyromyr)
Attachment #329579 -
Flags: review?(bienvenu)
Attachment #329656 -
Flags: superreview?(bienvenu)
Comment 31•16 years ago
|
||
Comment on attachment 329932 [details] [diff] [review]
(Av1a) Display part
>+function AddExtraAddressProcessing(emailAddress, addressNode)
>+{
>+ // If the "friendly name" feature is disabled,
>+ // don't care about the actual display name.
>+ const displayName = gShowCondensedEmailAddresses &&
>+ addressNode.getAttribute("displayName");
>+
>+ if (displayName)
>+ {
>+ // always show the address for the from and reply-to fields
>+ const parentElementId = addressNode.parentNode.id;
>+ const condenseName = parentElementId != "expandedfromBox" &&
>+ parentElementId != "expandedreply-toBox";
>+
>+ if (condenseName && useDisplayNameForAddress(emailAddress))
>+ {
>+ addressNode.setAttribute("label", displayName);
>+ addressNode.setAttribute("tooltiptext", emailAddress);
>+ }
>+ }
>+}
I think I would prefer:
function AddExtraAddressProcessing(emailAddress, addressNode)
{
if (!gShowCondensedEmailAddresses)
return;
const displayName = addressNode.getAttribute("displayName");
if (!displayName)
return;
if (!useDisplayNameForAddress(emailAddress))
return;
// don't condense the address for the from and reply-to fields
const parentElementId = addressNode.parentNode.id;
const condenseName = parentElementId != "expandedfromBox" &&
parentElementId != "expandedreply-toBox";
if (condenseName)
{
addressNode.setAttribute("label", displayName);
addressNode.setAttribute("tooltiptext", emailAddress);
}
}
Looks good otherwise.
Attachment #329932 -
Flags: superreview?(jag) → superreview+
Updated•16 years ago
|
Attachment #329933 -
Flags: review?(mnyromyr) → review+
Assignee | ||
Comment 32•16 years ago
|
||
(In reply to comment #31)
> const condenseName = parentElementId != "expandedfromBox" &&
> parentElementId != "expandedreply-toBox";
> if (condenseName)
And I could merge the lines and use an early return there too...
> Looks good otherwise.
Early returns were what I first thought of,
then I thought may be there was none because there might be another (different) extra processing added in the future...
Do you confirm that early returns are wanted ?
Reporter | ||
Comment 33•16 years ago
|
||
Comment on attachment 329847 [details] [diff] [review]
(Bv1b) Preference UI
>Index: mailnews/base/prefs/resources/content/pref-viewing_messages.xul
>===================================================================
>+ <hbox align="center">
>+ <label value="&fontPlainText.label;" control="mailFixedWidthMessages"
>+ accesskey="&fontPlainText.accesskey;"/>
Nit: one attribute per line please with control being the last one.
>+ <menulist id="mailFixedWidthMessages"
>+ preference="mail.fixed_width_messages">
>+ <menupopup>
>+ <menuitem value="true" label="&fontFixedWidth.label;"/>
>+ <menuitem value="false" label="&fontVarWidth.label;"/>
I would say one attribute per line, but as there is only two attributes in total I think it is okay in this case.
>Index: suite/locales/en-US/chrome/mailnews/pref/pref-viewing_messages.dtd
>===================================================================
>+<!ENTITY showCondensedAddresses.label "Show only display name for people in my address book">
Should we mention it is only in the personal address book?
>+<!ENTITY showCondensedAddresses.accesskey "p">
Wouldn't "o" be a better accesskey.
Whilst you are touching this file, is it worth fixing the duplicate accesskey "e"?
I would suggest:
change wrapInMsg.accesskey to "W"
this frees up "t" for markAsReadDelay.accesskey
r=me with the relevant changes for this bug and either the additional changes done here or spun off into another bug.
Attachment #329847 -
Flags: review?(iann_bugzilla) → review+
Reporter | ||
Comment 34•16 years ago
|
||
Comment on attachment 329932 [details] [diff] [review]
(Av1a) Display part
>Index: msgHdrViewOverlay.js
>===================================================================
>- if ("AddExtraAddressProcessing" in this)
>- AddExtraAddressProcessing(address.emailAddress, emailAddressNode);
Are we sure that no extensions rely on this call? We could always leave this call in and just change the name of the new function that is going in.
I presume there is no way of keeping the tooltip over attachments in the attachment window?
Other than that query I am happy with jag's comments wrt early return.
>+function useDisplayNameForAddress(emailAddress)
>+{
>+ // For now, if the email address is in the personal address book, then consider this user a 'known' user
>+ // and use the display name. I could eventually see our rules enlarged to include other local ABs, replicated
>+ // LDAP directories, and maybe even domain matches (i.e. any email from someone in my company
>+ // should use the friendly display name)
Nit: lines should not be any more than 80 characters wide so please reformat the above comment lines - should still fit within the 4 lines though.
r=me with those points addressed
Attachment #329932 -
Flags: review?(iann_bugzilla) → review+
Assignee | ||
Comment 35•16 years ago
|
||
(In reply to comment #34)
> (From update of attachment 329932 [details] [diff] [review])
> >- if ("AddExtraAddressProcessing" in this)
> >- AddExtraAddressProcessing(address.emailAddress, emailAddressNode);
> Are we sure that no extensions rely on this call? We could always leave this
> call in and just change the name of the new function that is going in.
You may have missed
> + AddExtraAddressProcessing(address.emailAddress, emailAddressNode);
;->
Assignee | ||
Comment 36•16 years ago
|
||
[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.0.2pre) Gecko/2008071502 SeaMonkey/2.0a1pre] (nightly) (W2Ksp4)
Bv1b, with comment 33 suggestion(s),
plus the 2nd duplicate accesskey fix.
Attachment #329847 -
Attachment is obsolete: true
Assignee | ||
Comment 37•16 years ago
|
||
(In reply to comment #33)
> Should we mention it is only in the personal address book?
(We could, but)
I think not, as there is bug 381555.
> r=me with the relevant changes for this bug and either the additional changes
> done here or spun off into another bug.
I had filed this as bug 445213 ;-)
Blocks: 381555
Assignee | ||
Updated•16 years ago
|
Attachment #329932 -
Attachment description: (Av1) Display part → (Av1a) Display part
Assignee | ||
Comment 39•16 years ago
|
||
Av1a, with comment 34 suggestion(s).
(In reply to comment #34)
> I presume there is no way of keeping the tooltip over attachments in the
> attachment window?
The tooltip is only set in another way, not removed: see bug 240138 comment 46.
Attachment #329932 -
Attachment is obsolete: true
Assignee | ||
Comment 40•16 years ago
|
||
David,
(SM) code is ready for checkin;
now waiting for your sr on the default (TB) pref value patch.
Flags: in-litmus-
Reporter | ||
Comment 41•16 years ago
|
||
(In reply to comment #35)
> (In reply to comment #34)
> > (From update of attachment 329932 [details] [diff] [review] [details])
> > >- if ("AddExtraAddressProcessing" in this)
> > >- AddExtraAddressProcessing(address.emailAddress, emailAddressNode);
> > Are we sure that no extensions rely on this call? We could always leave this
> > call in and just change the name of the new function that is going in.
>
> You may have missed
> > + AddExtraAddressProcessing(address.emailAddress, emailAddressNode);
> ;->
>
No I didn't. What I'm saying is that some extensions might provide their own "AddExtraAddressProcessing" and this patch now removes that hook by using that name for the new function. Now if you are sure that no extension makes use of this hook then fine, otherwise leave this hook and change the name of the function.
Reporter | ||
Comment 42•16 years ago
|
||
(In reply to comment #39)
> Created an attachment (id=330239) [details]
> (Av1b) Display part
>
> Av1a, with comment 34 suggestion(s).
>
> (In reply to comment #34)
> > I presume there is no way of keeping the tooltip over attachments in the
> > attachment window?
>
> The tooltip is only set in another way, not removed: see bug 240138 comment 46.
>
Okay, I am happy with that part now.
Assignee | ||
Comment 43•16 years ago
|
||
(In reply to comment #41)
> What I'm saying is that some extensions might provide their own
> "AddExtraAddressProcessing" and this patch now removes that hook by using that
> name for the new function. Now if you are sure that no extension makes use of
> this hook then fine, otherwise leave this hook and change the name of the
> function.
Let me (fully) dig history:
SeaMonkey:
r1.4 / 2000-01-23 18:00
added call (only) [bug 24399]
r1.29 / 2000-05-25 18:09
added if [(restricted) bug 40516: "fix aim presence for mail in the commercial tree." !]
<http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=MozillaTinderboxAll&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2000-05-25+18%3A07&maxdate=2000-05-25+18%3A10&cvsroot=%2Fcvsroot>
{{
2000-05-25 18:08 mscott%netscape.com mozilla/mailnews/base/resources/content/msgHdrViewAddresses.js 1.5
Bug #40516 --> this file is now obsolete....instead of having these
stub methods in the mozilla tree, we'll test to see if the function is
defined (it is in the commercial tree) then call it if it is defined.
r=sspitzer
2000-05-25 18:09 mscott%netscape.com mozilla/mailnews/base/resources/content/msgHdrViewOverlay.js 1.29
Bug #40516 --> fix aim presence for mail in the commercial tree.
Changes to make the toggle button an image instead of a button
r=sspitzer
}}
Thunderbird:
r1.1 / 2002-08-15 17:26
added initial (if and call) code
r1.17 / 2004-04-12 23:29
added/hijacked the function [bug 240138 !]
r1.77 / 2006-07-24 15:04
removed the (obsolete) if [bug 345650]
Indeed, only SM still has the hooks:
<http://mxr.mozilla.org/mozilla/search?string=fillEmailAddressMenu&case=on>
No results found
<http://mxr.mozilla.org/mozilla/search?string=FinishEmailProcessing&case=on>
/mailnews/base/resources/content/msgHdrViewOverlay.js (remnant hook)
<http://mxr.mozilla.org/mozilla/search?string=NotifyClearAddresses&case=on>
/mailnews/base/resources/content/msgHdrViewOverlay.js (remnant hook)
<http://mxr.mozilla.org/mozilla/search?string=AddExtraAddressProcessing&case=on>
/mail/base/content/msgHdrViewOverlay.js (TB "hijacked" function)
/mailnews/base/resources/content/mailWidgets.xml (See r1.42 : Bug 149515)
/mailnews/base/resources/content/msgHdrViewOverlay.js (See my current patch.)
/mailnews/base/resources/content/msgHdrViewAddresses.js (See Bug 445167 !)
Then:
Although I have no idea about possible current extensions using these hooks,
it seems the latters are only remnants of Netscape+Aim bundling,
which TB already got rid of.
Iiuc, we would be talking about SM-only and post-Aim extensions.
I think it should be worth/"safe" to try and remove (all !) the hooks,
then see if we get complains ... as we are still early enough in the SM v2.0 cycle.
Mnyromyr, what do you think ?
Comment 44•16 years ago
|
||
I never met an extension actually using it, but that doesn't necessarily mean anything (Mnenhy just mimics mailnews' usage and calls it if present, it doesn't actually use it). Given the amount of complaints (none I know of, but again, that doesn't necessarily mean anything as I'm not following TB fora), I guess the hooks can be removed.
By my own experience these hooks are almost meaningless, because for doing useful changes to header processing, you need to overlay most of the functions doing these callbacks with your own completely... :-|
Assignee | ||
Comment 45•16 years ago
|
||
Assignee | ||
Comment 46•16 years ago
|
||
Keeping
{{
(Ev1) Add/Regroup default prefence value(s)
mnyromyr: review+
}}
Attachment #329933 -
Attachment is obsolete: true
Attachment #330516 -
Flags: review+
Attachment #329933 -
Flags: superreview?(bienvenu)
Assignee | ||
Updated•16 years ago
|
Keywords: checkin-needed
Whiteboard: [c-n: Cv3-SM first, then Av1b and Bv1c]
Assignee | ||
Updated•16 years ago
|
Attachment #330516 -
Flags: superreview?(jag)
Comment 47•16 years ago
|
||
Cv3-SM still waits for sr, please add back keyword etc when it's ready
Keywords: checkin-needed
Whiteboard: [c-n: Cv3-SM first, then Av1b and Bv1c]
Reporter | ||
Comment 48•16 years ago
|
||
Might be worth switching your sr request to someone else? Neil perhaps?
Assignee | ||
Updated•16 years ago
|
Attachment #330516 -
Flags: superreview?(jag) → superreview?(neil)
Comment 49•16 years ago
|
||
(In reply to comment #37)
> (In reply to comment #33)
> > Should we mention it is only in the personal address book?
>
> (We could, but)
> I think not, as there is bug 381555.
Since that one is fixed now you might consider doing the necessary changes as part of this bug.
Reporter | ||
Comment 50•16 years ago
|
||
Also worth looking at bug 243631 and https://bugzilla.mozilla.org/attachment.cgi?id=334253
Updated•16 years ago
|
Attachment #330516 -
Flags: superreview?(neil) → superreview+
Assignee | ||
Updated•16 years ago
|
Flags: in-litmus-
Keywords: checkin-needed
Whiteboard: [c-n: Cv3-SM first, then Av1b and Bv1c // Leave opened]
Reporter | ||
Comment 51•16 years ago
|
||
Comment on attachment 330239 [details] [diff] [review]
(Av1b) Display part (Checkin: Comment 51)
http://hg.mozilla.org/comm-central/rev/be8f98ac3842
Attachment #330239 -
Attachment description: (Av1b) Display part → (Av1b) Display part (Checkin: Comment 51)
Attachment #330231 -
Attachment description: (Bv1c) Preference UI → (Bv1c) Preference UI (Checkin: Comment 51)
Attachment #330516 -
Attachment description: (Cv3-SM) Add SM default prefence value → (Cv3-SM) Add SM default prefence value (Checkin: Comment 51)
Reporter | ||
Comment 52•16 years ago
|
||
Leaving open for issues in comment 49 and comment 50 to be looked at / spun off.
Keywords: checkin-needed
Whiteboard: [c-n: Cv3-SM first, then Av1b and Bv1c // Leave opened] → [Leave opened]
Comment 53•16 years ago
|
||
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b1pre) Gecko/20080831003516 SeaMonkey/2.0a1pre
This doesn't work for me, does it work for anyone of you? I tried:
1. activated new pref (using UI, verified using about:config)
2. added Personal Address Book entry for my own email address
3. sent mail from myself to myself
4. triggered Get Messages for my IMAP account
5. selected the mail that I sent in step 3.
Expected results: "From:" in header pane should show only the name
Actual results: "From:" in header pane shows name and email address.
The same applies to news postings (test scenario: KaiRo's 1.1.12 candidates posting in m.d.a.seamonkey with KaiRo in the Personal Address Book) but I don't know if the mechanism is intended to work there.
FWIW: Collected Addresses is empty and there are no additional address books.
Comment 54•16 years ago
|
||
(In reply to comment #52)
> Leaving open for issues in comment 49 and comment 50 to be looked at / spun
> off.
Both affect the same file so they can be easily handled in one bug - or do you prefer to do it here?
Assignee | ||
Comment 55•16 years ago
|
||
(In reply to comment #53)
> Expected results: "From:" in header pane should show only the name
"From:" is never condensed.
> The same applies to news postings (test scenario: KaiRo's 1.1.12 candidates
SM 1.1.x branch will never have this feature.
(In reply to comment #54)
> Both affect the same file so they can be easily handled in one bug - or do you
> prefer to do it here?
Let me handle it, unless you want to port these additional bugs yourself (which I would be happy with ;->).
Comment 56•16 years ago
|
||
(In reply to comment #55)
> (In reply to comment #53)
> > Expected results: "From:" in header pane should show only the name
>
> "From:" is never condensed.
<Checking source code> You're right. But the only other exception is Reply-To. I cannot make it work for To either, though. Can you provide a screen shot and/or test case of how it should look like if it works?
> > The same applies to news postings (test scenario: KaiRo's 1.1.12 candidates
>
> SM 1.1.x branch will never have this feature.
Funny guy. I know that! I was describing a test case where you put KaiRo into your Personal Address Book and view a newsgroup posting. It could have been any other one but that's the one I found first. But now that we found that From: is never condensed it's a bad test case anyway.
> (In reply to comment #54)
> > Both affect the same file so they can be easily handled in one bug - or do you
> > prefer to do it here?
>
> Let me handle it, unless you want to port these additional bugs yourself (which
> I would be happy with ;->).
I could give it a try. That's why I asked.
Comment 57•16 years ago
|
||
This should be it. The fix for bug 381555 is a straight port; for bug 243631 I tried to keep the style of the present code.
I also found and fixed another bug: for SM we need to go up another two levels for parentElementId (found using DOMI and Venkman). Without that parentElementId is "" and matches neither From nor Reply-To.
BTW: I guess the comment that can be seen in bug 243631's attachment 334253 [details] [diff] [review] ("AddExtraAddressProcessing which is undefined in seamonkey") is now wrong.
BTW2: With my patch I now see only the display name for addresses that are both in my Personal Address Book and the To header row.
Attachment #336266 -
Flags: review?(sgautherie.bz)
Comment 58•16 years ago
|
||
Note that Serge's review won't be official and your official reviewer (usually whomever last reviewed changes to that code) may not agree with his comments.
Comment 59•16 years ago
|
||
(In reply to comment #58)
> Note that Serge's review won't be official and your official reviewer (usually
> whomever last reviewed changes to that code) may not agree with his comments.
That's OK; I just looked at the last patch (Cv3-SM) since I don't know who's the official reviewer or how to find one. The last changes to the code were through this bug. And if anything goes wrong there's still superreview and I don't have check-in rights anyway. ;-) If you think someone else should do the review I'm fine with that, too.
Assignee | ||
Comment 60•16 years ago
|
||
(In reply to comment #59)
> That's OK; I just looked at the last patch (Cv3-SM)
You were misleaded on this one: see comment 46 ;->
> since I don't know who's the official reviewer or how to find one.
(You're welcome to join us and learn ;-))
Read pages like
http://developer.mozilla.org/En/Getting_your_patch_in_the_tree
> If you think someone else should do the review I'm fine with that, too.
I'll have a look at it, then request official review ;-)
Reporter | ||
Comment 61•16 years ago
|
||
Comment on attachment 336266 [details] [diff] [review]
Port of bugs 243631 and 381555 plus parentElementId fix
> var gMessageListeners = new Array;
>@@ -1057,22 +1055,23 @@ function updateEmailAddressNode(emailAdd
> AddExtraAddressProcessing(address.emailAddress, emailAddressNode);
> }
>
> function AddExtraAddressProcessing(emailAddress, addressNode)
> {
> if (!gShowCondensedEmailAddresses)
> return;
>
>+ const card = useDisplayNameForAddress(emailAddress);
>+ if (!card)
> return;
>+ const displayName = card.displayName;
You don't really need this assignment just use card.displayName in the setAttribute further down.
>
> // Don't condense the address for the from and reply-to fields.
>+ const parentElementId = addressNode.parentNode.parentNode.parentNode.id;
> if (parentElementId == "expandedfromBox" ||
> parentElementId == "expandedreply-toBox")
> return;
>
> addressNode.setAttribute("label", displayName);
> addressNode.setAttribute("tooltiptext", emailAddress);
> }
>
>@@ -1087,30 +1086,32 @@ function fillEmailAddressPopup(emailAddr
>+ var books = Components.classes["@mozilla.org/abmanager;1"]
>+ .getService(Components.interfaces.nsIAbManager)
>+ .directories;
>+
>+ var card = null;
>+
>+ while (!card && books.hasMoreElements()) {
>+ var ab = books.getNext()
>+ .QueryInterface(Components.interfaces.nsIAbDirectory);
>+ try {
>+ card = ab.cardForEmailAddress(emailAddress);
>+ }
>+ catch (ex) { }
Our bracing style is to have a new brace on a separate line unless it opens and closes on the same line, so in this case the bracket for while and for try needs to be on a new line.
Comment 62•16 years ago
|
||
Same as before, with Ian's nits addressed. I also took the liberty to remove the space inside the curly brackets after "catch" to match the style used in the rest of the file.
Attachment #336266 -
Attachment is obsolete: true
Attachment #336367 -
Flags: review?(mnyromyr)
Attachment #336266 -
Flags: review?(sgautherie.bz)
Comment 63•16 years ago
|
||
I just read KaiRo's blog post about Alpha freeze next week. Of course the feature that is implemented through this bug isn't an Alpha blocker but if my patch won't go in until then then the feature will be only half working (since the parts that have already been checked in are unlikely to get removed). This statement is not meant to push anyone, just my 2c that I wanted to let you know before I'm AFK for the next two days.
Updated•16 years ago
|
Attachment #336367 -
Flags: review?(mnyromyr) → review-
Comment 64•16 years ago
|
||
Comment on attachment 336367 [details] [diff] [review]
Port v2 of bugs 243631 and 381555 plus parentElementId fix
>+++ patch/msgHdrViewOverlay.js 2008-09-01 18:38:58.000000000 +0200
> function AddExtraAddressProcessing(emailAddress, addressNode)
> {
...
>+ const card = useDisplayNameForAddress(emailAddress);
>+ if (!card)
> return;
Constants should have a k prefix, i.e. kCard.
OTOH, we usually don't use const in such cases, so "var card" (as you do further down) is preferred here.
>+ const parentElementId = addressNode.parentNode.parentNode.parentNode.id;
Eww, this is rather ugly. Not much we can do about it, though; let's hope no-one breaks this when touching the mail-multi-emailHeaderField XBL code. To aid him, add a comment like "get the id of the mail-multi-emailHeaderField binding parent".
Furthermore, use var instead of const here, too.
>@@ -1087,30 +1085,34 @@ function fillEmailAddressPopup(emailAddr
> function useDisplayNameForAddress(emailAddress)
<rant>Man, how I *hate* if new functions don't adhere to the current naming scheme!</rant>
(Just ignore this. ;-) )
>+ var card = null;
>+ while (!card && books.hasMoreElements())
>+ {
>+ var ab = books.getNext()
>+ .QueryInterface(Components.interfaces.nsIAbDirectory);
>+ try
>+ {
>+ card = ab.cardForEmailAddress(emailAddress);
>+ }
>+ catch (ex) {}
>+ return card;
Using the instanceof operator gains some extra safety; furthermore, you can use an early return if you find a card, so no outer card variable is needed.
while (books.hasMoreElements())
{
var ab = books.getNext();
if (ab instanceof Components.interfaces.nsIAbDirectory)
{
try
{
var card = ab.cardForEmailAddress(emailAddress);
if (card)
return card;
}
catch (ex) {}
}
}
return null;
Assignee | ||
Comment 65•16 years ago
|
||
(In reply to comment #63)
> I wanted to let you know before I'm AFK for the next two days.
I'm working on it: I'll update your patch ;-)
Assignee | ||
Comment 66•16 years ago
|
||
(In reply to comment #53)
> This doesn't work for me, does it work for anyone of you?
You're right !
My Av1b patch stopped working (before checkin) between
[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.1a2pre) Gecko/20080816002821 SeaMonkey/2.0a1pre] (nightly) (W2Ksp4)
and
[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.1a2pre) Gecko/20080817002455 SeaMonkey/2.0a1pre] (nightly) (W2Ksp4)
when
4f44120fd2a8 2008-08-16 10:54 -0400 Joshua Cranmer - Bug 449618 -- Move cardForEmailAddress to nsIAbDirectory r=Standard8, sr=dmose
landed... :-<
> The same applies to news postings [...] but I don't know if the mechanism is intended to work there.
Yes, this feature applies to newsgroups too.
(In reply to comment #56)
> Funny guy. I know that!
Sorry, I misread your comment and understood that you used a "1.1.12 candidates" build to test...
(In reply to comment #57)
> I also found and fixed another bug: for SM we need to go up another two levels
> for parentElementId (found using DOMI and Venkman).
Right ! I must have missed testing this :-<
> BTW: I guess the comment that can be seen in bug 243631's attachment 334253 [details] [diff] [review]
> ("AddExtraAddressProcessing which is undefined in seamonkey") is now wrong.
Removed by Bug 446343 ;-)
Assignee | ||
Comment 67•16 years ago
|
||
Porting/Copying
1.104 jminta%gmail.com 2008-06-01 07:10 Bug 436677 Clean up rdf consumers of the addressbook, r=mkmelin
(only <msgHdrViewOverlay.js> part applies to SM)
makes this feature work again.
And this is still true with
[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.1b1pre) Gecko/20080901134927 SeaMonkey/2.0a1pre] (home, optim default) (W2Ksp4)
Yet, that (part of the) patch was "replaced" by bug 449618.
(which works fine in current build too.)
Then |useDisplayNameForAddress()| was rewritten (again) by
b6532caf76a6 2008-08-18 16:25 +0100 Mark Banner - Bug 381555 'Show only display name' works only for personal address book. r=bienvenu
***
[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.1b1pre) Gecko/20080901134927 SeaMonkey/2.0a1pre] (home, optim default) (W2Ksp4)
*Port (bug 436677 > bug 449618 >) bug 381555.
*Additionally replace the 2 tabs in this file.
*|setFromBuddyIcon()|: optimize it a little and make it use |getCardForAddress()|.
*|updateEmailAddressNode()|: optimize the |*Attribute()| calls.
*|AddExtraAddressProcessing()|: fix |parentElementId|.
*|useDisplayNameForAddress()|:
*Rename it |getCardForAddress()|.
*Rewrite it some more.
*I would think |QueryInterface()| is "enough", but as Karsten wanted |instanceof|...
*Update documentation.
*****
I prefer to port bug 243631 later/separately.
Later, I'll file a bug about sharing |getCardForAddress()| with <mailWindowOverlay.js> too... (see bug 449618)
Attachment #336740 -
Flags: superreview?(neil)
Attachment #336740 -
Flags: review?(iann_bugzilla)
Comment 68•16 years ago
|
||
Comment on attachment 336740 [details] [diff] [review]
(Gv1) Port bug 381555 (++)
>+ if (card && card.setProperty("_AimScreenName")) {
I think this was supposed to be .getProperty (I did comment on that bug but I guess nothing happened...)
> function updateEmailAddressNode(emailAddressNode, address)
> {
>- emailAddressNode.setAttribute("label", address.fullAddress || address.displayName);
> emailAddressNode.setAttribute("emailAddress", address.emailAddress);
> emailAddressNode.setAttribute("fullAddress", address.fullAddress);
> emailAddressNode.setAttribute("displayName", address.displayName);
>- emailAddressNode.removeAttribute("tooltiptext");
>
>- AddExtraAddressProcessing(address.emailAddress, emailAddressNode);
>+ if (!AddExtraAddressProcessing(address.emailAddress, emailAddressNode))
>+ {
>+ emailAddressNode.setAttribute("label",
>+ address.fullAddress || address.displayName);
>+ emailAddressNode.removeAttribute("tooltiptext");
>+ }
> }
I'm not convinced by this change (and therefore the boolean returns below).
>+ if (!(ab instanceof Components.interfaces.nsIAbDirectory))
>+ continue;
>
>+ try
>+ {
>+ var card = ab.cardForEmailAddress(emailAddress);
>+ if (card)
>+ return card;
>+ }
>+ catch (ex)
>+ {
>+ // Unsearchable address books throw |NS_ERROR_NOT_IMPLEMENTED|.
>+ }
I think I might prefer
if (ab instanceof Components.interfaces.nsIAbDirectory) {
...
}
Attachment #336740 -
Flags: superreview?(neil) → superreview+
Assignee | ||
Comment 69•16 years ago
|
||
(In reply to comment #68)
> (From update of attachment 336740 [details] [diff] [review])
> >+ if (card && card.setProperty("_AimScreenName")) {
> I think this was supposed to be .getProperty (I did comment on that bug but I
> guess nothing happened...)
I very much thought so myself too, when I read the patch which changed it ! ;-<
Yet, I would rather not (look into and) change this here:
I would rather add a reminder to bug 453627, whether we end up keeping that code or not.
> >- AddExtraAddressProcessing(address.emailAddress, emailAddressNode);
> >+ if (!AddExtraAddressProcessing(address.emailAddress, emailAddressNode))
> I'm not convinced by this change (and therefore the boolean returns below).
While testing/debugging my patch, I noticed that the full address was first displayed then replaced by the displayName.
I didn't look much further but thought this could only "optimize" this behavior.
But, as you don't like it, I'll remove it.
> I think I might prefer
> if (ab instanceof Components.interfaces.nsIAbDirectory) {
For sure, I like this even less (than the |QueryInterface()|, but I'll do it too.
***
Ian, I'll wait for your review before updating the patch.
Comment 70•16 years ago
|
||
(In reply to comment #69)
> Yet, I would rather not (look into and) change this here:
> I would rather add a reminder to bug 453627, whether we end up keeping that
> code or not.
In that case could you add a reminder comment in the code too?
> While testing/debugging my patch, I noticed that the full address was first
> displayed then replaced by the displayName.
I can't see how that would happen, unless you stepped through it in Venkman; normally I would expect the display to update once at the end.
> For sure, I like this even less (than the |QueryInterface()|, but I'll do it
> too.
You mean var card = ab.QueryInterface(...)
.cardForEmailAddress(...); ?
Assignee | ||
Comment 71•16 years ago
|
||
(In reply to comment #70)
> In that case could you add a reminder comment in the code too?
I filed bug 454073: I hope you agree with my plan there.
> I can't see how that would happen, unless you stepped through it in Venkman;
> normally I would expect the display to update once at the end.
I used |alert()|.
I thought that might be what triggered this behavior,
I wasn't sure but I thought the "optimization" could not hurt.
> You mean var card = ab.QueryInterface(...)
> .cardForEmailAddress(...); ?
"Yes", see comment 64 (like Jens "direct port" code).
NB: Ian, Neil, I agree with Jens's comment 63: I'd like to land this patch (one way or the other) before SM 2.0a1 freeze ... and deal with (all) the rest after that. (Neil, could you do the review too, if Ian is not around ?) Thanks.
Comment 72•16 years ago
|
||
(In reply to comment #71)
> (In reply to comment #70)
> > In that case could you add a reminder comment in the code too?
> I filed bug 454073: I hope you agree with my plan there.
Looks good to me.
> > I can't see how that would happen, unless you stepped through it in Venkman;
> > normally I would expect the display to update once at the end.
> I used |alert()|.
Oh yes, that would also do it :-)
> I wasn't sure but I thought the "optimization" could not hurt.
It hurts code readability...
> > You mean var card = ab.QueryInterface(...)
> > .cardForEmailAddress(...); ?
> "Yes", see comment 64 (like Jens "direct port" code).
No, I was thinking the QueryInterface would be inside the try block.
> NB: Ian, Neil, I agree with Jens's comment 63: I'd like to land this patch (one
> way or the other) before SM 2.0a1 freeze ... and deal with (all) the rest after
> that. (Neil, could you do the review too, if Ian is not around ?) Thanks.
Given comment 64 maybe you should ask Mnyromyr if he has time to review?
Attachment #336740 -
Flags: review?(iann_bugzilla) → review+
Reporter | ||
Comment 73•16 years ago
|
||
Comment on attachment 336740 [details] [diff] [review]
(Gv1) Port bug 381555 (++)
>@@ -994,33 +990,21 @@ function OutputEmailAddresses(headerEntr
>
> function setFromBuddyIcon(email)
> {
> var fromBuddyIcon = document.getElementById("fromBuddyIcon");
>
> try {
> // better to cache this?
> var myScreenName = pref.getCharPref("aim.session.screenname");
>+ if (myScreenName)
>+ {
>
>+ var card = getCardForAddress(email);
>+ if (card && card.setProperty("_AimScreenName")) {
Please be consistent with your bracing, I know there's already a mix in this file but the preferred is to open/close braces on a new line.
r=me with that change.
Assignee | ||
Comment 74•16 years ago
|
||
Gv1, with comment 68 and comment 73 suggestion(s).
(In reply to comment #72)
> No, I was thinking the QueryInterface would be inside the try block.
When I first wrote the patch, I even thought to get rid of the |var ab| by moving all the code inside the block,
but, because of the "silent" catch, I decided to keep it (outside).
I'll checkin this patch tomorrow, unless anyone wants some other update...
Attachment #336740 -
Attachment is obsolete: true
Assignee | ||
Comment 75•16 years ago
|
||
Comment on attachment 337353 [details] [diff] [review]
(Gv1a) Port bug 381555 (++)
[Checkin: Comment 75]
http://hg.mozilla.org/comm-central/rev/b6fb6db81c5d
Attachment #337353 -
Attachment description: (Gv1a) Port bug 381555 (++) → (Gv1a) Port bug 381555 (++)
[Checkin: Comment 75]
Assignee | ||
Comment 76•16 years ago
|
||
[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.1b1pre) Gecko/20080901134927 SeaMonkey/2.0a1pre] (home, optim default) (W2Ksp4)
*Port bug 243631 attachment 334253 [details] [diff] [review].
*|getCardForAddress()|: Add and use |checkFunction| parameter.
*Add |"collapsedfromValue"| exclusion,
which everyone (including me) seems to have missed until now :->
*Check the UI before the address books, in hope this is a little faster.
(At least, it seems more logical this way.)
Attachment #336367 -
Attachment is obsolete: true
Attachment #337593 -
Flags: superreview?(neil)
Attachment #337593 -
Flags: review?(iann_bugzilla)
Comment 77•16 years ago
|
||
(In reply to comment #76)
> Created an attachment (id=337593) [details]
> (Hv1) Port bug 243631 attachment 334253 [details] [diff] [review] (++)
+ function hasDisplayName(card)
+ {
+ return card.displayName != "";
+ }
+
+ var card = getCardForAddress(emailAddress, hasDisplayName);
If you do this, you will come into problems later on. Assuming you're going to implement the add/edit menu options that TB now has, then if you have a matching card but with no display name you will fail to show that the card is in the address book - and you will be prompting the user to create a new card, rather than editing an existing one.
Updated•16 years ago
|
Attachment #337593 -
Flags: superreview?(neil) → superreview-
Comment 78•16 years ago
|
||
Comment on attachment 337593 [details] [diff] [review]
(Hv1) Port bug 243631 attachment 334253 [details] [diff] [review] (++)
> try
> {
>- var card = ab.cardForEmailAddress(emailAddress);
>- if (card)
>- return card;
>+ card = ab.cardForEmailAddress(emailAddress);
> }
> catch (ex)
> {
> // Unsearchable address books throw |NS_ERROR_NOT_IMPLEMENTED|.
>+ continue;
> }
>+
>+ if (!checkFunction || checkFunction(card))
>+ return card;
You're not null-checking card any more. I'd also prefer it if you put the checkFunction call inside the try/catch.
Assignee | ||
Comment 79•16 years ago
|
||
(In reply to comment #78)
> You're not null-checking card any more.
Ah, right :-<
> I'd also prefer it if you put the checkFunction call inside the try/catch.
Same story as with the QueryInterface() call:
I "moved" the checkFunction() call out of the try, because I didn't want the silent catch to mask a throw from it...
What I think I could do (probably for both), is to check the exception type inside the catch:
*NOT_IMPLEMENTED: remain silent.
*others: dump the exception but don't re-throw it.
Do you agree with this ?
Comment 80•16 years ago
|
||
(In reply to comment #79)
> (In reply to comment #78)
> > I'd also prefer it if you put the checkFunction call inside the try/catch.
> Same story as with the QueryInterface() call:
> I "moved" the checkFunction() call out of the try, because I didn't want the
> silent catch to mask a throw from it...
OK, I guess that makes sense.
Assignee | ||
Comment 81•16 years ago
|
||
Hv1, with (new) "bug 455714 part" removed.
Attachment #337593 -
Attachment is obsolete: true
Attachment #339075 -
Flags: superreview?(neil)
Attachment #339075 -
Flags: review?(iann_bugzilla)
Attachment #337593 -
Flags: review?(iann_bugzilla)
Updated•16 years ago
|
Attachment #339075 -
Flags: superreview?(neil) → superreview+
Attachment #339075 -
Flags: review?(iann_bugzilla) → review+
Attachment #339075 -
Flags: approval-seamonkey2.0a1+
Assignee | ||
Comment 82•16 years ago
|
||
Comment on attachment 339075 [details] [diff] [review]
(Hv2) Port bug 243631 attachment 334253 [details] [diff] [review] (++)
[Checkin: Comment 82]
http://hg.mozilla.org/comm-central/rev/dba6f5a634ec
Attachment #339075 -
Attachment description: (Hv2) Port bug 243631 attachment 334253 (++) → (Hv2) Port bug 243631 attachment 334253 (++)
[Checkin: Comment 82]
Assignee | ||
Comment 83•16 years ago
|
||
(In reply to comment #67)
> Later, I'll file a bug about sharing |getCardForAddress()| with
> <mailWindowOverlay.js> too... (see bug 449618)
I filed bug 456655.
No longer blocks: TB2SM
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Whiteboard: [Leave opened]
Assignee | ||
Comment 84•16 years ago
|
||
Keeping
{{
(Ev1) Add/Regroup default prefence value(s)
mnyromyr: review+
}}
Attachment #340037 -
Flags: superreview?(bienvenu)
Attachment #340037 -
Flags: review?(bienvenu)
Assignee | ||
Comment 85•16 years ago
|
||
Comment on attachment 340037 [details] [diff] [review]
(Ev1a) Regroup default prefence value(s)
I filed bug 456663 instead.
Attachment #340037 -
Attachment is obsolete: true
Attachment #340037 -
Flags: superreview?(bienvenu)
Attachment #340037 -
Flags: review?(bienvenu)
You need to log in
before you can comment on or make changes to this bug.
Description
•