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

RESOLVED FIXED

Status

RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: mconley, Assigned: neil)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 6 obsolete attachments)

(Reporter)

Description

7 years ago
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
(Assignee)

Comment 1

7 years ago
Created attachment 548285 [details] [diff] [review]
Option 1

This version tries to be clever and avoid rebuilding. Sadly there is no sane way to update the child cache.
(Assignee)

Comment 2

7 years ago
Created attachment 548323 [details] [diff] [review]
Option 2

This just saves the previously selected directory and its index and tries to restore that first before trying the parent or another address book.
(Assignee)

Comment 3

7 years ago
Created attachment 549358 [details] [diff] [review]
Option 2 version of directory adding fix

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)
(Assignee)

Comment 4

7 years ago
Created attachment 550191 [details] [diff] [review]
Option 3 version of directory adding fix

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)

Updated

7 years ago
Assignee: nobody → neil
Status: NEW → ASSIGNED
OS: Mac OS X → All
Hardware: x86 → All
Version: unspecified → Trunk

Updated

7 years ago
Attachment #549358 - Flags: review?(mnyromyr)

Updated

7 years ago
Attachment #550191 - Flags: review?(mnyromyr) → review+
(Assignee)

Comment 5

7 years ago
Created attachment 552672 [details] [diff] [review]
Option 3, complete patch

>+    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 6

7 years ago
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-
(Assignee)

Comment 7

7 years ago
Created attachment 554678 [details] [diff] [review]
Fixed patch [Checked in: Comment 11]

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?
(Assignee)

Comment 8

7 years ago
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 9

7 years ago
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+
(Assignee)

Comment 10

7 years ago
Created attachment 555698 [details] [diff] [review]
Resort after name change

Comment 11

7 years ago
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 ...?
(Assignee)

Comment 13

7 years ago
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 14

7 years ago
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-
(Assignee)

Comment 15

7 years ago
Created attachment 566483 [details] [diff] [review]
Fixed patch

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)

Updated

7 years ago
Attachment #566483 - Flags: review?(mnyromyr) → review+
(Assignee)

Comment 16

7 years ago
Comment on attachment 566483 [details] [diff] [review]
Fixed patch

Pushed changeset c52d179c5fd1 to comm-central.
(Reporter)

Updated

7 years ago
Component: Address Book → MailNews: Address Book & Contacts
Product: Thunderbird → SeaMonkey
QA Contact: address-book → addressbook
(Assignee)

Comment 17

7 years ago
Mike, I take it you'll file your own port patch should you want it.
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
(Reporter)

Comment 18

7 years ago
Yep, filed as bug 702611.
You need to log in before you can comment on or make changes to this bug.