Last Comment Bug 331670 - "Search" CM item needs to respect new & background keys/toggles
: "Search" CM item needs to respect new & background keys/toggles
Status: RESOLVED FIXED
: fixed1.8.1.1
Product: Camino Graveyard
Classification: Graveyard
Component: Toolbars & Menus (show other bugs)
: unspecified
: PowerPC Mac OS X
-- normal (vote)
: Camino1.5
Assigned To: Chris Lawson (gone)
:
:
Mentors:
Depends on: 337958
Blocks:
  Show dependency treegraph
 
Reported: 2006-03-24 22:57 PST by Smokey Ardisson (offline for a while; not following bugs - do not email)
Modified: 2006-11-06 09:41 PST (History)
2 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
uses our standard cmd/shift key toggles (1.86 KB, patch)
2006-05-14 19:24 PDT, Chris Lawson (gone)
nick.kreeger: review-
Details | Diff | Splinter Review
fixes comment style (1.86 KB, patch)
2006-05-14 22:04 PDT, Chris Lawson (gone)
stuart.morgan+bugzilla: review-
Details | Diff | Splinter Review
uses shared implementation from bug 337958 (1.63 KB, patch)
2006-05-26 20:33 PDT, Chris Lawson (gone)
no flags Details | Diff | Splinter Review
updated for new method name (1.63 KB, patch)
2006-10-15 20:06 PDT, Chris Lawson (gone)
froodian: review+
mikepinkerton: superreview+
Details | Diff | Splinter Review

Description User image Smokey Ardisson (offline for a while; not following bugs - do not email) 2006-03-24 22:57:02 PST
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 User image Chris Lawson (gone) 2006-05-14 19:13:29 PDT
Taking, patch coming.
Comment 2 User image Chris Lawson (gone) 2006-05-14 19:24:48 PDT
Created attachment 221997 [details] [diff] [review]
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 User image Nick Kreeger 2006-05-14 21:08:15 PDT
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 User image Chris Lawson (gone) 2006-05-14 22:04:27 PDT
Created attachment 222004 [details] [diff] [review]
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 User image Stuart Morgan 2006-05-16 22:23:06 PDT
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 User image Chris Lawson (gone) 2006-05-26 20:33:28 PDT
Created attachment 223523 [details] [diff] [review]
uses shared implementation from bug 337958

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

cl
Comment 7 User image Chris Lawson (gone) 2006-10-15 20:06:39 PDT
Created attachment 242370 [details] [diff] [review]
updated for new method name

Obsoleted old patch due to change in method name. This one fixes it.
Comment 8 User image froodian (Ian Leue) 2006-10-27 18:20:08 PDT
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 User image froodian (Ian Leue) 2006-10-27 19:38:18 PDT
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 User image Mike Pinkerton (not reading bugmail) 2006-11-06 06:47:37 PST
Comment on attachment 242370 [details] [diff] [review]
updated for new method name

sr=pink
Comment 11 User image froodian (Ian Leue) 2006-11-06 09:41:06 PST
Checked in on 1.8branch and trunk.  The followup is bug 359694.

Note You need to log in before you can comment on or make changes to this bug.