325939 - Cmd-click in bookmarks toolbar does not respect standard shift toggle

Status: RESOLVED FIXED

(Camino Graveyard :: Toolbars & Menus, defect)

Description

[Chris Lawson (gone)]

The shift key is our standard foreground/background toggle, but we forgot to hook it up for cmd-clicking on bookmarks in the Bookmarks toolbar. Original fix was bug 228478.

Comment 1

[Stuart Morgan]

I brought this up in bug 228840, and there's a fix for it in the patch there.  Since that patch needs to be reworked anyway, and the fix for this is an easily separable part (just snag the MainController.mm change) it might be worth separating that out to a new patch here and landing it sooner.

Comment 2

[Chris Lawson (gone)]

Attachment: lets shift key toggle pref

Totally untested but looks likely to work ;)

(OK, Håkan tested it in the other bug...)

cl

Comment 3

[Stuart Morgan]

Comment on attachment 210737 [details] [diff] [review]
lets shift key toggle pref

I also tested it in the other bug, and it works.

Comment 4

[Samuel Sidler (old account; do not CC)]

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

Comment 5

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 210737 [details] [diff] [review]
lets shift key toggle pref

sr=pink

Comment 6

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

Is this 1.0 material?

Setting to 1.1 and adding [patch] so we know there's an r/sr'd patch in the event it's not 1.0 stuff.

Comment 7

[Mark Mentovai]

Comment on attachment 210737 [details] [diff] [review]
lets shift key toggle pref

GetCurrentKeyModifiers() sucks, because it's not event-synchronized.  GetCurrentEventKeyModifiers() is better.  See http://developer.apple.com/documentation/Carbon/Conceptual/Carbon_Event_Manager/Tasks/chapter_3_section_7.html .

This is Cocoa, not Carbon, so [[NSApp currentEvent] modifiers] is the closest analogue to GetCurrentEventKeyModifiers().

Comment 8

[Håkan Waara]

Yes, please do not use the carbon event stuff - that only introduces new bugs (see bug 317214 where I'm fixing a few of them).

In fact, if we still have uses left of that in the tree - we should convert them to [[NSApp currentEvent] modifiers]. 

Comment 9

[Chris Lawson (gone)]

(In reply to comment #8)
> Yes, please do not use the carbon event stuff - that only introduces new bugs
> (see bug 317214 where I'm fixing a few of them).
> 
> In fact, if we still have uses left of that in the tree - we should convert
> them to [[NSApp currentEvent] modifiers]. 

Bug 320746.

cl

Comment 10

[Chris Lawson (gone)]

Attachment: fixed to be more Cocoa-friendly

Here's a more Cocoa-friendly patch.

Someone should probably test this before it gets checked in ;)

cl

Comment 11

[Chris Lawson (gone)]

Attachment: patch that actually works

Oops. The last one was broken. This one actually works.

cl

Comment 12

[Chris Lawson (gone)]

Comment on attachment 210737 [details] [diff] [review]
lets shift key toggle pref

Clearing + flags on obsolete attachment.

Comment 13

[Mark Mentovai]

Comment on attachment 216487 [details] [diff] [review]
patch that actually works

>+  if (([[NSApp currentEvent] modifierFlags] & NSShiftKeyMask) != 0)

|!= 0| is redundant and it harms readability.  I'll remove it on checkin.

Comment 14

[Mark Mentovai]

MainController.mm 1.175 HEAD, 1.142.2.31 MOZILLA_1_8_BRANCH.