349979 - [10.3] Items in the Dock are showed thrice

Status: RESOLVED FIXED

(Camino Graveyard :: General, defect)

Description

[Eiichi]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8.1b2) Gecko/20060823 Camino/1.0+
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8.1b2) Gecko/20060823 Camino/1.0+

Items in the bookmark folder flagged as Dock Menu are showed thrice in the Dock.

Reproducible: Always

Comment 1

[Eiichi]

Attachment: Screenshot

Comment 2

[Eiichi]

This problem started from 2006-08-23-00-1.1-M1.8.
2006-08-21-22-1.1-M1.8 has no problem.

Comment 3

[froodian (Ian Leue)]

I don't see this, even when I set a subfolder of my bookmarks bar as the dock folder.  But I should.  It's so my fault it's not funny.  bug 333765.  Not sure exactly how it's showing up though.

What folder do you have set as your dock menu?

Comment 4

[Eiichi]

I make a new folder "Extra" and set it as Dock Menu.

Comment 5

[Eiichi]

I try to make a fresh profile and use a default bookmark, but same deal.
For reference, I'm using Mac OS X Version 10.3.9 (Build 7W98).

Comment 6

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Fix my Dock menu you silly therapist! :P

I've got the Top 10 list as my Dock menu, fwiw.

Comment 7

[froodian (Ian Leue)]

Attachment: Possible Fix

This should do the trick.  Smokey, can you take this for a test-drive and see if it fixes the problem?

Comment 8

[Stuart Morgan]

Ideally the fix would be to change the way the dock menu is set, as opposed to making one bookmark folder behave differently than all the others throughout the UI.  (At the very least, if the fix goes this route it needs to be conditionalized for the affected OS versions)

Comment 9

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Comment on attachment 235349 [details] [diff] [review]
Possible Fix

This works, kind-of.

1) I had to change what was selected as my Dock menu for it to take effect

2a) If the Dock menu is set to the Top 10 list, the fix does not persist through restarts

2b) If the Dock menu is set to a folder that appears in the Bookmarks Menu or the Bookmark Bar, alternates don't work on the items in the folder in the bar/menu

Comment 10

[froodian (Ian Leue)]

Attachment: Patch

Well, this should do the trick.  I don't like how ugly it's all gotten, especially since we'll want to just rip it out once we're 10.4+ but I guess that's how it is.

Comment 11

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Comment on attachment 235393 [details] [diff] [review]
Patch

r=ardissone on working right on 10.3.9 :)  Thanks for the quick fix, Ian!

Comment 12

[Nick Kreeger]

Comment on attachment 235393 [details] [diff] [review]
Patch

+- (void)appendBookmarkItem:(BookmarkItem *)inItem buildingSubmenus:(BOOL)buildSubmenus withAlternates:(BOOL)withAlternates;
+  [self rebuildMenuIncludingSubmenus:NO andAlternates:YES];

Either stick with using "withX" or "andX". I would prefer "with" on both of them.

+      menuItem = [[NSMenuItem alloc] initWithTitle:title action: NULL keyEquivalent: @""];
+        NSMenuItem *cmdMenuItem      = [[NSMenuItem alloc] initWithTitle:title action:@selector(openMenuBookmark:) keyEquivalent: @""];
+        NSMenuItem *cmdShiftMenuItem = [[NSMenuItem alloc] initWithTitle:title action:@selector(openMenuBookmark:) keyEquivalent: @""];

There are a couple of occurances where you have a space between the call and the in-param. Please remove those.

+        [cmdShiftMenuItem setKeyEquivalentModifierMask:NSCommandKeyMask | NSShiftKeyMask];

More of a nit here, but try to wrap your mask operations in parens.

+  // This check is needed because 10.3 can't handle alternates in dock menus.  Remove it (and the |andAlternates| params
+  // to deal with it) once we're 10.4+
+  long version = 0;
+  ::Gestalt(gestaltSystemVersion, &version);
+  BOOL postPanther = (version >= 0x00001040);

Rather than copying this code around we have a utility method in NSWorkspace+Utils for this: |supportsSpotlight:| and/or |supportsUnifiedToolbar:|.

+  else { // safeguard for bookmark menus that don't have alternates yet
+    if (([[NSApp currentEvent] modifierFlags] & NSCommandKeyMask) != 0)
+      openBehavior = eBookmarkOpenBehavior_NewPreferred;
+  }

Two thing here:
1. The if statement can be written as:
	if (!([[NSApp currentEvent] modifierFlags] & NSCommandKeyMask))

2. This one is a nit, but please put your // comment above the else {.

Other than that the code looks OK, and a quick test seems OK as well.

r=me with these changes.

Comment 13

[froodian (Ian Leue)]

Attachment: r=kreeger patch

Addresses all comments except the
if (([[NSApp currentEvent] modifierFlags] & NSCommandKeyMask) != 0)
line per IRC and bug 333765 comment 37

Comment 14

[Stuart Morgan]

(In reply to comment #13)
> Addresses all comments except the
> if (([[NSApp currentEvent] modifierFlags] & NSCommandKeyMask) != 0)
> line per IRC and bug 333765 comment 37

That discussion isn't relevant to this line, since there's no conversion to a BOOL happening.  |if (foo)| and |if (foo != 0)| are equivalent, and the former is the preferred mozilla style.

>+  BOOL tigerOrLater = [NSWorkspace supportsSpotlight];

This is really ugly, as what you are doing has nothing to do with Spotlight. Either make a new isTigerOrHigher wrapper specific to your needs, or make isTigerOrHigher public and use that.

Comment 15

[froodian (Ian Leue)]

Attachment: Addresses smorgan's comments

Sorry for my misunderstanding of the =0 issue (I get it now.  It's the act of forcing it into BOOL that causes the potential danger).  I went for the "make |isTigerOrHigher| public" approach.

Comment 16

[Stuart Morgan]

Comment on attachment 237076 [details] [diff] [review]
Addresses smorgan's comments

r=me (on the code; testing it wouldn't accomplish much here)

One nit:

>+  else {
>+    if ([[NSApp currentEvent] modifierFlags] & NSCommandKeyMask)
>+      openBehavior = eBookmarkOpenBehavior_NewPreferred;
>+  }

else if (...)

Comment 17

[froodian (Ian Leue)]

Attachment: r=smorgan patch

Comment 18

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 237369 [details] [diff] [review]
r=smorgan patch

sr=pink

Comment 19

[froodian (Ian Leue)]

Checked in on trunk and 1.8branch