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

Status: RESOLVED FIXED

(Camino Graveyard :: Toolbars & Menus, enhancement)

Description

[Wevah (Nate Weaver)]

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

Comment 1

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

Isn't this the same as bug 181978, or is that just about opt-using shortcuts?

Comment 2

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

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?

Comment 3

[Wevah (Nate Weaver)]

They don't need shortcuts, AFAIK.

Comment 4

[froodian (Ian Leue)]

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.

Comment 5

[froodian (Ian Leue)]

Attachment: Non-functioning Patch

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?

Comment 6

[froodian (Ian Leue)]

Sorry, I should have mentioned that this makes a string change:

"Open in New Tabs", "Open in New Tabs"

Comment 7

[froodian (Ian Leue)]

Attachment: Patch

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.

Comment 8

[froodian (Ian Leue)]

Attachment: Patch

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.

Comment 9

[Stuart Morgan]

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

Comment 10

[froodian (Ian Leue)]

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.

Comment 11

[froodian (Ian Leue)]

sorry, wrong bug.  this is actually dep on bug 333765.

Comment 12

[froodian (Ian Leue)]

Attachment: Keys text off pref

We ended up getting your first issue for free.  This fixes the second.

Comment 13

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

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.

Comment 14

[Nick Kreeger]

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)

Comment 15

[froodian (Ian Leue)]

Attachment: r=kreeger patch

Addresses kreeger's comments.  Unfortunately, there's no constant for a modifier mask of |0|.

Comment 16

[Stuart Morgan]

Comment on attachment 237042 [details] [diff] [review]
r=kreeger patch

This both autoreleases and releases menuItem, but only retains it once; it will crash.

Comment 17

[froodian (Ian Leue)]

Attachment: A Patch Not On Crack

Oof.  Sorry, that was sloppy.

Comment 18

[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.

Comment 19

[Wevah (Nate Weaver)]

I'd try setting the menu items' titles appropriately in |-validateMenuItem:|. That way, it (IIRC) shouldn't change unless the menu item is shown.

Comment 20

[Simon Fraser [no longer active]]

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

Comment 21

[Wevah (Nate Weaver)]

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

Comment 22

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

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. :/

Comment 23

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

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 24

[froodian (Ian Leue)]

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?

Comment 25

[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.

Comment 26

[froodian (Ian Leue)]

Attachment: r=smorgan patch

Comment 27

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 241797 [details] [diff] [review]
r=smorgan patch

sr=pink

Comment 28

[froodian (Ian Leue)]

Checked in on 1.8branch and trunk

Comment 29

[froodian (Ian Leue)]

Marking 1.1a1+, since this landed.