245710 - [patch] bookmark manager info and contextual menu cleanup

Status: RESOLVED FIXED

(Camino Graveyard :: Bookmarks, defect)

Description

[Josh Aas]

This patch does the following:

1. Open the bm manager, select an item, and then deselect all items by clicking
in space that is not a row. The info button will disable itself when the
selection changes to having no row selected. But if you then go to another
collection (history or whatever) and then return to the collection that has no
row selected, the info button will incorrectly be enabled. This patch fixes that
by making sure that when the collection selection changes, not only are the
collection items editable, but also that their is a (single) item selected.

2. If multiple items are selected the get info button should not be enabled and
the info window should not be showing. Currently the bm manager allows the info
window on a multiple selection. This patch fixes that.

3. The contextual menu item that sets the dock menu from the bm manager has
slightly different wording in the item pane and the collections pane ("Use as
Dock Menu" and "Use for Dock Menu"). This patch fixes that.

4. The ExtendedTableView class does not allow for a contextual menu unless a row
is selected. Since ExtendedTableView is a general case class that is supposed to
extend and not restrict behavior, this is not OK. This patch fixes that by
correctly delegating responsibility for table view behavior.

5. The contextual menu for collection items doesn't have a very logical
organization in terms of when/where its separator item shows up. This patch
fixes that.

6. It isn't possible to make a new collection via contextual menu unless you
control click on an existing one (there is no menu for clicking on empty space
in the collection page). This is awkward and unexpected behavior. This patch
fixes the problem.

All this in a small patch containing a net reduction in lines of code (-4) and
nib data.

Comment 1

[Josh Aas]

Attachment: bookmark manager info and cm cleanup v1.0

Comment 2

[Josh Aas]

When applying this patch, open up BrowserWindow.nib and delete the second item
in the CollectionContextMenu. Only the item named "New Collection" should remain.

Comment 3

[Stuart Morgan]

Looks good from looking over it, and I'll test it in the next day or two.

One request, as long as there's context-menu cleanup: For some reason Top 10
List bookmarks are deletable. First off, they shouldn't be for consistency with
other collections that aren't user-built (such as the Address Book collection)
and the fact that it's not otherwise modifiable, as well as similar things in,
e.g., iTunes. Second, deleting a Top 10 List bookmark actually deletes the
bookmark itself, which is, I think, unexpected behavior, and made worse by the
fact that the user might not notice until later, when undo might not be an option.

If that's a small change, this seems like a great place to make it. If not, I'll
open another bug.

Comment 4

[Josh Aas]

Stuart: I totally agree, didn't notice that before. I'll fix it tomorrow and
post a new patch.

Comment 5

[Stuart Morgan]

Does this correctly handle the Get Info menu item as well?

Comment 6

[Stuart Morgan]

Comment on attachment 150125 [details] [diff] [review]
bookmark manager info and cm cleanup v1.0

Clearing review requests since a new patch is coming.

Comment 7

[Stuart Morgan]

*** Bug 179600 has been marked as a duplicate of this bug. ***

Comment 8

[Josh Aas]

Agreed that we want a contextual menu for deleting history folders? You can do
it with a key, you ought to be able to do it with a right click. The patch is on
the way...

Comment 9

[Josh Aas]

I am really close to a final version of my patch. In addition to the fixes
listed in the summary, I have also fixed the following bugs:

7. Don't allow delete key to delete items under the address book. We don't give
a contextual menu for that, and since we load dynamically from the system
address book, it isn't clear how deleting items interacts with the address book.
Does it delete the whole record? How do you get it back? Just don't allow it.
8. Deleting items from the Top Ten list is no longer allowed
9. Top Ten items are sorted now, with most visited site on top
10. Don't show contextual menu items for multiple selection functionality we
haven't implemented

All of these bugs combined give a feeling of sloppiness. Nominating these all
combined as blocking 0.8. Patch will come very soon.

Comment 10

[Josh Aas]

Attachment: bm manager cleanup v2.0

After applying this patch, open up BrowserWindow.nib and delete the second item

in the CollectionContextMenu. Only the item named "New Collection" should
remain.

Comment 11

[Josh Aas]

To summarize, my patch should fix the following bugs (I was not able to finish
sorting the top ten due to some crazy messaging code, so its not included):

1. Open the bm manager, select an item, and then deselect all items by clicking
in space that is not a row. The info button will disable itself when the
selection changes to having no row selected. But if you then go to another
collection (history or whatever) and then return to the collection that has no
row selected, the info button will incorrectly be enabled. This patch fixes that
by making sure that when the collection selection changes, not only are the
collection items editable, but also that their is a (single) item selected.

2. If multiple items are selected the get info button should not be enabled and
the info window should not be showing. Currently the bm manager allows the info
window on a multiple selection. This patch fixes that.

3. The contextual menu item that sets the dock menu from the bm manager has
slightly different wording in the item pane and the collections pane ("Use as
Dock Menu" and "Use for Dock Menu"). This patch fixes that.

4. The ExtendedTableView class does not allow for a contextual menu unless a row
is selected. Since ExtendedTableView is a general case class that is supposed to
extend and not restrict behavior, this is not OK. This patch fixes that by
correctly delegating responsibility for table view behavior.

5. The contextual menu for collection items doesn't have a very logical
organization in terms of when/where its separator item shows up. This patch
fixes that.

6. It isn't possible to make a new collection via contextual menu unless you
control click on an existing one (there is no menu for clicking on empty space
in the collection page). This is awkward and unexpected behavior. This patch
fixes the problem.

7. The "Get Info" menu is no longer enabled when it shouldn't be

8. Users are no longer allowed to delete top ten items from the top ten collection

9. Don't allow delete key to delete items under the address book. We don't give
a contextual menu for that, and since we load dynamically from the system
address book, it isn't clear how deleting items interacts with the address book.
Does it delete the whole record? How do you get it back? Just don't allow it.

10. Context-clicking on a multiple selection in the item pane changes the
selection to a single selection on the item under the click. This is consistent
with the collection pane. This behavior combined with some menu tweaks means
that we no longer show context menus for functionality we don't implement (i.e.
opening multiple bookmarks with multiple selection)

Please test this important patch more than you would a normal patch. Its not the
easiest code to mess with, and we don't have much time before release to find
any nasty side effects that might show up. That said, I obviously think its good
or I wouldn't be posting it ;)

Also, make sure this patch doesn't conflict with smorgan's patch for bug 242650.

Comment 12

[Josh Aas]

Attachment: bm manager cleanup v2.1

Fixed a memory leak that resulted from using the copy method and not
autoreleasing. Might be done a bit cleaner, but it works fine. Feel free to
clean it up before landing - I'm going to bed.

Comment 13

[Stuart Morgan]

First-pass stuff (more tonight when I've had time to test):

+    NSMenu *contextMenu = [[aTableView menu] copy];
...
+    if (contextMenu != nil) {
+      [contextMenu autorelease];
+    }

You hinted at this yourself; why not just replace all that with |NSMenu
*contextMenu = [[[aTableView menu] copy] autorelease];| ?

+- (BOOL)bookmarksAreVisible:(BOOL)inRequireSelection
allowMultipleSelection:(BOOL)allowMultipleSelection

Am I the only one totally baffled by what this function and its
parameters--especially the first one--are for (not just this new version, but
the original too)? This at the very least needs to be renamed to be clear about
what it and its parameteres are supposed to do, and maybe it needs to be split
apart if there's no way to explain what it is and what it's for.

+  // if we're not looking at the top ten list and its parent is a bookmark
folder, but not a smart one

This really bothers me, and did when I found the bug in the first place. We
shouldn't *need* a check for the top-10 list, because it should be a smart
folder. Any clue why it's not identifying itself as such?


By the way, judging by what's touched we should be safe in terms of having no
conflict with my bug 242650 patch.

Comment 14

[Stuart Morgan]

Hm, I missed this before:

+enum {
+  kBookmarkMenuContainerIndex = 0,
+  kToolbarContainerIndex = 1,
+  kHistoryContainerIndex = 2,
+  kTopTenListContainerIndex = 3,
+  kRendezvousContainerIndex = 4,
+  kAddressBookContainerIndex = 5
+};

I took most of the hard-coded index definitions out earlier because they aren't
at all safe to use--There's the possibility of the broken bookmark folder, plus
rendezvous and addressbook are 10.2+ only. In addition, it creates
inflexibility.  The correct way to do this is probably to replace your new
function to get the active collection index with something to get the active
collection, then check those against the specific smartfolder accessor functions
in BookmarkManager

Comment 15

[Stuart Morgan]

Ok, someone check my logic, but I'm pretty sure we can replace 

+  if (!outlineView ||
+      !([target isKindOfClass:[BookmarkViewController class]] &&
+        ([target indexOfActiveCollection] == kTopTenListContainerIndex))) {
+    if ([parent isKindOfClass:[BookmarkFolder class]] && ![parent isSmartFolder]) {

With the cleaner (or at least more flexible):

   if (!outlineView || ![target isKindOfClass:[BookmarkViewController class]] ||
         ![[target activeCollection] isSmartFolder] ||
         ([target activeCollection] == [self historyFolder])) {

It's still a bit ugly because of the safety check in the second ||, but it will
adapt to new smartfolders and is easier to understand, I think.

Comment 16

[Stuart Morgan]

Sorry, forgot to add that |activeCollection| is a new function to get the active
collection (assuming there isn't a function like that already)

Comment 17

[Stuart Morgan]

I'm an idiot, |activeCollection| already exists and does what I needed.

Comment 18

[Stuart Morgan]

Comment on attachment 150779 [details] [diff] [review]
bm manager cleanup v2.1

r- based mostly on comment 14. That will require enough changes that I'll hold
off on further review.

Comment 19

[Stuart Morgan]

If you feel like it, you could also toss in the incredibly tiny cleanup patch
from bug 226696 that pink is never going to land.

Comment 20

[Josh Aas]

Attachment: bm manager cleanup v3.0

Fixes issues raised in comments 13, 14, and 15. Not going to change any method
names because that is a mess I don't want to get into. Also not going to add a
patch for bug 226696 because I'm not sure I agree with the basis for it, and it
is not necessary now.

- apply patch
- apply nib change described above

Comment 21

[Stuart Morgan]

> Also not going to add a patch for bug 226696 because I'm not sure
> I agree with the basis for it

Sorry, I wasn't clear... There's a leftover patch there of incedental cleanup of
the BookmarkViewController, unrelated to the original patch, which is why I
thought of it. But it's inconsequential, so best not to deal with it.

Comment 22

[Stuart Morgan]

Looks good functionally. Does everything it says, and definitely tightens up the
feel of the bookmark manager. We still have issues (e.g., menu separators have
context itmes they shouldn't), but this is a huge improvement. My biggest
functionality issue is that changing the selection to single on context-click
feels wrong to me, but I agree that it's probably the best interim solution to
our lack of support for multi-item operations.

Code comments:

+  // if we're not looking at the top ten list and its parent is a bookmark
folder, but not a smart one

This comment no longer describes the logic of the if statement

+-(int)indexOfActiveCollection;
...
+-(int)indexOfActiveCollection {
+  return [mContainerPane selectedRow];
+}
+

Not used in the revised patch, so no need to add this function anymore

+- (BOOL)bookmarksAreVisible:(BOOL)inRequireSelection
allowMultipleSelection:(BOOL)allowMultipleSelection;

I still hate this, and now I have a solution. We can replace it with two functions:

- (BOOL)bookmarkManagerIsVisible
{
  return [mContentView isBookmarkManagerVisible];
}

- (BOOL)selectionExistsAllowingMultipleSelection:(BOOL)allowMultipleSelection
{
  if (![mContentView isBookmarkManagerVisible]) return NO;

  if (allowMultipleSelection)
    return [mBookmarkViewController haveSelectedRow];
  else
    return ([mBookmarkViewController numberOfSelectedRows] == 1);
}

Then replace the two calls to
  [self bookmarksAreVisible:NO allowMultipleSelection:NO]
with
  [self bookmarkManagerIsVisible]

and the one call to 
  [self bookmarksAreVisible:YES allowMultipleSelection:NO]
with
  [self selectionExistsAllowingMultipleSelection:NO]

It has exactly the same effect, but is far, far clearer.

Comment 23

[Stuart Morgan]

Comment on attachment 150880 [details] [diff] [review]
bm manager cleanup v3.0

r=smorgan with the above changes

Comment 24

[Stuart Morgan]

Comment on attachment 150880 [details] [diff] [review]
bm manager cleanup v3.0

Pinkerton, Josh said he doesn't have time to re-generate the patch for these
changes. Could you make them as you check it in? If not, I'll make a new patch
with these changes (assuming you agree with them).

Don't forget to remove the second item from the CollectionContextMenu in
BrowserWindow.nib when you land this.

Comment 25

[Mike Pinkerton (not reading bugmail)]

landed on trunk&branch

Comment 26

[Stuart Morgan]

Attachment: function renaming

The cleanup/re-naming additional patch, as discussed.

Comment 27

[Josh Aas]

Attachment: updated, more logical function cleanup patch

Comment 28

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 164915 [details] [diff] [review]
updated, more logical function cleanup patch

sr=pink

thanks, i thought that stuff looked awfully weird.

Comment 29

[Josh Aas]

landed