117267 - Delete a list in the dir pane, where should focus (selection) go?

Status: VERIFIED FIXED

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

Description

[Ninoschka Baca]

Trunk build 2001-12-27-03: WinMe

Overview: With a list selected in the directory pane, delete the list and where 
should the focus go?

Steps to reproduce: 
1. Have 2 address books
2. Select the 2nd address book
3. Create a list under the 2nd address book
4. With the newly created list selected in the Directory pane, select the Delete 
button

Actual Results: The list is deleted and the focus goes to the 1st address book 
which is the Personal Address Book.

Expected Results: I would expect the focus to go to the parent level of the 
recently deleted list. In this case focus should go to the 2nd address book and 
not the 1st.

Comment 1

[Ninoschka Baca]

Note: bug# 117225 covers the case where you delete a mailing list and the change 
is not automatically reflected in the Sidebar AB.

Comment 2

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

adding jglick for feedback.

Comment 3

[jglick]

I would agree with nbaca. The parent AB would get focus in this case. Focus 
moves Down the list. When an item is deleted, the item below it gets focus. If 
the item removed is the Last item in the entire pane, the item above it gets 
focus.

Comment 4

[Matthew T (active 1999-2002)]

*** This bug has been marked as a duplicate of 27417 ***

Comment 5

[Ninoschka Baca]

Reopening because I believe this bug depends on bug# 27417. I also want to test
this case once 27417 is fixed.

Comment 6

[Mark Banner (:standard8)]

I'm looking at a fix for this bug, it isn't dependent on 27417 as the code
explicitly sets the selection to the PAB. Therefore removing dependency and
assigning to self, cc'ing seth as original owner.

Also changing OS to all as it affects Linux as well.

Comment 7

[Mark Banner (:standard8)]

Attachment: Alter dir pane selection on delete

This patch changes the code so that we select the parent if we are deleting a
mailing list, and the next address book if we are deleting an address book. If
it can't select the next address book, then we select the Personal Address
Book.

Also, I have removed the address book listener that was attached to
nsIAddrBookSession, this seems redundant because of the way that the selection
of the new item occurs.

In the case where we remove an item and not have a different directory selected
before hand should never occur, if it did there would be an assertion raised
when updating cmd_delete (there was one here before this patch, I think I
accidently created it when implementing bug 112959 - sorry).

Comment 8

[Mark Banner (:standard8)]

Dan, Any chance of an r on this sometime? Thanks.

Comment 9

[Mark Banner (:standard8)]

Comment on attachment 168153 [details] [diff] [review]
Alter dir pane selection on delete

Notes to self from discussion on IRC 19/01:

Standard8: is the only way to delete addressbooks / lists via the address book
UI on the address book dir pane?
dmose: ldap prefs pane is one way
dmose: i wonder if it's possible to delete cards from the address book search
window results pane
Standard8: we also have the following potentials: side bar address book,
compose -> select addresses, search addresses as you say ....
Standard8: given that I think it may be better to retain the listener, but
still implement the select next or whatever I called it function.
dmose: and, in fact, add a comment by the listener function mentioning why it's
there
dmose: also, why put that function in abCommon.js? are other pieces of code
likely to need it?
Need to also check thunderbird.

Comment 10

[Mark Banner (:standard8)]

Attachment: Alter selection on delete v2

This patch keeps the listener (see previous comment) and ensures that we always
get a good result with ensuring that we always select something. The changes in
the DirPaneController fix an assertion that was happening when items where
deleted from elsewhere.

Comment 11

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 172080 [details] [diff] [review]
Alter selection on delete v2

r- at least for the intruding tab... the other stuff may just be pieces I'm not
grokking yet.

