Last Comment Bug 290212 - Implement bookmark folders' "Open in Tabs"/"Open in New Tabs" with alternate menu items
: Implement bookmark folders' "Open in Tabs"/"Open in New Tabs" with alternate ...
Status: RESOLVED FIXED
: fixed1.8.1, regression
Product: Camino Graveyard
Classification: Graveyard
Component: Toolbars & Menus (show other bugs)
: unspecified
: PowerPC Mac OS X
-- enhancement (vote)
: Camino1.5
Assigned To: froodian (Ian Leue)
:
:
Mentors:
Depends on: 333765 342780
Blocks: 311840 328173
  Show dependency treegraph
 
Reported: 2005-04-13 13:01 PDT by Wevah
Modified: 2006-10-10 09:43 PDT (History)
4 users (show)
froodian: camino1.1a1+
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Non-functioning Patch (2.07 KB, patch)
2006-06-19 10:35 PDT, froodian (Ian Leue)
no flags Details | Diff | Splinter Review
Patch (5.05 KB, patch)
2006-06-19 15:22 PDT, froodian (Ian Leue)
no flags Details | Diff | Splinter Review
Patch (2.37 KB, patch)
2006-06-24 11:13 PDT, froodian (Ian Leue)
stuart.morgan+bugzilla: review-
Details | Diff | Splinter Review
Keys text off pref (2.85 KB, patch)
2006-08-22 06:55 PDT, froodian (Ian Leue)
nick.kreeger: review+
Details | Diff | Splinter Review
r=kreeger patch (2.88 KB, patch)
2006-09-06 18:23 PDT, froodian (Ian Leue)
stuart.morgan+bugzilla: review-
Details | Diff | Splinter Review
A Patch Not On Crack (2.86 KB, patch)
2006-09-06 22:49 PDT, froodian (Ian Leue)
stuart.morgan+bugzilla: review+
Details | Diff | Splinter Review
r=smorgan patch (2.94 KB, patch)
2006-10-09 22:19 PDT, froodian (Ian Leue)
mikepinkerton: superreview+
Details | Diff | Splinter Review

Description User image Wevah 2005-04-13 13:01:24 PDT
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 User image Smokey Ardisson (offline for a while; not following bugs - do not email) 2005-08-26 16:06:09 PDT
Isn't this the same as bug 181978, or is that just about opt-using shortcuts?
Comment 2 User image Smokey Ardisson (offline for a while; not following bugs - do not email) 2005-10-28 04:07:03 PDT
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 User image Wevah 2005-10-28 20:19:13 PDT
They don't need shortcuts, AFAIK.
Comment 4 User image froodian (Ian Leue) 2006-06-19 10:33:04 PDT
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 User image froodian (Ian Leue) 2006-06-19 10:35:12 PDT
Created attachment 226172 [details] [diff] [review]
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 User image froodian (Ian Leue) 2006-06-19 11:02:21 PDT
Sorry, I should have mentioned that this makes a string change:

"Open in New Tabs", "Open in New Tabs"
Comment 7 User image froodian (Ian Leue) 2006-06-19 15:22:15 PDT
Created attachment 226225 [details] [diff] [review]
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 User image froodian (Ian Leue) 2006-06-24 11:13:01 PDT
Created attachment 226931 [details] [diff] [review]
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 User image Stuart Morgan 2006-08-03 18:27:27 PDT
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 User image froodian (Ian Leue) 2006-08-04 22:40:37 PDT
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 User image froodian (Ian Leue) 2006-08-04 22:47:28 PDT
sorry, wrong bug.  this is actually dep on bug 333765.
Comment 12 User image froodian (Ian Leue) 2006-08-22 06:55:07 PDT
Created attachment 234927 [details] [diff] [review]
Keys text off pref

We ended up getting your first issue for free.  This fixes the second.
Comment 13 User image Smokey Ardisson (offline for a while; not following bugs - do not email) 2006-08-24 16:55:37 PDT
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 User image Nick Kreeger 2006-09-06 16:13:45 PDT
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 User image froodian (Ian Leue) 2006-09-06 18:23:49 PDT
Created attachment 237042 [details] [diff] [review]
r=kreeger patch

Addresses kreeger's comments.  Unfortunately, there's no constant for a modifier mask of |0|.
Comment 16 User image Stuart Morgan 2006-09-06 22:36:34 PDT
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 User image froodian (Ian Leue) 2006-09-06 22:49:35 PDT
Created attachment 237073 [details] [diff] [review]
A Patch Not On Crack

Oof.  Sorry, that was sloppy.
Comment 18 User image Stuart Morgan 2006-09-07 07:02:24 PDT
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 User image Wevah 2006-09-07 08:29:40 PDT
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 User image Simon Fraser 2006-09-07 08:41:02 PDT
(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 User image Wevah 2006-09-07 09:11:30 PDT
-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 User image Smokey Ardisson (offline for a while; not following bugs - do not email) 2006-09-25 15:31:37 PDT
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 User image Smokey Ardisson (offline for a while; not following bugs - do not email) 2006-10-04 10:56:13 PDT
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 User image froodian (Ian Leue) 2006-10-04 11:52:07 PDT
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 User image Stuart Morgan 2006-10-09 21:05:59 PDT
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 User image froodian (Ian Leue) 2006-10-09 22:19:17 PDT
Created attachment 241797 [details] [diff] [review]
r=smorgan patch
Comment 27 User image Mike Pinkerton (not reading bugmail) 2006-10-10 06:09:41 PDT
Comment on attachment 241797 [details] [diff] [review]
r=smorgan patch

sr=pink
Comment 28 User image froodian (Ian Leue) 2006-10-10 08:06:43 PDT
Checked in on 1.8branch and trunk
Comment 29 User image froodian (Ian Leue) 2006-10-10 09:43:21 PDT
Marking 1.1a1+, since this landed.

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