Last Comment Bug 570578 - Port |Bug 559040 - Make the internal archiving code work for any set of messages thus being nicer to extensions| to SeaMonkey
: Port |Bug 559040 - Make the internal archiving code work for any set of messa...
Status: RESOLVED FIXED
:
Product: SeaMonkey
Classification: Client Software
Component: MailNews: General (show other bugs)
: Trunk
: All All
: -- enhancement (vote)
: seamonkey2.1a2
Assigned To: Nobody; OK to take it and work on it
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2010-06-07 14:29 PDT by Jens Hatlak (:InvisibleSmiley)
Modified: 2010-06-19 14:53 PDT (History)
1 user (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch [Checkin: comment 6] (3.62 KB, patch)
2010-06-07 14:29 PDT, Jens Hatlak (:InvisibleSmiley)
mnyromyr: review+
mnyromyr: superreview+
Details | Diff | Review

Description Jens Hatlak (:InvisibleSmiley) 2010-06-07 14:29:39 PDT
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.
Comment 1 Karsten Düsterloh 2010-06-17 14:56:58 PDT
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 2010-06-17 19:07:26 PDT
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 2010-06-18 13:37:31 PDT
> 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) 2010-06-18 13:59:54 PDT
(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 2010-06-19 13:57:57 PDT
> 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) 2010-06-19 14:52:39 PDT
Comment on attachment 449721 [details] [diff] [review]
patch [Checkin: comment 6]

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

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