Expand archive granularity option backend to support UI

RESOLVED FIXED in Thunderbird 3.3a1

Status

Thunderbird
Account Manager
--
enhancement
RESOLVED FIXED
8 years ago
6 years ago

People

(Reporter: rsx11m, Assigned: Magnus Melin)

Tracking

Trunk
Thunderbird 3.3a1
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 8 obsolete attachments)

(Reporter)

Description

8 years ago
+++ 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)
(Assignee)

Comment 1

8 years ago
Created attachment 403819 [details] [diff] [review]
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...
Assignee: nobody → mkmelin+mozilla
Status: NEW → ASSIGNED
Attachment #403819 - Flags: ui-review?(clarkbw)
Attachment #403819 - Flags: superreview?(neil)
Attachment #403819 - Flags: review?(bienvenu)
(Assignee)

Comment 2

8 years ago
Created attachment 403820 [details]
screenshot of the fix

Screenshot.
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

8 years ago
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...
(Assignee)

Comment 5

8 years ago
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.
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
(Assignee)

Updated

8 years ago
Attachment #403819 - Attachment is obsolete: true
Attachment #403819 - Flags: ui-review?(clarkbw)
Attachment #403819 - Flags: superreview?(neil)
Attachment #403819 - Flags: review?(bienvenu)
(Assignee)

Comment 7

8 years ago
Created attachment 407330 [details] [diff] [review]
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.
Attachment #403820 - Attachment is obsolete: true
Attachment #407330 - Flags: ui-review?(clarkbw)
Attachment #407330 - Flags: superreview?(neil)
Attachment #407330 - Flags: review?(bienvenu)
(Assignee)

Comment 8

8 years ago
Created attachment 407331 [details] [diff] [review]
proposed fix, v2/v3 (frontend only) - for testing only

Unfinished frontend for testing only.

Comment 9

8 years ago
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();
(Assignee)

Updated

8 years ago
Attachment #407330 - Attachment is obsolete: true
Attachment #407330 - Flags: ui-review?(clarkbw)
Attachment #407330 - Flags: superreview?(neil)
Attachment #407330 - Flags: review?(bienvenu)
(Assignee)

Comment 10

8 years ago
Created attachment 408437 [details] [diff] [review]
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.)
Attachment #408437 - Flags: ui-review?(clarkbw)
Attachment #408437 - Flags: superreview?(neil)
Attachment #408437 - Flags: review?(bienvenu)
(Assignee)

Updated

8 years ago
Attachment #407331 - Attachment description: proposed fix, v2 (frontend only) - for testing only → proposed fix, v2/v3 (frontend only) - for testing only

Comment 11

8 years ago
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
(Assignee)

Comment 12

8 years ago
Created attachment 408885 [details] [diff] [review]
proposed fix, v4 (backend)

Unbitrotted again.
Attachment #408437 - Attachment is obsolete: true
Attachment #408885 - Flags: ui-review?(clarkbw)
Attachment #408885 - Flags: superreview?(neil)
Attachment #408885 - Flags: review?(bienvenu)
Attachment #408437 - Flags: ui-review?(clarkbw)
Attachment #408437 - Flags: superreview?(neil)
Attachment #408437 - Flags: review?(bienvenu)

Comment 13

8 years ago
+1 for this feature, on a per-account basis!
just to note, bug 533815 is not the same but similar
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.
Attachment #408885 - Flags: ui-review?(clarkbw) → ui-review+
(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

7 years ago
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.
(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.

Updated

7 years ago
Attachment #408885 - Flags: superreview?(neil) → superreview+

Comment 19

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

this patch is severely bit-rotted. I'll try to revive it.
Attachment #408885 - Attachment is obsolete: true
Attachment #408885 - Flags: review?(bienvenu)

Comment 20

7 years ago
Created attachment 483002 [details] [diff] [review]
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.
Attachment #483002 - Flags: review?(bugzilla)

Comment 21

7 years ago
Created attachment 483007 [details] [diff] [review]
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.
Attachment #483002 - Attachment is obsolete: true
Attachment #483007 - Flags: review?(bugzilla)
Attachment #483002 - Flags: review?(bugzilla)
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

7 years ago
Created attachment 483166 [details] [diff] [review]
fix drive by comments
Attachment #483007 - Attachment is obsolete: true
Attachment #483166 - Flags: review?(bugzilla)
Attachment #483007 - Flags: review?(bugzilla)
Created attachment 484289 [details] [diff] [review]
Final fix for SM parts

This fixes up the SM parts so that they work again. This has r=Standard8,bienvenu.
Attachment #483166 - Attachment is obsolete: true
Attachment #484289 - Flags: review+
Attachment #483166 - Flags: review?(bugzilla)
Keywords: checkin-needed
(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.
Summary: Provide UI for new archive granularity option → Expand archive granularity option backend to support UI
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
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 3.3a1
(Reporter)

Comment 27

7 years ago
> (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.  ;-)
(Reporter)

Updated

7 years ago
Blocks: 607295
(Reporter)

Comment 28

7 years ago
Continued in bug 607295, please comment there for the UI implementation.
Blocks: 476217

Updated

7 years ago
Depends on: 608789
Depends on: 622794

Updated

6 years ago
Depends on: 640342

Updated

6 years ago
Duplicate of this bug: 573336
You need to log in before you can comment on or make changes to this bug.