Expand archive granularity option backend to support UI

RESOLVED FIXED in Thunderbird 3.3a1

Status

--
enhancement
RESOLVED FIXED
10 years ago
8 years ago

People

(Reporter: rsx11m.pub, Assigned: mkmelin)

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

10 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

10 years ago
Posted patch proposed fix (obsolete) — Splinter Review
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

10 years ago
Posted image screenshot of the fix (obsolete) —
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

10 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

10 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

10 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

10 years ago
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

10 years ago
Unfinished frontend for testing only.

Comment 9

10 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

10 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

10 years ago
Posted patch proposed fix, v3 (backend) (obsolete) — Splinter Review
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

10 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

10 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

10 years ago
Posted patch proposed fix, v4 (backend) (obsolete) — Splinter Review
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

10 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

9 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.
Attachment #408885 - Flags: superreview?(neil) → superreview+

Comment 19

9 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

9 years ago
Posted patch un-bitrotted patch (obsolete) — Splinter Review
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

9 years ago
Posted patch remove unused default prefs (obsolete) — Splinter Review
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

9 years ago
Posted patch fix drive by comments (obsolete) — Splinter Review
Attachment #483007 - Attachment is obsolete: true
Attachment #483166 - Flags: review?(bugzilla)
Attachment #483007 - Flags: review?(bugzilla)
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: 9 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 3.3a1
(Reporter)

Comment 27

9 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

9 years ago
Blocks: 607295
(Reporter)

Comment 28

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

Updated

9 years ago
Blocks: 476217

Updated

9 years ago
Depends on: 608789
Depends on: 622794

Updated

8 years ago
Depends on: 640342

Updated

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