517514 - Expand archive granularity option backend to support UI

Status: RESOLVED FIXED

(Thunderbird :: Account Manager, enhancement)

Description

[rsx11m]

+++ This bug was initially created as a clone of Bug #486827 +++

Spun off per bug 486827 comment #19 to provide an easier access to the archive_granularity attribute for a server. A possible location would be
the Copies & Folders page of the Account Manager, e.g., as a drop-down
menu right underneath the selector for the Archives folder location.

Possible values of archive_granularity:

0 = Flat archive (no subfolders)
1 = subfolders in Archives for year (1-level structure, default)
2 = subfolders for year, then for month (2-level structure)

Comment 1

[Magnus Melin [:mkmelin]]

Attachment: proposed fix

Currently the archiving granuality is per server, which is really suboptimal as archive folder is per identity. 

This patch makes granuality per identity, and adds UI to choose how to archive.
Past string freeze, so i don't know if this has a chance to make tb3. Anyway, at least the patch is more testable with UI.

With this we should be able to close bug 476217 too.

Screenshot coming up.

SeaMonkey:
 - run out of accesskeys on that pane it seems :( 
 - I just copy-pasted over the archiving function from tb. Was missing a few fixes there anyway...

Comment 2

[Magnus Melin [:mkmelin]]

Attachment: screenshot of the fix

Screenshot.

Comment 3

[David Ascher (:davida)]

The word "flat" feels wrong to me -- it implies a very widely spread tree, whereas I think what it means is "just one folder", right?

Comment 4

[David :Bienvenu]

I'm not sure about the term "archive hierarchy" either.  Maybe something along the lines of Archive messages "by month" "by year" "in one folder" - ugly, I know...

Comment 5

[Magnus Melin [:mkmelin]]

Not sure it's the best word or not, but googling "flat hierarchy" seems to support that it is an hierarchy with only same-level children.

I'll look in to bienvenu's suggestion.

Comment 6

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

I was going to suggestion a new dialog window for this.  For reasons of having more room to explain the choices given as well as giving space for extensions to offer alternate archiving patterns.

Basic proposal would be this:

* replace the Archive Hierarchy radios with an ( Archive Options... ) button

* create new dialog window with the radio buttons and examples near by.

+-------------------------------+
|
| (o) Flat Folder
|     /Archives/
|
| (o) Yearly Archived Folders
|     /[-] Archives/
|       |--- 2009
|       +--- 2008
|
| (o) Yearly Archived Folders
|     /[-] Archives/
|       |--[-] 2009
|       |   |--- 2
|       |   +--- 1
|       +--[-] 2008
|           |--- 12
|           +--- 11
|
+-------------------------------+

I think we might actually want the examples beside the radio buttons but that was harder to do in ascii art.  For these types the examples could all be done in JS such that the year and folder are close to current.

Future Archive schemes provided via extensions could just append to that dialog for their options.

I could try a real mockup some time later when I can breath a little

Comment 7

[Magnus Melin [:mkmelin]]

Attachment: proposed fix, v2 (backend only)

This makes the archive granuality per identity. I'd really like to get this done for tb3 as otherwise we'll have to have migration later. 

Since archive folder is per identity the mismatch makes for impossible ui.

This part is the backend only. I'll attach the (unfinished) front end patch for testing in a bit.

Comment 8

[Magnus Melin [:mkmelin]]

Attachment: proposed fix, v2/v3 (frontend only) - for testing only

Unfinished frontend for testing only.

Comment 9

[David :Bienvenu]

Comment on attachment 407330 [details] [diff] [review]
proposed fix, v2 (backend only)

this patch has bit-rotted - can you attach a non-bittrotted one? I tried to make it as little bit-rotted as possible when I fixed the archive batching code...

One reason archive granularity is per-server is that different servers tend to have different tolerance for giant folder. Making it per-identity blurs that a little but the benefit of having a consistent UI is probably greater...

Is this an unrelated change? :

-
-    // We're just going to select the message now.
-    let treeView = gDBView.QueryInterface(Components.interfaces.nsITreeView);
-    treeView.selection.select(this.messageToSelectAfterWereDone);
-    treeView.selectionChanged();

Comment 10

[Magnus Melin [:mkmelin]]

Attachment: proposed fix, v3 (backend)

Backend part - attachment 407331 [details] [diff] [review] can still be used to test.

For multi-msg archive there is still a minor issue, as the messages will all archive according to the first selected msg. We can fix that later though. I can't really say i understand why the archiving is implemented like it is, the batching seems odd to me (especially that it's in two functions.)

Comment 11

[David :Bienvenu]

Comment on attachment 408437 [details] [diff] [review]
proposed fix, v3 (backend)

this still fails to apply on trunk or 1.9.1 branch for me:

patching file mail/base/content/mailWindowOverlay.js
Hunk #1 FAILED at 1283.
Hunk #2 succeeded at 1352 (offset 19 lines).
1 out of 3 hunks FAILED -- saving rejects to file mail/base/content/mailWindowOv
erlay.js.rej

