308988 - support delegated use of imap shared folders

Status: RESOLVED FIXED

(SeaMonkey :: MailNews: Message Display, enhancement)

Description

[Ian Neal]

This is the mailnews version of TB's bug 285474

Comment 1

[Ian Neal]

Attachment: sender: header patch v0.1

This patch:
* Displays the sender header if it is different to the from one (as per first
patch on TB bug 285474)

Comment 2

[Karsten Düsterloh]

Comment on attachment 196466 [details] [diff] [review]
sender: header patch v0.1

>--- msgHdrViewOverlay.js.old	2005-08-23 20:48:05.013875000 +0100
>--- msgHdrViewOverlay.xul.old	2005-09-11 02:04:57.344875000 +0100

Erm... ;-)
And, btw, you forgot to diff the msgHdrViewOverlay.dtd (I suppose), because I'm
getting the Infamous Red XUL Error for &senderField.label;.b

>         if (lowerCaseHeaderName == "x-mailer" ||
>             lowerCaseHeaderName == "x-newsreader" ||
>             lowerCaseHeaderName == "x-mimeole")
>           lowerCaseHeaderName = "user-agent";          
>         
>+        if (lowerCaseHeaderName == "sender")
>+        {
>+          if (fromMailbox == header.headerValue)
>+            continue;
>+        }

If we've hit the first 'if', we most probably ;-) can't hit the second, thus it
should be in an 'else' branch. Furthermore, the inner 'if' could just be an
&&-part of the outer one... 

And now the most important question: how is this supposed to do anything useful
wrt the intended purpose? I.e. displaying the sender header if it is different
to the from one?

With this patch, the sender is displayed almost always if it exists, because:
- "if (fromMailbox == header.headerValue)" will always fail if the sender
header comes _before_ the from header.
- $sender.headerValue is compared to
msgHeaderParser.extractHeaderAddressMailboxes(null, $from.value);, which does
even fail for 
  Sender: FROM <pkdfglkjf@lgjldgjk>
  From: FROM <pkdfglkjf@lgjldgjk>

Comment 3

[Ian Neal]

Attachment: Working sender header patch v0.1a

Changes since v0.1:
* Included missing dtd diff
* Now compare mailbox with mailbox instead of mailbox with headerValue
* Test for header.headerValue which is created unlike header.value
* sender header name check is now an else if statement

Comment 4

[Karsten Düsterloh]

Comment on attachment 198176 [details] [diff] [review]
Working sender header patch v0.1a

Sorry, but this still just can't work like this.
If the 'sender' header comes _before_ the 'from' header (which is perfectly
legitimate), it will always be shown, because fromMailbox is undefined (it
should be initialized anyway). Furthermore, the 'from' header can contain
multiple mailboxes, while the 'sender' must(!) not.
So checking for equality is pretty useless in those cases where a 'sender' must
appear.

This patch just tackles the border case where both 'from' and 'sender' are a
single mailbox and the 'sender' comes coincidentally after the 'from'...

JFTR: <http://www.faqs.org/rfcs/rfc2822.html>
Section 3.6.: "It is important to note that the header fields are not
guaranteed to be in a particular order."
Section 3.6.2. Originator fields

Comment 5

[Ian Neal]

How is the from header specified when it contains multiple mailboxes? A comma
separated list?

Comment 6

[Ian Neal]

Attachment: Multiple from patch v0.1b

Changes since v0.1a:
* Does not assume sender comes after from header - checks once all headers are
read.
* Correctly displays from headers in expanded header view - was assuming from
was single mailbox whereas it could me multiple mailboxes.
* Correctly displays from headers in collapsed header view - assumption was as
above.

Comment 7

[Ian Neal]

Attachment: Multiple from and try/catch patch v0.1c

Changes since v0.1b:
* Adds changes from patch for bug 312474 - no headers displayed when no address book to collect addresses to.

Comment 8

[Karsten Düsterloh]

Comment on attachment 201305 [details] [diff] [review]
Multiple from and try/catch patch v0.1c

