[dev] Problems with the new IMP_Message functions

[Michael M Slusarz]

Quoting Chuck Hagenbuch <chuck@horde.org>:

| Quoting Michael M Slusarz <slusarz@bigworm.colorado.edu>:
| 
| > Hmmmmm.  I really thought I was only copying/pasting the existing code
| > from mailbox.php and message.php to the class.  I didn't think I added 
| > any new calls to sort - the new code _should_ only resort when the old 
| > code did. But this is exactly one of the reasons why I tried to 
| > consolidate - so we can catch stuff like this.  I will take a look at 
| > this some more.
| 
| If you look at the CVS history of message.php 
| (http://cvs.horde.org/diff.php/imp/message.php?r1=2.354&r2=2.355&ty=h for
| the relevant diff), you'll see that the DELETE_MESSAGE case had a lot
| nits accounted for in it.

OK - I see.  This was intentional - all the nits the old code had _should_ 
still be valid with the new code.  There is a lot of duplicate/excess 
coding there, and a reason why I wanted to put it all in the same place.  I 
will go through the new code though and verify that all these cases are 
being handled.

| Part of the problem with the new code is that you've lost the distinction
| of the mailbox page and the message page; we don't want to resort on the 
| message page if we can avoid it, mainly because we might end up with a 
| message that just arrived showing up somewhere in the sort order that 
| throws us off. If you were using IMAP, didn't have a trash folder, and
| had 
| hide_deleted turned off, then this is the only code that was run after a 
| message was deleted:
| 
|              $array_index++;
|              $index = $sorted[$array_index];
| 
| No re-sort. Also note that that's the _only_ case in which we incremented
| 
| $array_index; every other time it stays the same, because with either
| pop3, 
| a trash folder, or deleted messages hidden, the message you just deleted 
| has disappeared out of the message set, leaving the array index actually 
| pointing at the correct next message.

This was actually very buggy for me - cases where I would use to delete the 
last 2 messages in a mailbox would dump me to an error screen about an 
undefined array index.  I didn't want to go through the present code and 
figure out the problem - I thought a much better solution would be to code 
it better (going by the mantra: don't complain, fix.)  

I realize that the organization may not be the greatest, but IMHO it is 
better than what it used to be.  Hopefully I can tighten stuff up even more.
I definitely think the code for when to sort/not sort can be improved (as 
you have mentioned previously).  I'm starting to finally get really 
familiar with all this code does, so that should help things out.

| So, just having thought through all of this, the pseudocode for that
| bottom bit of IMP_Message::delete() ought to look something like this:
| 
| If we're updating indices:
| 
|   If things were successful:
| 
|     If using a trash folder:
|       remove current message
|     If using pop3 or hide_deleted:
|       re-sort (this was in the old code; guess it's needed, but don't 
|                remember why), keep old array index
|     Otherwise:
|       increment array index
|       DON'T re-sort.

I'll run through this and see what that accomplishes.

michael

______________________________________________
Michael Slusarz [slusarz@bigworm.colorado.edu]
The University of Colorado at Boulder