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

RESOLVED FIXED in Camino1.5

Status

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

People

(Reporter: Wevah, Assigned: froodian (Ian Leue))

Tracking

({fixed1.8.1, regression})

unspecified
Camino1.5
PowerPC
Mac OS X
fixed1.8.1, regression
Dependency tree / graph
Bug Flags:
camino1.1a1 +

Details

Attachments

(1 attachment, 6 obsolete attachments)

(Reporter)

Description

12 years ago
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)).
(Reporter)

Updated

12 years ago
Target Milestone: --- → Camino1.1
Isn't this the same as bug 181978, or is that just about opt-using shortcuts?
Blocks: 311840
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?
(Reporter)

Comment 3

12 years ago
They don't need shortcuts, AFAIK.
Blocks: 328173
(Assignee)

Comment 4

11 years ago
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
(Assignee)

Comment 5

11 years ago
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?
(Assignee)

Comment 6

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

"Open in New Tabs", "Open in New Tabs"
(Assignee)

Comment 7

11 years ago
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.
Attachment #226172 - Attachment is obsolete: true
Attachment #226225 - Flags: review?(mozilla)
(Assignee)

Comment 8

11 years ago
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.
Attachment #226225 - Attachment is obsolete: true
Attachment #226931 - Flags: review?(mozilla)
Attachment #226225 - Flags: review?(mozilla)
(Assignee)

Updated

11 years ago
Whiteboard: [New strings in comment 6]
(Assignee)

Updated

11 years ago
Attachment #226931 - Flags: review?(mozilla) → review?(stuart.morgan)
(Assignee)

Updated

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

11 years ago
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-
(Assignee)

Comment 10

11 years ago
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
(Assignee)

Comment 11

11 years ago
sorry, wrong bug.  this is actually dep on bug 333765.
Depends on: 333765
No longer depends on: 317214
(Assignee)

Comment 12

11 years ago
Created attachment 234927 [details] [diff] [review]
Keys text off pref

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

Updated

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

11 years ago
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+
(Assignee)

Comment 15

11 years ago
Created attachment 237042 [details] [diff] [review]
r=kreeger patch

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

Updated

11 years ago
Depends on: 342780

Comment 16

11 years ago
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-
(Assignee)

Comment 17

11 years ago
Created attachment 237073 [details] [diff] [review]
A Patch Not On Crack

Oof.  Sorry, that was sloppy.
Attachment #237042 - Attachment is obsolete: true
Attachment #237073 - Flags: review?(stuart.morgan)

Comment 18

11 years ago
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-
(Reporter)

Comment 19

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

11 years ago
(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.
(Reporter)

Comment 21

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

Comment 24

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

11 years ago
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+
(Assignee)

Comment 26

11 years ago
Created attachment 241797 [details] [diff] [review]
r=smorgan patch
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+
(Assignee)

Comment 28

11 years ago
Checked in on 1.8branch and trunk
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Keywords: fixed1.8.1
Resolution: --- → FIXED
(Assignee)

Comment 29

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