640147 - Port |Bug 542998 - Need option to disable Archiving completely|

Status: RESOLVED FIXED

(SeaMonkey :: MailNews: General, defect)

Description

[Jens Hatlak (:InvisibleSmiley)]

Thunderbird just added a pref for disabling archiving completely in bug 542998. This pref will be exposed in the Account Manager UI later so we should have it, too.

Parts to be ported from files listed in
http://hg.mozilla.org/comm-central/rev/0347b42406e6

mail3PaneWindowCommands.js
mailWindowOverlay.js
messageWindow.js
nsContextMenu.js

The rest is either stuff we don't have, tests, back-end or shared (like the default pref in mailnews.js).

Comment 1

[Jens Hatlak (:InvisibleSmiley)]

messageWindow.js: Forget it, we don't have an Archive button.
nsContextMenu.js: Our file is called mailContextMenus.js.

Comment 2

[Jens Hatlak (:InvisibleSmiley)]

Attachment: patch

Note that this slightly changes the logic (in sync with TB): The first message's folder's server's archiveKeepFolderStructure setting is no longer taken into account (see the removed comments).

Comment 3

[Karsten Düsterloh]

Comment on attachment 518104 [details] [diff] [review]
patch

>-  ShowMenuItem("mailContext-archive", showMailItems && oneOrMore);
>+  var canArchive = CanArchiveMsg();
>+  ShowMenuItem("mailContext-archive", showMailItems && oneOrMore && canArchive);

No need for the variable.

>+ * Checks if the selected messages can be archived. This depends on what folder
>+ * they're in and whether archiving is enabled for that identity.

s/what/which/ (I think)

>+  return gFolderDisplay.selectedMessages.every(function(msg) {
>+      return GetIdentityForHeader(msg).archiveEnabled;

Permission granted to make it one long line or move the { on its own line. ;-)

Activating this now will mean that all archiving will cease to function, because archiveEnabled will be false in old profiles? What is the migration path?

Comment 4

[Jens Hatlak (:InvisibleSmiley)]

(In reply to comment #3)
> No need for the variable.

Sure; I was just looking at the context, but in fact even had it inlined at one point in time locally.

> >+ * Checks if the selected messages can be archived. This depends on what folder
> >+ * they're in and whether archiving is enabled for that identity.
> 
> s/what/which/ (I think)

I'd say "what" refers to the folder type (as in "what kind of" -> Archive) rather than a certain folder ("which" -> "Archive"). But I won't get into a fight over this, so if you really want it you'll get it. ;-)

> >+  return gFolderDisplay.selectedMessages.every(function(msg) {
> >+      return GetIdentityForHeader(msg).archiveEnabled;
> 
> Permission granted to make it one long line or move the { on its own line. ;-)

In that case even the "return " can go, no?

> Activating this now will mean that all archiving will cease to function,
> because archiveEnabled will be false in old profiles? What is the migration
> path?

Checked current TB trunk with an existing profile: works. From what I can tell, if there is no specific setting, the value of the default pref is used (which is in mailnews.js and set to true):
http://hg.mozilla.org/comm-central/rev/0347b42406e6 (nsMsgIdentity.cpp):
NS_IMPL_IDPREF_BOOL(ArchiveEnabled, "archive_enabled") -> http://mxr.mozilla.org/comm-central/search?string=NS_IMPL_IDPREF_BOOL -> GetBoolAttribute -> http://mxr.mozilla.org/comm-central/source/mailnews/base/util/nsMsgIdentity.cpp#487

BTW: Base bug 542998 is still in flux, will wait for the final checkin which will move the check function to folderDisplay.js (gFolderDisplay). We should definitely stay in sync here; rushing it now would only cost me time later.

Comment 5

[[e-mail]]

Comment on attachment 518104 [details] [diff] [review]
patch

>+function CanArchiveMsg()
I would have expected this to have been called MsgCanArchive() ;-)

>+  if (gFolderDisplay.selectedCount == 0)
>+    return false;
Hmm, this isn't exactly equal to the previous test (selectedMessages.length)

>+  return gFolderDisplay.selectedMessages.every(function(msg) {
>+      return GetIdentityForHeader(msg).archiveEnabled;
>+    });
Nit: incorrect indentation; only 2 spaces for code in an inline function.

Comment 6

[Jens Hatlak (:InvisibleSmiley)]

Attachment: patch v2 [Checkin: comment 7]

New version now that attachment 518416 [details] [diff] [review] has a positive review

Comment 7

[Jens Hatlak (:InvisibleSmiley)]

Comment on attachment 518928 [details] [diff] [review]
patch v2 [Checkin: comment 7]

http://hg.mozilla.org/comm-central/rev/59cc6cab8244