185184 - Add an option to download only mail headers from POP account

Status: RESOLVED FIXED

(MailNews Core :: Networking: POP, enhancement)

Description

[Jure Repinc (JLP)]

I frequaently need to acces my POP mail account from where I work and
unfortunately  we have only limited diskspace available. If I download the whole
mail (headers, body, attachements, ...) I quickly fill all my available space.
So I would like to see an option in Mozilla, that would enable you to select
only to download mail headers initialy and then when you click on a mail header
to view mail Mozilla would fetch the whole mail. This way I could save a lot of
space and be able to delete spam from POP server without downloading the whole mail.

Comment 1

[Nicu Buculei]

I think this feature is very useful for dial-up users.

Comment 2

[R.K.Aa.]

similar: bug 173344

Comment 3

[bk039]

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

Comment 4

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

mass re-assign.

Comment 5

[Michael Gabriel]

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

Comment 6

[Michael Gabriel]

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

Comment 7

[Michael Gabriel]

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

Comment 8

[Jo Hermans]

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

Comment 9

[Felix Miata]

Even better: Download only headers on messages larger than a specified size, and
be able to delete larger ones from the server selectively without ever
downloading the body. Nice way to not waste bandwidth downloading spam & virii,
regardless whether broadband or POTS. :-)

Comment 10

[Mike Coppins]

Attachment: example of a mail client (YAM)'s mail download system

