122704 - Need UI to refresh the list of subscribed folders

Status: NEW

(MailNews Core :: Networking: IMAP, enhancement)

Description

[Terrel Shumway]

If another IMAP client executes (UN)SUBSCRIBE commands, there does not seem to
be a way to refresh mozilla's folder list, short of completely exiting mozilla.

Solution: add UI to trigger an LSUB. e.g. right-click on account |"Refresh
Folder List"

Also useful: Account|Log(out|in)

Comment 1

[Ian Neal]

Marking as an enhancement

Comment 2

[Boris 'pi' Piwinger]

Is this a dupe of bug 97700? How about bug 38943?

pi

Comment 3

[Boris 'pi' Piwinger]

I guess my dupe guesses were not the best, but this feature seems to be needed.
Confirming.

pi

Comment 5

[Jeff Beckley]

Attachment: Refresh Folder List implementation

Submitting this patch, actually implemented by Dale Wiggins <[e-mail]>.

Comment 6

[Karsten Düsterloh]

Comment on attachment 284420 [details] [diff] [review]
Refresh Folder List implementation

Some very incomplete comments, based alone on patch reading without testing, centered upon SeaMonkey.

SeaMonkey-UI is missing, but that's not too bad, since the TB-UI added is rather incomplete anyway. ;-)

>Index: mail/base/content/mailWindowOverlay.xul
>===================================================================
>+    <menuitem id="folderPaneContext-discoverSubFolders"
>+        label="&folderContextDiscoverSubFolders.label;"
>+        accesskey="&folderContextDiscoverSubFolders.accesskey;"
>+        oncommand="MsgDiscoverSubFolders();"/>

Adding items to all context menus regardless whether IMAP or not does not seem helpful.

>Index: mail/locales/en-US/chrome/messenger/messenger.dtd
>===================================================================
>+<!ENTITY folderContextDiscoverSubFolders.label "Refresh Folder List">
>+<!ENTITY folderContextDiscoverSubFolders.accesskey "f">

Why the small f and not the F?


>Index: mailnews/base/resources/content/widgetglue.js
>===================================================================

This file is SM only, so:

