Last Comment Bug 640147 - Port |Bug 542998 - Need option to disable Archiving completely|
: Port |Bug 542998 - Need option to disable Archiving completely|
Status: RESOLVED FIXED
:
Product: SeaMonkey
Classification: Client Software
Component: MailNews: General (show other bugs)
: Trunk
: All All
: -- normal (vote)
: seamonkey2.1b3
Assigned To: Jens Hatlak (:InvisibleSmiley)
:
Mentors:
Depends on: 542998
Blocks: 651629
  Show dependency treegraph
 
Reported: 2011-03-09 04:32 PST by Jens Hatlak (:InvisibleSmiley)
Modified: 2011-04-20 14:26 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch (4.25 KB, patch)
2011-03-09 10:20 PST, Jens Hatlak (:InvisibleSmiley)
no flags Details | Diff | Review
patch v2 [Checkin: comment 7] (3.97 KB, patch)
2011-03-12 02:22 PST, Jens Hatlak (:InvisibleSmiley)
mnyromyr: review+
neil: superreview+
Details | Diff | Review

Description Jens Hatlak (:InvisibleSmiley) 2011-03-09 04:32:16 PST
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) 2011-03-09 09:51:27 PST
messageWindow.js: Forget it, we don't have an Archive button.
nsContextMenu.js: Our file is called mailContextMenus.js.
Comment 2 Jens Hatlak (:InvisibleSmiley) 2011-03-09 10:20:05 PST
Created attachment 518104 [details] [diff] [review]
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 2011-03-10 14:35:30 PST
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) 2011-03-10 16:26:17 PST
(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 neil@parkwaycc.co.uk 2011-03-11 14:33:03 PST
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) 2011-03-12 02:22:16 PST
Created attachment 518928 [details] [diff] [review]
patch v2 [Checkin: comment 7]

New version now that attachment 518416 [details] [diff] [review] has a positive review
Comment 7 Jens Hatlak (:InvisibleSmiley) 2011-03-14 12:44:22 PDT
Comment on attachment 518928 [details] [diff] [review]
patch v2 [Checkin: comment 7]

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

Note You need to log in before you can comment on or make changes to this bug.