570578 - Port |Bug 559040 - Make the internal archiving code work for any set of messages thus being nicer to extensions| to SeaMonkey

Status: RESOLVED FIXED

(SeaMonkey :: MailNews: General, enhancement)

Description

[Jens Hatlak (:InvisibleSmiley)]

Attachment: patch [Checkin: comment 6]

From Bug 559040:
"The BatchMessageMover only works for gFolderDisplay.selectedMessages which is a
pity because other extensions might need to archive messages as well. As the
code is very dense and complicated, it is unlikely that anyone will ever
maintain a copy of this code.

Please find a patch attached that tries to fix this by adding an extra aMsgHdrs
parameter. (...) I changed the function name so that
extensions can easily test if the new feature is available."

Changeset: <http://hg.mozilla.org/comm-central/rev/2ff7872f4d1a>

In the attached patch I changed the BatchMessageMover internal function names accordingly to achieve compatibility with TB.

Comment 1

[Karsten Düsterloh]

Comment on attachment 449721 [details] [diff] [review]
patch [Checkin: comment 6]

> In the attached patch I changed the BatchMessageMover internal function names
> accordingly to achieve compatibility with TB.

What's even worse than having lowercase member function names in an object is having mixed case function names.  :-/
But given the greater good 'extension compatibility', we may as well connive at that her.
Asking Philip for quick feedback.

Otherwise, r/moa=me

Comment 2

[Philip Chee]

Comment on attachment 449721 [details] [diff] [review]
patch [Checkin: comment 6]

If this is supposed to be a public API in Thunderbird then in the interest of extension cross-app compatibility I'll give this a virtual f+ on the naming changes. I don't know mailnews code well enough to comment on the other changes so removing feedback request.

Just some random comments of no import.

-  ArchiveSelectedMessages: function()
+  archiveMessages: function(aMsgHdrs)

Appears to be a helper function only used in MsgArchiveSelectedMessagesm (see later).

-  ProcessNextBatch: function()
+  processNextBatch: function()

Internal helper function used only inside BatchMessageMover

>  function MsgArchiveSelectedMessages(event)
>  {
>    let batchMover = new BatchMessageMover();
> -  batchMover.ArchiveSelectedMessages();
> +  batchMover.archiveMessages(gFolderDisplay.selectedMessages);
>  }

So is |event| ever referenced?

Comment 3

[Karsten Düsterloh]

> If this is supposed to be a public API in Thunderbird then in the interest of
> extension cross-app compatibility I'll give this a virtual f+ on the naming
> changes.

That was the question in question. :)

> >  function MsgArchiveSelectedMessages(event)
> >  {
> >    let batchMover = new BatchMessageMover();
> > -  batchMover.ArchiveSelectedMessages();
> > +  batchMover.archiveMessages(gFolderDisplay.selectedMessages);
> >  }
> 
> So is |event| ever referenced?

Extensions can easily overload functions, but exchanging attributes (which might even appear where you don't know) is hard. Especially with event handlers it's good practice, imo, to pass down the event so extension authors can just overload the function and need not to worry about how to get the event...

That said, the parameter should be named aEvent, of course.

Comment 4

[Jens Hatlak (:InvisibleSmiley)]

(In reply to comment #3)
> That said, the parameter should be named aEvent, of course.

So, should I change it as part of this bug, despite the already given r+moa=you?

Comment 5

[Karsten Düsterloh]

> So, should I change it as part of this bug, despite the already given
> r+moa=you?

Yes, please, you can do so on check-in.

Comment 6

[Jens Hatlak (:InvisibleSmiley)]

Comment on attachment 449721 [details] [diff] [review]
patch [Checkin: comment 6]

http://hg.mozilla.org/comm-central/rev/4b59c5e973f6