Comment 12

[Magnus Melin [:mkmelin]]

Attachment: proposed fix, v4 (backend)

Unbitrotted again.

Comment 13

[peutch]

+1 for this feature, on a per-account basis!

Comment 14

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

just to note, bug 533815 is not the same but similar

Comment 15

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

Comment on attachment 408885 [details] [diff] [review]
proposed fix, v4 (backend)

There isn't much UI here really.  My only concern is that a per identity setting would complicate our options UI.  We should still have the Archive options available outside of the, very hidden, identity settings.  Not sure how that will work out right now.

Comment 16

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

(In reply to comment #14)
> just to note, bug 533815 is not the same but similar

And by that I meant bug 522761

Comment 17

[Mitra Ardron]

The obvious place I'd thought would be in the Account Settings, Disk Settings. Its too obscure hidden away in a preferences file. 

As a data point -- I've noticed that on Mac's its a particularly BAD idea to use the yearly archive, as it means the Archive file, which gets HUGE gets backed up every time TimeMachine runs, because it always changes. With monthly archives only the most recent month gets backed up.

Comment 18

[Jens Hatlak (:InvisibleSmiley)]

(In reply to comment #1)
> SeaMonkey:
>  - run out of accesskeys on that pane it seems :( 
>  - I just copy-pasted over the archiving function from tb. Was missing a few
> fixes there anyway...

I don't think we should go that way. The SM MailNews reviewers (Karsten et. al.) probably won't like that (correct me if I'm wrong!), and there are some deliberate differences between the two implementations, e.g. this:

(In reply to comment #9)
> Is this an unrelated change? :
> 
> -
> -    // We're just going to select the message now.
> -    let treeView = gDBView.QueryInterface(Components.interfaces.nsITreeView);
> -    treeView.selection.select(this.messageToSelectAfterWereDone);
> -    treeView.selectionChanged();

AFAICS that's not part of the latest patch, but it just shows.

I'm already in the process of porting the missing Archive feature changes, e.g. in bug 573392, one at a time. I think it's very generous of you to do that for us/me, but personally I'd prefer to do it in separate, SM-only bugs. Of course this shouldn't cause stop-energy on your side, so I'd be fine if a change here temporarily broke (part of) SM's Archive functionality. I'd fix it in a follow-up, then. Of course Karsten's view on that may be different and is authoritative there.

Comment 19

[David :Bienvenu]

Comment on attachment 408885 [details] [diff] [review]
proposed fix, v4 (backend)

this patch is severely bit-rotted. I'll try to revive it.

Comment 20

[David :Bienvenu]

Attachment: un-bitrotted patch

I've de-bitrotted this patch. I've also fixed the mozmill test. It was non-trivial because of the folder structure archiving that landed subsequently. I've also incorporated the SeaMonkey fixes to the folder structure archiving (thx to Jens for those fixes).

I'm mainly asking for r from standard8 for the SeaMonkey changes and the mozmill fixes, but it would be good to have a quick look at the whole thing since I had a bear of a time getting the mozmill tests to pass.

Comment 21

[David :Bienvenu]

Attachment: remove unused default prefs

I noticed that the original patch didn't remove the now unused default server prefs.

Which reminds me, I'm not migrating the old prefs, since they were hidden, and I think we're going to have UI for the new prefs.

Comment 22

[Jens Hatlak (:InvisibleSmiley)]

Comment on attachment 483007 [details] [diff] [review]
remove unused default prefs

Drive-by comment regarding the SM part:

>+      let archiveGranularity;
>+      archiveKeepFolderStructure;

The second line doesn't look right.

>+      if (srcFolder.server.type == "rss") {
>+        // RSS servers don't have an identity so we special case the archives URI.
>+        archiveFolderUri =  srcFolder.server.serverURI + "/Archives";

Nit: Double space.

Comment 23

[David :Bienvenu]

Attachment: fix drive by comments

Comment 24

[Mark Banner (:standard8)]

Attachment: Final fix for SM parts

This fixes up the SM parts so that they work again. This has r=Standard8,bienvenu.

Comment 25

[Jens Hatlak (:InvisibleSmiley)]

(In reply to comment #24)
> Created attachment 484289 [details] [diff] [review]
> Final fix for SM parts

Nit: All four occurrences of "msgHdr.folder.server" in the patch can be replaced by "server" which is an in-scope local variable in the existing code (both TB and SM), defined just outside the area visible in the patch.

Comment 26

[Mark Banner (:standard8)]

I've renamed this bug to reference that its just supporting the backend.

If we want UI think we should do that in a different bug.

Checked in: http://hg.mozilla.org/comm-central/rev/081b2f960f1b

Comment 27

[rsx11m]

> (comment #26) If we want UI think we should do that in a different bug.

I've expected you to close this after the backend patch checked in, thus I'm already working on the follow-up bug and will clone this in a moment.  ;-)

Comment 28

[rsx11m]

Continued in bug 607295, please comment there for the UI implementation.