>Index: content/msgHdrViewOverlay.js
>===================================================================
>+      if (("from" in currentHeaderData) && ("sender" in currentHeaderData) && msgHeaderParser)
>+      {
>+        var deleteSender = false;
>+        var senderMailbox = msgHeaderParser.extractHeaderAddressMailboxes(null, currentHeaderData["sender"].headerValue);
>+        var fromMailboxes = msgHeaderParser.extractHeaderAddressMailboxes(null, currentHeaderData["from"].headerValue).split(/\s*,\s*/);

This won't always work, since
  ",[e-mail],[e-mail]"@somewhere.us
is a perfectly legitimate email address.
Thus extractHeaderAddressMailboxes uses ", " as a separator, because the space character can't appear unescaped in a local-part (RfC 2822, section 3.2).
(Furthermore, there's no need to user ["name"] here, you could use just use .name in both cases.)

>+        var i = 0;
>+        while (i < fromMailboxes.length && !deleteSender)
>+        {
>+          if (fromMailboxes[i] == senderMailbox)
>+            deleteSender = true;
>+          i++;
>+        }

This is overly complicated, imo. How about this, solving the above problem, too:

const kMailboxSeparator = ", ";
const kSenderMailbox = kMailboxSeparator + msgHeaderParser.extractHeaderAddressMailboxes(null, currentHeaderData.sender.headerValue) + kMailboxSeparator;
const kFromMailboxes = kMailboxSeparator + msgHeaderParser.extractHeaderAddressMailboxes(null, currentHeaderData.from.headerValue) + kMailboxSeparator;
deleteSender = kFromMailboxes.indexOf(kSenderMailbox) >= 0;

> function OutputEmailAddresses(headerEntry, emailAddresses)
> {
>   if ( !emailAddresses ) return;

Make that 
  if (!emailAddresses)
    return;
while you're here anyway. ;-)  

>Index: content/msgHdrViewOverlay.xul
>===================================================================
>-    <column id="collapsedfromBox" collapsed="true">
>+    <column id="collapsedfromColumn" flex="1">

This is not a good idea, since *all* other header boxes adhere to the canonical "<collapsed|expanded><headername>Box" scheme ... 

>       <hbox align="start">
>-        <label class="collapsedHeaderDisplayName" value="&fromField.label;"/>          
>-        <mail-emailaddress id="collapsedfromValue" flex="1"/>
>+        <mail-multi-emailHeaderField id="collapsedfromBox" class="collapsedHeaderDisplayName" label="&fromField.label;" collapsed="true" flex="1"/>

..., so just use id="collapsedfromValue" here like eg. collapsedtoBox does.


Rest of the file ignored since you seem to have pasted the diff twice. ;-)
I already wondered what made this patch be twice as large...

Comment 9

[Ian Neal]

Attachment: Keep box/value for multiple from patch v0.1d

Changes from v0.1c:
* Kept collapsedfromBox/Value ids as they were
* Adjusted createHeaderEntry function to cope with the above
* Adjusted sender within from detection as suggested by reviewer (almost)

Comment 10

[Karsten Düsterloh]

Comment on attachment 201373 [details] [diff] [review]
Keep box/value for multiple from patch v0.1d

Looks good. Sorry for the delay. :-(

Comment 11

[Ian Neal]

Comment on attachment 201373 [details] [diff] [review]
Keep box/value for multiple from patch v0.1d

I suspect you will ask me to do the same for TB, if I get the sr=, before checking in this patch

Comment 12

[Ian Neal]

Attachment: Equivalent patch for TB v0.1d

This patch synchronises changes from mailnews to TB i.e.
* Deals with all cases for sender/from header
* Deals with multiple from headers properly and not just displaying one of them

Comment 13

[David :Bienvenu]

Comment on attachment 201373 [details] [diff] [review]
Keep box/value for multiple from patch v0.1d

couple nits:

+    useShortView = headerListInfo.useShortView;
+    if (useShortView)
+    {
+      this.enclosingBox = this.textNode;
+    }
+    else
+    {
+      this.enclosingBox.emailAddressNode = this.textNode;
+    }

you don't need the extra local variable, or the extra braces.

+                if (gCollectOutgoing)
+                  // collect, but only update existing cards, unknown preferred send format
+                  gCollectAddressTimer = setTimeout('abAddressCollector.collectUnicodeAddress(gCollectAddress, false, Components.interfaces.nsIAbPreferMailFormat.unknown);', 2000);
+                else
+                  // collect, and add card if doesn't exist, unknown preferred send format
+                  gCollectAddressTimer = setTimeout('abAddressCollector.collectUnicodeAddress(gCollectAddress, true, Components.interfaces.nsIAbPreferMailFormat.unknown);', 2000);
+              }

this can be one line using !gCollectOutgoing for the boolean parameter.

Comment 14

[Ian Neal]

Attachment: Less braces combined patch v0.1e

Changes from v0.1e
* Removed un-needed braces from if (useShortView)
* Removed un-needed braces from if (this.textNode)
* Used !gCollectOutgoing as suggested and updated comments for it
* Combined mailnews/TB patches into one patch

I did not remove the variable useShortView as it is used within the if (this.textNode) later on.

Carrying forward r= and requesting sr=

Comment 15

[Ian Neal]

Comment on attachment 203750 [details] [diff] [review]
Less braces combined patch v0.1e

Checking in
mailnews/base/resources/content/mailWidgets.xml;
new revision: 1.85; previous revision: 1.84
mailnews/base/resources/content/msgHdrViewOverlay.js;
new revision: 1.146; previous revision: 1.145
mailnews/base/resources/content/msgHdrViewOverlay.xul;
new revision: 1.66; previous revision: 1.65
mail/base/content/msgHdrViewOverlay.js;
new revision: 1.63; previous revision: 1.62
mail/base/content/msgHdrViewOverlay.xul;
new revision: 1.15; previous revision: 1.14
done

Comment 16

[Ian Neal]

Comment on attachment 203750 [details] [diff] [review]
Less braces combined patch v0.1e

Checking in missing file
msgHdrViewOverlay.dtd;
msgHdrViewOverlay.dtd
new revision: 1.14; previous revision: 1.13
done

Comment 17

[David :Bienvenu]

Attachment: fix tb regression

I'm going to back out part of this for tb, so we'll only show sender when its different from from.

Comment 18

[Ian Neal]

Attachment: full fix to tb regression patch v0.1f

Changes since v0.1e
* Fixed missing cut and paste from my tweaked jar files to patching tree

http://lxr.mozilla.org/seamonkey/source/mail/base/content/msgHdrViewOverlay.js#385 to 390 would have been still checking for fromMailbox

Comment 19

[David :Bienvenu]

Comment on attachment 203857 [details] [diff] [review]
full fix to tb regression patch v0.1f

ok, thx, sorry for the incorrect fix...

Comment 20

[Ian Neal]

Comment on attachment 203857 [details] [diff] [review]
full fix to tb regression patch v0.1f

Checking in
msgHdrViewOverlay.js;
new revision: 1.65; previous revision: 1.64
done

Comment 21

[walter]

(In reply to comment #15)
> (From update of attachment 203750 [details] [diff] [review] [edit])
> Checking in
> mailnews/base/resources/content/mailWidgets.xml;
[...]

This check-in had a weird side-effect on the display of RSS messages from
www.weather.gov which still exists in the nightly build of TB trunk as of
today, 27 Nov.  As your checkin has to do with headers, I will paste the
headers from one of the messages which displays incorrectly (the vertical
sliders disappear from both the articles pane and the message body pane):

From - Sun Nov 27 2005 04:14:45 GMT-0800 (PST)
X-Mozilla-Status: 0001
X-Mozilla-Status2: 00000000
Date: Sun Nov 27 2005 04:14:45 GMT-0800 (PST)
Message-Id: <http://www.weather.gov/alerts/ca.html#[e-mail]>
From: <California - Current Watches, Warnings and Advisories for California Issued by the National Weather Service>
MIME-Version: 1.0
Subject: Red Flag Warning - CAZ258 (?) (California)
Content-Transfer-Encoding: 8bit
Content-Base: http://www.weather.gov/alerts/ca.html#CAZ258.SGXRFWSGX.110400
Content-Type: text/html; charset=UTF-8
[html body snipped]

My guess is the word 'From' in the first line is confusing everything from
that point onwards.  It's pretty dumb to start the message that way, but TB
did display these messages normally until this check-in.

Can the previous behavior be restored without getting the U.S. government to
change its website?

Comment 22

[walter]

(In reply to comment #21)

> My guess is the word 'From' in the first line is confusing everything from
> that point onwards...

Sorry, wrong guess.  The problem apparently is due to the length of the 'From'
line.  I see now that the vertical sliders return if I make the TB window wide
enough to display the entire 'From' line, which in this case is absurdly wide.

Comment 23

[[e-mail]]

At least one of the changes to msgHdrViewOverlay.js checked in referring to this bug doesn't match up with any of these patches. The incoming address collector no longer works correctly. The code currently in CVS never updates addresses for news posts, nor does it add addresss for incoming mail unless both the outgoing preference is false and either(!) the incoming or news preference is true. As a reminder, the idea is that an address may be added if the appropriate incoming or news pref is set based on the misnamed dontCollectAddress flag.

Comment 24

[walter]

Ian, you have two significant (and unacknowleged) problem reports with your
last check-in. I would ask that you back out your changes until you can fix
both problems.

Comment 25

[Ian Neal]

(In reply to comment #24)
> Ian, you have two significant (and unacknowleged) problem reports with your
> last check-in. I would ask that you back out your changes until you can fix
> both problems.
> 

Other than the problem Neil mentioned, which one are you talking about? I thought you had already confirmed that what you saw as a bug was not one but just a very long from header. If it is still a problem please log a new bug, attach images from 20th and 22nd builds to show before and after landing and reference this bug in the dependencies.

Comment 26

[Ian Neal]

Attachment: Correct the logic patch v0.1g

This patch:
* Corrects the logic for when to create a card in the addressbook or not

As a reminder to Neil, you did mention something about perhaps doing it via read timeouts instead.

Comment 27

[walter]

(In reply to comment #25)
> ... If it is still a problem please log a new bug,
> attach images from 20th and 22nd builds to show before and after landing and
> reference this bug in the dependencies.

I opened Bug #319028 and added you to the CC list.  Thanks.

Comment 28

[Ian Neal]

Attachment: Better logic patch v0.1h

Changes since v0.1g:
* Changed to using ' + createCard + ' as suggested by Neil on IRC

Comment 29

[Ian Neal]

Attachment: Better logic with correct brackets patch v0.1i

Changes from v0.1h:
* Removed extra bracket from if statement

Comment 30

[Ian Neal]

Comment on attachment 204979 [details] [diff] [review]
Better logic with correct brackets patch v0.1i

Checking in
mailnews/base/resources/content/msgHdrViewOverlay.js;
new revision: 1.147; previous revision: 1.146
mail/base/content/msgHdrViewOverlay.js;
new revision: 1.66; previous revision: 1.65
done

Comment 31

[Ian Neal]

Attachment: Branch 1.8.1 version of patch v0.1j (Checked in)

This is the 1.8.1 branch version of the patch, mailWidgets.xml changes have previously been landed by David on 4th Feb.
Carrying forward r/sr and requesting approval for branch1.8.1 and SM1.1a

Comment 32

[Scott MacGregor]

Iann, I checked in the thunderbird portin onto the branch, but I didn't have a seamonkey build at the moment to test the seamonkey half, so I'll leave that for you. 

Comment 33

[Robert Kaiser]

Comment on attachment 213769 [details] [diff] [review]
Branch 1.8.1 version of patch v0.1j (Checked in)

If mscott is OK with it for branch, I guess so are we :)
a=me for SeaMonkey 1.1

Comment 34

[Ian Neal]

Comment on attachment 213769 [details] [diff] [review]
Branch 1.8.1 version of patch v0.1j (Checked in)

Checking in (1.8.1 branch)
msgHdrViewOverlay.xul;
new revision: 1.64.8.5; previous revision: 1.64.8.4
msgHdrViewOverlay.js;
new revision: 1.141.2.7; previous revision: 1.141.2.6
done

Comment 35

[Serge Gautherie (:sgautherie)]

(In reply to comment #34)
> (From update of attachment 213769 [details] [diff] [review] [edit])
> Checking in (1.8.1 branch)

This checkin has caused a regession between
[Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8) Gecko/20060419 SeaMonkey/1.1a] (nightly) (W98SE)
and
[Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8) Gecko/20060420 SeaMonkey/1.1a] (nightly) (W98SE)
{{
Error: undefined entity
Source File: chrome://messenger/content/msgHdrViewOverlay.xul
Line: 166, Column: 5
Source Code:
    <mail-emailheaderfield id="expandedsenderBox" label="&senderField.label;" collapsed="true"/>----^

Error: messageHeaderSink is not defined
Source File: chrome://messenger/content/mailWindow.js
Line: 212
}}
when starting MailNews, which is unusable as is.

Comment 36

[Ian Neal]

Attachment: Missing file patch for branch v0.1j1 (Checked in 1.8.1 branch)

Missed adding entity for branch (did the same for trunk originally too)

Comment 37

[Karsten Düsterloh]

Comment on attachment 219358 [details] [diff] [review]
Missing file patch for branch v0.1j1 (Checked in 1.8.1 branch)

I actually can't give sr, so just regard this as a SM-only mailnews moa. ;-)

Comment 38

[Ian Neal]

Comment on attachment 219358 [details] [diff] [review]
Missing file patch for branch v0.1j1 (Checked in 1.8.1 branch)

Checking in (1.8.1 branch)
msgHdrViewOverlay.dtd;
new revision: 1.12.8.4; previous revision: 1.12.8.3
done

Comment 39

[Serge Gautherie (:sgautherie)]

(In reply to comment #38)
> (From update of attachment 219358 [details] [diff] [review] [edit])
> Checking in (1.8.1 branch)

[Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8) Gecko/20060422 SeaMonkey/1.1a] (nightly) (W98SE)

Comment 35 regression fixed.