331670 - "Search" CM item needs to respect new & background keys/toggles

Status: RESOLVED FIXED

(Camino Graveyard :: Toolbars & Menus, defect)

Description

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

From bug 327470: 

------- Comment #2 From Chris Lawson  2006-02-18 10:12 PST  [reply] -------

As with other links/searches, holding cmd while selecting this menu item should
kick the search to a new tab/window, too.

Comment 1

[Chris Lawson (gone)]

Taking, patch coming.

Comment 2

[Chris Lawson (gone)]

Attachment: uses our standard cmd/shift key toggles

This patch has the same problem as all the other [[NSApp currentEvent] modifierFlags] calls, i.e. it won't work if you hit the modifiers after the menu is triggered.

It'll be easier to fix later on if we have one thing to look for to fix, though, so I'm submitting this patch with that caveat.

cl

Comment 3

[Nick Kreeger]

Comment on attachment 221997 [details] [diff] [review]
uses our standard cmd/shift key toggles

First, put your curly braces on a new line, makes things much easier to read.

+    if (modifiers & NSShiftKeyMask)
+      loadInBG = !loadInBG; // shift key should toggle the foreground/background pref as it does elsewhere

Put this comment above the if statement.

Other than those style things, looks clean to me.

Comment 4

[Chris Lawson (gone)]

Attachment: fixes comment style

As mentioned in other bugs, smfr and pink can fight out the bracing style issues in sr. Fixed the location of the comment, though.

cl

Comment 5

[Stuart Morgan]

Comment on attachment 222004 [details] [diff] [review]
fixes comment style

Again, needs a shared implementation (see bug 337958).  And that will make comment 2 even easier to address later.

Comment 6

[Chris Lawson (gone)]

Attachment: uses shared implementation from bug 337958

Fixed. Needs bug 337958 to land first to be functional.

cl

Comment 7

[Chris Lawson (gone)]

Attachment: updated for new method name

Obsoleted old patch due to change in method name. This one fixes it.

Comment 8

[froodian (Ian Leue)]

Comment on attachment 242370 [details] [diff] [review]
updated for new method name

Looks good, except that for the search menu item, current even modifier flags won't do the trick.

So you get to do my favorite thing.  Use alternates!

A cursory glance seems to indicate that you should just give the search context menu item kItemsNeedingOpenBehaviorAlternatesTag (in the nib).  Since it already has a tag, we might have to convert the tags to be binary, and use them bitwise.

Then, in |searchForSelection| if [aSender isAlternate] check [aSender keyEquivalentModifierMask], and do what you're doing now if not.

Comment 9

[froodian (Ian Leue)]

Comment on attachment 242370 [details] [diff] [review]
updated for new method name

Looking at this further, adding an alternate the Right Way for the context menu item will be a little bit tricky, since it requires that menu items can have two tags at once, or hacking up a solution where we have bitwise tag masks (or something).  It's a big enough project for a fringe enough case that I think it should be covered in a followup bug.

r=me

Comment 10

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 242370 [details] [diff] [review]
updated for new method name

sr=pink

Comment 11

[froodian (Ian Leue)]

Checked in on 1.8branch and trunk.  The followup is bug 359694.