Closed Bug 669203 Opened 8 years ago Closed 8 years ago

directoryTreeView shouldn't alter selection when an address book is added/removed from places other than the address book

Categories

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

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: mconley, Assigned: neil)

Details

Attachments

(2 files, 6 obsolete files)

From neil@parkwaycc.co.uk:

"LDAP address books can be added and removed from the Edit Directories dialog, which (at least in SeaMonkey) can be opened from several windows, such as the Preferences window and the Account Settings window. In this case [directoryTreeView] would cause the address book window to change its selection depending on the actions in the apparently unrelated window. Users might find this surprising, especially if this causes the addressbook to prompt for an LDAP password.

Relevant code:

http://hg.mozilla.org/comm-central/rev/d5ac0eb39141#l2.279
Attached patch Option 1 (obsolete) — Splinter Review
This version tries to be clever and avoid rebuilding. Sadly there is no sane way to update the child cache.
Attached patch Option 2 (obsolete) — Splinter Review
This just saves the previously selected directory and its index and tries to restore that first before trying the parent or another address book.
This is the quick-and-dirty hack to restore the previous functionality, that is to say, don't automatically select (e.g. LDAP) addressbooks created (in another window). Given the headache I had trying to remove an arbitrary addressbook from the tree without rebuilding, I'm not going to try adding an addressbook without rebuilding unless you (Mnyromyr) really want me to.

If the new directory/addressbook/list menuitems should select the addressbook they create, that's entirely reasonable, but belongs in a separate bug.
Attachment #549358 - Flags: review?(mnyromyr)
This works slightly differently, it temporarily sets tree to null to stop _rebuild from touching it, then it fixes up the inserted row(s*) afterwards.

* In case it's possible to undelete an addressbook with mailing lists.
Attachment #550191 - Flags: review?(mnyromyr)
Assignee: nobody → neil
Status: NEW → ASSIGNED
OS: Mac OS X → All
Hardware: x86 → All
Version: unspecified → Trunk
Attachment #549358 - Flags: review?(mnyromyr)
Attachment #550191 - Flags: review?(mnyromyr) → review+
Attached patch Option 3, complete patch (obsolete) — Splinter Review
>+    if (parentIndex > -1)
>+      tree.invalidateRow(parentIndex);
>+
>+    if (!this.selection.count)
>     {
>+      // The previously selected item was a member of the deleted subtree.
>+      // Select the parent of the subtree.
>+      // If there is no parent, select the next item.
>+      // If there is no next item, select the first item.
>+      var newIndex = parentIndex;
>+      if (newIndex < 0)
>+        newIndex = itemIndex;
>+      if (newIndex >= this._rowMap.length)
>+        newIndex = 0;
>+
>+      this.selection.select(newIndex);
>     }
I could have taken a leaf out of attachment 548285 [details] [diff] [review] and written it like this:
if (!this.selection.count)
  this.selection.select(parentIndex < 0 ? itemIndex % this._rowMap.length
                                        : parentIndex);
else if (parentIndex > -1)
  tree.invalidateRow(parentIndex);
Attachment #548285 - Attachment is obsolete: true
Attachment #548323 - Attachment is obsolete: true
Attachment #549358 - Attachment is obsolete: true
Attachment #550191 - Attachment is obsolete: true
Attachment #552672 - Flags: review?(mnyromyr)
Comment on attachment 552672 [details] [diff] [review]
Option 3, complete patch

Test setup:
- started SM with -addressbook,
- opened preferences with Mail&News → Addressing and hit „Edit Directories“
- manipulated the addressbook list from that „Edit Directories“ dialog and watched the addressbook main window

Addressbooks added from the dialog will show immediately, selection will stay with selected element → okay.
But deleting addressbooks does not update the display immediately. Only when I switch focus to the addressbook main window the deleted items will disappear — and just as many unusable ghost addressbooks will appear at the end of the list …
Attachment #552672 - Flags: review?(mnyromyr) → review-
Somehow I had deleted a vital variable. Unfortunately for some reason the error gets swallowed and doesn't make it to the Error Console :-(
Attachment #552672 - Attachment is obsolete: true
Attachment #554678 - Flags: review?
Comment on attachment 554678 [details] [diff] [review]
Fixed patch [Checked in: Comment 11]

>You entered a username that did not match any known Bugzilla users, so we have instead left the requestee field blank.
Gee, thanks Bugzilla!
Attachment #554678 - Flags: review? → review?(mnyromyr)
Comment on attachment 554678 [details] [diff] [review]
Fixed patch [Checked in: Comment 11]

That's better.

I noticed, though, this oddity not literally in the scope of this bug:
- Have an LDAP addressbook, eg. named 'm' and select it in the main addressbook window.
- From preferences, add an LDAP addressbook before this, eg. named 'a'.
- From preferences, rename 'a' to 'z'.
The addressbook name will change in the background main addressbook window immediately, but its position will not, until you add/remove something else.
Attachment #554678 - Flags: review?(mnyromyr) → review+
Attached patch Resort after name change (obsolete) — Splinter Review
Comment on attachment 554678 [details] [diff] [review]
Fixed patch [Checked in: Comment 11]

http://hg.mozilla.org/comm-central/rev/0465ad0e1bc9
Attachment #554678 - Attachment description: Fixed patch → Fixed patch [Checked in: Comment 11]
Comment on attachment 555698 [details] [diff] [review]
Resort after name change

Neil, did you want to ask for review, or ...?
Comment on attachment 555698 [details] [diff] [review]
Resort after name change

Probably meant to ask for review after checking in the previous patch but then forgot to note the check in let alone ask for review...
Attachment #555698 - Flags: review?(mnyromyr)
Comment on attachment 555698 [details] [diff] [review]
Resort after name change

>diff --git a/suite/mailnews/addrbook/abTrees.js b/suite/mailnews/addrbook/abTrees.js
>--- a/suite/mailnews/addrbook/abTrees.js
>+++ b/suite/mailnews/addrbook/abTrees.js
>@@ -338,15 +338,24 @@ directoryTreeView.prototype =

The patch is broken and both hg/patch bitterly complain. The header suggests the addition of nine lines, but the diff only adds seven.
Not surprisingly, it doesn't work even with the chunk header repaired: on rename from preferences, the addressbook main window tree is updated, but wrongly. Only when focussing the addressbook main window it'll get corrected.
Attachment #555698 - Flags: review?(mnyromyr) → review-
Attached patch Fixed patchSplinter Review
Trees are hard. Did I mention that RDF does this all for you automagically?
Attachment #555698 - Attachment is obsolete: true
Attachment #566483 - Flags: review?(mnyromyr)
Attachment #566483 - Flags: review?(mnyromyr) → review+
Comment on attachment 566483 [details] [diff] [review]
Fixed patch

Pushed changeset c52d179c5fd1 to comm-central.
Component: Address Book → MailNews: Address Book & Contacts
Product: Thunderbird → SeaMonkey
QA Contact: address-book → addressbook
Mike, I take it you'll file your own port patch should you want it.
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.