Closed Bug 290212 Opened 16 years ago Closed 14 years ago

Implement bookmark folders' "Open in Tabs"/"Open in New Tabs" with alternate menu items

Categories

(Camino Graveyard :: Toolbars & Menus, enhancement)

PowerPC
macOS
enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Camino1.5

People

(Reporter: moz, Assigned: froodian)

References

Details

(Keywords: fixed1.8.1, regression)

Attachments

(1 file, 6 obsolete files)

When support for OS X 10.2 is dropped, we should re-implement certain commands
via alternate menu items (for example, Command-Shift-R (bug 165015)).
Target Milestone: --- → Camino1.1
Isn't this the same as bug 181978, or is that just about opt-using shortcuts?
Will we be able to have "Open in Tabs" display as "Open in New Tabs" when Cmd is held down, or does the alternate menu items stuff only work for menu items that have a shortcut next to them?
They don't need shortcuts, AFAIK.
OK.  The support for this (UI for Command-Shift-R) is in the forthcoming patch for bug 181978.  Turns out it's just a nib change.  The "Open in Tabs"/"Open in New Tabs" is a different issue, however, since we make the bookmark menus programatically.  Therefore, I'm morphing this bug to be about that only.  Sorry if that annoys anybody, but I think it's simpliest in the long run.
Assignee: mikepinkerton → stridey
QA Contact: toolbars
Summary: Re-implement certain hidden commands with alternate menu items when 10.2 support is dropped. → Implement for "Open in Tabs"/"Open in New Tabs" alternate menu items
Attached patch Non-functioning Patch (obsolete) — Splinter Review
That said, I tried to impliment this and failed.  If NSCommandKeyMask is anything else (NSAlternateKeyMask, NSShiftKeyMask, whatever), it works fine, but the alternate never shows up when it's command.  

Wevah, do you have any idea why that might be?
Sorry, I should have mentioned that this makes a string change:

"Open in New Tabs", "Open in New Tabs"
Attached patch Patch (obsolete) — Splinter Review
I figured this out.  It's because menu items get a keyEquivalentModifierMask of  NSCommandKeyMask by default, so it wasn't seeing the alternate as a real alternate.  The solution is a bit... erm, hackish, but I don't think there are any negative side-effects.
Attachment #226172 - Attachment is obsolete: true
Attachment #226225 - Flags: review?(mozilla)
Attached patch Patch (obsolete) — Splinter Review
That patch was for a totally different bug.  Sorry about that, must've just picked the upload file incorrectly.  Anyway, this manually sets the first menu item's key equivalent modifier to 0.
Attachment #226225 - Attachment is obsolete: true
Attachment #226931 - Flags: review?(mozilla)
Attachment #226225 - Flags: review?(mozilla)
Whiteboard: [New strings in comment 6]
Attachment #226931 - Flags: review?(mozilla) → review?(stuart.morgan)
Summary: Implement for "Open in Tabs"/"Open in New Tabs" alternate menu items → Implement bookmark folders' "Open in Tabs"/"Open in New Tabs" with alternate menu items
Comment on attachment 226931 [details] [diff] [review]
Patch

Two problems here:

