60107 - Preference for limiting number of Collected Addresses not working

Status: VERIFIED FIXED

(SeaMonkey :: MailNews: Address Book & Contacts, defect, P2)

Description

[esther]

Using trunk build on win98, mac and linux the preference item for limiting the
cards in Collected Addresses book is not working.

1. Create a New Profile
2. Select Preferences from the Edit menu
3. Select Mail and Newsgroups|Addresses
4. Change the number for "Limit Collected Addressed to[4] cards.  OK the pref.
5. Open mail and select 6 email messages with a different sender.
6. Open the Collected addresses book

Result: notice all 6 email addresses are listed in the right pane.
Expected:  only senders email addresses from the last 4 messages should be listed.

Seth and I discussed how it should work after the limit is reached.  1st in 1st
out when limit is reached allowing new cards to be added. (Not in spec)
Also note: when this is fixed how should we handle a limit that is set to a
number below the number of card you already have. Example: you have 100 cards in
collected address book, you change the limit to 50.  Now you select a message,
the collected address book adds the new card and removes the 1st 51 cards that
were collected so the book meets the 50 card limit.  Jennifer, can you comment
on if this behavior is what we want.  We didn't see this mentioned in spec,
Note: We we did see what the expected behavior was for the hidden preference
limit for performance, which we feel should not be the same.

Comment 1

[esther]

changing owner to candice

Comment 2

[scottputterman]

well, we have a LRU type algorithm in place. I think what we have is probably
fine. The question is to find out why it's not working. My understanding is that
we were off by one. That is if you said 4 we would do 5, but you're saying we're
off by 2.

We also chose to not remove cards when you lower the limit below where you are
currently at. At some point in time we should do that. There's another bug out
there that has a lot of this discussion in it. We just chose not to implement it.

Comment 3

[jglick]

Esther, I agree with your comments:

1st in 1st out when limit is reached allowing new cards to be added. 

When a limit is set to a number below the number of card you already have: 
Example: you have 100 cards in collected address book, you change the limit to 
50.  Now you select a message, the collected address book adds the new card and 
removes the 1st 51 cards that were collected so the book meets the 50 card 
limit.

Comment 4

[scottputterman]

You don't want to do first in first out. You really want to do least recently
used.  We currently do a form of that. I don't think that's going to change.

When Dimi was working on this, one thing he noticed was that it was time
consuming to drop from 700 to 200 cards immediately. We had talked about doing
this incrementally, say 20 at a time.  If we ever fix this, then we should play
around with this.

Comment 5

[jglick]

Least recently used, first out is even better. 

Incrementally, dropping entries to get down to a new limit is fine. 

Comment 6

[chuang]

I can tell from the code that the card count we use is not right.  I may be able 
to reduce the delete time by not using the AbCard but delete directly from 
database.  I still need to understand the code more to have a better idea.

Comment 7

[(not reading, please use [e-mail] instead)]

marking mail2, since this is performance related.

it is performance related because if we don't heed that pref, the CAB gets huge
and autocomplete (therefore msg compose) slows way down.

cropping the CAB when the pref drops is also performance related.  say the user
sets that pref to 1000, notices the slow down, and then sets it to 10.

if we don't crop the CAB, performance will never improve for that user.

Comment 8

[[e-mail] (gone - use [e-mail] instead)]

pmock: did you want this bug?

Comment 9

[R.K.Aa.]

*** Bug 61138 has been marked as a duplicate of this bug. ***

Comment 10

[pmock]

Changing qa assign.

Comment 11

[scottputterman]

marking nsbeta1+ and moving to mozilla0.8

Comment 12

[chuang]

Attachment: Patch file

Comment 13

[chuang]

This patch fixed the off by one problem.  Also fixed the problem (same email 
address seems to be collected more than once) described in bug 65640.  I changed 
the delete extra cards by deleting directly from db not through card,  hopefully 
this can fix some performance problem.  

In the code there used to be a constant served as a buffer when deleting the 
extra card.  That is when we reduced from 50 to 20, we actually only delete 28.  
Now I changed to have the exact number in cab as pref.  

Comment 14

[chuang]

I found a problem which I can not reproduce all the time.  Sometime the number 
in collected address book is one off the pref value.  I'll keep looking into it.  
If I can't reproduce it constantly, I'll file a bug.  If I can fix it, I'll post 
another patch.

Comment 15

[chuang]

Attachment: A new patch

Comment 16

[chuang]

The new patch fixed the one off problem and another problem I found which may 
related to bug 65654.  The way we did to remove the extra card doesn't work 
right .  As I know, the only way to iterate through db table is a sequential 
search (use NextRow() in RowCursor).  When we delete the first row, all row 
index shifted forward.  The NextRow() we get after the first deletion actually 
is the 3rd row, not the 2nd row we want. 

I have fixed this by cachig the rows we want to delete into an array, then 
delete it later.  

Comment 17

[chuang]

Attachment: Final patch with correct mork error checking

Comment 18

[chuang]

sr=bienvenu per David's email.  The new patch has added the correct mork error 
checking (use rv==NS_OK instead NS_FAILED(rv)).

Comment 19

[Jean-Francois Ducarroz]

R=ducarroz (for "final" patch)

Comment 20

[David :Bienvenu]

sr=bienvenu

Comment 21

[chuang]

Fix checked in last night.

Comment 22

[pmock]

*** Bug 66379 has been marked as a duplicate of this bug. ***

Comment 23

[pmock]

Verified as fixed on win32 and linux commercial builds:
 win32 commercial seamonkey build 2001-012406-mtrunk installed on P500 Win98
 linux commercial seamonkey build 2001-012408-mtrunk installed on P200 RedHat 
6.2

No recent Mac yet to verify on...

Comment 24

[pmock]

Verified as fixed on Mac commercial semaonkey build 2001-012413-mtrunk.



Verified bug as fixed.