437619 - Centralise and de-branch the Address Book results view functions

Status: RESOLVED FIXED

(MailNews Core :: Address Book, defect, P1)

Description

[Mark Banner (:standard8)]

The functions directly relating to the address book results view are currently spread over a mixture of files, including:

abCommon.js, abResultsPane.js, addressbook.js

with copies in both Thunderbird and SeaMonkey. The locations of these functions mean you have to go around all over the place to try and work out what is happening. They are also partially out of sync.

Given that we have abResultsPane.js I'm proposing:

1) Take all the SeaMonkey functions directly relevant to the results pane (or where they would benefit from being shared) and move them into abResultsPane.js
2) Merge differences in functionality between Thunderbird & SeaMonkey
3) Make Thunderbird use all the functions in abResultsPane.js

Unfortunately, as Thunderbird currently uses abResultsPane.js I'm going to have to do this in one big landing, but I'll separate the patches into three for ease of review.

Comment 1

[Mark Banner (:standard8)]

Attachment: Remove nsIAbView::swapFirstNameLastName

First things first, let's get rid of this redundant function... This functionality is actually performed by an observer on the preference from within nsAbView and doesn't use this function.

Comment 2

[Mark Banner (:standard8)]

Attachment: Part 1 - move SeaMonkey functions

This part moves the SeaMonkey functions from their current locations to abResultsPane.js, most of this is just copy and paste and some whitespace cleanup, these are the addtiional changes I made:

