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

Status: RESOLVED FIXED

(Thunderbird :: General, defect)

Description

[Jonathan Protzenko [:protz]]

User-Agent:       Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.3a4) Gecko/20100413 Minefield/3.7a4
Build Identifier: Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.1.9) Gecko/20100413 Shredder/3.0.4

http://mxr.mozilla.org/comm-central/source/mail/base/content/mailWindowOverlay.js#1301

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. As this function is only called in one place, namely http://mxr.mozilla.org/comm-central/source/mail/base/content/mailWindowOverlay.js#1508, it is trivial to port the rest of the code. I changed the function name so that extensions can easily test if the new feature is available.

I did not find any other occurrences of BatchMessageMover so I think this should be just fine (http://mxr.mozilla.org/comm-central/search?string=BatchMessageMover).

sid0 raised some concerns on IRC which I'm afraid I didn't understand fully (sorry) so if anyone can explain the quote below I'd be very indebted to them

<sid0> protz: so I also wonder whether it'll require changes to http://mxr.mozilla.org/comm-central/source/mail/base/content/folderDisplay.js#1043
<sid0> protz: at a quick glance the logic there in case this._nextViewIndexAfterDelete is non-null looks a little iffy

jonathan.

Reproducible: Always

Comment 1

[Jonathan Protzenko [:protz]]

Attachment: Add an extra parameter and rename archiveSelectedMessages

Comment 2

[Rain [:rain1] (inactive)]

(mail/ bugs are generally Thunderbird)

Comment 3

[Rain [:rain1] (inactive)]

Comment on attachment 438747 [details] [diff] [review]
Add an extra parameter and rename archiveSelectedMessages

>diff -r 5e3d80a05323 mail/base/content/mailWindowOverlay.js
>--- a/mail/base/content/mailWindowOverlay.js	Mon Apr 12 23:39:58 2010 +0200
>+++ b/mail/base/content/mailWindowOverlay.js	Tue Apr 13 15:33:10 2010 +0200
>@@ -1296,20 +1296,19 @@
> 
> BatchMessageMover.prototype = {
> 
>-  archiveSelectedMessages: function()
>+  archiveMessages: function(aMsgHdrs)

Please name this function something like BatchMessageMover_archiveMessages so that it's easier to debug call stacks.

This looks like it'll easily be testable, so by our test policy. Please add a MozMill test for moving messages. It'd be nice if you covered moving messages from different folders and so on.

I am indeed concerned about onMessagesRemoved not doing the right thing. I think I'd be more confident about it if the right message turns out to be selected in the end. So you'd have a test that does move the currently displayed message into the folder, in which case the next message would be selected, and you'd have a test that doesn't touch the currently displayed message, in which case the selection shouldn't change. You'd also ideally have tests covering selections of multiple messages being partially or completely disrupted. <http://mxr.mozilla.org/comm-central/source/mail/test/mozmill/folder-display/test-right-click-middle-click-messages.js#432> should be a good start, except that you want to archive particular messages and not delete them.

Comment 4

[Jonathan Protzenko [:protz]]

Attachment: New patch with comments taken into account and mozmill test added

Here's a new version of the patch, with the anonymous functions given a name and a mozmill test. The test covers the following scenarios:
- archiving part of a thread
- archiving the root message of a thread
- archiving a whole thread
- archiving messages hidden under a collapsed thread
- archiving part of a multiple message selection

When messages [M1, M2, M3] are selected, if I archive M2, then only M1 ends up selected. This is the last part of the test, I only assert for M1 being selected, not M3. Is this a bug or a feature?

Comment 5

[Jonathan Protzenko [:protz]]

David, any chance you can have a quick look at this? I'd like to get that in before rc1 because I believe this can be a nice extra for addon developers to be able to hook into that archiving code.

Comment 6

[David :Bienvenu]

Comment on attachment 442416 [details] [diff] [review]
New patch with comments taken into account and mozmill test added

this looks reasonable. Some nits:

+var NUM_MESSAGES_IN_THREAD = 6;

could use const

lose this comment:

+/*
+ * Test the many horrors involving right-clicks, middle clicks, and selections.
+ */
+

and probably put yourself as the contributer, not asuth, and change the license year to 2010.

Comment 7

[Jonathan Protzenko [:protz]]

Attachment: New patch with nits taken into account

I'm uploading this new patch but I'm not requesting review because I still need to fix the issue from comment #4 (archiving part of the current selection destroys the current selection). I think that's quite important and this patch may be more or less useless if committed like that.

Any ideas about how to fix this?

Comment 8

[Jonathan Protzenko [:protz]]

Comment on attachment 444443 [details] [diff] [review]
New patch with nits taken into account

Following discussion on IRC with asuth, I'm requesting review for this patch only. Fixing the selection issues turns out to be way too complicated, and it will be much simpler if I just restore the selection myself properly when I need to from the extension code.

Moreover, while I believe making this code available to extensions is important because archiving messages is a complex operation, the use-case where we're deleting/archiving *part* of the current selection is a little bit too specific, and it doesn't make much sense to waste energy into this. So requesting review for the first part only.

David, you already approved in the previous comment, so that should just be a matter of setting review+.

Comment 9

[David :Bienvenu]

Comment on attachment 444443 [details] [diff] [review]
New patch with nits taken into account

looks reasonable, thx for the patch.

Comment 10

[Jonathan Protzenko [:protz]]

Great, thanks for the review!

Comment 11

[Jonathan Protzenko [:protz]]

Comment on attachment 444443 [details] [diff] [review]
New patch with nits taken into account

Requesting approval for 3.1 as well. Reasons are:
- I believe this is ultra-low risk. This code is only used in one place and one can trivially check that I just added an extra parameter.
- I also believe this can be nice to addon authors who want to manipulate the new archive function. It'll be easier to just say this is available on 3.1 series than to say available after 3.1.1 but also on 3.2. Once again, this is in the spirit of "Make 3.1 nicer for addon authors" :-)

Comment 12

[Mark Banner (:standard8)]

Checked in: http://hg.mozilla.org/comm-central/rev/2ff7872f4d1a

Comment 13

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 444443 [details] [diff] [review]
New patch with nits taken into account

a=dmose after discussion with Standard8.  Thanks much for the patch, the tests, and careful analysis of costs & benefits, Jonathan!

Comment 14

[Mark Banner (:standard8)]

Checked in to branch: http://hg.mozilla.org/releases/comm-1.9.2/rev/e47e724f6827