First, the display changes live as I press and release Command, but the behavior is determined by what it was when the menu was shown (which perhaps means this bug is blocked by bug 317214; I haven't been following all the details).

Second, the menu says "open in new tabs", but the behavior if the 'Command == new window' pref is set is to open the tab set in a new window.  In the bookmark bar, "open in new tabs" does what I would expect, and "open tabs in new window" does what this menu item does. Either the menu item behavior needs to change, or the alternate text needs to be keyed off the pref (probably the latter).
Attachment #226931 - Flags: review?(stuart.morgan) → review-
Depending on how we implement bug 317214 it may or may not block this bug.  That is, if we fix it one way (using alternates) we get the fix for this for free, and otherwise we'll have to put in a check for the sender's |keyEquivalentModifierMask| somewhere along the line.

So I'll wait to see which solution we use, at which point I'll also fix the text to be keyed off the pref.
Depends on: 317214
sorry, wrong bug.  this is actually dep on bug 333765.
Depends on: 333765
No longer depends on: 317214
Attached patch Keys text off pref (obsolete) — Splinter Review
We ended up getting your first issue for free.  This fixes the second.
Attachment #226931 - Attachment is obsolete: true
Attachment #234927 - Flags: review?(stuart.morgan)
Whiteboard: [New strings in comment 6]
Right now (post-bug 333765) holding down Cmd doesn't work at all (not before opening the menu, not when selecting the item), so we always overwrite instead of append.
Keywords: regression
Comment on attachment 234927 [details] [diff] [review]
Keys text off pref

+    NSMenuItem* menuItem = [[[NSMenuItem alloc] initWithTitle:NSLocalizedString(@"Open in Tabs", nil)
                                                        action:nil
+                                                keyEquivalent:@""] autorelease];;

Since BookmarkMenu is a subclass of NSMenu, we don't really need to autorelease these NSMenuItem's here. 
Just make sure you release the NSMenuItem's after you call |addItem:|.

+    [menuItem setKeyEquivalentModifierMask:0]; //Needed since by default NSMenuItems have NSCommandKeyMask

Is there a constant defined for |0| modifier mask? 

This code looks pretty good. r=me with these comments. (Not trying to step on your toes here smorgan, but I figured I would get the queue moving)
Attachment #234927 - Flags: review+
Attached patch r=kreeger patch (obsolete) — Splinter Review
Addresses kreeger's comments.  Unfortunately, there's no constant for a modifier mask of |0|.
Attachment #234927 - Attachment is obsolete: true
Attachment #234927 - Flags: review?(stuart.morgan)
Depends on: 342780
Comment on attachment 237042 [details] [diff] [review]
r=kreeger patch

This both autoreleases and releases menuItem, but only retains it once; it will crash.
Attachment #237042 - Flags: review-
Attached patch A Patch Not On Crack (obsolete) — Splinter Review
Oof.  Sorry, that was sloppy.
Attachment #237042 - Attachment is obsolete: true
Attachment #237073 - Flags: review?(stuart.morgan)
Comment on attachment 237073 [details] [diff] [review]
A Patch Not On Crack

Code-wise it's fine now, but it has a behavior issue: once a given folder's menu has been shown, its alternate never changes even if the pref does. Rebuilding the entire bookmark menu and all submenus when the pref changes doesn't seem like a good option though.
Attachment #237073 - Flags: review?(stuart.morgan) → review-
I'd try setting the menu items' titles appropriately in |-validateMenuItem:|. That way, it (IIRC) shouldn't change unless the menu item is shown.
(In reply to comment #19)
> I'd try setting the menu items' titles appropriately in |-validateMenuItem:|.
> That way, it (IIRC) shouldn't change unless the menu item is shown.

IIRC, bookmark menus don't auto-update, so validate won't be called for them.
-validateMenuItem: is called for each item on the object that handles the given item's action whenever you pull down the menu (I use this for updating titles/states quite often in my own stuff).
This is the last bit of the GCEKM regression that is not fixed, and it's annoying as all get out since it works everywhere else. :/
Flags: camino1.1a1?
Per discussion at this morning's meeting, froodian needs to whip up a patch that fixes the cmd-holding-before-opening regression, and we'll fix the text-syncing-with-prefs after the regression fix has landed.
Comment on attachment 237073 [details] [diff] [review]
A Patch Not On Crack

No whipping necessary, this patch should do the trick.  Stuart, can you review this with everything but the title-changes in mind?
Attachment #237073 - Flags: review- → review?(stuart.morgan)
Comment on attachment 237073 [details] [diff] [review]
A Patch Not On Crack

r=me.  Style stuff:

>+    NSMenuItem* menuItem = [[NSMenuItem alloc] initWithTitle:NSLocalizedString(@"Open in Tabs", nil)
>                                                        action:nil
>+                                                keyEquivalent:@""];
>+    NSMenuItem* altMenuItem;
>+    if ([[PreferenceManager sharedInstance] getBooleanPref:"browser.tabs.opentabfor.middleclick" withSuccess:NULL])
>+      altMenuItem = [[NSMenuItem alloc] initWithTitle:NSLocalizedString(@"Open in New Tabs", nil)
>+                                                action:nil
>+                                         keyEquivalent:@""];
>+    else
>+      altMenuItem = [[NSMenuItem alloc] initWithTitle:NSLocalizedString(@"Open in Tabs in New Window", nil)
>+                                                action:nil
>+                                         keyEquivalent:@""];

Fix the |:| alignment here.

>     [menuItem setAction:@selector(openMenuBookmark:)];
...
>+    [altMenuItem setAction:@selector(openMenuBookmark:)];

Set these in the init call, rather than passing nil there and setting it here.
Attachment #237073 - Flags: superreview?(mikepinkerton)
Attachment #237073 - Flags: review?(stuart.morgan)
Attachment #237073 - Flags: review+
Attached patch r=smorgan patchSplinter Review
Attachment #237073 - Attachment is obsolete: true
Attachment #241797 - Flags: superreview?(mikepinkerton)
Attachment #237073 - Flags: superreview?(mikepinkerton)
Comment on attachment 241797 [details] [diff] [review]
r=smorgan patch

sr=pink
Attachment #241797 - Flags: superreview?(mikepinkerton) → superreview+
Checked in on 1.8branch and trunk
Status: NEW → RESOLVED
Closed: 14 years ago
Keywords: fixed1.8.1
Resolution: --- → FIXED
Marking 1.1a1+, since this landed.
Flags: camino1.1a1? → camino1.1a1+
You need to log in before you can comment on or make changes to this bug.