535437 - Activity Manager: pop activity is not showing

Status: RESOLVED FIXED

(Thunderbird :: General, enhancement)

Description

[Wayne Mery (:wsmwk)]

Activity Manager: pop activity is not showing
regression?

Comment 1

[Mark Banner (:standard8)]

What pop activity? We don't show any IMAP or POP activity. The only imap-related activity we show is auto sync.

Comment 2

[Wayne Mery (:wsmwk)]

(In reply to comment #1)
> What pop activity? We don't show any IMAP or POP activity. The only
> imap-related activity we show is auto sync.

yeah, I guess that's true. I guess I was surprised that davida was surprised :) ... perhaps it's time to hook up pop. 

sent message activity is displayed.

Comment 3

[Mark Banner (:standard8)]

Definitely not blocking 3.0.x then.

Comment 4

[David Ascher (:davida)]

We don't show moves/deletes for Local Folders is the impression I got.

Comment 5

[David :Bienvenu]

We do show moves/deletes for local folders. AFAIK, we don't show pop3 activity, e.g., the downloading of pop3 messages.

Comment 6

[David Ascher (:davida)]

Ok, probably good to have one entry for downloading pop3 messages.

Comment 7

[Wayne Mery (:wsmwk)]

we also show sent message activity if mailnews.sendInBackground = true (with the imap account identified), but not when false.

Comment 8

[Mark Banner (:standard8)]

(In reply to comment #7)
> we also show sent message activity if mailnews.sendInBackground = true (with
> the imap account identified), but not when false.

Separate bug that is already filed.

(In reply to comment #6)
> Ok, probably good to have one entry for downloading pop3 messages.

FTR If someone wants to do this on the 3.0.x branch this would have to use the same string as the autosync one for "<account> is up to date".

Comment 9

[Dan Mosedale (:dmosedale, :dmose)]

As per discussion with bienvenu and others yesterday, we're going to shoot for getting this into b1.

Comment 10

[David :Bienvenu]

Attachment: wip

this gets us pop3 notifications in the activity manager. I need to add a unit test for the pop3 notifications. I also need some feedback from Bryan about if and how we should coalesce the activity manager entries for pop3 download attempts...

Comment 11

[David :Bienvenu]

Attachment: proposed fix with unit test for pop3 notifications

This adds activities for the start of pop3 download, and when download is completed, along with how many messages were downloaded. It attempts to avoid multiple "no messages downloaded" notifications, in the interest of not polluting the activity manager too much.

I'm not doing progress though the hooks from the backend exist for it. I also test these notifications in the unit test.

I could not resist doing some whitespace cleanup in the pop3 code while I was there...

Comment 12

[Mark Banner (:standard8)]

Comment on attachment 427660 [details] [diff] [review]
proposed fix with unit test for pop3 notifications

This doesn't compile on Mac:

+  nsTObserverArray<nsCOMPtr<nsIPop3ServiceListener>>::ForwardIterator

This type of statement should use "> >" not ">>".

+  /**
+   * Notification about download progress.
+   * @param aFolder folder in which the download is happening.
+   * @param aNumDownloaded number of the messages that have been downloaded.
+   * @param aTotalToDownload total number of messages to download.
+   */

nit: blank line with star on before the @params please (a couple of places).

>diff --git a/mailnews/local/src/nsParseMailbox.cpp b/mailnews/local/src/nsParseMailbox.cpp

>+     if (customDBHeaders.Find("content-base") == -1)
>+      customDBHeaders.Insert(NS_LITERAL_CSTRING("content-base "), 0);

I don't think this is part of this patch...

> 
>+  nsCOMPtr <nsIInputStream> inboxInputStream = do_QueryInterface(m_outFileStream);
>+  rv = m_newMailParser->Init(serverFolder, m_folder, (m_downloadingToTempFile) ? m_tmpDownloadFile : path,
>+                            inboxInputStream, aMsgWindow, m_downloadingToTempFile);
>+// if we failed to initialize the parser, then just don't use it!!!
>+// we can still continue without one...

nit: should be indented a little bit

>-nsPop3Sink::GetPopServer(nsIPop3IncomingServer* *server)
>+NS_IMETHODIMP
>+nsPop3Sink::GetPopServer(nsIPop3IncomingServer**aServer)

nit: space one side or other of the ** please.

Generally looks fine, but I haven't run with it yet, which I'll do tomorrow.

Comment 13

[Bryan Clark (DevTools PM) [:clarkbw]]

Comment on attachment 427660 [details] [diff] [review]
proposed fix with unit test for pop3 notifications

The only one I was unsure about here was the last message.

pop3EventStatusTextNoMsgs=No messages downloaded

"No messages downloaded" doesn't seem quite right, though I'm not sure how to change it.  It feels more like we should be saying "No new messages to download" but that's not right either.  Any ideas?

Otherwise it looks great to me assuming we get some resolution on that text

Comment 14

[David :Bienvenu]

"No new messages to download" or "No new messages on server" both seem OK - do you have a preference between those two?

Comment 15

[Bryan Clark (DevTools PM) [:clarkbw]]

(In reply to comment #14)
> "No new messages to download" or "No new messages on server" both seem OK - do
> you have a preference between those two?

"No new messages to download" looks better to me, I think we want to avoid the use of the word "server" as much as possible.

Comment 16

[Bryan Clark (DevTools PM) [:clarkbw]]

clear up some flags, everything looks good to me with the above strings

Comment 17

[Mark Banner (:standard8)]

Comment on attachment 427660 [details] [diff] [review]
proposed fix with unit test for pop3 notifications

Ok, this looks fine subject to my comments previously.

Comment 18

[David :Bienvenu]

fix checked in, with comments addressed.