- removed AddPrefObservers in addressbook.js (they weren't required because the call backs from nsAbView cause the update in the card preview pane)
- SetAbView() now sets up gAbResultsTree and uses it

You'll need attachment 324040 [details] [diff] [review] if you want to apply this patch.

I won't check this in until the other patches are done for merge and Thunderbird as per comment 0, but would like to get reviews as we go.

Comment 3

[Mark Banner (:standard8)]

(In reply to comment #1)
> Created an attachment (id=324040) [details]
> Remove nsIAbView::swapFirstNameLastName
> 
> First things first, let's get rid of this redundant function... This
> functionality is actually performed by an observer on the preference from
> within nsAbView and doesn't use this function.
> 
mxr link for this:

http://mxr.mozilla.org/seamonkey/search?string=swapFirstNameLastName

note that there's two AbSwapFirstNameLastName functions, which call swapFirstNameLastName on the abView. However there's nothing that calls the AbSwapFirstNameLastName functions either (as shown by the same search).

Comment 4

[David :Bienvenu]

Comment on attachment 324040 [details] [diff] [review]
Remove nsIAbView::swapFirstNameLastName

it looks a little more complicated than that - maybe I'm missing something, but there seems to be a menu item which you can enable in localizations, and that menu item calls AbSwapFirstNameLastName().

http://mxr.mozilla.org/seamonkey/search?string=hideSwapFnLnUI&find=&findi=&filter=^[^\0]*%24&hitlimit=&tree=seamonkey

Comment 5

[[e-mail]]

Comment on attachment 324041 [details] [diff] [review]
Part 1 - move SeaMonkey functions

>+function SetAbView(uri, searchView)
>+{
>+  // No changes are needed if we're not changing the URI (the URI includes the
>+  // search string, so we don't need to worry about searchView being different
>+  // or not).
>+  if (gAbView.uri == uri)
gAbView can be null here.

>+function onEnterInSearchBar()
>+{
>+  ClearCardViewPane();
ClearCardViewPane only applies in the addressbook, not the sidebar/dialog.

>+function GetAbResultsBoxObject()
>+{
>+  return gAbResultsTree.treeBoxObject;
>+}
gAbResultsTree was null on unload, presumably a side-effect.

Comment 6

[Mark Banner (:standard8)]

Comment on attachment 324040 [details] [diff] [review]
Remove nsIAbView::swapFirstNameLastName

yeah, looks like I missed that.

Comment 7

[Mark Banner (:standard8)]

Attachment: Sync TB results pane code with SM

I have already done some synchronisations in other bugs, this patch finishes them off:

- Removes cmd_printcard and cmd_printcardpreview command handlers. This will regress being able to select one of these menus when no cards are selected. SeaMonkey currently has this bug. My plan is to rework these menu options similar to bug 440513, but I would rather fix this bug (and the associated nsAbView one) first. Hence I'm just dropping these options for now to make it clear what I am doing.

- SetAbView(). I have matched the code the same way as SeaMonkey fixed bug 252759 - this will retain sort options correctly across different searches as well as simplifying the code.

- Pref Observer for "mail.addr_book.lastnamefirst", the observer defined in addressbook.js wasn't actually required because nsAbView listens for that pref and the subsequent handling causes the correct updates to occur without the additional code in addressbook.js.

Comment 8

[Mark Banner (:standard8)]

Attachment: [checked in] Sync TB results pane code with SM

Apologies, here is the correct patch.

Comment 9

[Mark Banner (:standard8)]

Attachment: Drop unnecessary observer from SM

Like I said in comment 7, this pref observer isn't necessary. Hence dropping it from the SM code as well (in a separate patch to the main one, to make reading easier).

Comment 10

[Mark Banner (:standard8)]

Attachment: Centralise and de-branch the results view functions

This patch needs applying after attachment 326274 [details] [diff] [review] and attachment 326295 [details] [diff] [review]. It moves all the functions (that are sensible to) into abResultsPane.js, and adjusts TB and SM to accommodate the moves. No functional changes (I compared the functions before moving them), mainly just moves.

This has the great benefit of locating similar functions together and reducing the amount of mixed code in addressbook.js/abCommon.js that SM and TB have.

Comment 11

[Mark Banner (:standard8)]

Comment on attachment 326300 [details] [diff] [review]
Centralise and de-branch the results view functions

I've just landed the abDragDrop.js changes into cvs as I messed up with the original patch separation. I've done this so that drag and drop in Thunderbird's AB won't be broken in the nightlies (especially as that's the only way of copying/moving cards).

Comment 12

[[e-mail]]

Comment on attachment 326295 [details] [diff] [review]
Drop unnecessary observer from SM

Sorry, but this observer is necessary in the case that the results view is sorted in some irrelevant order; what you're seeing when the results pane is sorted in name order is that the pane is resorted and the selected card reselected which triggers the redisplay of the card pane.
You could of course tweak the address book view to send a selection event in the case where a re-sort isn't necessary.

Comment 13

[Mark Banner (:standard8)]

Attachment: [checked in] Drop unnecessary observer from SM and fix display update

Improvement on the previous version - if we refresh the tree, but the sort order hasn't changed (i.e. first/last name order has), fire a SelectionChanged (which goes via nsIAbViewListener) which will cause a redisplay of the selected card in the preview pane.

Comment 14

[Mark Banner (:standard8)]

Attachment: [checked in] Centralise and de-branch the results view functions v1a

Updated patch to work with latest cvs, so this doesn't include the abDragDrop.js changes that were in the first version and that I checked in early.

Comment 15

[[e-mail]]

Comment on attachment 327391 [details] [diff] [review]
[checked in] Centralise and de-branch the results view functions v1a

>+  gAbResultsTree.treeBoxObject.view =
>+    gAbView.QueryInterface(Components.interfaces.nsITreeView);
This looked odd until I realised that we call nsITreeView methods on gAbView which is why we can't rely on xpconnect's implicit QI.

>+    if (gCurFrame && 
>+        gCurFrame.getAttribute("src") == abPanelUrl &&
>+        document.commandDispatcher.focusedWindow == gCurFrame.contentDocument.defaultView) {
>+      abView = gCurFrame.contentDocument.defaultView.gAbView;
>+    }
Everywhere else where you moved the code you remove the braces completely but here you just tacked the opening { onto the end of the previous line.

>@@ -470,7 +470,7 @@ function onAdvancedAbSearch()
> 
> function onEnterInSearchBar()
> {
>-  ClearCardViewPane();  
>+  ClearCardViewPane(); 
This is still wrong :-P Anyway, you shouldn't be doing miscellaneous whitespace cleanup here, that's Serge's job ;-) Of course, you should clean up trailing whitespace in the lines that you move, although you haven't always done so.

Comment 16

[Mark Banner (:standard8)]

Comment on attachment 327391 [details] [diff] [review]
[checked in] Centralise and de-branch the results view functions v1a

For testers, ensure that anywhere that displays lists of address book cards/mailing lists, works to the same extent as before.