Closed Bug 522761 Opened 10 years ago Closed 10 years ago

Need option 'keep folders scheme' under the archive feature to don't forget the messages organization

Categories

(Thunderbird :: Mail Window Front End, enhancement)

enhancement
Not set

Tracking

(thunderbird3.1 beta2-fixed)

RESOLVED FIXED
Tracking Status
thunderbird3.1 --- beta2-fixed

People

(Reporter: iagosrl, Assigned: chrissc.humbert)

References

(Blocks 2 open bugs)

Details

(Whiteboard: [UXprio])

Attachments

(3 files, 9 obsolete files)

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.0.14) Gecko/2009090216 Ubuntu/9.04 (jaunty) Firefox/3.0.14
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1.4pre) Gecko/20091007 Shredder/3.0pre

A new boolean option to the archive feature that allows re-create the folders scheme when a message is archived from the original location.

And example: 
   have a message under the folder 'Projects' and inside under the folder 'Project X' (a filter move message to here),
   click hover 'archive message' and this go to the defined archive folder (for example, inside local folders -> Archived);
  
 this is the current behavior, but with the new option,

   thunderbird must move the message into the 'Archived' folder -> Projects -> Project X (if the folders don't exists, are created auto).

Reproducible: Always
confirming RFE. 

Bryan is this something that would make sense ?
Blocks: 473212
Severity: minor → enhancement
Status: UNCONFIRMED → NEW
Component: General → Mail Window Front End
Ever confirmed: true
QA Contact: general → front-end
I want to second this.

I archive my emails manually into an archive folder with an identical folder structure since TB 2.

The question is not if this makes sense, which it does. To me it's obvious that dumping all messaged into one folder through the archive feature, like it is done now, doesn't make sense.

The archive feature right now is only useful for people with low email volume or who don't organize their email at all.
As previously stated, the current scheme does not make any sense (why does it put messages into a folder named for the year of the first message moved?). As stated above, it also loses the folder structure.
(In reply to comment #3)
> As previously stated, the current scheme does not make any sense (why does it
> put messages into a folder named for the year of the first message moved?).
That's a bug that has been fixed in nightly builds and should be fixed in 3.01.
I'd love to see several options:

[ ] don't archive from this folder (for unsorted mails - mostly inbox)

[ ] archive to a custom archive folder (+ year/month) [Folder...]

[ ] keep folder hierarchy (year/month + hiearchy)

Archive granularity is probably enough to be global, as it is now.


However, it seems to me that:
 - archives
 - folders
 - tags
are somewhat orthogonal issues. there's no perfect way to organize things in the current folder based only system (without perf issues and many views anyway).
I feel like I just commented on this, bug 533815
Also magnus made some progress on a granularity UI earlier in bug 517514
Duplicate of this bug: 533815
Hello,

As said in 533815, I have extended an existing extension and it works under Exchange Imap and Cyrus Imap server.

If you want it ask for it

I still have an issue with the XUL part.
Kind Regards
Attached file First proposal (obsolete) —
I have an extension somehow working.

This is a proposal to inculde the function directly in thunderbird. Need UI to set preferences and creation of one preference.

Kind Regards
Hi,
 Thank you Christophe for your work.

 I am testing it and works well with elements (mails and folders) under INBOX folder (I have defined 2 levels of granularity), but with elements in another 'main' folder ('Send Items', or another custom folder sibling of Inbox) Thunderbird freezes it, use more and more memory and I need close it with the task manager -windows xp-. When restart Thunderbird, I can see that under the archive folder there are three new folders: /2010/02/02 (duplicate the folder of the day), all empty.
 Testing without levels (single folder option), keep folders structure don't work.

Thank you again
Hello Iago,

What type of servers are you using Imap or pop and if imap servers could you precise which one 
Do you have any message in error console or anything

I have not tested with the sent folders.

I have tested only with many subfolders under the inox one up to 5 levels and yes sometimes it was somehow frozen but i have no clue on what is the reason, maybe in the imap folder creation process.

I have some ideas to improve the code but it will help if you have any error message and if someone more acquainted with TB code could have a quick look because it is my first extension and back to coding after 5 years...

Kind REgards
Christophe do you intend to release it on AMO ?
Hello Ludovic,

In fact it is existing on AMO as I am not the initial developper, but not in this version. I have only extended it to take into account to try to keep the folder structure and the initial dev told me it had not time ...

Plus there is a small XUL issue, I was not able to solve

And as you see it appears that it does not work for everybody

Kind Regards
Hi Christophe,

 My server is IMAP Zarafa (http://www.zarafa.com/) version 6.30.9 Community (last stable).
 I tested again, disabling all extensions except yours:
 - When mail is in INBOX folder or any subfolder of INBOX: no problem, no alert in Error Console.
 - When mail is in another folder sibling of INBOX or subfolder of folder sibling of INBOX (like Send Items, or another you can create manually): Thunderbird freeze, memory consumption increases in more than double and then crash, the Mozilla Crash Report Agent appear and Crash ID: bp-febbcc90-0179-4d33-bbed-d75a12100211. Thunderbird restart and I cannot see any previous messages in Error Console.

 I think the problem can be related to the feature: when you 'Archive' a message in the INBOX, this is moved to the archive folder directly (not in a 'inbox folder' under the 'archive folder', example: ArchiveFolder/2010/02/Inbox/). Can be?

 I am learning how create a Thunderbird extension too and working in create my first.

 Thanks
Hello Iago,

I think I know where the problem is. I have no time before end of next week to work on it...so please be patient 

Kind Regards
I filed comment 16's crash as bug 545657
Whiteboard: [UXprio]
Attached patch Diif for patch (obsolete) — Splinter Review
Hello

Here is the diff. I have added a preference defaulted to false.
This take care of preserving folder structure in case pref is set to true.
It will need UI as the rest of Archive settings/preferences

Hope This helps

PS: Don't be too harsh, I have not done js for years
Attachment #424968 - Attachment is obsolete: true
Attachment #426003 - Attachment is obsolete: true
Attachment #433901 - Flags: review?(bienvenu)
Attached patch Previous was wrong (obsolete) — Splinter Review
Same comment as previous one
Attachment #433901 - Attachment is obsolete: true
Attachment #433902 - Flags: review?(bienvenu)
Attachment #433901 - Flags: review?(bienvenu)
Hello,

There is no test as I Don't know how to write one. If I can have help or someone want to write one, I would be grateful

Thanks
(In reply to comment #21)
> Hello,
> 
> There is no test as I Don't know how to write one. If I can have help or
> someone want to write one, I would be grateful

Look into mail/test/mozmill where we have plenty of examples and documentation is at : https://developer.mozilla.org/en/Thunderbird/Thunderbird_MozMill_Testing
Attached file Mozmill test case
Hello,

Included in this attachment a simplified mozmill test case with 3 levels of folder
No taking into account special folders like trash/junk/sent etc...

Kind Regards
Attachment #434548 - Flags: review?(bienvenu)
(In reply to comment #23)
> Created an attachment (id=434548) [details]
> Mozmill test case

Bienvenu ping for reviews.
Comment on attachment 433902 [details] [diff] [review]
Previous was wrong

thx for the patch, sorry for the delay - can I get a patch without tabs? We use two space indents, no tabs...

this comment isn't adding any info so we should just remove it:
+// New pref to keep folder structure
+pref("mail.server.default.archive_keep_folder_structure", false);

space after the // here:

+			//Detect the root folder of message

this var name is ambiguous. Something like startFolderLevel might be clearer. Initial might be better than start, also.

 This detects the most common case of INBOX. being the personal namespace, but you might want a more generic test here. It's a bit tricky because personal namespace uses the server's hierarchy delimiter (usually either '.' or '/') but the URI uses the canonical delimiter of '/'. I guess since this pref defaults to false, we don't have to worry so much about other personal namespaces at the moment...
+			if (folderURI[1] == "INBOX") 
+				startI = 2;

+	  let startI = 1;
Attachment #433902 - Flags: review?(bienvenu) → review-
Assignee: nobody → c.humbert
Status: NEW → ASSIGNED
Hello David,

I have adressed your first comments.
As far as personal namespace, I don't know how to get around this for the moment

Kind Regards
Attachment #433902 - Attachment is obsolete: true
Attachment #435648 - Flags: review?(bienvenu)
Attachment #434548 - Attachment mime type: application/x-javascript → text/plain
Comment on attachment 434548 [details]
Mozmill test case

is this supposed to be a replacement for test-message-commands.js? I don't see it changing the default pref for whether the folder hierarchy is preserved, which I think it would need to work...
Comment on attachment 435648 [details] [diff] [review]
Updated patch with space replacing tab

this patch doesn't apply - patch complains about the lines starting here:

diff --git a/mail/base/content/mailWindowOverlay.js b/mail/base/content/mailWindowOverlay.js
--- a/mail/base/content/mailWindowOverlay.js
+++ b/mail/base/content/mailWindowOverlay.js
@@ -1328,22 +1328,30 @@ BatchMessageMover.prototype = {

I'll try to see if I can tweak it locally.

I'm sorry but I messed up my comments previously. I was hoping we could have a better variable name than startI, something like initialFolderLevel.
Hi Christophe, I've attached a patch that applies cleanly (it looks like you edited the diff directly previously, which is very tempting, but can easily break the diff).

More importantly, your previous patch broke the non-folder-level archiving, such that we weren't setting the dstFolder if you had the default yearly archiving set. I've tweaked your patch a little to fix that. My understanding is that this is an either/or setting - if you pick the folder-level archiving, we ignore the granularity setting, so I re-ordered the code so that we check that first, and if not set, fall back to the old code (you had stuck the folder-level archiving in the middle of the other checks, which didn't seem right). I also cleaned out a remaining tab, and some spaces at the end of lines, and the variable name I mentioned before.

It would be great if you could test this version of the patch and make sure it works the way you want it to, and let me know. Thx!
Attachment #435648 - Attachment is obsolete: true
Attachment #435648 - Flags: review?(bienvenu)
Comment on attachment 434548 [details]
Mozmill test case

clearing review request until I'm clear on how this is supposed to work.
Attachment #434548 - Flags: review?(bienvenu)
Hello David,

Thanks for the work you have done and will do better next time I hope.

I will test your patch and give you my comments.

Kind regards
Hello David,

I have setup my linux machine and made a build with your patch (newbie on linux eheh but i have suceeded.)

The patch works well but it seems that the fact to keep folder structure overrides the archive year/month feature.

What I had in mind first was more something like that:

Folder A
   --> Folder A.1
      ---> Folder A.2
          Message 1 Year x Month 2
          Message 2 Year x Month 5
          Message 3 Year y Month 6

To have as archives if granularity set to 2
Archive
   ---> Year X
     --> Month 2
      --> Folder A (if A different from Inbox)
       --> Folder A.1
        --> Folder A.2
            Message 1
     --> Month 5
      --> Folder A (if A different from Inbox)
       --> Folder A.1
        --> Folder A.2
            Message 2

That is why my first patch put the folder structure after the year/month test.
Do you want me to try a new patch now that I have A linux box?

Kind REgards
I would suggest to introduce a formatting string so that the
archive structure can be easily customized by users. Having
a fixed structure is asking for trouble.

It could be an idea to have a pref like
'mail.server.default.archive_folder_structure_format' where
you can specify strftime-like formatting to suit your needs.
Define '/' to be the folder separator, and you can have things
like:

  "%Y/%m/"     -> 2010/04/<message>
  "%Y-%m/"     -> 2010-04/<message>
  "%Y/%B/"     -> 2010-April/<message>
  "%Y/%V/"     -> 2010/14/<message>    # ISO 8601 week number
  "Arch %Y/"   -> Arch 2010/<message>

Sorry for only supplying opinion and not code.
Hello Sven,

Good point, I think we should follow your proposal in a follow-up bug.
Christophe, ah, ok; I did not know what you intended the patch to do. I can try to rearrange the code so that it does that...
David,

I have made a build with this patch and it works as I intented. 

Kind REgards
Hi Christophe, 
  Did you make sure it works if the "keep folder scheme" pref is turned off?

thx, - David
Hello David,

Yes I tested with pref turned off.

Do we make a follow-up bug for personal namespace and another one for sven suggestion?

Kind Regards
(In reply to comment #39)
 
> Do we make a follow-up bug for personal namespace and another one for sven
> suggestion?

yes !
Hello,

I have added a follow up bug i.e bug Bug 558332 for Sven proposal.
I am waiting for David comment for namespace follow up bug.
Depends on: 558123
Duplicate of this bug: 546292
Strictly speaking the dependency I just added isn't quite standard, but whatever....

At least this is moving along well!
Attached patch patch with mozmill test (obsolete) — Splinter Review
this tweaks the prev patch a bit to implement the per server pref for whether we keep folder structure on archiving, and extends the existing mozmill test to test the new pref.
Attachment #437554 - Attachment is obsolete: true
Attachment #438413 - Flags: superreview?(bugzilla)
Attachment #437554 - Flags: review?(bienvenu)
Christophe, one concern I have is if you try to archive messages that are already in the archive hierarchy, which I think we allow so people can re-archive after changing archiving prefs. Can you try that? I suspect we may end up duplicating the archive hierarchy when we do that with the keep folder structure pref turned on. Thx!
Hello David,

I am off this week and away with limited connectivity. Could it wait for Monday 19th to be included in 3.1 or do you need it before.
BTW without testing I think you guess it right. 
But Generally speaking could we disable the archive button in archive folders?

Kind Regards
I have tested and once the message archived the archive button is not present so it is not possible to rearchive the mail.

Kind REgards
Christophe, thx for checking. However, the archive command ('A') is still enabled and does what we suspected. We don't disable archive so that people can re-archive after changing the prefs. We're likely to have a code freeze on Tuesday the 20th, though it might slip a day or two, but the 19th might be cutting it close. But if it doesn't make beta 2, we might be able to slip it in after beta 2, since the pref is not on by default and has no UI, yet.
Hello David,

What is your proposal? It is when under archives to disabled the command ('A') or to treat that in the code i.e detect when we are under the archives folder and tell the user that his message(s) are already archived?


Kind Regards
Hi Christophe,

My thinking was that in the case of a message in a folder under the Archives folder, we strip off the Archives folder hierarchy, though that obviously begs the question of which folder to use as the sub-parent in the Archives hierarchy, since we don't know where the message came from. Given that, I think we need to disable the archive command when keep folder structure is turned on and the selected message(s) are already under the Archives folder.
David,

That was also my idea. How to implement that?

Kind REgards
(In reply to comment #51)
> David,
> 
> That was also my idea. How to implement that?
> 
> Kind REgards

Look for cmd_archive in mail3PaneWindowCommands.js

right now, it's enabled when

        (gFolderDisplay.selectedCount > 0 );

if we're keeping folder structure, we want it enabled like button_archive is
        
        return gFolderDisplay.selectedCount > 0 && gFolderDisplay.displayedFolder &&
          !gFolderDisplay.displayedFolder.isSpecialFolder(
             Components.interfaces.nsMsgFolderFlags.Archive, true);

to tell if we're keeping folder structure, you could look at the first selected message,

        let selectedMessages = gFolderDisplay.selectedMessages;
        selectedMessages.length > 0 && selectedMessages[0].folder &&
               selectedMessages[0].folder.server.archiveKeepFolderStructure
Hello David,

Why not disabled it generally speaking when a message is already archived and not only when the keep folderstructure is enabled?

Kind Regards
(In reply to comment #53)

> Why not disabled it generally speaking when a message is already archived and
> not only when the keep folderstructure is enabled?

Because, when people change the granularity from monthly to yearly, for example, and they want to re-archive, they can do that now.
(In reply to comment #55)
> Because, when people change the granularity from monthly to yearly, for
> example, and they want to re-archive, they can do that now.

Will the button/menu-item say "re-archive" when that scenario/context is the likely/relevant one? Will the tool-tip explain it? (e.g., "re-archive your archive with the new settings")
Hello David,

I will Change my patch in your sense... but maybe we should do it for the button archive because for now the possibility to re-archive is limited to people knowing the command. What is your thinking?

Kind Regards
Hello David,

Here is the patch as requested.

The only thing worrying me is that even before the keepfolderstructure patches the A command and Archive button enabling/disabling were not aligned.

What are you thinking about an alignment?

Kind Regards
Attachment #439773 - Attachment is obsolete: true
Attachment #440171 - Flags: review?(bienvenu)
Attachment #439773 - Flags: review?(bienvenu)
Comment on attachment 440171 [details] [diff] [review]
Disable the A command when keepFolderStructure is on

you're missing the check that the displayed folder is the archive folder...I'll try to come up with a quick fix.
Attachment #440171 - Flags: review?(bienvenu) → review-
Hello David,

I have tested with a build and it was working...

Nice if you could improve
cumulative patch - Christophe, does this work for you?

Re inconsistency between button and archive command, it already exists and this doesn't make it any worse.
Attachment #438413 - Attachment is obsolete: true
Attachment #440171 - Attachment is obsolete: true
Attachment #438413 - Flags: superreview?(bugzilla)
Comment on attachment 440241 [details] [diff] [review]
fix enabling of archive

sr requested for the mailnews/base part of this patch, which I wrote...
Attachment #440241 - Flags: superreview?(bugzilla)
Attachment #440241 - Flags: review+
Ok David,

I will test tomorrow the patch you wrote, sorry not to be as good as you expected.

I understant this does not make it worse but the A command and the button arhive way of being enabled shouldn't they be aligned?

Thanks for supporting me and bearing my poor coding during this patch
Christophe, thx very much for all your work on this.  It's much appreciated.

Re aligning the two commands, one reason not to do so is that if we disable the archive button cmd, we remove the button from the message header area, which gives more room for other things in that area. Re-archiving an existing archived message using the archive button in the message header area is pretty unlikely. But we'd like the archive shortcut to still work. But that also disables the archive toolbar button, unfortunately.
Ok I understand and thanks for the explanation.

I will provide results of tests tomorrow morning for me i.e Central European Time.
Comment on attachment 440241 [details] [diff] [review]
fix enabling of archive

sr=Standard8 for the mailnews parts.
Attachment #440241 - Flags: superreview?(bugzilla) → superreview+
Hello David,

It is working like a charm. Sorry for late testing. I had a hectic day.
For information, here are the tests done
Keepfolderstructure activated: A command disabled
Keepfolderstructure deactivated: A command enabled and when changing archive granularity from 2 to 1 or 1 to 2 the re-archiving process is working well.

Kind Regards
bienvenu said he forgot to land this before the freeze. As this isn't available by default and it should be low risk, a=Standard8 for landing in the tree today.
fixed for beta2. Thx, Christophe.
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Thank to you David
Hi!

Because was not ready before the freeze, there is no strings nor UI for this feature, ok, but my question is: We must open a new bug for UI implementation or reopen this? What is better?

Thanks very much for the work!
A new bug (with a comment here pointing to it) is generally the recommended strategy.
I opened the Bug 573336: 'Option "keep folders structure when archive" needs UI' like proposed Jim.
Note that this new type of archiving is completely broken with respect to folders with international characters, spaces etc. in the name, because the actual folder name cannot be extracted from folder.URI in those cases. See bug 573392 for how we will fix it for SeaMonkey. You might want to back-port/adapt that.
Hello Guys,

The names of the automatically created archive folders are still broken if they contain international characters. (Mozilla/5.0 (X11; U; Linux i686; sk; rv:1.9.2.8) Gecko/20100802 SUSE/3.1.2 Thunderbird/3.1.2). Is there a possibility for fix this soon. It really crates a lot of problems for users that have folders contain such characters.
Has this i18n-bug been fixed? before I turn archiving loose on my 100.000+ emails - I'd like to know Thunderbird won't mess it up - due to this bug with i18n characters in folder names.
You need to log in before you can comment on or make changes to this bug.