Index: mailnews/addrbook/resources/content/abCommon.js
===================================================================
RCS file: /cvsroot/mozilla/mailnews/addrbook/resources/content/abCommon.js,v
retrieving revision 1.99
diff -u -u -8 -p -r1.99 abCommon.js
--- mailnews/addrbook/resources/content/abCommon.js	1 Dec 2004 22:25:11
-0000	    1.99
+++ mailnews/addrbook/resources/content/abCommon.js	22 Jan 2005 10:10:53
-0000
@@ -175,9 +176,14 @@ var DirPaneController =
	 // so we forward select all to the results pane
	 // but if there is no gAbView
	 // don't bother sending to the results pane
	 return (gAbView != null);
       case "cmd_delete":
       case "button_delete":
	 var selectedDir = GetSelectedDirectory();
	 if (command == "cmd_delete") {
-	   goSetMenuValue(command, GetDirectoryFromURI(selectedDir).isMailList
? "valueList" : "valueAddressBook");
+	   if (selectedDir)
+	     goSetMenuValue(command,
GetDirectoryFromURI(selectedDir).isMailList ? "valueList" :
"valueAddressBook");
+	   else
+	     // If we don't have a selected directory here, the best we
+	     // can do is to select the first address book
+	     SelectFirstAddressBook();

I'm not understanding this change.  Why does it make sense to change
selection inside an isCommandEnabled function?	If there's nothing
selected, isn't the proper thing to do just return false?

--- mailnews/addrbook/resources/content/addressbook.js	16 Aug 2004 15:31:29
-0000	   1.111
+++ mailnews/addrbook/resources/content/addressbook.js	22 Jan 2005 10:10:57
-0000
@@ -54,29 +55,54 @@ var gCardViewBoxEmail1;
 // Constants that correspond to choices
 // in Address Book->View -->Show Name as
 const kDisplayName = 0;
 const kLastNameFirst = 1;
 const kFirstNameFirst = 2;
 const kLDAPDirectory = 0; // defined in nsDirPrefs.h
 const kPABDirectory  = 2; // defined in nsDirPrefs.h

+function SelectParentOrNextAddressBook(currentSelection)
+{
+  var newRow = dirTree.view.getParentIndex(currentSelection);
+  // if we have no parent (i.e. we are an address book), select the
+  // next or the PAB.
+  if (newRow == -1) {
+    newRow = currentSelection + 1;
+    while ((newRow < dirTree.view.rows) &&
+	    (dirTree.view.getParentIndex(newRow) != -1)) {
+      newRow++;
+    }
+    // Select the Personal Address Book if we didn't find another one.
+    if (newRow == dirTree.view.rows)
+		  newRow = 0;

The above line appears to have a tab in it.

@@ -557,11 +583,13 @@ function AbDeleteDirectory()
   if (!promptService.confirm(window, null, confirmDeleteMessage))
     return;

   var resourceArray =
Components.classes["@mozilla.org/supports-array;1"].createInstance(Components.i
nterfaces.nsISupportsArray);
   var selectedABResource =
GetDirectoryFromURI(selectedABURI).QueryInterface(Components.interfaces.nsIRDFR
esource);

   resourceArray.AppendElement(selectedABResource);

+  // Select the parent/next address book before deleting the old one.
+  // That way we can move the selection to where we want to properly.
+  SelectParentOrNextAddressBook(dirTree.currentIndex);
   top.addressbook.deleteAddressBooks(dirTree.database, parentArray,
resourceArray);
-  SelectFirstAddressBook();
 }

Actually, won't this selection happen in onItemRemoved too, making it
unnecessary here?

Comment 12

[Mark Banner (:standard8)]

> I'm not understanding this change.  Why does it make sense to change
> selection inside an isCommandEnabled function?	If there's nothing
> selected, isn't the proper thing to do just return false?
It doesn't make sense, my fault.

> +  // Select the parent/next address book before deleting the old one.
> +  // That way we can move the selection to where we want to properly.
> +  SelectParentOrNextAddressBook(dirTree.currentIndex);
>    top.addressbook.deleteAddressBooks(dirTree.database, parentArray,
> resourceArray);
> -  SelectFirstAddressBook();
>  }
> 
> Actually, won't this selection happen in onItemRemoved too, making it
> unnecessary here?

Unfortunately, by the time we get to onItemRemoved, dirTree.currentIndex is -1,
hence we have lost the original selection from before the delete - hence we call
it here to select the new item.

We keep the onItemRemoved situation to ensure that when we get selected items
removed elsewhere (e.g. from the sidebar), we auto-select the PAB as a best effort.

Hmmm, if dirTree.currentIndex is always -1 in onItemRemoved, that also means we
can automatically assume in onItemRemoved that we don't know the selection and
therefore just do a straight call to SelectFirstAddressBook in onItemRemoved as
we did before.

New patch coming soon.

Comment 13

[Mark Banner (:standard8)]

Attachment: Patch v3

Changes:
- sorted out tabs
- no longer attempt to change selection from within isCommandEnabled
- onItemRemoved just sets the selection to the first book (by the time we get
there, currentIndex is not valid)
- Added extra notification option for the listener so that we pick up
notification of lists being deleted as well.

Comment 14

[Dan Mosedale (:dmosedale, :dmose)]

(In reply to comment #12)
> > +  // Select the parent/next address book before deleting the old one.
> > +  // That way we can move the selection to where we want to properly.
> > +  SelectParentOrNextAddressBook(dirTree.currentIndex);
> >    top.addressbook.deleteAddressBooks(dirTree.database, parentArray,
> > resourceArray);
> > -  SelectFirstAddressBook();
> >  }
> > 
> > Actually, won't this selection happen in onItemRemoved too, making it
> > unnecessary here?
> 
> Unfortunately, by the time we get to onItemRemoved, dirTree.currentIndex is -1,
> hence we have lost the original selection from before the delete - hence we call
> it here to select the new item.
> 
> We keep the onItemRemoved situation to ensure that when we get selected items
> removed elsewhere (e.g. from the sidebar), we auto-select the PAB as a best
effort.

So now we have a situation where one thing happens in delete from the main
addressbook window, and something else happens on delete from elsewhere. 
Wouldn't we do better to have something keep track of the previous selection
state in the dirtree so that the correct selection can be done in onItemRemoved?

Comment 15

[Mark Banner (:standard8)]

Attachment: Patch v4

As discussed on IRC this patch now uses the previous selection to work out what
to select next. Fixes both suite & thunderbird, also handles selection when
items are deleted from elsewhere. Note: thunderbird doesn't need the extra
change in the cmd_delete section.

Comment 16

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 183312 [details] [diff] [review]
Patch v4

If there's no previous selection, this patch currently falls back to selecting
the first item.  Perhaps the first selection could populate the previous
selection variable with the parent's index, so that the parent gets selected
if a list is deleted?

Comment 17

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 183312 [details] [diff] [review]
Patch v4

I think I'm confused.  So the following code is in onItemRemoved(), which gets
called _after_ whatever item you select has been removed, right?

>+      else {
>+        // Don't reselect if we already have a valid selection
>+        if (dirTree.currentIndex == -1) {

Assuming the answer to my question is yes, the selected item will have just
been deleted, meaning that the above if clause is always guaranteed to be true,
I think.  Or do I misunderstand how the tree works?

>+          var directory = item.QueryInterface(Components.interfaces.nsIAbDirectory);
>+
>+          // If we are a mail list, move the selection up the list before
>+          // trying to find the parent. This way we'll end up selecting the
>+          // parent address book when we remove a mailing list.
>+          //
>+          // For simple address books we don't need to move up the list, as
>+          // we want to select the next one upon removal.
>+          if (directory && directory.isMailList && gPreviousDirTreeIndex > 0)
>+            --gPreviousDirTreeIndex;
>+
>+          // Now get the parent of the row.
>+          var newRow = dirTree.view.getParentIndex(gPreviousDirTreeIndex);
>+
>+          // if we have no parent (i.e. we are an address book), use the
>+          // previous index - which will be the next address book in the list.
>+          if (newRow == -1)
>+            newRow = gPreviousDirTreeIndex;

I don't see why the comment above is true -- why is gPreviousDirTreeIndex
guaranteed to be the next addressbook in the list?  The code itself does what
we want, I think.

>   // this listener only cares when a directory is removed
>-  addrbookSession.addAddressBookListener(gAddressBookAbListener, Components.interfaces.nsIAbListener.directoryRemoved);
>+  addrbookSession.addAddressBookListener(gAddressBookAbListener, Components.interfaces.nsIAbListener.directoryRemoved | Components.interfaces.nsIAbListener.directoryItemRemoved);

So why is the above change necessary?

(Similar comments apply to the T-Bird part of the patch.)

Comment 18

[Mark Banner (:standard8)]

(In reply to comment #17)
> I think I'm confused.  So the following code is in onItemRemoved(), which gets
> called _after_ whatever item you select has been removed, right?
> 
> >+        // Don't reselect if we already have a valid selection
> >+        if (dirTree.currentIndex == -1) {
> 
> Assuming the answer to my question is yes, the selected item will have just
> been deleted, meaning that the above if clause is always guaranteed to be true,
> I think.  Or do I misunderstand how the tree works?

The answer to the first question is yes, but you've forgotten the case where we
have a valid selection in the dir tree and someone goes and deletes a mailing
list or address book elsewhere (not the selected one) i.e. the sidebar panel.
This protects us from moving the selection where it's not necesary.
> 
> >+          var directory =
item.QueryInterface(Components.interfaces.nsIAbDirectory);
> >+
> >+          // If we are a mail list, move the selection up the list before
> >+          // trying to find the parent. This way we'll end up selecting the
> >+          // parent address book when we remove a mailing list.
> >+          //
> >+          // For simple address books we don't need to move up the list, as
> >+          // we want to select the next one upon removal.
> >+          if (directory && directory.isMailList && gPreviousDirTreeIndex > 0)
> >+            --gPreviousDirTreeIndex;
> >+
> >+          // Now get the parent of the row.
> >+          var newRow = dirTree.view.getParentIndex(gPreviousDirTreeIndex);
> >+
> >+          // if we have no parent (i.e. we are an address book), use the
> >+          // previous index - which will be the next address book in the list.
> >+          if (newRow == -1)
> >+            newRow = gPreviousDirTreeIndex;
> 
> I don't see why the comment above is true -- why is gPreviousDirTreeIndex
> guaranteed to be the next addressbook in the list?  The code itself does what
> we want, I think.

Ok, so it's not necessarily the *next* address book in the list, it's an address
book. I forgot about the case where you delete the only mailing list in an
address book.

> >   // this listener only cares when a directory is removed
> So why is the above change necessary?
It's to bring in the mailing list removed notifications, see the patch I've just
added to bug 292350.

Comment 19

[timeless]

Comment on attachment 183312 [details] [diff] [review]
Patch v4

>Index: mailnews/addrbook/resources/content/addressbook.js
>@@ -44,39 +45,71 @@ var gSearchTimer = null;
>+        if (dirTree.currentIndex == -1) {
>+          var directory = item.QueryInterface(Components.interfaces.nsIAbDirectory);
...
>+          if (directory && directory.isMailList && gPreviousDirTreeIndex > 0)

directory will be true, QueryInterface will throw an exception, never return
null. do you want that exception to bubble beyond this function?

Comment 20

[Mark Banner (:standard8)]

Attachment: Patch v5

Updated after discussion on irc - added some more comments and removed an
unnecessary check.

Comment 21

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 187866 [details] [diff] [review]
Patch v5

>+  // this listener cares when a directory (= address book), or a directory item
>+  // is/are removed. In the case of directory items, we are only really
>+  // interested in mailing list changes and not cards but we have to have both.
>+  addrbookSession.addAddressBookListener(gAddressBookAbListener, Components.interfaces.nsIAbListener.directoryRemoved | Components.interfaces.nsIAbListener.directoryItemRemoved);

Make both instances of this line wrap at < column 80, and you've got r=dmose.

Thanks for the patch!

Comment 22

[Mark Banner (:standard8)]

Attachment: Patch v6 (Checked in)

addressed dmose's review nit, carrying forward r as well. Requesting sr.

Comment 23

[Mark Banner (:standard8)]

Comment on attachment 187872 [details] [diff] [review]
Patch v6 (Checked in)

Requesting approval for low risk patch to fix what happens to selections, also
fixes some assertions. Affects address book in seamonkey & thunderbird.

Comment 24

[Ian Neal]

Comment on attachment 187872 [details] [diff] [review]
Patch v6 (Checked in)

Checking in mailnews/addrbook/resources/content/abCommon.js;
new revision: 1.102; previous revision: 1.101
mailnews/addrbook/resources/content/addressbook.js;
new revision: 1.115; previous revision: 1.114
mail/components/addrbook/content/abCommon.js;
new revision: 1.11; previous revision: 1.10
mail/components/addrbook/content/addressbook.js;
new revision: 1.18; previous revision: 1.17
done

Comment 25

[[e-mail]]

I assume that deleting from another window updates the last selected index?

Comment 26

[Mark Banner (:standard8)]

Thanks to IanN for checking this in for me, this bug is now fixed.

Comment 27

[Mark Banner (:standard8)]

(In reply to comment #25)
> I assume that deleting from another window updates the last selected index?

Opps, only just saw the comment - had to check, but yes doing the
dirTree.select. statement in onItemRemoved follows up by calling
DirPaneSelectionChange which sets the last selected index.

Comment 28

[Stephen Donner [:stephend] Not actively reading bugmail]

Setup:

abook1 (1st address book)
abook2 (2nd address book)
>abooklist2 (mailing list for 2nd address book)

Following the steps in comment 0, focus goes back up to 'abook2' after deletion
of 'abooklist2' occurs.

Verified FIXED with build 2005-07-15-05 on Windows XP SeaMonkey trunk.