Closed Bug 349979 Opened 14 years ago Closed 14 years ago

[10.3] Items in the Dock are showed thrice

Categories

(Camino Graveyard :: General, defect)

PowerPC
macOS
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Camino1.5

People

(Reporter: suishouen, Assigned: froodian)

Details

(Keywords: fixed1.8.1, regression)

Attachments

(2 files, 4 obsolete files)

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
Attached file Screenshot
This problem started from 2006-08-23-00-1.1-M1.8.
2006-08-21-22-1.1-M1.8 has no problem.
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?
I make a new folder "Extra" and set it as Dock Menu.
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).
Fix my Dock menu you silly therapist! :P

I've got the Top 10 list as my Dock menu, fwiw.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: regression
Summary: Items in the Dock are showed thrice → [10.3] Items in the Dock are showed thrice
Target Milestone: --- → Camino1.1
Attached patch Possible Fix (obsolete) — Splinter Review
This should do the trick.  Smokey, can you take this for a test-drive and see if it fixes the problem?
Assignee: nobody → stridey
Status: NEW → ASSIGNED
Attachment #235349 - Flags: review?(alqahira)
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 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
Attachment #235349 - Flags: review?(alqahira) → review-
Attached patch Patch (obsolete) — Splinter Review
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.
Attachment #235349 - Attachment is obsolete: true
Attachment #235393 - Flags: review?(stuart.morgan)
Comment on attachment 235393 [details] [diff] [review]
Patch

r=ardissone on working right on 10.3.9 :)  Thanks for the quick fix, Ian!
Attachment #235393 - Flags: review+
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.
Attachment #235393 - Flags: review?(stuart.morgan) → review-
Attached patch r=kreeger patch (obsolete) — Splinter Review
Addresses all comments except the
if (([[NSApp currentEvent] modifierFlags] & NSCommandKeyMask) != 0)
line per IRC and bug 333765 comment 37
Attachment #235393 - Attachment is obsolete: true
Attachment #237039 - Flags: superreview?(mikepinkerton)
(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.
Attached patch Addresses smorgan's comments (obsolete) — Splinter Review
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.
Attachment #237039 - Attachment is obsolete: true
Attachment #237076 - Flags: review?(stuart.morgan)
Attachment #237039 - Flags: superreview?(mikepinkerton)
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 (...)
Attachment #237076 - Flags: superreview?(mikepinkerton)
Attachment #237076 - Flags: review?(stuart.morgan)
Attachment #237076 - Flags: review+
Attached patch r=smorgan patchSplinter Review
Attachment #237076 - Attachment is obsolete: true
Attachment #237369 - Flags: superreview?(mikepinkerton)
Attachment #237076 - Flags: superreview?(mikepinkerton)
Comment on attachment 237369 [details] [diff] [review]
r=smorgan patch

sr=pink
Attachment #237369 - Flags: superreview?(mikepinkerton) → superreview+
Checked in on trunk and 1.8branch
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Keywords: fixed1.8.1
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.