223701 - Incorrect Info window displays the wrong data for a empty folder

Status: RESOLVED FIXED

(Camino Graveyard :: Bookmarks, defect)

Description

[Chris Petersen]

Build: 2003102409
Platform: 10.3
Expected Results: The info window should be empty
What I got: The info window uses a title of %bookmark_title% and displays two
info tabs.


Steps to reproduce:

1) Select Manage Bookmarks
2) Control -click in Bookmarks collection pane (far left side) and select New
collection. This should create a new folder.
3) With focus on this new folder, control - click on the bookmarks main pane.
4) Select Get Info from the contextual menu .
5) Notice, the info window that opens is incorrect.

Comment 1

[Chris Petersen]

Attachment: screen shot

Comment 2

[Jasper]

Yep, we shouldn't be able to open info windows for the root folders.

Comment 3

[Josh Aas]

Looks like the problem is in the method here:

line 1544 of BookmarkViewController.mm

Problem is the "Get Info" part is retained in the contextual menu whether or not
the clicked-on view is relevent or not. Shouldn't be there unless a bookmark or
maybe a bookmark folder is actually clicked on.

Comment 4

[Mike Pinkerton (not reading bugmail)]

bookmarkViewController doesn't have 1500 lines ;)

Comment 5

[Stuart Morgan]

FYI, one of my upcoming projects is to pick up bug 225915 and get it ready to
land. That might already fix this bug, so we probably don't need to worry about
this too much yet.

Comment 6

[Josh Aas]

My bad - I meant line 1144 in comment #3.

Comment 7

[Josh Aas]

Stuart - glancing through the patch in bug 225915, it does appear to fix the
problem. Let me know when you're going to get around to cleaning it up. If you
can't do it for a while I'll do it later this week. If you're going to do it
soon I'll review ASAP.

Comment 8

[Josh Aas]

Taking this bug because I plan on fixing it this week and I want it in my list.

Comment 9

[Josh Aas]

Attachment: clean up contextual menu behavior

Comment 10

[Josh Aas]

Attachment: modified nib file, v1

Comment 11

[Mike Pinkerton (not reading bugmail)]

+- (NSMenu *)contextMenuForItem:(id)item fromView:(BookmarkOutlineView
*)outlineView target:(id)target

should probably check for |item| being non-null before going too far and return
something reasonable.

+  if( isFolder || (outlineView && ([outlineView numberOfSelectedRows] > 1)) )

keep consistent spacing, should be:
  if (isFolder || ...)))

happens in several places

+    menuTitle = NSLocalizedString(@"Open Tabs in New Window", @"");
+  else
+    menuTitle = NSLocalizedString(@"Open in New Window", @"");

we should probably add these strings to Localized.strings so that localizers
know they have to update them w/out scanning the code.

+  if ([item isKindOfClass:[BookmarkFolder class]]) {

are toplevel containers and smart folders also |BookmarkFolder|s? just checking

Comment 12

[Josh Aas]

Attachment: contextual menu cleanup #2

This patch addresses Pinkerton's comments and also fixes some issues with the
default menus in smart folders where new folder cannot be created. Use the same
modified nib file as was posted with version 1 of this patch.

"are toplevel containers and smart folders also |BookmarkFolder|s? just
checking"

Yes they are.

Comment 13

[Mike Pinkerton (not reading bugmail)]

+  // don't do anything if item == nil
+  if (!item)
+    return nil;

while i can't think of anything better here, are we sure that the menu code can
handle a nil being returned?

+-(NSMenu*)menu
+{
+  BookmarkManager *bm = [BookmarkManager sharedBookmarkManager];
+  BookmarkFolder *activeCollection = [[self delegate] activeCollection];
+  // only give a default menu if its the bookmark menu or toolbar

what's this for? when does it get called compared to the other code?

Comment 14

[Josh Aas]

The menu code takes a nil return to mean that there is no menu to display.

The reason for "// only give a default menu if its the bookmark menu or toolbar"
is that the default menu consists of an option to make a new folder. The only
collections where this is legal are the bookmark menu and the bookmark toolbar.
This code gets called whenever somebody right-clicks in the righthand bm manager
view.

Comment 15

[Mike Pinkerton (not reading bugmail)]

the one issue that i see is that in the history collection, there's no separator
between open in new tab and delete. everywhere else there is one. is that a flaw
in this patch or something that can be fixed in another patch?

Comment 16

[Josh Aas]

To fix that, just add this line:

[contextMenu addItem:[NSMenuItem separatorItem]];

around line 447 of HistoryDataSource.mm

Comment 17

[Josh Aas]

Also, Localizable.strings needs:

"Get Info" = "Get Info";
"Use as Dock Menu" = "Use as Dock Menu";

appended to the end of the file

Comment 18

[Mike Pinkerton (not reading bugmail)]

landed on trunk and branch

Comment 19

[Stuart Morgan]

Comment on attachment 148149 [details] [diff] [review]
contextual menu cleanup #2

Removing review requests