>+// Refresh the list of folders by discovering all subscribed folders under the
>+// currently selected folder.
>+function MsgDiscoverSubFolders()
>+{
...
>+    // Bail now if the currently selected folder isn't an IMAP folder.
>+    if (selectedFolder.server.type != "imap")
>+    {
>+      return;
>+    }

No braces needed/wanted here.


>Index: mailnews/base/src/nsMessenger.cpp
>===================================================================
>+nsMessenger::DiscoverSubFolders(nsIRDFCompositeDataSource* db,
>+                                nsIRDFResource* folderResource)
>+{
>+  nsresult rv = NS_ERROR_NULL_POINTER;
>+  
>+  if (!db || !folderResource) return rv;

Please put the return on its own line.

>+  nsCOMPtr<nsISupportsArray> folderArray;
>+
>+  rv = NS_NewISupportsArray(getter_AddRefs(folderArray));
>+  if (NS_FAILED(rv)) return rv;

Ditto.

Comment 7

[David :Bienvenu]

Collapsing and expanding the server in the folder pane refreshes the list, doesn't it?

Comment 8

[David :Bienvenu]

Is collapsing and expanding the server just too obscure, obscure enough to warrant adding UI for something that I imagine will be pretty rarely needed for most users?

Comment 9

[Steve Dorner]

We will need this in the Penelope UI, because it's in the Eudora UI that way.  Whether or not it needs to be in TBird or SeaMonkey UI is a different matter.  As a user, I don't know that I'd expect collapse/expand to require fiddling with the server, so I don't find the current behavior particularly beneficial or obvious.  But that's just my opinion.

Comment 10

[Jeff Beckley]

I didn't know about the collapse/re-expand behavior to do refresh (maybe Dale did).  Seems like it would be good to have something a little more obvious, like the context menu item.  Like Karsten notes, it should be conditioned on IMAP folder trees.

I actually got in to a situation today in trunk code on the Mac (testing this new feature) where deleting a mailbox didn't cause the folders to refresh.  That's probably a bug, but could have been cleared up with the refresh menu item.  For some reason the collapse/re-expand didn't work either, though.

Comment 11

[David :Bienvenu]

re the folder pane refresh after deleting a folder, that's a general trunk bug, and applies to local folders as well as imap.

re folder refresh, sorry, I'm not on a machine with Eudora on it right now - does it have to be a sub-folder refresh, or could it just refresh the whole folder list and not limit itself to sub-folders of a selected folder?

Comment 12

[Jeff Beckley]

In Eudora it refreshes just the sub-tree that is selected.

Comment 13

[Karsten Düsterloh]

For the time being, SeaMonkey is not interested in UI for this, so no worries there. ;-)

Comment 14

[Dale Wiggins]

Regarding collapse/expand doing a refresh, it didn't do this in the codeline where I originally implemented it. Even if it had done that, the UI is still important to us because of how Eudora worked. I'll look over my code and see if ties in well with the current refresh behavior, but I am certainly inclined to leave the menu in place.

Regarding the menu showing up on local mailboxes, my bad. I thought I had addressed that issue but apparently not.

Comment 15

[David :Bienvenu]

Expand/collapse has always done a refresh, but only at the server level, not at the sub-folder level. Unless you're not using imap subscription, in which case, I believe expand/collapse of folders LISTs the folders two levels deep. The reason for that is that we don't LIST the whole hierarchy if you're not using subscription, because it could be slow. Not that the user cares, of course.

An alternative to not showing the menu for local mailboxes is making it work for local mailboxes. I often save mailboxes into my profile directory, and then I have to restart to see the new mailboxes. But I'm being facetious - normal users wouldn't use that very much, I don't think :-)

Comment 16

[David :Bienvenu]

Looking at that patch, it looks like it rediscovers all folders, not just sub-folders of the selected folder. If that's the desired behavior, it seems like all the functions should have that name instead, instead of referring to subfolders.

Or, since we're only doing this for IMAP, it seems to me that we could simply skip the rdf and nsIMessenger changes, and go straight to the imap interfaces from the js method, which would cut down on the patch quite a bit. In fact, couldn't you just call nsIMsgIncomingServer::performExpand from the js and be done with it? If I'm reading the patch correctly, that should do the same thing...admittedly, performExpand is a poor name, since it's really refreshing the folder list, which we do *on* expand...

Comment 17

[Dale Wiggins]

OK, someone is going to have to tell me what I am missing.

If I add a top level mailbox (beside Inbox, not under it) via another client I can collapse and expand all day and TB never finds this new mailbox regardless of what value mail.server.default.using_subscription has.

The mail.server.default.using_subscription pref defaults to true meaning that expanding a mailbox will not actually go to the server to look for new subfolders unless the user (or Penelope) changes this setting.

Even if mail.server.default.using_subscription is set to false, expanding a folder will not remove existing folders that have been deleted via another client.

My menu item, implemented in the way it is, addresses all three of the above concerns.

Comment 18

[David :Bienvenu]

Dale, if you generate an imap protocol log http://www.mozilla.org/quality/mailnews/mail-troubleshoot.html#imap when you do these steps, are we issuing an lsub in the log, but not adding the new folder to the folder pane? If so, I think the problem has to do with an unrelated trunk regression in updating the folder pane. Do the same steps work with the 2.0 branch?

Comment 19

[Magnus Melin [:mkmelin]]

FWIW, the folder refresh bug should be bug 399769.

Comment 21

[Wayne Mery (:wsmwk)]

Dale, can you get a protocol log per comment 18.  And since investigation is ongoing here it probably makes sense to cancel the patch review requests

note - mscott no longer actively reviewing

Comment 22

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Comment on attachment 284420 [details] [diff] [review]
Refresh Folder List implementation

Patch has bit-rotted.

$ patch -p0 --dry-run < ~/Desktop/tbTestPatches/attachment.cgi 
(Stripping trailing CRs from patch.)
patching file mail/base/content/mailWindowOverlay.xul
Hunk #1 FAILED at 773.
1 out of 1 hunk FAILED -- saving rejects to file mail/base/content/mailWindowOverlay.xul.rej
(Stripping trailing CRs from patch.)
patching file mail/base/content/widgetglue.js
Hunk #1 FAILED at 185.
1 out of 1 hunk FAILED -- saving rejects to file mail/base/content/widgetglue.js.rej
(Stripping trailing CRs from patch.)
patching file mail/locales/en-US/chrome/messenger/messenger.dtd
Hunk #1 FAILED at 498.
1 out of 1 hunk FAILED -- saving rejects to file mail/locales/en-US/chrome/messenger/messenger.dtd.rej
(Stripping trailing CRs from patch.)
patching file mailnews/base/public/nsIMessenger.idl
Hunk #1 FAILED at 90.
1 out of 1 hunk FAILED -- saving rejects to file mailnews/base/public/nsIMessenger.idl.rej
(Stripping trailing CRs from patch.)
patching file mailnews/base/public/nsIMsgFolder.idl
Hunk #1 FAILED at 348.
1 out of 1 hunk FAILED -- saving rejects to file mailnews/base/public/nsIMsgFolder.idl.rej
(Stripping trailing CRs from patch.)
patching file mailnews/base/resources/content/widgetglue.js
Hunk #1 FAILED at 187.
1 out of 1 hunk FAILED -- saving rejects to file mailnews/base/resources/content/widgetglue.js.rej
(Stripping trailing CRs from patch.)
patching file mailnews/base/src/nsMessenger.cpp
Hunk #1 FAILED at 1463.
1 out of 1 hunk FAILED -- saving rejects to file mailnews/base/src/nsMessenger.cpp.rej
(Stripping trailing CRs from patch.)
patching file mailnews/base/src/nsMsgFolderDataSource.cpp
Hunk #1 succeeded at 117 (offset 1 line).
Hunk #3 succeeded at 297 (offset -1 lines).
Hunk #4 succeeded at 664 (offset -22 lines).
Hunk #5 succeeded at 700 (offset -22 lines).
Hunk #6 FAILED at 800.
1 out of 6 hunks FAILED -- saving rejects to file mailnews/base/src/nsMsgFolderDataSource.cpp.rej
(Stripping trailing CRs from patch.)
patching file mailnews/base/src/nsMsgFolderDataSource.h
Hunk #1 succeeded at 260 (offset -2 lines).
(Stripping trailing CRs from patch.)
patching file mailnews/base/src/nsMsgRDFUtils.h
Hunk #1 succeeded at 107 (offset -1 lines).
(Stripping trailing CRs from patch.)
patching file mailnews/base/util/nsMsgDBFolder.cpp
Hunk #1 succeeded at 1404 with fuzz 2 (offset 173 lines).
(Stripping trailing CRs from patch.)
patching file mailnews/imap/public/nsIImapIncomingServer.idl
Hunk #1 succeeded at 97 (offset 6 lines).
(Stripping trailing CRs from patch.)
patching file mailnews/imap/src/nsImapIncomingServer.cpp
Hunk #1 succeeded at 902 (offset -63 lines).
Hunk #2 succeeded at 2341 (offset -494 lines).
Hunk #3 succeeded at 2437 (offset -493 lines).
(Stripping trailing CRs from patch.)
patching file mailnews/imap/src/nsImapIncomingServer.h
Hunk #1 FAILED at 75.
1 out of 1 hunk FAILED -- saving rejects to file mailnews/imap/src/nsImapIncomingServer.h.rej
(Stripping trailing CRs from patch.)
patching file mailnews/imap/src/nsImapMailFolder.cpp
Hunk #1 FAILED at 2182.
1 out of 1 hunk FAILED -- saving rejects to file mailnews/imap/src/nsImapMailFolder.cpp.rej
(Stripping trailing CRs from patch.)
patching file mailnews/imap/src/nsImapMailFolder.h
Hunk #1 FAILED at 250.
1 out of 1 hunk FAILED -- saving rejects to file mailnews/imap/src/nsImapMailFolder.h.rej

Comment 23

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Jeff / Dale, any chance of a new patch?