Added screenshot of a mail client called YAM's interface to download mails that
require attention (due to say a configuration that says "don't automatically
download mails that fall into this criteria").

Comment 11

[Jeff Walden [:Waldo]]

Does this require another bug to apply to Mozilla Thunderbird as well?  Or would
this setting be accessible through the config features (UI later, of course) of
Thunderbird if it's added to MozMail?

Comment 12

[Fernando Cassia]

I was going to add a new RFE/BUG suggesting the inclusion of "delete spam ON THE
SERVER" (without actually downloading the messages).

Could this bug be stretched to include this functionality, or should I file it
separatelly?

In other words, I want Mozilla Mail OPTIONALLY doing this before fetching mail:

-going to the pop3 server, retrieving sender and subject for every message.
-running the bayesean filters to identify spam, based only on sender/subject
-display a small dialog saying "Mozilla has identified the following emails as
spam on the server, click OK to delete the marked messages or Cancel to download
everything".
[Scrollable list of messages that can be unselected via Ctrl-Click]

Also "[x] Do not ask again" would make the dialog go away and all suspected spam
deleted on every following email retrieval. (High risk, I know, but the user is
supposed to know what he is doing if he selects this).

It would also be available (if not enabled for every pop3 email fetch), from
Tools-> "Delete mail identified as Junk in SERVER" just below the current
"Delete mail identified as Junk in Folder".

Sounds good? I'd like to hear comments.

Comment 13

[Alfonso Martinez]

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

Comment 14

[Benedikt Kantus]

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

Comment 15

[Stephen Donner [:stephend] Not actively reading bugmail]

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

Comment 16

[Howard Chu]

(In reply to comment #12)
> In other words, I want Mozilla Mail OPTIONALLY doing this before fetching mail:
> 
> -going to the pop3 server, retrieving sender and subject for every message.
> -running the bayesean filters to identify spam, based only on sender/subject

I want something like this as well, but it needs to use more than just the
sender and the subject. It should use all the available headers. For example,
the recipient is very telling; aside from a few lists that I subscribe to, I get
a fair amount of spam in my mailbox that doesn't list my email address in the
To: header.

I've just done a CVS checkout and have started looking into this. My initial
thought was to remodel the POP code based on the IMAP handler since that already
has much of the desired behavior, but maybe the NNTP handler is closer.

Of course, unlike the NNTP handler, I don't want the message automatically
downloaded when I select it; I want the opportunity to hit "Delete" upon
selection. You can almost get the desired behavior by setting a small download
size limit (in the Disk Space menu). But setting this to zero makes it default
to 50k, which defeats that purpose. Also, after messages have been trapped in
this manner, it would be nice to have a bulk option to download all the
remaining messages after the spam has been deleted.

So, it seems that we want a single property that says "only download headers",
and a popup menu option to "download selected messages" and then everything will
be great. Upon reading the code thus far, adding the property looks easy enough,
but I don't know about the popup action.

Comment 17

[Howard Chu]

Attachment: Adds a "Fetch Headers Only" option for POP3

This patch adds a Fetch Headers Only preference for a POP3 account as described
in my comment #16 to this bug. I haven't yet investigated the other half of the
features mentioned in my comment.

Comment 18

[Howard Chu]

(In reply to comment #17)
> Created an attachment (id=149732)
> Adds a "Fetch Headers Only" option for POP3
> 
> This patch adds a Fetch Headers Only preference for a POP3 account as described
> in my comment #16 to this bug. I haven't yet investigated the other half of the
> features mentioned in my comment.

The initial patch makes the "Fetch Headers" option dependent on the "Leave
messages on Server" option. After using it a little bit, I think that's not
necessary, so in my own build I've now moved the option out on its own. (This
makes the change to the am-server.js file unnecessary, and moves the change in
am-server.xul up above the leaveOnServer checkbox.)

Comment 19

[Howard Chu]

Comment on attachment 149732 [details] [diff] [review]
Adds a "Fetch Headers Only" option for POP3

>Index: base/prefs/resources/content/am-server.xul
>===================================================================
>RCS file: /cvsroot/mozilla/mailnews/base/prefs/resources/content/am-server.xul,v
>retrieving revision 1.107
>diff -u -r1.107 am-server.xul
>--- base/prefs/resources/content/am-server.xul  4 May 2004 22:28:14 -0000       1.107
>+++ base/prefs/resources/content/am-server.xul  1 Jun 2004 05:03:10 -0000
>@@ -150,6 +150,12 @@
>               accesskey="&downloadOnBiff.accesskey;"
>               prefstring="mail.server.%serverkey%.download_on_biff"/>
>
>+    <checkbox wsm_persist="true" id="pop3.headersOnly"
>+              label="&headersOnly.label;"
>+              accesskey="&headersOnly.accesskey;"
>+              prefattribute="value"
>+              prefstring="mail.server.%serverkey%.headers_only"/>
>+
>     <checkbox wsm_persist="true" id="pop3.leaveMessagesOnServer"
>               label="&leaveOnServer.label;" oncommand="setupMailOnServerUI();"
>               accesskey="&leaveOnServer.accesskey;"
>Index: base/prefs/resources/locale/en-US/am-server-top.dtd
>===================================================================
>RCS file: /cvsroot/mozilla/mailnews/base/prefs/resources/locale/en-US/am-server-top.dtd,v
>retrieving revision 1.37
>diff -u -r1.37 am-server-top.dtd
>--- base/prefs/resources/locale/en-US/am-server-top.dtd	4 May 2004 22:28:14 -0000	1.37
>+++ base/prefs/resources/locale/en-US/am-server-top.dtd	1 Jun 2004 04:19:42 -0000
>@@ -31,6 +31,8 @@
> <!ENTITY useSecAuth.accesskey "i">
> <!ENTITY leaveOnServer.label "Leave messages on server">
> <!ENTITY leaveOnServer.accesskey "g">
>+<!ENTITY headersOnly.label "Fetch headers only">
>+<!ENTITY headersOnly.accesskey "f">
> <!ENTITY deleteByAgeFromServer.label "For at most">
> <!ENTITY deleteByAgeFromServer.accesskey "o">
> <!ENTITY daysEnd.label "days">
>Index: local/public/nsIPop3IncomingServer.idl
>===================================================================
>RCS file: /cvsroot/mozilla/mailnews/local/public/nsIPop3IncomingServer.idl,v
>retrieving revision 1.18
>diff -u -r1.18 nsIPop3IncomingServer.idl
>--- local/public/nsIPop3IncomingServer.idl	18 May 2004 13:19:28 -0000	1.18
>+++ local/public/nsIPop3IncomingServer.idl	1 Jun 2004 04:19:43 -0000
>@@ -46,6 +46,7 @@
> [scriptable, uuid(758a8970-e628-11d2-b7fc-00805f05ffa5)]
> interface nsIPop3IncomingServer : nsISupports {
>   attribute boolean leaveMessagesOnServer;
>+  attribute boolean headersOnly;
>   attribute boolean deleteMailLeftOnServer;
>   attribute boolean authLogin;
>   attribute boolean dotFix;
>Index: local/src/nsPop3IncomingServer.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/mailnews/local/src/nsPop3IncomingServer.cpp,v
>retrieving revision 1.75
>diff -u -r1.75 nsPop3IncomingServer.cpp
>--- local/src/nsPop3IncomingServer.cpp	18 May 2004 14:53:26 -0000	1.75
>+++ local/src/nsPop3IncomingServer.cpp	1 Jun 2004 04:19:43 -0000
>@@ -106,6 +106,10 @@
>                         "leave_on_server")
> 
> NS_IMPL_SERVERPREF_BOOL(nsPop3IncomingServer,
>+                        HeadersOnly,
>+                        "headers_only")
>+
>+NS_IMPL_SERVERPREF_BOOL(nsPop3IncomingServer,
>                         DeleteMailLeftOnServer,
>                         "delete_mail_left_on_server")
> 
>Index: local/src/nsPop3Protocol.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/mailnews/local/src/nsPop3Protocol.cpp,v
>retrieving revision 1.218
>diff -u -r1.218 nsPop3Protocol.cpp
>--- local/src/nsPop3Protocol.cpp	30 May 2004 00:08:28 -0000	1.218
>+++ local/src/nsPop3Protocol.cpp	1 Jun 2004 04:19:44 -0000
>@@ -798,6 +798,7 @@
>     // size limit
>     
>     m_pop3Server->GetLeaveMessagesOnServer(&m_pop3ConData->leave_on_server);
>+    m_pop3Server->GetHeadersOnly(&m_pop3ConData->headers_only);
>     PRBool limitMessageSize = PR_FALSE;
> 
>     nsCOMPtr<nsIMsgIncomingServer> server = do_QueryInterface(m_pop3Server);
>@@ -2636,10 +2637,12 @@
>         {
>           m_pop3ConData->next_state = POP3_GET_MSG;
>         }
>-        else if ((c != TOO_BIG) && (m_pop3ConData->size_limit > 0) &&
>-          (info->size > m_pop3ConData->size_limit) && 
>+        else if ((c != TOO_BIG) &&
>           (TestCapFlag(POP3_TOP_UNDEFINED | POP3_HAS_TOP)) &&
>-          !m_pop3ConData->only_uidl) 
>+	  (m_pop3ConData->headers_only ||
>+	  ((m_pop3ConData->size_limit > 0) &&
>+          (info->size > m_pop3ConData->size_limit) && 
>+          !m_pop3ConData->only_uidl))) 
>         { 
>           /* message is too big */
>           m_pop3ConData->truncating_cur_msg = PR_TRUE;
>@@ -2694,8 +2697,9 @@
>  */
> PRInt32 nsPop3Protocol::SendTop()
> {
>-   char * cmd = PR_smprintf( "TOP %ld 20" CRLF,
>-     m_pop3ConData->msg_info[m_pop3ConData->last_accessed_msg].msgnum);
>+   char * cmd = PR_smprintf( "TOP %ld %d" CRLF,
>+     m_pop3ConData->msg_info[m_pop3ConData->last_accessed_msg].msgnum,
>+     m_pop3ConData->headers_only ? 0 : 20);
>    PRInt32 status = -1;
>    if (cmd)
>    {
>Index: local/src/nsPop3Protocol.h
>===================================================================
>RCS file: /cvsroot/mozilla/mailnews/local/src/nsPop3Protocol.h,v
>retrieving revision 1.66
>diff -u -r1.66 nsPop3Protocol.h
>--- local/src/nsPop3Protocol.h	8 May 2004 15:35:50 -0000	1.66
>+++ local/src/nsPop3Protocol.h	1 Jun 2004 04:19:44 -0000
>@@ -199,6 +199,8 @@
> typedef struct _Pop3ConData {
>     PRBool leave_on_server;     /* Whether we're supposed to leave messages
>                                    on server. */
>+    PRBool headers_only;        /* Whether to just fetch headers on initial
>+                                   downloads. */
>     PRInt32 size_limit;         /* Leave messages bigger than this on the
>                                    server and only download a partial
>                                    message. */

Comment 20

[Ray]

Attachment: Screenshot from "The Bat!" as an example

This is an example of the "Mailbox-Inspector" (Postfach-Inspektor) from the
Mail-Client "The Bat!". There you can decide to delete, read, or/and download
the mails. In the account settings it is possible to choose the number of lines
which will be shown in the "inspector". Additionally the size of a mail can be
limited up to the download starts automatically. Mails with a higher size will
be shown before downloading.

Comment 21

[Mike Cowperthwaite]

Howard Chu, to move forward with that patch you'll need to request a review.

Comment 22

[David :Bienvenu]

thx for working on this. Yes, you'll need reviews, but I'm not sure what this
patch does at this point - if you select this option, how do you actually
download the messages? That part isn't in this patch, is it? I assume mail
filters and spam filters run after the headers are downloaded, which is what we
want, right?

Comment 23

[Howard Chu]

(In reply to comment #22)
> thx for working on this. Yes, you'll need reviews, but I'm not sure what this
> patch does at this point - if you select this option, how do you actually
> download the messages? That part isn't in this patch, is it? I assume mail
> filters and spam filters run after the headers are downloaded, which is what we
> want, right?

OK. I'm not ready to have this reviewed yet, still trying to make it more usable.

Right, the headers are downloaded, and then filters are run on those headers.
Currently, the only way to download the full message is to click the URL that is
generated for you in the "This message was Truncated" notice.

I'm now looking into using the MSG_FLAG_OFFLINE bit and associated methods to
handle downloading multiple messages. I will attach a new patch when that's
done, and then request a review.

Comment 24

[David :Bienvenu]

I'm not sure using the offline flag would be helpful - you've already got the
MSG_FLAG_PARTIAL flag indicating that the message hasn't been downloaded.

I think your idea of being able to select a bunch of messages and say "download
non-downloaded messages" is a reasonable one. It would just pull out the partial
ones, and then download just those messages. There might also be a command that
says "download all partial messages in this folder"

Comment 25

[Howard Chu]

Attachment: More comprehensive patch

This patch extends the previous one:
  added a pop3 state "REFETCH" that means a Partial message should be retrieved
in full.
  added a Filter action that can mark a message with REFETCH status. (but the
corresponding UI changes aren't present. Should be trivial to add.)
  added a DownloadMessagesForOffline method to mark selected messages with
REFETCH status. This can be invoked using the File/Offline/Get Flagged,Get
Selected Messages commands.

These actions only set the REFETCH flag on the selected messages. You still
have to hit "Get Msgs" (or let the periodic poll get it) to actually retrieve
the messages.

This patch has a big problem - when the full message is retrieved, the original
stub is still present in the local mailbox. I don't understand why it's not
getting deleted/overwritten, and could use help finishing this off.

Comment 26

[Howard Chu]

(In reply to comment #25)

> This patch has a big problem - when the full message is retrieved, the original
> stub is still present in the local mailbox. I don't understand why it's not
> getting deleted/overwritten, and could use help finishing this off.

OK, I see what the deal is. I now have it deleting the stubs, so it's pretty
close to what I want. The big remaining issue is just about doing things in the
right order; the pop3stat info gets deleted before the message is downloaded and
that makes me nervous...

Comment 27

[David :Bienvenu]

I think marking the messages for refetch and retrieving them should be one
action - why make them two? It's just a few more lines of code to run get new
mail after marking the messages for refetch. Yes, we'll also get new mail, but
we could probably even fix that.

I'm torn about whether overloading file | offline | get selected/flagged
messages is the way to go - but I think it's probably OK since in essence, we're
making pop3 behave more like IMAP and news.

Comment 28

[Howard Chu]

Attachment: Working, ready for review

After wandering thru a few rat holes, the final solution was simpler than I
thought. This patch works pretty well for me. Let me know if anything needs to
be cleared up / cleaned up...

Comment 29

[Christian Eyrich]

Howard, I'm not sure this is what most people want. The feature that e.g YAM,
The Bat and many external tools like Magic Mail Monitor offer is somewhat different.

Download only the mail header and insert it in the normal thread pane is more
easy to code but has some limitations.
1. Mails with strange dates (e.g. Spam) will go somewhere in the list, at the
bottom of the downloaded mails, between your x thousands a.s.o. It's expensive
for the user to get them to decide.
2. Which one of the mails in the list is truncated and which one is regularly
downloaded? Which one is truncated because of size and which one has only
haeders? I can't distinguish them from the list.
3. When should such a "header-"mail get deleted? Is it really wanted that it get
deleted on the server (i.e. "d" flag in the popstate.dat set) when deleted on
the client? I as user wouldn't expect this - not all mails I don't wanna keep
here should get deleted automatically on the server (especially not when "Leave
messages on server" is selected).

Yes, I know that at least the summary only talks about fetching only mail
headers. This is not what the screenshots show. But IMHO the implementation from
the patch would help only few people but cumber/annoy more if used.

Comment 30

[David :Bienvenu]

Howard, thx a lot for working on this.

>Download only the mail header and insert it in the normal thread pane is more
>easy to code but has some limitations.
If you sort by order received, then newly downloaded message bodies will go at
the end (or top, if reverse sorted), because the old hdr-only msg in the local
folder is deleted and the new message put at the end of the local folder (not
much choice here). But if you're in any other sort, the message should stay were
it is.

>2. Which one of the mails in the list is truncated and which one is regularly
>downloaded? Which one is truncated because of size and which one has only
>headers? I can't distinguish them from the list.

It looks to me like Howard has made it so downloaded messages are marked
offline, so we can distinguish downloaded from not downloaded in the UI, though
I'm not sure from the code if that's for all messages or what. And it's true you
can't tell partial download from header downloaded, but I'm not sure that's an
important distinction. In fact, if you have header download only set, will you
ever get partial messages? Howard, what happens when you have the msg size limit
set, and then mark a large message for downloading, and then download messages?
Do we only do a partial download of the large message? That would be a pain, I
think, because then you'd have to redownload the message a 3rd time.

> When should such a "header-"mail get deleted? Is it really wanted that it get
>deleted on the server (i.e. "d" flag in the popstate.dat set) when deleted on
> the client?

That's a really good question. What do other clients do? My first thought was
that this should follow the normal delete from server when deleted locally pref
- whether the msg has been downloaded or not is not relevant. But, if you don't
have delete from server when deleted locally set, and you have leave on server
set, you can't cause these messages that you really want to throw away to be
deleted from the server. But, that's true for all messages, so I think my first
thought is correct. This could be solved by adding a "delete from server
command" that would work for any message, orthogonal to this feature.

A few comments about the patch. code-wise, I'm impressed. Is this really your
first patch? 

"Refetch" is not a descriptive enough var name for what's going on - I'd call it
it fetchMsgBody so it's clear what's really going on - all the vars and UI with
Refetch should be changed, I think.


+      case nsMsgFilterAction::RefetchFromPop3Server:
+        {

the filter action just sets a flag that the message body should be fetched on
the next get new mail, right? Or will it be fetched in the current get new mail
session, like the delete action? And, presumably this filter will fire again -
do you guard against the repeated fetching of the message body?

here, just use strdup, I think. Also, return NS_ERROR_OUT_OF_MEMORY if strdup or
PR_NewZap fails.

+  if (ue)
+  {
+    ue->uidl = PL_strdup(aUidl);
+    if (ue->uidl)
+    {
+      ue->status = (aMark == POP3_DELETE) ? DELETE_CHAR :
+  	(aMark == POP3_REFETCH) ? REFETCH : KEEP;
+      m_uidlsToMark.AppendElement(ue);
+    } else
+    {
+      PR_Free(ue);
+    }
+  }

+  NS_IMETHOD DownloadMessagesForOffline(nsISupportsArray *messages,
nsIMsgWindow *window);

these arg names should be aMessages and aWindow - for NS_IMETHODs,  in both the
decl and impl, the first char of an arg name is a, to indicate arg.

nit:

+  Pop3UidlEntry *ue;
+
+  ue = PR_NEWZAP(Pop3UidlEntry);

could you call this var uidlEntry instead of ue? It makes the code easier to
read if you don't use acronyms for vars. It's a little more typing, but it's
easier to read for people not that familiar with the code.

I haven't finished digesting the code, but that's enough for now.

Comment 31

[Christian Eyrich]

> If you sort by order received

Yes, sure. But should one resort the list after every retrieval? But ok, maybe
I'm the only one not sorting mails by order received, I sort by date.

> It looks to me like Howard has made it so downloaded messages are marked offline

Hu? I've to look again.
No, sorry, I can't see any difference between complete and not complete messages.

> What do other clients do?

I only know this feature from special pop monitor software and The Bat. They use
an extra window to display all messages on the server and you can directly
choose "delete on server" and "Download". So you can't muddle it up with normal
mails.
It's more like a window to view at the server there and manage the mails
directly and that's clear. If a message appears in Mozilla/TB I don't expect to
manipulate the message on the server if I don't explicitly say so.

> This could be solved by adding a "delete from server command" that would work for
> any message, orthogonal to this feature.

Yep, this is what bug 47297 is about.
The patch there should be useable without big modifications. "Delete from
server" should also delete the local placeholder.
Problem: how to differentiate a headermail (to be deleted if only the header is
here) from a truncated message (e.g. at 50kb, not to be deleted I think)?

And what's with header-only messages in the list that aren't on the server
anymore (e.g. deleted by another client)? Is it good to keep them until one
tries to fetch the rest (it then disappears from the list, but not from the mail
window which can be irritating too)?


Another remarks. It would be great to see the size on the server. The
information is available from the LIST answer (mostly accurate), but how to get
it into the size column?

Also the message displayed in the header message should be changed. "message
exceeded the Maximum Message Size set in Preferences" could be irritating since
people will think of the disk space pref.

Comment 32

[David :Bienvenu]

>No, sorry, I can't see any difference between complete and not complete messages.
I'm just guessing because he sets the offline flag on the message. I'm not sure
if that has an effect on a local message thread pane or not. I would have
thought it would.

>Problem: how to differentiate a headermail (to be deleted if only the header is
>here) from a truncated message (e.g. at 50kb, not to be deleted I think)?
If you delete it from the server, and it's not completely downloaded, I think
you would delete it from the client, in either case. A partial message is of
little use. You could warn the user, with one of those "don't warn me again"
checkboxes, when they try to delete from server a partially downloaded e-mail.

>What's with header-only messages in the list that aren't on the server
>anymore (e.g. deleted by another client)? Is it good to keep them until one
>tries to fetch the rest (it then disappears from the list, but not from the
>mail window which can be irritating too)?

In the IMAP model, they'd be deleted when we first found out they weren't on the
server. We realize that when we get new messages the first time around, right?

> It would be great to see the size on the server. The
>information is available from the LIST answer (mostly accurate), but how to get
>it into the size column?

I agree it would be cool to display the size. To do this, we'd need to do what
we do for offline imap/news messages - have a message size (the size on the
server), and an offline size (the size of what we've downloaded). We'd need to
make sure the code that manipulates the message locally (display, copy...) uses
the offline size.

Comment 33

[Christian Eyrich]

> A partial message is of little use.

Ah sorry, I should have tested it before. Using the size restriction feature
we're only fetching the first 20 line of the body. I thought up to max kbytes.
That would have made it possible that the message itself is downloaded (and want
to be kept) but not an imaginable attachment.

> In the IMAP model, they'd be deleted when we first found out they weren't on
> the server. We realize that when we get new messages the first time around,
> right?

I'd follow this IMAP model. We could realize that the message isn't on the
server anymore by comparing the available uidl's with the ones from popstate.

Comment 34

[Howard Chu]

(In reply to comment #30)
> Howard, thx a lot for working on this.

My pleasure. This feature was the main thing preventing me from dumping Outlook,
so I figured it was time to kill that reason...

> >2. Which one of the mails in the list is truncated and which one is regularly
> >downloaded? Which one is truncated because of size and which one has only
> >headers? I can't distinguish them from the list.
> 
> It looks to me like Howard has made it so downloaded messages are marked
> offline, so we can distinguish downloaded from not downloaded in the UI, though
> I'm not sure from the code if that's for all messages or what.

Right, all full messages will have the OFFLINE flag set. The UI doesn't seem to
do anything special with them at the moment. I think that's OK, it would be more
useful to have a new icon for the partial messages.

> And it's true you
> can't tell partial download from header downloaded, but I'm not sure that's an
> important distinction. In fact, if you have header download only set, will you
> ever get partial messages? Howard, what happens when you have the msg size limit
> set, and then mark a large message for downloading, and then download messages?
> Do we only do a partial download of the large message? That would be a pain, I
> think, because then you'd have to redownload the message a 3rd time.

No. When you mark a message for downloading, it does it unconditionally,
bypassing the size limit check. The header download option makes the size limit
useless, so you will only have headers, never partial messages.
> 
> > When should such a "header-"mail get deleted? Is it really wanted that it get
> >deleted on the server (i.e. "d" flag in the popstate.dat set) when deleted on
> > the client?
> 
> That's a really good question. What do other clients do? My first thought was
> that this should follow the normal delete from server when deleted locally pref
> - whether the msg has been downloaded or not is not relevant. But, if you don't
> have delete from server when deleted locally set, and you have leave on server
> set, you can't cause these messages that you really want to throw away to be
> deleted from the server. But, that's true for all messages, so I think my first
> thought is correct.

That sounds right to me.

> This could be solved by adding a "delete from server
> command" that would work for any message, orthogonal to this feature.
> 
> A few comments about the patch. code-wise, I'm impressed. Is this really your
> first patch? 

Yep. May 31 is the first time I ever looked at this code, it took a lot of
reading to grok enough to get the final solution...

> "Refetch" is not a descriptive enough var name for what's going on - I'd call it
> it fetchMsgBody so it's clear what's really going on - all the vars and UI with
> Refetch should be changed, I think.
> 
> 
> +      case nsMsgFilterAction::RefetchFromPop3Server:
> +        {

OK. 'fetchMsgBodyFromPop3Server' then?
 
> the filter action just sets a flag that the message body should be fetched on
> the next get new mail, right? Or will it be fetched in the current get new mail
> session, like the delete action? And, presumably this filter will fire again -
> do you guard against the repeated fetching of the message body?

Hm... I haven't considered this because I have LeaveOnServer off. But the Delete
and Fetch actions should both behave the same, since the same Marking routine
does both.

> here, just use strdup, I think. Also, return NS_ERROR_OUT_OF_MEMORY if strdup or
> PR_NewZap fails.

OK.
> 
> +  if (ue)
> +  {
> +    ue->uidl = PL_strdup(aUidl);
> +    if (ue->uidl)
> +    {
> +      ue->status = (aMark == POP3_DELETE) ? DELETE_CHAR :
> +  	(aMark == POP3_REFETCH) ? REFETCH : KEEP;
> +      m_uidlsToMark.AppendElement(ue);
> +    } else
> +    {
> +      PR_Free(ue);
> +    }
> +  }
> 
> +  NS_IMETHOD DownloadMessagesForOffline(nsISupportsArray *messages,
> nsIMsgWindow *window);
> 
> these arg names should be aMessages and aWindow - for NS_IMETHODs,  in both the
> decl and impl, the first char of an arg name is a, to indicate arg.
> 
> nit:
> 
> +  Pop3UidlEntry *ue;
> +
> +  ue = PR_NEWZAP(Pop3UidlEntry);
> 
> could you call this var uidlEntry instead of ue? It makes the code easier to
> read if you don't use acronyms for vars. It's a little more typing, but it's
> easier to read for people not that familiar with the code.

OK.

> I haven't finished digesting the code, but that's enough for now.

Comment 35

[Christian Eyrich]

> the filter action just sets a flag that the message body should be fetched on
> the next get new mail, right? Or will it be fetched in the current get new mail
> session, like the delete action?

It needs another get mail to fetch the messages marked for refetch.
I don't think that's good, it would be better if it could run in one session.
Either by starting another mail get by some high level function or better in one
go in the protocol handler.

Comment 36

[David :Bienvenu]

>Right, all full messages will have the OFFLINE flag set. The UI doesn't seem to
>do anything special with them at the moment. I think that's OK, it would be 
>more useful to have a new icon for the partial messages.

You're setting the OFFLINE flag, but I'm pretty sure I'm turning it off later,
for local messages, and that's why the UI doesn't reflect it. I believe the
reason has to do with this code:

http://lxr.mozilla.org/seamonkey/source/mailnews/db/msgdb/src/nsMsgHdr.cpp#591

which returns the message file offset into the local store of a mail message,
for local mail, and offline imap and news.  For local folders, the offset is the
same as the msg key, and I didn't want to have to write an extra field
duplicating that information into the db for local folders. But, I can probably
remove the code that clears the offline flag for local messages, and switch the
linked code above to look at the folder type to determine how to get the local
offset.

fetchMsgBodyFromPop3Server is fine, and the other var names and #defines/types
should change accordingly.

>Yep. May 31 is the first time I ever looked at this code, it took a lot of
>reading to grok enough to get the final solution...

that's pretty quick!

Re Christian's comment, thx for checking. I agree it should be fetched when the
get new mail process is finished. I think it would be fairly easy to set a flag
somewhere that we'd marked a message for download, or even just iterate over the
pop state when finished get new mail, and if there are still messages marked for
fetching, just fetch them. I kinda like doing it in the pop3 protocol handler,
like Christian suggests.

Comment 37

[Howard Chu]

(In reply to comment #36)
> >Right, all full messages will have the OFFLINE flag set. The UI doesn't seem to
> >do anything special with them at the moment. I think that's OK, it would be 
> >more useful to have a new icon for the partial messages.
> 
> You're setting the OFFLINE flag, but I'm pretty sure I'm turning it off later,
> for local messages, and that's why the UI doesn't reflect it. I believe the
> reason has to do with this code:
> 
> http://lxr.mozilla.org/seamonkey/source/mailnews/db/msgdb/src/nsMsgHdr.cpp#591
> 
> which returns the message file offset into the local store of a mail message,
> for local mail, and offline imap and news.  For local folders, the offset is the
> same as the msg key, and I didn't want to have to write an extra field
> duplicating that information into the db for local folders. But, I can probably
> remove the code that clears the offline flag for local messages, and switch the
> linked code above to look at the folder type to determine how to get the local
> offset.

Ah... That explains that problem. My code for
nsMsgLocalMailFolder::DownloadMessagesForOffline assumes that the incoming array
of messages are all Partial messages. I should add a check for the PARTIAL flag,
otherwise if you selected a regular message, it will get deleted during this pass.

> fetchMsgBodyFromPop3Server is fine, and the other var names and #defines/types
> should change accordingly.

OK.

> >Yep. May 31 is the first time I ever looked at this code, it took a lot of
> >reading to grok enough to get the final solution...
> 
> that's pretty quick!

I don't have much time to drag out on this, OpenLDAP is my main project...

> Re Christian's comment, thx for checking. I agree it should be fetched when the
> get new mail process is finished. I think it would be fairly easy to set a flag
> somewhere that we'd marked a message for download, or even just iterate over the
> pop state when finished get new mail, and if there are still messages marked for
> fetching, just fetch them. I kinda like doing it in the pop3 protocol handler,
> like Christian suggests.

Hm... I don't know enough to make that work. Looping back over the pop state is
OK, but deleting the stubs is dicy. I got corrupted message folders all the
other times I tried to do that inline with retrieval.

Comment 38

[Howard Chu]

(In reply to comment #36)

> You're setting the OFFLINE flag, but I'm pretty sure I'm turning it off later,
> for local messages, and that's why the UI doesn't reflect it.

It looks to me like there's no reason to mess with the OFFLINE flag then. I'm
dropping that from my revised patch. It would still be nice for the UI to show
something different for PARTIAL messages though.

Comment 39

[Howard Chu]

Attachment: minor cleanup of previous

This is pretty much the same as the previous, with a couple of the changes that
David Bienvenu suggested.

Comment 40

[Howard Chu]

Attachment: More UI tweaks

This patch adds an onlineSize property for recording the size of the message on
the server so it can be displayed for partial messages. It also sets the
Offline property when displaying a fully downloaded local message.

Comment 41

[Howard Chu]

Attachment: threadpane context menu

Added the Download commands to the threadpane context menu. It's a bit more
convenient to get to this way. It probably ought to try to hide itself when
"not applicable" but that seemed like too much trouble so I'm just using it
like this.

Comment 42

[Howard Chu]

Attachment: Partial msg verbiage

This is an additional patch to provide alternate, less-startling text when
displaying a message stub.

Comment 43

[Howard Chu]

Attachment: Partial msg verbiage, again

The previous patch didn't take into account the linebreak length....

Comment 44

[Mike Cowperthwaite]

How does this change interact with POP3's ability to filter by body content, or 
with Junk Mail Controls (which require the message body)?  Are filters run when 
the full message is downloaded, or only during the initial, download-headers 
phase?  If the latter, what happens when a filter defined to act on the body is 
executed against the headers-only message?

IMAP doesn't have filter-on-body; it is subject to Junk Controls, but when 
that's active, the message body is downloaded anyway.

Comment 45

[Howard Chu]

(In reply to comment #44)
> How does this change interact with POP3's ability to filter by body content, or 
> with Junk Mail Controls (which require the message body)?  Are filters run when 
> the full message is downloaded, or only during the initial, download-headers 
> phase?  If the latter, what happens when a filter defined to act on the body is 
> executed against the headers-only message?

The filters are run for the full message as well as for the headers-only
message. As for the Junk Mail Controls, Mozilla has been happily marking things
as Junk for me, even with only the headers available. (And it's still pretty
easy to manually junk the things it misses.)

Comment 46

[Howard Chu]

Attachment: All together again

The whole patch again. This version fixes the Fetch filter action so that it
occurs immediately during a given download session, instead of needing a
separate GetNewMail invocation. It also includes the UI patches that were
listed separately above, just for completeness' sake.

Comment 47

[Howard Chu]

Summary...

>Comment #29 From Christian Eyrich  2004-06-03 07:00 PDT

>1. Mails with strange dates (e.g. Spam) will go somewhere in the list, at the
>bottom of the downloaded mails, between your x thousands a.s.o. It's expensive
>for the user to get them to decide.

I guess so. Personally I think it would be more cumbersome to switch back and
forth from a "Remote POP Window" and the main thread/message pane.

>2. Which one of the mails in the list is truncated and which one is regularly
>downloaded? Which one is truncated because of size and which one has only
>haeders? I can't distinguish them from the list.

Full messages now have the Offline property in the UI, and partial messages don't.

>3. When should such a "header-"mail get deleted? Is it really wanted that it get
>deleted on the server (i.e. "d" flag in the popstate.dat set) when deleted on
>the client? I as user wouldn't expect this - not all mails I don't wanna keep
>here should get deleted automatically on the server (especially not when "Leave
>messages on server" is selected).

That depends on the Leave Messages on Server | Until I Delete or Move Them
preference. If you don't select that, it won't be deleted.

>Yes, I know that at least the summary only talks about fetching only mail
>headers. This is not what the screenshots show. But IMHO the implementation
>from the patch would help only few people but cumber/annoy more if used.

I think this is more streamlined, really.

>Comment #31 From Christian Eyrich  2004-06-03 10:20 PDT

>And what's with header-only messages in the list that aren't on the server
>anymore (e.g. deleted by another client)? Is it good to keep them until one
>tries to fetch the rest (it then disappears from the list, but not from the
>mail window which can be irritating too)?

If these headers are selected/flagged for a Download operation, they will be
removed and will disappear from the mail window.

>Another remarks. It would be great to see the size on the server. The
>information is available from the LIST answer (mostly accurate), but how to get
>it into the size column?

The current patch accomplishes this.

>Also the message displayed in the header message should be changed. "message
>exceeded the Maximum Message Size set in Preferences" could be irritating since
>people will think of the disk space pref.

The current patch has new text for this situation.

>Comment #35 From Christian Eyrich  2004-06-03 13:11 PDT

> the filter action just sets a flag that the message body should be fetched on
>> the next get new mail, right? Or will it be fetched in the current get new
>> mail session, like the delete action?

>It needs another get mail to fetch the messages marked for refetch.
>I don't think that's good, it would be better if it could run in one session.
>Either by starting another mail get by some high level function or better in
>one go in the protocol handler.

The current patch accomplishes this.

I'd like to request a review for the current patch.

Comment 48

[David :Bienvenu]

Comment on attachment 150094 [details] [diff] [review]
All together again

r=bienvenu, with this nit:

+      case nsMsgFilterAction::FetchBodyFromPop3Server:
+	 {
+	  PRUint32 flags = 0;
+	   nsCOMPtr <nsIMsgFolder> downloadFolder;
+	   msgHdr->GetFolder(getter_AddRefs(downloadFolder));
+	   nsCOMPtr <nsIMsgLocalMailFolder> localFolder =
do_QueryInterface(downloadFolder);
+	  msgHdr->GetFlags(&flags);
+	   if (localFolder && (flags & MSG_FLAG_PARTIAL))
+	   {
+	     nsCOMPtr<nsISupportsArray> messages;
+	     rv = NS_NewISupportsArray(getter_AddRefs(messages));
+	     NS_ENSURE_SUCCESS(rv, rv);
+	     nsCOMPtr<nsISupports> iSupports = do_QueryInterface(msgHdr);
+	     messages->AppendElement(iSupports);
+	     localFolder->MarkMsgsOnPop3Server(messages, POP3_FETCH_BODY);
+	     m_msgMovedByFilter = PR_TRUE;

the indentation is a little funny here. I can fix it in my tree before checking
in, I guess.

A comment here about why you're setting m_msgMovedByFilter = PR_TRUE; - I
assume this is so we'll truncate the partial msg from the local store and not
add the hdr to the db...

Comment 49

[Christian Eyrich]

> Personally I think it would be more cumbersome to switch back and forth from a
> "Remote POP Window" and the main thread/message pane.

Switching back and forth? You hit Get Mail, a window showing all messages on the
server opens, you can select which to delete on the server, which to download
(the rest stays on the server, undownloaded) and hit OK, the window closes and
the selected options are executed. What's cumbersome with this?

> Full messages now have the Offline property in the UI, and partial messages
> don't.

And does the UI show different icons for full/partial messages now? I still
can't distinguish them.

> That depends on the Leave Messages on Server | Until I Delete or Move Them
> preference. If you don't select that, it won't be deleted.

I see another behaviour. If I delete a headermail, it get marked with the d flag
and deleted on the next fetch - regardless if "Leave Messages on Server" is
checked or not and if checked, if with or without "Until I Delete or Move Them".
And even if it would work as you propose, what should one do to remove the
headermail from the maillist without deleting it on the server if "Leave
Messages on Server" | "Until I Delete or Move Them" is checked? Uncheck, delete,
check again?


One more problem. Normally, a Mail Get with "Leave Messages on Server" downloads
the mail but doesn't delete it. If the option is switched off then, the mail
doesn't get downloaded but deleted on the next fetch.
But now the mail doesn't get deleted anymore:
1. Get a mail with headers only
2. Fetch the full mail with "Leave Messages on Server" activated
3. Switch off "Leave Messages on Server"
4. Get Mail

Result: The mail won't get deleted on the server.
I think the problem is, that the b flag in the popstate persists even if the
full mail is fetched through the link while it should become k then.

> If these headers are selected/flagged for a Download operation, they will be
> removed and will disappear from the mail window.

Yes, but only then. Why not if a subsequent mail fetch notices that the message
isn't on the server anymore?

Comment 50

[Howard Chu]

(In reply to comment #49)
> Switching back and forth? You hit Get Mail, a window showing all messages on the
> server opens, you can select which to delete on the server, which to download
> (the rest stays on the server, undownloaded) and hit OK, the window closes and
> the selected options are executed. What's cumbersome with this?

Is that what Imap does? I know it's not what News does. It seems to me that
introducing a new UI just for POP is not a good idea, just creates more work for
not much benefit. You have to duplicate a lot of functionality from other
modules to accomplish this, and the message selection process will be totally
manual. Or, you have to create a new "TemporaryFolder" entity that can store the
headers, to give the Filters some place to find them. I'm not interested in
doing that much work, but maybe someone else is.

> > Full messages now have the Offline property in the UI, and partial messages
> > don't.
> 
> And does the UI show different icons for full/partial messages now? I still
> can't distinguish them.

In both the Classic and Modern skins, offline messages have a darker icon. I
don't know about any other skins.

> > That depends on the Leave Messages on Server | Until I Delete or Move Them
> > preference. If you don't select that, it won't be deleted.
 
> I see another behaviour. If I delete a headermail, it get marked with the d flag
> and deleted on the next fetch - regardless if "Leave Messages on Server" is
> checked or not and if checked, if with or without "Until I Delete or Move Them".

I'll look into this.

> And even if it would work as you propose, what should one do to remove the
> headermail from the maillist without deleting it on the server if "Leave
> Messages on Server" | "Until I Delete or Move Them" is checked? Uncheck, delete,
> check again?

This makes no sense. Why should the header have special treatment different from
full messages? If you select "Until I Delete or Move Them" then that should mean
just that, you don't care about the message and you want it to be Gone.

Perhaps you're arguing for separate actions "Delete locally only" or "Delete
from server only" but that's a general question orthogonal to whether a message
is a full message or just a header. As such, I don't feel the need to answer
this question in this patch. 

> One more problem. Normally, a Mail Get with "Leave Messages on Server" downloads
> the mail but doesn't delete it. If the option is switched off then, the mail
> doesn't get downloaded but deleted on the next fetch.
> But now the mail doesn't get deleted anymore:
> 1. Get a mail with headers only
> 2. Fetch the full mail with "Leave Messages on Server" activated
> 3. Switch off "Leave Messages on Server"
> 4. Get Mail
> 
> Result: The mail won't get deleted on the server.
> I think the problem is, that the b flag in the popstate persists even if the
> full mail is fetched through the link while it should become k then.

I'll look into this.

> > If these headers are selected/flagged for a Download operation, they will be
> > removed and will disappear from the mail window.
> 
> Yes, but only then. Why not if a subsequent mail fetch notices that the 
> message isn't on the server anymore?

I see what you mean. This is hard to do, the header may not be in the current
folder. How would you find it so it can be removed?

Comment 51

[Howard Chu]

(In reply to comment #50)
> > I see another behaviour. If I delete a headermail, it get marked with the d flag
> > and deleted on the next fetch - regardless if "Leave Messages on Server" is
> > checked or not and if checked, if with or without "Until I Delete or Move Them".
> 
> I'll look into this.

I believe this bug existed prior to my changes; the code in
nsLocalMailFolder.cpp is ignoring the deleteMailLeftOnServer preference. It
retrieves the flag but never tests it. I can fix it easily enough, but maybe
that should be a separate bug report?

> > One more problem. Normally, a Mail Get with "Leave Messages on Server" downloads
> > the mail but doesn't delete it. If the option is switched off then, the mail
> > doesn't get downloaded but deleted on the next fetch.
> > But now the mail doesn't get deleted anymore:
> > 1. Get a mail with headers only
> > 2. Fetch the full mail with "Leave Messages on Server" activated
> > 3. Switch off "Leave Messages on Server"
> > 4. Get Mail
> > 
> > Result: The mail won't get deleted on the server.
> > I think the problem is, that the b flag in the popstate persists even if the
> > full mail is fetched through the link while it should become k then.

You're right. I think I changed something I didn't need to, testing now...

Comment 52

[Howard Chu]

Attachment: fixes for Delete...

>> > I see another behaviour. If I delete a headermail, it get marked with the
d flag
>> > and deleted on the next fetch - regardless if "Leave Messages on Server"
is
> > checked or not and if checked, if with or without "Until I Delete or Move
Them".

>I believe this bug existed prior to my changes; the code in
>nsLocalMailFolder.cpp is ignoring the deleteMailLeftOnServer preference. It
>retrieves the flag but never tests it. I can fix it easily enough, but maybe
>that should be a separate bug report?

This bug existed before, but I have fixed it in this patch. The main change is
in nsLocalMailFolder.cpp MarkMsgsOnPop3Server, with a flag change in
nsarseMailbox.cpp to allow the Filter action DeleteFromPop3Server to keep
working.

> > One more problem. Normally, a Mail Get with "Leave Messages on Server"
downloads
> > the mail but doesn't delete it. If the option is switched off then, the
mail
> > doesn't get downloaded but deleted on the next fetch.
> > But now the mail doesn't get deleted anymore:
> > 1. Get a mail with headers only
> > 2. Fetch the full mail with "Leave Messages on Server" activated
> > 3. Switch off "Leave Messages on Server"
> > 4. Get Mail
> > 
> > Result: The mail won't get deleted on the server.
> > I think the problem is, that the b flag in the popstate persists even if
the
> > full mail is fetched through the link while it should become k then.

This bug also existed before my patch; if you had a partial message (due to
size limit) and used the URL in the "Truncated!" display to retrieve it, the
popstate was not updated. These bugs are fixed in the current patch.

Comment 53

[Christian Eyrich]

> Is that what Imap does? I know it's not what News does. It seems to me that
introducing a new UI
> just for POP is not a good idea, just creates more work for not much benefit.

No, that's not what IMAP does. But POP isn't like IMAP, it's an own protocol
that requires own functionality to be really convenient.
But yes, I know that it would need much work and code. And I can live with your
solution cause I don't need it on my own and can just ignore it. But I know what
other users are using right now and what advantages that solutions have.

> In both the Classic and Modern skins, offline messages have a darker icon. I
> don't know about any other skins.

Oh, I currently only build TB and yes, I just verified that Qute doesn't have
that icon.


>> Why not if a subsequent mail fetch notices that the message isn't on the
server anymore?
>>
>> I see what you mean. This is hard to do, the header may not be in the current
>> folder. How would you find it so it can be removed?

Getting the X-UIDL of the message should be quite easy to do by comparing the
popstate with the UIDL list of the current fetch. The hard part is to find the
message with this X-UIDL header in the inbox.

> This makes no sense. Why should the header have special treatment different from
> full messages?

Well, maybe it's just a different concept in our heads. Coming from the already
cited POP managers, I see the headermails only as a view for what's on the
server. So deleting a headermail I only remove the view to that mail, not the
mail itself. You implemented it as an interface.


> This bug existed before, but I have fixed it in this patch.

Hu? I never saw messages getting deleted if "Leave Messages on Server" was
checked. And before your patch it was impossible to have the message on the
server without this option active.
But ok, it works now, thanks.


> This bug also existed before my patch; if you had a partial message (due to
> size limit)

Ah ok, I never used this feature and forgot to test it after running in the
problem now. Great you fixed it though.


I don't know what we're currently doing with impossible scenarios. But wouldn't
it be nice to disable the filter action "Fetch body from POP server" when
filtering by body?

Comment 54

[Howard Chu]

(In reply to comment #53)
> >> Why not if a subsequent mail fetch notices that the message isn't on the
> server anymore?
> >>
> >> I see what you mean. This is hard to do, the header may not be in the current
> >> folder. How would you find it so it can be removed?
> 
> Getting the X-UIDL of the message should be quite easy to do by comparing the
> popstate with the UIDL list of the current fetch. The hard part is to find the
> message with this X-UIDL header in the inbox.

Right, since the UIDL isn't one of the searchable columns of the msgDB, it's a
pain. In one version of my patch I added the Message-ID to the popstate file
since that is a searchable field, but I got rid of that because I didn't need it
after all. We can certainly add that again to cleanup the Inbox. But this turns
into more tradeoffs I don't want to think about - what if the message was moved
from the Inbox to another folder? Do we have to search all the user's folders to
find and delete it?

> Well, maybe it's just a different concept in our heads. Coming from the already
> cited POP managers, I see the headermails only as a view for what's on the
> server. So deleting a headermail I only remove the view to that mail, not the
> mail itself. You implemented it as an interface.

I see what you're saying. As you can see, the patch is fairly minimal. I started
from the "Don't download msgs larger than X kbytes" functionality and just
extended it. As such, what I've implemented is just a minor tweak of what the
client could already do. Maybe that limits my perception by only looking at it
from what is already there, but it also limited the amount of damage I needed to
do to the existing source to get what I want.

> > This bug existed before, but I have fixed it in this patch.
 
> Hu? I never saw messages getting deleted if "Leave Messages on Server" was
> checked. And before your patch it was impossible to have the message on the
> server without this option active.
> But ok, it works now, thanks.

Bug #245098, according to the CVS history for nsLocalMailFolder.cpp.

> I don't know what we're currently doing with impossible scenarios. But wouldn't
> it be nice to disable the filter action "Fetch body from POP server" when
> filtering by body?

The code for that filter will only act if the current message is marked with
MSG_FLAG_PARTIAL. So I think that answers your concern, but I'm not totally sure
I understand the question. Is "filter by body" a specific option?

Comment 55

[David :Bienvenu]

Comment on attachment 150113 [details] [diff] [review]
fixes for Delete...

r=bienvenu, requesting sr from mscott.

Comment 56

[David :Bienvenu]

Comment on attachment 150094 [details] [diff] [review]
All together again

clearing request on obsolete patch

Comment 57

[Scott MacGregor]

can you attach a patch with the appropriate UI changes (such as
mailWindowOverlay.xul) for thunderbird as well please?

Comment 58

[David :Bienvenu]

Howard, I can do that...

Comment 59

[David :Bienvenu]

the thunderbird changes will need to go into the offline extension, which is a
bit odd, but I don't see any other choice. This is a fairly advanced option, but
for correctness sake, we shouldn't allow thunderbird users to pick this pref if
they haven't got the thunderbird offline extension installed.

Comment 60

[David :Bienvenu]

the UI changes to mailwindowoverlay.xul only add a couple items to the thread
pane context menu that already exist in the file menu, so getting them working
for thunderbird shouldn't block this patch in general, while I figure out how to
add them to the thread pane with the offline extension.

Comment 61

[Scott MacGregor]

Comment on attachment 150113 [details] [diff] [review]
fixes for Delete...

can you guys make sure to link up any regressions or additional changes that
end up going in back up to this bug? That'll make it easy to take this change
on the aviary 1.0 branch after this has had some testing.

Comment 62

[Christian Eyrich]

Is anyone about to update the help files with this new feature?

Comment 63

[David :Bienvenu]

Attachment: fix a couple problems

Comment 64

[David :Bienvenu]

Attachment: same as before, except rev IID's on changed interfaces

this is what I'll check in.

Comment 65

[David :Bienvenu]

Attachment: cumulative fix

same as before, except that MarkMsgsOnPop3Server now will delete partial
messages from the server if deleted on the client, when leave on server and
delete from server are both false. Otherwise, they'd never get deleted if left
to the client on this machine. Also, remove the diff between 1.459 and 1.460
since it conflicts with the changes for this bug.

Comment 66

[Howard Chu]

Attachment: Help file changes

(In reply to comment #62)
>Is anyone about to update the help files with this new feature?

Here's a first stab at it. The text about Offline support in general is pretty
poor about pointing out that it generally is aimed at IMAP and News. I've tried
to make that clearer but it's still pretty awkward.

Comment 67

[Howard Chu]

Attachment: Cleaning up some loose ends

This patch builds on the previous (already committed) code to better
synchronize the inbox with the server:

  It adds a check of partial messages in the Inbox to see if they've been
deleted on the server. If so, the partial messages are deleted from the Inbox.

  The DownloadMessagesForOffline function has been tweaked to try to remember
the first partial message that was selected, and reselect its corresponding
full message after the download completes.

Comment 68

[Howard Chu]

Attachment: Better version of previous

Same idea as previous, but with various juggling to make sure that the old
msgDBHdrs get deleted right before the new one gets added, to keep the hash
tables pristine...

Comment 69

[Christian Eyrich]

Thank you Howard for your ongoing effort.

But I've a problem with the feature in the trunk and also with your latest patch.
The first is: I've "only headers" activated and a filter on the subject with the
action of "Delete from POP server". I expect the mails to be removed from the
server while retrieving them and *no* headermail appears in my Inbox.
Instead they appear in my Inbox and don't get deleted.

With your latest patch: The messages still appear in my Inbox and don't get
deleted from the server.
But no entries are added in the popstate.dat. So successive retrieves always
reget the messages.
And clicking on the link of one message it gets downloaded, but all other
disappear from my inbox while they're still on the server.

So it was not good but got weird now.

Comment 70

[Howard Chu]

(In reply to comment #69)
> But I've a problem with the feature in the trunk and also with your latest patch.
> The first is: I've "only headers" activated and a filter on the subject with the
> action of "Delete from POP server". I expect the mails to be removed from the
> server while retrieving them and *no* headermail appears in my Inbox.
> Instead they appear in my Inbox and don't get deleted.

I'll take a look at this later. Note that "Delete from POP server" was not
written to prevent the message from appearing in your Inbox. You need to add the
regular "Delete" action to the filter if you want that. But the fact that it
doesn't delete from the server, and doesn't record the msg in the popstate.dat
is definitely a problem...

> And clicking on the link of one message it gets downloaded, but all other
> disappear from my inbox while they're still on the server.

Your whole Inbox is getting deleted? That's really bad! I haven't seen this at
all, but I haven't tested any filter options lately either. Or is this happening
to you even without any filters?

Comment 71

[Howard Chu]

Attachment: Fix for filter / delete from server

re: Comment #69 this patch makes the Delete from POP Server filter action act
immediately, like it is supposed to(?). Also, if the current message is only a
header, it is dropped silently instead of going into the Inbox. This seems to
make sense because if the message on the server has been deleted, the header
itself is no use any more. A possible downside - if you ever wanted to go back
and examine the message headers, there's no way to Undo this action. (You can
manually edit the Inbox file to find the headers...)

Comment 72

[Christian Eyrich]

re: Comment #70

>> And clicking on the link of one message it gets downloaded, but all other
>> disappear from my inbox while they're still on the server.
> 
> Your whole Inbox is getting deleted?

No, only the messages the filter should have deleted on the server.

Comment 73

[Christian Eyrich]

Yep, that's great. As you wrote, the header would be useless.

> A possible downside - if you ever wanted to go back and examine the message
headers,
> there's no way to Undo this action.

That's true but very unlikely. If some stochastic spam filter would deletes the
mail that would be kind of unpredictable. But here the user created a filter for
this special case.


I've another (a bit demonic) combination. Filter action "Delete from POP server"
together with "Fetch body from POP server". This with "Fetch headers only" and
"Leave messages on server" could be a real world scenario.
Currently the message is deleted on the server first and then of course no body
fetched. I think it should be the other way round.

Comment 74

[Christian Eyrich]

Another nit:

1. Have some mails on the server.
2. Set up a filter that matches mails on the server, action: fetch Body and move
to some folder x.
3. Get messages.
4. The header message and the full message of each mail is in folder x.

That's understandable and not bad though I don't think any user will see this as
desired behaviour. Maybe it's possible to put some AI in the code to not apply
other actions for a message if "Fetch body from POP server" is executed.

Comment 75

[David :Bienvenu]

Comment on attachment 150795 [details] [diff] [review]
Fix for filter / delete from server

this looks OK, though I'm not sure how Christian's most recent comments fit
into this...

Comment 76

[David :Bienvenu]

Comment on attachment 150782 [details] [diff] [review]
Better version of previous

thx for doing this - I think this will make the user experience much nicer.

no need to make these noscript:

+  [noscript] void replaceHeader(in nsIMsgDBHdr aMsgHdr);

+  [noscript] boolean checkMessage(in string aUidl);

+  m_selectKey = 0;
instead of 0, which is a valid key for the first message in a folder, use
nsMsgKey_None, and change corresponding check(s)

+    nsCString newuri;
+    char *uritext;
+    nsBuildLocalMessageURI(m_baseMessageURI, m_selectKey, newuri);
+    uritext =	ToNewCString(newuri);
+    m_window->SelectMessage(uritext);

I think this leaks uritext. Also, use an nsCAutoString instead of nsCString.
The rule of thumb is if the string is likely to be < 64 chars, use the
AutoString - it saves an allocation because the AutoString will use a 64 byte
stack-based buffer.
You should be able to just do window->SelectMessage(newuri.get())

We prefer nsnull instead of NULL (don't ask me why :-) )

+struct partialRecord
+{
+  nsIMsgDBHdr *msgDBHdr;
+  char *uidl;
+};

consider making this an object with a destructor and using an nsCString for
uidl and nsCOMPtr for nsMsgDBHdr - makes memory mgmt and ref-counting cleaner 

+    if (!found)
+    {
+      m_newMailParser->m_mailDB->DeleteHeader(partialMsg->msgDBHdr, nsnull,
PR_FALSE, PR_TRUE);
+    }
no need for the extra braces, for a one line if (which is the style I want to
prevail in this code :-) other areas do it differently)

Comment 77

[Howard Chu]

(In reply to comment #74)
> Maybe it's possible to put some AI in the code to not apply
> other actions for a message if "Fetch body from POP server" is executed.

Sounds like a good idea. Also we need to change the sort order that
GetSortedActionList uses so that FetchBody occurs before DeleteFrom...

Only slightly relevant: Was just looking at the RuleActionsTableEntry in
base/search/src/nsMsgFilter.cpp, it seems that the POP3 Server filter actions
(DeleteFromPop3Server, LeaveOnPop3Server, FetchBodyFromPop3Server) really should
be restricted to nsMsgFilterType::Inbox, yes?

Comment 78

[Howard Chu]

Attachment: Filter fixes again

Basically the same as before:
  patch to nsPop3Protocol.cpp is the same as previous

  nsParseMailbox.cpp stops filter at FetchBodyFromPop3Server
  nsMsgFilter.cpp makes sure Fetch is first, restricts Pop3 filters to Inbox

Comment 79

[Christian Eyrich]

Thanks Howard, that's nearly perfect.
Nearly because the f flag is set in popstate for messages that have been
"filtered" by "Fetch body from POP server" if the server has "Fetch headers
only" and "Leave messages on server" set.
So the messages are fetched at the next get mail.

Comment 80

[Howard Chu]

Attachment: Filter fix with refetch/LeaveOnServer

re: Comment #79 OK, try this one. It works for me...
(Testing was a real joy, got into a Retrieve loop and had hundreds of copies of
my test message in no time. Fixed that. ;)

Comment 81

[Howard Chu]

Comment on attachment 150824 [details] [diff] [review]
Filter fixes again

patch is obsolete

Comment 82

[Howard Chu]

Attachment: Cleanup of UI cleanup...

re: Comment #76 this patch is cleaned up a fair bit. The UI response looks
better to me as well.

Comment 83

[Christian Eyrich]

re: Comment #80
Yep, that patch fixed it, great.

Unfortunately I've discovered another flaw that can be irritating.

The status line shows msg counts like 9 of 6.
If you've e.g. 6 mails, three of them get filtered with action "Fetch body from
POP server", the code counts the messages twice - one time for the header and
one time for the body.
One solution would be to increment the total number but I think counting each
msg instead of each download is better, so 6 of 6.

Comment 84

[Howard Chu]

(In reply to comment #83)
> Unfortunately I've discovered another flaw that can be irritating.
> 
> The status line shows msg counts like 9 of 6.
> If you've e.g. 6 mails, three of them get filtered with action "Fetch body from
> POP server", the code counts the messages twice - one time for the header and
> one time for the body.
> One solution would be to increment the total number but I think counting each
> msg instead of each download is better, so 6 of 6.

Hm, I'm not seeing this. But I have an idea, can you try adding this line to
your patched nsParseMailbox.cpp:
--- nsParseMailbox.cpp  Wed Jun 16 05:31:31 2004
+++ nsParseMailbox.cpp.N        Wed Jun 16 05:31:21 2004
@@ -1755,6 +1755,7 @@
            // Don't add this header to the DB, we're going to replace it
            // with the full message.
             m_msgMovedByFilter = PR_TRUE;
+            msgIsNew = PR_FALSE;
            // Don't do anything else in this filter, wait until we
            // have the full message.
            *applyMore = PR_FALSE;

and see if it makes a difference?

Comment 85

[Howard Chu]

Attachment: More tweaks to UI cleanup patch

This is functionally the same as Attachment #150912 [details] [diff], but I split out the Inbox
searching code into its own functions, so it didn't have to be copied in two
places.

Comment 86

[Christian Eyrich]

(In reply to comment #84)

> Hm, I'm not seeing this.

Hu? Strange.
And no, adding that line doesn't help. It still fetches 10 of 6 mails.

Comment 87

[David :Bienvenu]

Comment on attachment 150925 [details] [diff] [review]
More tweaks to UI cleanup patch

great, I think this will be a lot nicer. And I'm glad you've combined the dup
code.

re +struct nsLocalMailInboxState

you'll need a destructor so the comptrs will be released, to avoid mem leaks.
And I'd think you'd need a constructor to init the comptrs...

The struct name is too general and too specific :-) - it's used for folders
other than the Inbox, and the name should be more specific about what the state
is for - maybe nsLocalFolderReadState or nsLocalFolderScanState...

and the related method names and comments should change from Inbox. Also,
OpenInbox should be called something like GetReadState() or GetScanSate()

nsLocalMailInboxState
::uidl

can this be an nsCString?
+  void deleteDlMsg(in nsIMsgDBHdr aMsgHdr, out boolean aDoSelect);
+  void selectDlMsg();

can you spell out Download instead of Dl?

Comment 88

[Howard Chu]

Attachment: Filter fix with unread counter...

re: Comment #83 etc.
OK, this patch is the same as before but with a fix for the new/unread status
display problem. It also uses spaces instead of TABs...

I've tacked on a patch for nsPop3IncomingServer.cpp here, to remove an unused
variable. You can do whatever with it, I was just getting annoyed by the
warning message on my builds...

Comment 89

[David :Bienvenu]

Comment on attachment 150980 [details] [diff] [review]
Filter fix with unread counter...

r=bienvenu

Comment 90

[Howard Chu]

Attachment: UI cleanup patch cleaned up some more

I think this takes care of Comment #87.

The only other point worth noting is DOWNLOAD_NOTIFY_STYLE which affects the
order of events when notifying a window about headers being deleted and added.
The actual database operations MUST be "delete old first then add new" but this
macro controls whether the window is told about the delete first or the add.
Defaults to Delete first.

Comment 91

[David :Bienvenu]

Attachment: cumulative patch to check in

Comment 92

[David :Bienvenu]

->Howard before marking fixed.

Comment 93

[David :Bienvenu]

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

Comment 94

[Jan Vlug]

I can't see the changes in the help file using Mozilla/5.0 (Windows; U; Windows
NT 5.1; en-US; rv:1.8a2) Gecko/20040620.

Comment 95

[David :Bienvenu]

help file changes checked into trunk and aviary branch

Comment 96

[Daniel Wang]

CC doc people for UI change

Comment 97

[Iain MacDonnell]

Aviary branch is still busted on Solaris / Sun Studio:

"nsLocalMailFolder.cpp", line 3701: Error: Cannot assign const char* to char*.
1 Error(s) detected.
gmake[5]: *** [nsLocalMailFolder.o] Error 1
gmake[5]: *** Waiting for unfinished jobs....
gmake[5]: Leaving directory
`/export/stuff/mozilla/thunderbird-20040626/mozilla/mailnews/local/src'
gmake[4]: *** [libs] Error 2

Comment 98

[Howard Chu]

(In reply to comment #97)
> Aviary branch is still busted on Solaris / Sun Studio:
> 
> "nsLocalMailFolder.cpp", line 3701: Error: Cannot assign const char* to char*.
> 1 Error(s) detected.
 
Then try this:
Index: nsLocalMailFolder.cpp
===================================================================
RCS file: /cvsroot/mozilla/mailnews/local/src/nsLocalMailFolder.cpp,v
retrieving revision 1.465
diff -u -r1.465 nsLocalMailFolder.cpp
--- nsLocalMailFolder.cpp       18 Jun 2004 20:18:39 -0000      1.465
+++ nsLocalMailFolder.cpp       28 Jun 2004 00:59:04 -0000
@@ -3673,7 +3673,7 @@
   PRUint32 messageOffset;
   PRBool more = PR_FALSE;
   PRUint32 size = 0, len = 0;
-  const char *accountKey = nsnull;
+  char *accountKey = nsnull;

   aMsgDBHdr->GetMessageOffset(&messageOffset);
   rv = aState->m_seekableStream->Seek(PR_SEEK_SET, messageOffset);

Comment 99

[Iain MacDonnell]

(In reply to comment #98)

Oddly, bonsai says that David checked in the opposite of this (rev 1.465) in order
to fix SunOS build bustage.

Comment 100

[Mike Cowperthwaite]

Due to a discussion in n.m.u.general, I discovered that there are now two new 
context menu items in the threadpane: Get Selected Messages and Get Flagged 
Messages.  I guess I should have noted this in the earlier discussion.

(In reply to bienvenu's comment #60)
> the UI changes to mailwindowoverlay.xul only add a couple items to the thread
> pane context menu that already exist in the file menu

Already exist?  Am I missing something?  I don't see these in 1.8a2 or 1.7 -- 
maybe this is the Thunderbird File menu you refer to?


Anyway: I'm not particularly thrilled about these.  Ideally, these items would 
only appear in a threadpane where a delayed Get makes sense -- POP accounts 
configured with this bug's option (or, if/when such functionality is in place, 
imap or news accounts with a similar offline setup).  However, since multiple 
accounts can be delivered to the same Inbox now, and header-only msgs can be 
dragged around, that's probably a little impractical.

I would like to see the items a little more dynamic: if no messages are flagged, 
disable the 'get flagged' item; if only one message is selected, check to see if 
it's already completely downloaded and disable it if so, otherwise change the 
menuitem text to "Get this message" instead of "get selected messages."

Howard, David: what do you think?  I'll open a new bug for these menu issues if 
it sounds worthwhile.

Comment 101

[Howard Chu]

(In reply to comment #100)
> Due to a discussion in n.m.u.general, I discovered that there are now two new 
> context menu items in the threadpane: Get Selected Messages and Get Flagged 
> Messages.  I guess I should have noted this in the earlier discussion.
> 
> (In reply to bienvenu's comment #60)
> > the UI changes to mailwindowoverlay.xul only add a couple items to the thread
> > pane context menu that already exist in the file menu
> 
> Already exist?  Am I missing something?  I don't see these in 1.8a2 or 1.7 -- 
> maybe this is the Thunderbird File menu you refer to?

File -> Offline -> ...
The items I added to the threadpane context menu are identical to the Get
Flagged / Get Selected commands in the Offline menu.

> Anyway: I'm not particularly thrilled about these.  Ideally, these items would 
> only appear in a threadpane where a delayed Get makes sense -- POP accounts 
> configured with this bug's option (or, if/when such functionality is in place, 
> imap or news accounts with a similar offline setup).  However, since multiple 
> accounts can be delivered to the same Inbox now, and header-only msgs can be 
> dragged around, that's probably a little impractical.

Yes. Like I mentioned in comment #40, this was a big hassle to make it happen.

> I would like to see the items a little more dynamic: if no messages are flagged, 
> disable the 'get flagged' item; if only one message is selected, check to see if 
> it's already completely downloaded and disable it if so, otherwise change the 
> menuitem text to "Get this message" instead of "get selected messages."
> 
> Howard, David: what do you think?  I'll open a new bug for these menu issues if 
> it sounds worthwhile.

David and I discussed this in private email before; we agreed that the original
Offline behavior was somewhat broken already(it never enables/disables itself
properly), and it wasn't a high priority to change it. As you already pointed
out, toggling the menu items based on the folder/account is definitely
impractical. Also, to your point, it actually "makes sense" for *all* the
account types (imap, pop, news) now, so there doesn't seem to be any reason to
bother differentiating that.

Comment 102

[Jure Repinc (JLP)]

First I have to say thanks for making this happen. thanks to all working hard on
it and testing it out!

Now to my little problem with this option. I have set Mozilla Mail to
automatically move junk mail to Junk folder. The problem is if I have set
Mozilla to download only headers and the mail at first arrives into my Inbox
folder. Then I see that the header is interesting and I open the mail. As
expected it shows an information that only header was downloaded and a link to
download and display the body too. I click on the link and the whole message
downloads. Now here comes the problem. If the whole message is not recognized as
junk the body shows just fine. But if Mozilla thinks it is junk then the message
is never shown and the message body pane just remains blank.

I think that even if the message is marked as junk and moved to the Junk folder
Mozilla should display the message after clicking that link and not just show
blank window.

Comment 103

[Mike Cowperthwaite]

Jure Repinc: re comment 102, see bug 248742.

Comment 104

[R.K.Aa.]

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

Comment 105

[David :Bienvenu]

spinning off bug 259649 for a related issue having to do with headers filtered
into other folders.

Comment 106

[David :Bienvenu]

Attachment: fix get offline selected (I messed up applying Howard's patch)

Comment 107

[Mike Cowperthwaite]

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

Comment 108

[Torsten Bondieck]

I use the new option "Fetch Headers Only preference" in Mozilla/5.0 (Windows; U;
Windows NT 5.0; de-AT; rv:1.8a5) Gecko/20041122.

Is the full message is retrieved, the message with only header is still present
in local mailbox. Why delete the only header mail not immediately if the full
message have load?

Momentarily the messages with only header primary delete by next mailbox connect.

Comment 109

[David :Bienvenu]

that's a trunk only problem - i need to look into that

Comment 110

[David :Bienvenu]

see bug 273063 for a fix for that problem.

Comment 111

[Torsten Bondieck]

If i use a mailfilter to move the specific message with only header in trash,
then the mail not delete on the server. Only delete the mail on server, if i
delete it in post office entrance by hand.

Comment 112

[Torsten Bondieck]

(In reply to comment my #111)

precise:
If i use a mailfilter to move the specific message with only header in trash
folder AND exit mozilla-mail the message is delete(Option: Empty Trash on exit).

BUT the mail not delete on the server by next connection. Only delete the mail
on server, if i delete the mail in mozilla trashfolder by hand before i exit
mozilla mail!

on Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8a6) Gecko/20050111

Comment 113

[WADA:World Anti-bad-Duping Agency]

(In reply to comment #112)

Sounds new problem.
[e-mail], get POP3 protocol log, then open new bug and write down
"X-UIDL" header contents in the bug, and attach protocol log & back-ups of
popstate.dat to the bug.

Test scenario will probably be as follows.
(1) Create a filter which move test mail to Trash.
    Eg. If subject is "test test test" Then move to Trash"
(2) Send a mail of subject="test test test" to yourself.
(3) Terminate Thunderbird
(4) Keep backup of popstate.dat in <server_name> directry
    under your Mail directry (UIDL and the mail status is held in this file).
(5) Enable POP3 protocol log and start Thunderbird
(6) Get mail(automatically on startup or manually)
    => The test mail will be moved to Trash
(7) Check UIDL of the mail
    - See "X-UIDL" header using View/Message Source
(8) Exit Thunderbird
    => The mail will be purged from Trash on exit
(9) Keep backup of protocol log file.
(10) Keep backup of popstate.dat again
(11) Enable POP3 protocol log and start Thunderbird again
(12) Connect to server again
(13) Exit Thunderbird
(14) Keep backup of protocol log file.
(15) Keep backup of popstate.dat again

See http://www.mozilla.org/quality/mailnews/mail-troubleshoot.html#pop for
protocol log.
Do not forget to change "protocol:5" to "POP3:5" when POP3.
Please note that log file is overlayed on restart.

Comment 114

[Torsten Bondieck]

(In reply to comment #113)

> Sounds new problem.
> [e-mail], get POP3 protocol log, then open new bug and write down
> "X-UIDL" header contents in the bug, and attach protocol log & back-ups of
> popstate.dat to the bug.

i open new bug https://bugzilla.mozilla.org/show_bug.cgi?id=279040