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

RESOLVED FIXED in Camino1.5

Status

Camino Graveyard
Toolbars & Menus
RESOLVED FIXED
12 years ago
11 years ago

People

(Reporter: Smokey Ardisson (offline for a while; not following bugs - do not email), Assigned: Chris Lawson (gone))

Tracking

({fixed1.8.1.1})

unspecified
Camino1.5
PowerPC
Mac OS X
fixed1.8.1.1

Details

Attachments

(1 attachment, 3 obsolete attachments)

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.
(Assignee)

Comment 1

11 years ago
Taking, patch coming.
Assignee: mikepinkerton → bugzilla
(Assignee)

Comment 2

11 years ago
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
Attachment #221997 - Flags: review?(stuart.morgan)

Comment 3

11 years ago
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.
Attachment #221997 - Flags: review?(stuart.morgan) → review-
(Assignee)

Comment 4

11 years ago
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
Attachment #221997 - Attachment is obsolete: true
Attachment #222004 - Flags: review?(stuart.morgan)

Comment 5

11 years ago
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.
Attachment #222004 - Flags: review?(stuart.morgan) → review-
(Assignee)

Comment 6

11 years ago
Created attachment 223523 [details] [diff] [review]
uses shared implementation from bug 337958

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

cl
Attachment #222004 - Attachment is obsolete: true
Attachment #223523 - Flags: review?(stuart.morgan)
(Assignee)

Updated

11 years ago
Depends on: 337958
(Assignee)

Comment 7

11 years ago
Created attachment 242370 [details] [diff] [review]
updated for new method name

Obsoleted old patch due to change in method name. This one fixes it.
Attachment #223523 - Attachment is obsolete: true
Attachment #242370 - Flags: review?(stridey)
Attachment #223523 - Flags: review?(stuart.morgan)

Comment 8

11 years ago
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

11 years ago
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
Attachment #242370 - Flags: superreview?(mikepinkerton)
Attachment #242370 - Flags: review?(stridey)
Attachment #242370 - Flags: review+
Comment on attachment 242370 [details] [diff] [review]
updated for new method name

sr=pink
Attachment #242370 - Flags: superreview?(mikepinkerton) → superreview+

Comment 11

11 years ago
Checked in on 1.8branch and trunk.  The followup is bug 359694.
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Keywords: fixed1.8.1.1
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.