Closed Bug 73868 Opened 24 years ago Closed 23 years ago

addressbook results pane, select address dialog results pane, ab sidebar should use outliner

Categories

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

Product:
SeaMonkey
Component:
MailNews: Address Book & Contacts
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
VERIFIED FIXED
Milestone:
mozilla0.9.8

People

(Reporter: cathleennscp, Assigned: sspitzer)

References

(
URL
)

Details

(Keywords: perf)

Attachments

(4 files, 44 obsolete files)

screen shot
63.80 KB, image/jpeg
Details
screen shot, all columns, icons in result pane
51.69 KB, image/jpeg
Details
bloat / leak log when switching around between addressbooks (LDAP and local)
81.54 KB, text/plain
Details
updated patch, fixed conflicts.
604.28 KB, patch
dmosedale
: review+
Details | Diff | Splinter Review
need to convert addressbook to use new RDF outliner
QA Contact: esther → fenella
Blocks: 73948
--> me
Assignee: chuang → blake
QA Contact: fenella → nbaca
hey blake (and others), if possible, please hold off on this until I land #83023
Blocks: 25554
Target Milestone: --- → mozilla0.9.7
stephend, when you post your address book perf numbers on mozilla.org, can you add a link here? thinking ahead about this bug, for the addressbook "elements" pane, I want to create an addressbook view (implementing an nsIOutlinerView), instead of using the rdfliner. The performance improvements would be close to what we got when we converted the mailnews thread pane. switching from XUL templates | RDF datasource | addressbook db (mork) to outliner | addressbook view | addressbook db (mork) is quite a bit of work, but worth it. before starting on that work, I need to investigate what code uses the datasource that I'd be removing. I wouldn't be getting rid of the datasource behind the addressbook "addressbook / mailing list" pane. Like the folder pane, we'd switch over to use the rdfliner. blake, feel free to give this back to me.
taking from blake. I'm going to be working on ab perf.
Assignee: blakeross → sspitzer
No longer blocks: 107151
ok got the basics going. see the screen shot. I'll attach my initial patch, it's still a work in progress.
Status: NEW → ASSIGNED
No longer depends on: 83023
here's how the results pane currently works: rdf / xul tree templates (on top of) directory datasource (on top of) nsIAbDirectory (mork, ldap, outlook, etc). we learned from the thread pane that the way to make it fast and use less memory is to use outliner (which means now that we only have the content node for the outliner body, instead of content for every tree cell, tree row, etc for every entry in the addressbook), and to remove the datasource. what I've got so far is: outliner nsAbView (implements nsIAbView) nsIAbDirectory (mork, ldap, outlook, etc) showing data isn't hard (that part is working). the harder part is fixing all the other code in the fe that used to use the tree, and now has to use the outliner, including drag & drop, commands, etc.
Blocks: 83023
Summary: addressbook to use new outliner → addressbook results pane to use new outliner
Attachment #56207 - Attachment is obsolete: true
Just curious: did you try using the rdfliner for this? Did it not provide a big win? (Were you going to get rid of the AB datasource anyway?)
Attachment #56274 - Attachment is obsolete: true
> Just curious: did you try using the rdfliner for this? no, but I plan to for the "addressbook" pane, like we did for the folder pane. > Did it not provide a big win? yes, a big win. I haven't measured officially, but I see the difference between an opt build with the old way and a debug build with the new way in scrolling, ab loading, and startup (which loads the first ab). the bigger the AB, the bigger the win. it also allows to hide columns, and if we want, to add more columns easily. > (Were you going to get rid of the AB datasource > anyway?) there's no need to go throug the AB datasource for the results pane, as it just fwds on to nsIAbDirectory.
Attachment #56275 - Attachment is obsolete: true
Attachment #56279 - Attachment is obsolete: true
Attachment #56653 - Attachment is obsolete: true
loading a large ab is still slow, but I've found out why. *every* card is a listener on the addressbook. zoinks! removing that will make ab loading much, much faster. nsAddrDatabase::AddListener(nsAddrDatabase * const 0x05ce4830, nsIAddrDBListener * 0x086370f4) line 267 + 16 bytes nsAddrDatabase::CreateCard(nsIMdbRow * 0x05305068, unsigned int 0x00000000, nsIAbCard * * 0x08e37084) line 3757 nsAddrDatabase::CreateABCard(nsIMdbRow * 0x05305068, nsIAbCard * * 0x08e37084) line 3775 nsAddrDBEnumerator::CurrentItem(nsAddrDBEnumerator * const 0x08e37070, nsISupports * * 0x0012a73c) line 3522 + 52 bytes nsAbView::EnumerateCards(nsIAbDirectory * 0x05ce8a30) line 100 + 47 bytes nsAbView::Init(nsAbView * const 0x08e283a0, const char * 0x08e33040) line 81 + 17 bytes
finding more badness, but should be easy to fix. I've added all the column the spec requires, and I'm using the same column token in the xul as we do in the mork ab, so that in theory, we should be able to pass it back from GetCellText() without having to do string compares before we get to mork. unfortunately, the addressbook is horked on crack. so even if we call getCardValue("DisplayName"), we *still* do extra work. I'll make sure to get to that cleanup. now the available columns include: "Name", "Email", "Presence", "Screen Name", "Work Phone", "Organization", "Nick Name", "Home Phone", "Fax", "Pager", "Cellular", "Additional Email", "Title" and "Department". I'll attach a screen shot, and update the patch.
here's one really bad thing that the ab does: when loading a directory, we bloat (into memory) a nsIAbCard for each card in the database. that would be ok, if we only built it up with the card properties that we cared about, and if we did it lazily. but we don't instead, we copy all the properties (whether we ever need them or not). this is sucks ass with autocomplete, as we'll try to get all the attributes for each card, and not just the attributes we need for autocomplete.
my todo list for the next few days: 1) sorting 2) fix bloat, be lazy with nsIAbCard (help with autocomplete perf, footprint) 3) no strcmp, map right to mork. 4) get add / delete working (view will have to listen on db) 5) edit changes to work 6) double click to edit mailing list 7) get "Properties" button to work again 8) make sure ab sidebar works 9) remove the unnecessary datasource 10) sort indicators 11) menu items for sort 12) menu items for show / hide columns 13) start up, select first card. what works now: 1) use outliner 2) selection in outliner load card pane 3) add all columns per the spec, column picker, column reordering, persistence 4) improve ab load, every card is a no longer a listener on the db 5) double click to edit card 6) icons in result pane (card and mailing list)
Attached patch updated patch, add sorting (obsolete) — — Splinter Review
Attachment #56781 - Attachment is obsolete: true
Attachment #56801 - Attachment is obsolete: true
Attachment #56809 - Attachment is obsolete: true
Attachment #56811 - Attachment is obsolete: true
Attachment #56846 - Attachment is obsolete: true
Attachment #56951 - Attachment is obsolete: true
this affects perf substantially, marking as a keyword
Keywords: perf
Attached patch updated patch (obsolete) — — Splinter Review
Attachment #57468 - Attachment is obsolete: true
Attached patch updated patch, edit card working (obsolete) — — Splinter Review
Attachment #57470 - Attachment is obsolete: true
here's what's left before I can land: 0) double clicking on list in folder pane to edit 1) sort indicators 2) make sure ab sidebar works 3) restore sort selection 4) drag n drop 5) make sure db or mailing list works 6) cmd updating (delete, properties, etc) 7) changing mailing list name doesn't always update folderpane 8) get "cards in ab" status text working again 9) test ab sync 10) double check for leaks 11) makefile / .mcp changes for mac and linux 12) test ab import 13) test ldap / outlook here's what needs to be done, after the landing: 1) fix bloat, be lazy with nsIAbCard (work on autocomplete perf, CAB perf) 2) no strcmp, map right to mork. 3) rename collecter -> collector 4) improve listener foo (too much going on) 5) default height for splitter (to save a paint) 6) more perf work, footprint work on ab.
updated todo list: here's what's left before I can land: a) fix select address dialog b) fix ab sidebar c) sending assertions (in addressbook) d) updates to mailing list not reflecting in card pane until repaint 1) double clicking in folder pane to edit mailing list 2) sort indicators 3) restore sort selection 4) drag n drop 5) make sure db or mailing list works 6) cmd updating (delete, properties, etc) 7) changing mailing list name doesn't always update folderpane 8) get "cards in ab" status text working again 9) test ab sync 10) double check for leaks 11) fix duplicate access keys 12) makefile / .mcp changes for mac and linux 13) test ab import 14) test ldap / outlook 15) test how it works with no AB. 16) show name as needs work 17) look for any XXXX / dump / printf 18) remove old results tree xul here's what needs to be done, after the landing: 1) fix bloat, be lazy with nsIAbCard (work on autocomplete perf, CAB perf) 2) no strcmp, map right to mork. 3) rename collecter -> collector 4) improve listener foo (too much going on) 5) default height for splitter (to save a paint) 6) more perf work, footprint work on ab.
Summary: addressbook results pane to use new outliner → addressbook results pane, select address dialog results pane, ab sidebar should use outliner
Attachment #57747 - Attachment is obsolete: true
Attachment #57946 - Attachment is obsolete: true
there's now a branch. win32: cvs co -r AB_OUTLINER_BRANCH mozilla/client.mak cd mozilla nmake -f client.mak linux: cvs co -r AB_OUTLINER_BRANCH mozilla/client.mk cd mozilla gmake -f client.mk mac: not yet see http://www.sspitzer.org/ab.html for the todo list, and the items completed.
URL: http://www.mozilla.org/mailnews/perfo... → http://www.sspitzer.org/ab.html
I noticed the ETA on your AB page... There are going to be test builds before you land this (and turn it on) on the trunk, right?
there's are branch bits. see ftp://ftp.mozilla.org/pub/mozilla/nightly/latest-AB_OUTLINER/ there's a bunch of known issues that I'm working on. (see http://www.sspitzer.org/ab.html)
Seth, curious if you are doing ok with all the stuff you have on the list.. are you in need of assistance getting this one out of the way?
also some commercial bits on sweetlou for example, ftp://sweetlou/products/client/seamonkey/windows/32bit/x86/2001-12-05-18- AB_OUTLINER/
these are the changes for the branch, I still need to re-review all the changes, clean it up, apply to the trunk, and get a trunk based diff, for review.
Attachment #59197 - Attachment is obsolete: true
here's a patch for the trunk. I'm testing / self-reviewing it now.
Attachment #60960 - Attachment is obsolete: true
Attached patch patch for review. (obsolete) — — Splinter Review
files removed: mailnews/addrbook/resources/content/abResultsTreeOverlay.xul mailnews/addrbook/resources/locale/en-US/abResultsTreeOverlay.dtd mailnews/addrbook/src/nsAbMDBRDFResource.h mailnews/addrbook/src/nsCardDataSource.h mailnews/addrbook/src/nsAbMDBRDFResource.cpp mailnews/addrbook/src/nsCardDataSource.cpp files added: (the trunk version of them are cat'd to the end of the patch, so you can review them) mailnews/addrbook/resources/content/abResultsPaneOverlay.xul mailnews/addrbook/resources/content/abResultsPane.js mailnews/addrbook/resources/content/addressbook-panel.js mailnews/addrbook/resources/locale/en-US/abResultsPaneOverlay.dtd mailnews/addrbook/public/nsIAbView.idl mailnews/addrbook/src/nsAbView.h mailnews/addrbook/src/nsAbView.cpp themes/classic/messenger/addressbook/mac/abResultsPane.css themes/classic/messenger/addressbook/win/abResultsPane.css themes/modern/messenger/addressbook/abResultsPane.css
Attachment #61163 - Attachment is obsolete: true
Here are some nits with the patch: + * Edits an existing mail in the directory + * defined by the uri, or pass in the card for the list in this function: +function goToggleSplitter( id, elementID ) you have some logic that relies on the element's |checked| property's value. You should check that it works, if you are messing with checkboxes, due to blake's recent landing to make .getAttribute("checked","false") not work (and replace it with .hasAttribute("checked")). I don't know if that affects you, just a heads-up for the record. + catch (ex) { + return 0; } What's the point of this? That function will terminate one line thereafter anyway... + var treechildren; + for (var i=0;i<children.length; i++) { + if (children[i].localName == "treechildren") { + treechildren = children[i]; + break; + } + } Did you mean to fetch all the nodes with localName "treechildren" into treechildren? Maybe this should be |treechildren += children[i]| then? (Otherwise treechildren will always be the last "treechildren" node...afaics) in this file: Index: mailnews/addrbook/resources/content/addressbook.js You are adding an old-style NPL-license. It should be a tri-license... in these files: Index: mailnews/addrbook/src/nsAbDirProperty.cpp Index: mailnews/addrbook/src/nsAbMDBDirProperty.cpp There's no newline at end of the file. This will hork some ports, IIRC. In these two cases: + if (lastName.Length()) + else if (firstName.Length()) you should probably check for !firstName.IsEmpty(), which I think is (will be?) cheaper to do. Suddenly, at a certain point in this patch, it became normal |cvs diff| output (as opposed to |cvs diff -u|)... This is very nice cleanup, it's gonna rock to be able to use a fast addressbook! r=hwaara
BTW, disregard the treechildren comment, I didn't notice the break in that loop.
hwaara, missed the |break;| semantics [short explanation for posterity] javascript:for(a=0;a<6;++a)break;document.write(a) outputs 0, not 6. @@ -137,9 +142,13 @@ // defined by the uri void addMailListToDatabase(in string uri); - // Edits an existing mail in the directory - // defined by the uri - void editMailListToDatabase(in string uri); + /** + * Edits an existing mail in the directory + * defined by the uri, or pass in the card for the list whitespace ^ RCS file: /cvsroot/mozilla/mailnews/addrbook/resources/content/abCardViewOverlay.js,v @@ -94,6 +94,14 @@ odd alignment, i'm assuming you should be indenting to match... +function GetAddressesFromURI(uri) +{ + var addresses = ""; + + var editList = GetDirectoryFromURI(uri); + var addressList = editList.addressLists; + if (addressList) { + var total = addressList.Count(); + if (total) { + for ( var i = 0; i < total; i++ ) { + var current = addressList.GetElementAt(i).QueryInterface(Components.interfaces.nsIAbCard); + + if (i == 0) + addresses = current.primaryEmail; + else + addresses += "," + current.primaryEmail; } - // for seperator all other display formats - else { - separator = cvPrefs.lastFirstSeparator; } } i think you should drop the if (total) js> function a(){try { for (;c<d;)throw "aa";} catch (e){return 1;}return 2;} js> function b(){try { for (;c<d+1;)throw "aa";} catch (e){return 1;}return 2;} js> var c=d=0;print (a()+" "+b()) 2 1 @@ -178,10 +200,27 @@ whitespace in +'d lines. + cvSetVisible(data.cvhDescription, false); + cvSetVisible(data.cvbDescription, false); you're doing a lot of paired hiding, wouldn't it make more sense to use some sort of grouping mechanism so you only hide one thing? + <description class="CardViewLink" hide="true" id="cvListNameBox"> + <html:p><html:a href="" id="cvListName"/></html:p></description> could you move </description> to line up w/ <description...> (either vertically or horizontally, i'm not picky. + if (gAbView) + return gAbView; + else + return null; brendan would say else after return: bad. i'll instead ask why not just return gAbView (showing my complete ignorance of the code). +function ChangeDirectoryByDOMNode(dirNode) ... +return; +} there's no need for this return. +function InvalidateResultsPane() +{ + var outliner = GetAbResultsOutliner(); + outliner.boxObject.invalidate(); } why not just GetAbResultsOutliner().boxObject.invalidate(); ? [note, rginda says my suggestion is ugly, so this suggestion like all others is discardable] +function GenerateAddressFromCard(card) +{ + var address = ""; + var name = card.displayName + if (name.length) { + address += "\"" + name + "\" "; + } + + if (card.isMailList) { + if (name.length) { + address += "<" + name + ">"; + } + } + else { + var email = card.primaryEmail; + if (email.length) { + address += "<" + email + ">"; + } + } + return address; +} would if (foo) work as well as if (foo.length)? <<<<<< +function GetParentDirectoryForMailingList(abURI) +{ + var parentURI = null; + + var abURIArr = abURI.split("/"); + /* + turn turn "moz-abmdbdirectory://abook.mab/MailList6" + into ["moz-abmdbdirectory:","","abook.mab","MailList6"] + then, turn ["moz-abmdbdirectory:","","abook.mab","MailList6"] + into "moz-abmdbdirectory://abook.mab" + */ + if (abURIArr.length == 4 && abURIArr[0] == "moz-abmdbdirectory:") { + parentURI = abURIArr[0] + "/" + abURIArr[1] + "/" + abURIArr[2]; + } + return parentURI; } ====== +function GetParentDirectoryForMailingList(abURI) +{ + /* + in "moz-abmdbdirectory://abook.mab/MailList6" + out "moz-abmdbdirectory://abook.mab" + */ + return abURI.match(RegExp("^moz-abmdbdirectory\://[^/]*/")); } >>>>>> +var abDirTreeObserver = { + onDragStart: function (aEvent, aXferData, aDragAction) this function appears to want to be a consumer of GetParentDirectoryForMailingList but is not <<<<<< + var targetArr = targetURI.split("/"); + var srcArr = srcURI.split("/"); + if (srcArr.length == targetArr.length && srcArr.length == 4) { + if (srcArr[0] == "moz-abmdbdirectory:" && + srcArr[0] == targetArr[0] && + srcArr[1] == targetArr[1] && + srcArr[2] == targetArr[2]) { + needToCopyCard = false; + } + } ====== if (GetParentDirectoryForMailingList(targetURI)==GetParentDirectoryForMailingList (srcURI)) needToCopyCard = false; >>>>>> [feel free to assign the ruturns of that function to local variables for readability] @@ -589,22 +619,19 @@ can you change from return(false); to return false; to match what i think is the predominant style? the tail of function UpdateCardView() is the same as the function OnClickedCard(card) RCS file: /cvsroot/mozilla/mailnews/addrbook/src/nsAbDirProperty.cpp,v \ No newline at end of file RCS file: /cvsroot/mozilla/mailnews/addrbook/src/nsAbMDBDirectory.cpp,v reeks of tabs (they appear to be in your + lines) + else { + if (lastName.Length()) + *aName = nsCRT::strdup(lastName.get()); + else if (firstName.Length()) + *aName = nsCRT::strdup(firstName.get()); + else + *aName = nsCRT::strdup(NS_LITERAL_STRING("").get()); + *aName = nsnull; + } [timeless] jag should i use: nsCRT::strdup(NS_LITERAL_STRING("").get()); <bz> timeless: why would use strdup there?? <jag> timeless: no <bz> timeless: that seems very wrong.... (embedded nulls and the like) <jag> timeless: ToNewUnicode(NS_LITERAL_STRING("")) if you really must <bz> timeless: that looks incredibly broken if (!lastName.IsEmpty()) *aName = ToNewUnicode(lastName); else if (!firstName.IsEmpty()) *aName = ToNewUnicode(firstName); else *aName = nsnull; a caller is doing: + if (NS_SUCCEEDED(abSession->GenerateNameFromCard(workCard, format, &aName)) && (aName)) the other caller does .get() so i think we have a problem + rv = abSession->GenerateNameFromCard(this, format, getter_Copies(properties [index_DisplayName])); + NS_ENSURE_SUCCESS(rv,rv); + if (*properties [index_DisplayName].get() == 0) { so i think this should be changed to: if (properties [index_DisplayName]) { sorry for the long review.
So far, besides the earlier reviews comments, these are all nits and up to you to change or not: some of the accessors (e.g., GetIsRemote) don't ensure the arg pointer is non-null - not sure if that's required or not currently. There's not much reason to change a single PRBool into a PRPackedBool since there will be no savings and could be a eensy teensy performance hit (not that it would matter in this code). In nsAbMDBDirProperty::GetValue* NS_ENSURE_SUCCESS(rv,rv); + return NS_OK; + these could all be NS_ASSERTION(NS_SUCCEEDED(rv)); return rv; which could generate one less return statement at the cost of not looking so pretty :-) . I've gotten to the aboutlook stuff so far, and I'm taking a break for now. Looks like a vast improvement so far.
In nsAddrDatabase, I think I see some tabs: + // we need to do this for dnd + PRUint32 key = 0; + err = GetIntColumn(cardRow, m_RecordKeyColumnToken, &key, 0); + PRUint32 count; + addressList->Count(&count); + + nsXPIDLString newEmail; + rv = newCard->GetPrimaryEmail(getter_Copies(newEmail)); + NS_ENSURE_SUCCESS(rv,rv); + + PRUint32 i; + for (i = 0; i < count; i++) { this left-over comment is a little cryptic //let it starts from 1 more tabs? I know sometimes cvs diff -uw shows funny things... + nsIMdbRow* cardRow = nsnull; + mdbOid rowOid; + rowOid.mOid_Scope = m_CardRowScopeToken; you can use an nsCOMPtr here if you want. + nsIMdbRow* cardRow = nsnull; and you might want to since I don't see you releasing this cardRow. This seems to be in several routines in this file so you might want to look over the uses of GetRow.
looks like tabs horked in > nsresult nsAbView::EnumerateCards() and in nsAbView::CreateCollationKey > // ensure restored selection is visible > if (cardWasSelected) { > if (mOutliner) > mOutliner->EnsureRowIsVisible(index); > } could be: if (cardWasSelected && mOutliner) mOutliner->EnsureRowIsVisible(index)
moving to 0.9.8, the AB_OUTLINER_BRANCH should land in early 0.9.8
Target Milestone: mozilla0.9.7 → mozilla0.9.8
sorry for the delay. I'll go work on the reviewers comments. the next patch will also have support for exporting addressbooks (to just .csv and .tab for now), some code cleanup, and some performance fixes to nsAbCardProperty::Get/SetCardValue(). I'll attach a new patch once I address all the comments.
new patch on the way. includes changes to export directories in .csv or .tab format. also some additional code cleanup (removed dead export code), and added file types for when doing text addressbook import. for the reviewers comments that I didn't address in the new patch: 1) > you have some logic that relies on the element's |checked| property's value. You > should check that it works, if you are messing with checkboxes, due to blake's > recent landing to make .getAttribute("checked","false") not work (and replace it > with .hasAttribute("checked")). The method in the patch works. has blake landed yet, or is he going to land? 2) > ChangeDirectoryByDOMNode(children[i]); > // XXX need this? return; I need that so I select just the first treeitem, instead of all the treeitems, and resulting in the the last one as being selected. 3) NS_ENSURE_SUCCESS(rv,rv); return NS_OK; vs NS_ASSERTION(NS_SUCCEEDED(rv),"..."); you are right, that in a non debug mode this results in one less if () check and one less return. I'm just not sure what to fill in for "...", so I've continued the pattern, since I get the assertion text for free. objections to me leaving it as is?
Attached patch updated patch (obsolete) — — Splinter Review
updated patch, I haven't javadoc'd the new idl files (as dmose has requested) yet. this patch includes more cleanup, and code to handling exporting of directories to .csv / .tab formats.
Attachment #61192 - Attachment is obsolete: true
also in the last patch are some perf changes to Set/GetCardProperty() that code will hopefully go away, but until then, a long list of string compares is now some hairy switch statements that we can optimize more, by giving more common columns (last name, first name, primary email, phone, etc, stuff that commonly appears in the results pane) shorter code paths.
Attached patch updated patch (obsolete) — — Splinter Review
localized some strings I forgot to localize, added a title to the export addressbook dialog, made it so if you delete the "current" addressbook (in the addressbook or the addressbook sidebar panel) we'll switch to the personal addressbook. I've fixed the nsIAbListener to do selective notification (like we do for folder listeners). we now delete the existing file on export (if the user confirmed to overwrite).
Attachment #61561 - Attachment is obsolete: true
Attached patch updated patch (obsolete) — — Splinter Review
this patch fixes some problems with exporting, adds some code (but turned off with a comment) about how our .csv format is not like Outlook Express. this code also fixes a problem with the import code. the import code runs on another thread, so we need to proxy back to access the nsAddrDataBase when importing.
Attachment #61607 - Attachment is obsolete: true
this patch includes a new comment about how we should be integrating with the per server "maxHits" patch for ldap addressbooks, and has some ab / ldap cleanup.
Attachment #61636 - Attachment is obsolete: true
bloat / leak log when switching around between addressbooks (LDAP and local) the local ab code doesn't seem to leak, at least not the nsAb* objects. when using LDAP, we do leak. dmose says that might be a known problem, but we can investigate. here's the nsAb* object that leak, when you use ldap in addressbook. 108 nsAbBooleanConditionString 56 56 1 1 ( 1.00 +/- 0.00) 15 1 ( 1.69 +/- 0.66) 109 nsAbBooleanExpression 20 20 1 1 ( 1.00 +/- 0.00) 18 4 ( 2.63 +/- 1.43) 110 nsAbCardProperty 748 27676 552 37 ( 249.09 +/- 144.91) 5978 37 ( 254.44 +/- 153.75) 112 nsAbDirSearchListener 16 48 3 3 ( 2.00 +/- 1.00) 12 3 ( 2.71 +/- 1.10) 113 nsAbDirectoryQueryArguments 28 84 3 3 ( 2.00 +/- 1.00) 241 3 ( 2.98 +/- 1.09) 118 nsAbQueryLDAPMessageListener 68 204 3 3 ( 2.00 +/- 1.00) 27 6 ( 4.83 +/- 2.29)
Comment on attachment 61667 [details] [diff] [review] updated patch, includes some more clean up r=bienvenu
Attachment #61667 - Flags: review+
OK, here's my first set of comments; I'm only about 20% of the way through the patch, so there'll be more to come. Note that most of these are nits, and shouldn't hold up the landing of this patch, so if they should move to another bug, that's fine by me. nsAbFactory.cpp --------------- a) an extra commented out GENERIC_FACTORY_CONSTRUCTOR has been left here nsIAbCard.idl ------------- b) what's the point of getCardUnicharValue? nsIAttrDatabase.idl ------------------- c) getCardFromAttribute has "value" typed as a |string|. But values aren't necessarily just ASCII, right? ie shouldn't that be an |AString|? abCardViewOverlay.js ------------------ d) in GetAddressesFromURI, how about priming addresses with the first email address before beginning the loop and the start the loop with i=1? That way you don't have to evaluate the "if (i == 0)" on every pass through the loop. abCommon.js ----------- e) the license boilerplate added here is outdated; can you change it to the latest tri-license boilerplate from <http://www.mozilla.org/MPL>? f) GetSelectedAddresses: same as comment d g) GetSelectedRows: the |cards| variable doesn't appear to be used at all. h) GetSelectedRows: what's the point of outlinerSelection? why not just use gAbView.selection? i) GetSelectedRows: same as comment d j) GetSelectedAbCards: same as comment h k) GenerateAddressFromCard: I don't think it's necessary (and doesn't look so good) to have quotes around the display name if there's no unusual chars in it. One way to avoid that would be to have this function nsIMsgHeaderParser::MakeFullAddress under the hood? Although that seems to (in spite of the idl comments) use UTF8, so hmmmm.... I guess it depends what these generated addresses are used for. l) GetParentDirectoryFromMailingList: since the code here doesn't actually reference the abURI param, does this function work? And even if so, wouldn't it be significantly lighter weight to just search from the right end of the string for / and return a substring? general ------- a) any reason not to switch from addbook: to moz-addbook: URLs while we're here? b) for newly added methods, how about using AString instead of wstring? This will cut down on unnecessary mallocs(). c) I think you've missed adding yourself to the Contributor lists in some of the license boilerplates
Patch looks good. Here are couple of things I want to point out : 1. Looks like couple of globals (like gPrefs, gAddrbookSession in abCardViewOverlay.js) are defined in the middle of file, can be moved to the top. 2. You have already made note in the code to use string bundles (while copying cards, alerts, etc) in couple of places. If there is a major revision coming up, then you can fix those. Otherwise, a new bug will do. 3. Per http://www.mozilla.org/mailnews/specs/addressbook/ & section "Drag and Drop of Card Entries from One AB to Another", we should be doing a move instead of copy (copy when crtl key is used). If the change is simple fix this one. There is a open bug to track this already. I will get that one for you later. 4. looks like you forgot to remove #if 0 segment 5. if same primary addresses constitues equality, we shall get that into card's Eqauls API. 6. All that unused code from AddressBookParser::AddLdifRowToDatabase should go away. 7. + rv = abDataBase->GetCardFromAttribute(abDirectory, kPriEmailColumn, emailStr.get(), PR_TRUE /* lowerCase */, getter_AddRefs(existingCard)); if you care about the case here, the caseinsensitive boolean should be PR_FALSE...right..? r=bhuvan.
Additional comments; more to come, hopefully will finish tommorrow. abSelectAddressesDialog.js -------------------------- m) DropOnBucketPane: + var dragService = Components.classes["@mozilla.org/widget/dragservice;1"].getService(); + if (dragService) + dragService = dragService.QueryInterface(Components.interfaces.nsIDragService); + if (!dragService) + return false; + Isn't it true that the only way drag service is gonna end up null will throw an exception? So I think try/catch is really what's needed here instead of those if statements. Same goes for the createInstance of |trans|, I think. n: DropOnBucketPane: it looks like this function returns |false| in all cases. Is this really what you want? addressbook.js -------------- o: outdated license boilerplate p: AddPrefObservers, RemovePrefObservers: does this syntax do the right thing? + var prefService = Components.classes["@mozilla.org/preferences-service;1"] + .getService(Components.interfaces.nsIPrefService); I thought the required syntax was .getService().QueryInterface(Components.interfaces.nsIPrefService). I seem to remember the currently used syntax causing problems with LDAP autocomplete. nsAbAddressCollecter.cpp, nsAbAutoCompleteSession.cpp: ------------------------------------------------------ q: any particular reason to use the CID for the message header parser rather than the contract ID? nsAbAutoCompleteSession.cpp --------------------------- r: how about using an NS_STATIC_CAST here, rather than C-style? + if (!aSubstrLen || (nsCRT::strlen(aString) < (PRUint32) aSubstrLen)) s: nsAbAutoCompleteSession::SearchDirectory(). any reason not to just use the libc version of strcmp here (since those are sometimes hand-optimized)? nsAbLDAPDirectoryQuery.cpp -------------------------- t: Assuming we don't mind a link-time dependency on libxpcom, you can get the proxy object service and an object all in one shot using NS_GetProxyForObject(), which is defined in nsIProxyObjectEventManager.idl. u: instead of using the deprecated n.EqualsWithConversion, how about using n.Equals(NS_LITERAL_STRING("card:nsIAbCard"))? nsAbMDBDirectory.cpp -------------------- v: There are a couple of instances of the c-style (const char *) cast; how about using NS_CONST_CAST instead? w: GetAbDatabase: Instead of using PL_strlen (instead of possibly hand-optimized libc version) and old string tech: + nsAutoString file; file.AssignWithConversion(&(mURI[PL_strlen(kMDBDirectoryRoot)])); how about one of: + NS_ConvertASCIItoUCS2 file(&(mURI[strlen(kMDBDirectoryRoot)])); or + NS_ConvertUTF8toUCS2 file(&(mURI[strlen(kMDBDirectoryRoot)])); nsAbQueryStringToExpression.cpp ------------------------------- x) nsISupports::AddRef is templatized in such a way that if you write this: + *expression = expr; + NS_IF_ADDREF(*expression); as this: + NS_IF_ADDREF(*expression = expr); it'll be compiled to code that is a bit faster. There are other instances of this in this patch, so you may wanna do a quick grep for NS_IF_ADDREF. nsAddrBookSession.cpp --------------------- y) NotifyDirectoryDeleted: c-style cast nsAddrDatabase.cpp ------------------ z) nsAddrDatabase::SetCardValue: any reason not to use NS_ConvertUCS2toUTF8 utf8str(value); instead of the INTL_ConvertFromUnicode stuff?
Remaining comments: nsAddressBook.cpp ----------------- A) The attribPairsType stuff looks like it duplicates code in nsAbLDAPProperties.cpp B) ::ExportDirectory: There doesn't appear to be any error checking on any of the calls through outputStream. Presumably we need to notice any errors (eg disk full) and propagate them to the UI. addressingWidgetOverlay.js -------------------------- C) DragOverTree, DropOnAddressingWidgetTrees: I think that .getService and .QueryInterface will throw exceptions for anything that's gonna make dragService or dataObj be null, so you probably need try/catch clauses here. fieldMapImport.js ----------------- D) same as C, with trans and dragService importDialog.js --------------- E) GetStringBundle: same as D F) ImportAddress: If it's really necessary to leave a dump() in production code (is it?), how about using a more meaningful message so that someone who hits this knows where to look? nsImportAddressBooks.cpp ------------------------ G) same as t abResultsPaneOverlay.xul abResultsPane.js addressbook-panel.js abResultsPaneOverlay.dtd abResultsPane.css (all versions) ------------------------ H) wrong license header addressbook-panel.js -------------------- I) gAddressBookPanelAbListener.onItemRemoved: you don't need the "directory &&" half of the if clause. If directory is null, the QI will have thrown an exception by this point, which will have kicked control to the catch clause. nsAbView.cpp ------------ J) why do all the NS_INTERFACE_MAP stuff by hand rather than using NS_IMPL_ISUPPORTS4()? K) ::GetURI(): how about using ToNewCString(mURI) instead of nsCRT::strdup? L) ::Init(): * if you make GENERATED_NAME be an NS_NAMED_LITERAL_STRING rather than a #define, wrapping it multiple times won't be necessary. * it looks as though actualSortColumn can be unnecessarily copied into twice in the same invocation. * it should be possible to use .Equals() and avoid the .get() required for strcmp * I think ToNewUnicode() is preferred to nsCRT::strdup() M) ::GetCellProperties * should the first if actually be |if (colID[0] != PRUnichar('G'))|? N) ::GetRowProperties: * why pre-initialize isMailList, since the existing value will be overwritten? * the call to properties->AppendElement() should be error-checked O) ::GetCardValue: * same as M * column can be declared as NS_LossyConvertUCS2toASCII column(colID); P) ::inplaceSortCallback(): same as M Q) ::GetSelectedCards(): NS_NewISupportsArray(selectedCards) seems to be called twice. As before, most of these are just nits; in general, this looks great and is clearly a huge step forward for the addressbook code.
I'm working on a patch for these issues. the only two I don't understand are two from bhuvan: 5. if same primary addresses constitues equality, we shall get that into card's Eqauls API. 6. All that unused code from AddressBookParser::AddLdifRowToDatabase should go away. bhuvan, can you elaborate?
5. if same primary addresses constitues equality, we shall get that into card's Eqauls API. I guess the real question is In nsAddrDatabase.cpp, at the context @@ -1656,22 +1568,64 @@ don't we already do, + if (!nsCRT::strcmp(newEmail.get(), currentEmail.get())) { while checking equality in + rv = newCard->Equals(currentCard, &equals); + NS_ENSURE_SUCCESS(rv,rv); If so, then I thought emailaddress check is redundant. If email is to be considered as a valid key to check while checking the equality, I thought that check can be placed in ::Equals() directly. 6. All that unused code from AddressBookParser::AddLdifRowToDatabase should go away. There are several instances of unused like the following in that routine - // else if ( -1 != colType.Find("charset") ) + // else if ( kNotFound != colType.Find("charset") ) // ioRow->AddColumn(ev, this->ColCharset(), yarn); Are we going to use any of these commented out code in the future..? If not, we should remove that code. bhuvan
thanks for the solid reviews. I've addressed all the comments (except for bhuvan's two, I'll go look into those.) new patch on the way. while I test my patch, and update my tree to the trunk, here's some feedback for what I didn't address: >l) GetParentDirectoryFromMailingList: since the code here doesn't >actually reference the abURI param, does this function work? And even >if so, wouldn't it be significantly lighter weight to just search from >the right end of the string for / and return a substring? whoops, not sure when abURI.match(RegExp()) became RegExp() I went back to the way I original did it, using split. 've added a comment to indicate that the function returns null when the uri is not a mailing list. >7. + rv = abDataBase->GetCardFromAttribute(abDirectory, >kPriEmailColumn, emailStr.get(), PR_TRUE /* lowerCase */, >getter_AddRefs(existingCard)); > >if you care about the case here, the caseinsensitive boolean should be >PR_FALSE...right..? fixed, the comment was wrong, the code was correct >q: any particular reason to use the CID for the message header parser >rather than the contract ID? fixed my usage and others, I noticed we had a mix of GetService and createInstance, so I switched them all to GetService(), as it is a service has no state. >u: instead of using the deprecated n.EqualsWithConversion, how about >using n.Equals(NS_LITERAL_STRING("card:nsIAbCard"))? even better that code can just be n.Equals("card:nsIAbCard"). I need to understand that code better to fully criticize what's going on. but on quick glance, it doesn't look kosher. >v: There are a couple of instances of the c-style (const char *) cast; >how about using NS_CONST_CAST instead? did foo.get() instead of (const char *)foo >z) nsAddrDatabase::SetCardValue: any reason not to use > >NS_ConvertUCS2toUTF8 utf8str(value); cleaned up a lot of the string foo, removed INTL_ConvertToUnicode() and INTL_ConvertFromUnicode() from nsDirPrefs.* > i) GetSelectedRows: same as comment d in this case, I'm interating over each range count, and then each value in each range count. so instead of just unrolling the first in a for loop I'd be unrolling the the first of the outer and more. I decided it wasn't worth it. > a) any reason not to switch from addbook: to moz-addbook: URLs while > we're here? I'll spin up a separate bug for that issue, for cavin, who will be working on printing and ab sync. > 3. Per http://www.mozilla.org/mailnews/specs/addressbook/ & section "Drag and > Drop of Card Entries from One AB to Another", we should be doing a move instead > of copy (copy when crtl key is used). If the change is simple fix this one. > There is a open bug to track this already. I will get that one for you later. yes, I've left it as a copy on purpose, until we discuss the behaviour. move has interesting issues, like what does it mean to move a mailing list? or move a card from directory 1 to directory 2, where the card is part of a mailing list on directory 1. > A) The attribPairsType stuff looks like it duplicates code in > nsAbLDAPProperties.cpp. it is. This table of columns, if we do it right, could be shared for import LDIF and .csv, exporting LDIF and .csv, and for LDAP. for now, I've removed the used ldif attributes from the table in nsAddrDatabase.cpp. getting all three places (import, export, LDAP) to use the same table is going to be an involved task, I'll spin it off to a seperate bug. > c) getCardFromAttribute has "value" typed as a |string|. But values > aren't necessarily just ASCII, right? ie shouldn't that be an |AString|? GetCardFromAttribute() and GetRowFromAttribute() require the value to be UTF8, I've fixed the variable name and added comments. >p: AddPrefObservers, RemovePrefObservers: does this syntax do the >right thing? > >I thought the required syntax was >.getService().QueryInterface(Components.interfaces.nsIPrefService). I >seem to remember the currently used syntax causing problems with LDAP >autocomplete. getService(Components.interfaces.nsIPrefService) works, we do it all over. I vaguely remember that with autocomplete, I'm not sure why we had to do that.
> 5. if same primary addresses constitues equality, we shall get that into > card's Eqauls API. now I understand. no, primary email address is not part of card equality, as you could have two cards with the same email. that's for comparing nsIAbCard instances, for cards in the local addressbook, we can use special attributes to determine if two nsIAbCards are the same. they way the addressbook code works, you could create more than on nsIAbCard for a given card in the addressbook. > 6. All that unused code from AddressBookParser::AddLdifRowToDatabase > should go away. I'll log a bug about cleaning up that code. it might be getting turned on, to expand our ldif import capabilities. patch coming soon...
Attached patch updated page (obsolete) — — Splinter Review
this patch addresses the reviewers comments, and some polish: 1) existing or new profiles will only get the default columns, not all columns in the addressbook 2) when importing, we select the first item. 3) import module names are sorted. 4) "Text Files" -> "text files (LDIF, .csv, .tab, .txt)" 5) "converting mailbox from Text" -> "converting addresses from text file" 6) header parser, all callers do getService() instead of a mix of getService() and createInstance(). 7) after import of an addressbook, we now commit. there was a bug that if you quit right away, you won't save your imported ab. 8) support for "Map It!" addressbook feature.
Attachment #61667 - Attachment is obsolete: true
Comment on attachment 62530 [details] [diff] [review] updated page whoops, that patch has 2 conflicts.
Attachment #62530 - Attachment is obsolete: true
I'm not sure this is the right thread, but the address book really needs an address picker pane a la Netscape 4.76. Finding names by scrolling (with poor response time) a big address book is a total pain. I cannot get mailing lists to work at all. They do not import, do not display, and you cannot send mail to them! I tried the 12/7 build and was unable to use it at all because once one item (e.g., browser or mail) was opened, no others could be (like the address book).
> but the address book really needs an > address picker pane a la Netscape 4.76. Finding names by scrolling (with poor > response time) a big address book is a total pain. that's AB quick search, see http://bugzilla.mozilla.org/show_bug.cgi?id=83023 > I cannot get mailing lists to work at all. They do not import, do not display, > and you cannot send mail to them! can you log a bug to track this issue?
>> I cannot get mailing lists to work at all. They do not import, do not >> display, and you cannot send mail to them! > can you log a bug to track this issue? they work for me, so if you are having problems, please log a seperate bug.
Comment on attachment 62533 [details] [diff] [review] updated patch, fixed conflicts. r=dmose@netscape.com
Attachment #62533 - Flags: review+
this has sr=bienvenu. landed.
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
Trunk build 2002-01-28-03: WinMe Trunk build 2002-01-28-08: Linux RH 7.1 Trunk build 2002-01-28-08: Mac 10.1 Verified Fixed.
Status: RESOLVED → VERIFIED
Product: Browser → Seamonkey
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: