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

RESOLVED FIXED in seamonkey2.1a2

Status

SeaMonkey
MailNews: General
--
enhancement
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: InvisibleSmiley, Unassigned)

Tracking

Trunk
seamonkey2.1a2

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

7 years ago
Created attachment 449721 [details] [diff] [review]
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.
Attachment #449721 - Flags: review?(mnyromyr)

Comment 1

7 years ago
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
Attachment #449721 - Flags: superreview+
Attachment #449721 - Flags: review?(mnyromyr)
Attachment #449721 - Flags: review+
Attachment #449721 - Flags: feedback?(philip.chee)

Comment 2

7 years ago
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?
Attachment #449721 - Flags: feedback?(philip.chee)

Comment 3

7 years ago
> 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.
(Reporter)

Comment 4

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

7 years ago
> 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.
(Reporter)

Comment 6

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

http://hg.mozilla.org/comm-central/rev/4b59c5e973f6
Attachment #449721 - Attachment description: patch → patch [Checkin: comment 6]
(Reporter)

Updated

7 years ago
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.1a2
You need to log in before you can comment on or make changes to this bug.