342780 - Cmd and Shift modifiers for "View Image" don't work when pushing command after opening context menu

Status: VERIFIED FIXED

(Camino Graveyard :: Toolbars & Menus, defect)

Description

[froodian (Ian Leue)]

This is for the same reason as bug 333765.  What needs to happen is to implement alternate menu items for "View Image in new foo" and for "view image in new foo in background" (though the name for the second one probably shouldn't actually change).

The problem is that IB doesn't allow alternate menu items with only a shift modifier, so the menu needs to be ported to code to make it work.

bug 186591 introduced this (just for code reference).

Comment 1

[froodian (Ian Leue)]

Attachment: Patch

This is a big patch, but it doesn't really do that much; mostly just ports the nib.

STRING CHANGES:
-The following strings used to appear in BrowserWindow.nib, but now appear in localizable.strings:
"View Image"
"Copy Image"
"Save Image..." <- Be sure to give the english version of this a real ellipsis
"Open Frame In New Window"
"Open Frame In New Tab"
-The following are new strings:
"View Image in New Tab"
"View Image In New Window"

Comment 2

[froodian (Ian Leue)]

Attachment: new BrowserWindow.nib

This just deletes the image context menu from the nib, since it's not used anymore.

Comment 3

[froodian (Ian Leue)]

Hrm... should have been more careful typing those localizable strings.  All instances of "In" should be lowercase ("in"), per standard english grammar.

Comment 4

[froodian (Ian Leue)]

Attachment: Uses correct capitalization of strings

Comment 5

[froodian (Ian Leue)]

Attachment: New BrowserWindow.nib

This is the bitrot police.

Comment 6

[Nick Kreeger]

Comment on attachment 227173 [details] [diff] [review]
Uses correct capitalization of strings

 enum BWCOpenDest {
   kDestinationNewWindow = 0,
   kDestinationNewTab,
   kDestinationCurrentView
 };
 
+enum BWCContextTags {
+  kFrameRelatedItemsTag = 100,
+  kFrameInapplicableItemsTag = 101,
+  kSelectionRelatedItemsTag = 102
+};
+

Granted you were just following the pattern above, but the appropriate exension for a enum is: "eFrame...". Also if you could declare those enums the same way as seen below:

typedef enum
{
  eAppendTabs,
  eReplaceTabs,
  eReplaceFromCurrentTab
	  
} ETabOpenPolicy;

-  IBOutlet NSMenu*              mImageMenu;
++ (NSMenu*)imageContextMenu:(id)aSender;
++ (NSMenu*)imageContextMenu:(id)aSender
+{
+  NSMenu *mImageMenu   = [[[NSMenu alloc] initWithTitle:@"notitle"] autorelease];
+  NSMenuItem *menuItem = [[NSMenuItem alloc] initWithTitle:NSLocalizedString(@"View Image", @"")
+                                                    action:@selector(viewOnlyThisImage:)
+                                             keyEquivalent:@""];
+  [menuItem setTarget:aSender];
+  [menuItem setKeyEquivalentModifierMask:0]; //Needed since by default NSMenuItems have NSCommandKeyMask
+  [mImageMenu addItem:menuItem];
+
+  menuItem = [[NSMenuItem alloc] initWithTitle:NSLocalizedString(@"", @"")
+                                        action:@selector(viewOnlyThisImage:)
+                                 keyEquivalent:@""];
+  if ([[PreferenceManager sharedInstance] getBooleanPref:"browser.tabs.opentabfor.middleclick" withSuccess:NULL])
+    [menuItem setTitle:NSLocalizedString(@"View Image in New Tab", @"")];
+  else
+    [menuItem setTitle:NSLocalizedString(@"View Image in New Window", @"")];
+  [menuItem setTarget:aSender];
+  [menuItem setKeyEquivalentModifierMask:NSCommandKeyMask];
+  [menuItem setAlternate:YES];
+  [mImageMenu addItem:menuItem];
+
+  menuItem = [[NSMenuItem alloc] initWithTitle:NSLocalizedString(@"", @"")
+                                        action:@selector(viewOnlyThisImage:)
+                                 keyEquivalent:@""];
+  if ([[PreferenceManager sharedInstance] getBooleanPref:"browser.tabs.opentabfor.middleclick" withSuccess:NULL])
+    [menuItem setTitle:NSLocalizedString(@"View Image in New Tab", @"")];
+  else
+    [menuItem setTitle:NSLocalizedString(@"View Image in New Window", @"")];
+  [menuItem setTarget:aSender];
+  [menuItem setKeyEquivalentModifierMask:NSCommandKeyMask | NSShiftKeyMask];
+  [menuItem setAlternate:YES];
+  [mImageMenu addItem:menuItem];
+
+  menuItem = [[NSMenuItem alloc] initWithTitle:NSLocalizedString(@"Copy Image", @"")
+                                        action:@selector(copyImage:)
+                                 keyEquivalent:@""];
+  [menuItem setTarget:aSender];
+  [mImageMenu addItem:menuItem];
+
+  menuItem = [[NSMenuItem alloc] initWithTitle:NSLocalizedString(@"Copy Image Location", @"")
+                                        action:@selector(copyImageLocation:)
+                                 keyEquivalent:@""];
+  [menuItem setTarget:aSender];
+  [mImageMenu addItem:menuItem];
+
+  menuItem = [[NSMenuItem alloc] initWithTitle:NSLocalizedString(@"Save Image...", @"")
+                                        action:@selector(saveImageAs:)
+                                 keyEquivalent:@""];
+  [menuItem setTarget:aSender];
+  [mImageMenu addItem:menuItem];
+
+  menuItem = [NSMenuItem separatorItem];
+  [menuItem setTag:kFrameRelatedItemsTag];
+  [mImageMenu addItem:menuItem];
+
+  menuItem = [[NSMenuItem alloc] initWithTitle:NSLocalizedString(@"Open Frame in New Window", @"")
+                                        action:@selector(frameToNewWindow:)
+                                 keyEquivalent:@""];
+  [menuItem setTarget:aSender];
+  [menuItem setTag:kFrameRelatedItemsTag];
+  [mImageMenu addItem:menuItem];
+
+  menuItem = [[NSMenuItem alloc] initWithTitle:NSLocalizedString(@"Open Frame in New Tab", @"")
+                                        action:@selector(frameToNewTab:)
+                                 keyEquivalent:@""];
+  [menuItem setTarget:aSender];
+  [menuItem setTag:kFrameRelatedItemsTag];
+  [mImageMenu addItem:menuItem];
+
+  return mImageMenu;
+}
+


I don't like this bit here. Not just because its static for for no reason at all (The only caller is BWC and it is defined there?). But you don't actually do any dynamic building of the menu in the method. It doesn't make sense to have to programatically create a new menu each time when it could be done once in a nib. Third, you leak 8 times by not releasing your menu item that you allocate. Also don't forget that the localizable strings function call should pass nil as the second argument rather than a blank string.


-    unsigned int modifiers = [[NSApp currentEvent] modifierFlags];
-
-    if (modifiers & NSCommandKeyMask) {
+    if ([aSender keyEquivalentModifierMask] & NSCommandKeyMask) {
       BOOL loadInTab = [[PreferenceManager sharedInstance] getBooleanPref:"browser.tabs.opentabfor.middleclick" withSuccess:NULL];
       BOOL loadInBG  = [[PreferenceManager sharedInstance] getBooleanPref:"browser.tabs.loadInBackground" withSuccess:NULL];
-      if (modifiers & NSShiftKeyMask)
+      if ([aSender keyEquivalentModifierMask] & NSShiftKeyMask)

Why are you changing this around?

Comment 7

[froodian (Ian Leue)]

Attachment: Patch

(In reply to comment #6)
> (From update of attachment 227173 [details] [diff] [review] [edit])
>  enum BWCOpenDest {
>    kDestinationNewWindow = 0,
>    kDestinationNewTab,
>    kDestinationCurrentView
>  };
> 
> +enum BWCContextTags {
> +  kFrameRelatedItemsTag = 100,
> +  kFrameInapplicableItemsTag = 101,
> +  kSelectionRelatedItemsTag = 102
> +};
> +
> 
> Granted you were just following the pattern above, but the appropriate exension
> for a enum is: "eFrame...".

Somewwhere in the time between when I wrote that patch and when it was reviewed, those were changed (by pink, since it's spellcheck) to constants, so I'm leaving them like that for now.

> I don't like this bit here. Not just because its static for for no reason at
> all (The only caller is BWC and it is defined there?). But you don't actually
> do any dynamic building of the menu in the method. It doesn't make sense to
> have to programatically create a new menu each time when it could be done once
> in a nib. Third, you leak 8 times by not releasing your menu item that you
> allocate. Also don't forget that the localizable strings function call should
> pass nil as the second argument rather than a blank string.

We need to do this programmatically, since IB won't let you use shift as a modifier for alternates.  Also, the "tab/window" is determined dynamically.  All other comments addressed.

> -    unsigned int modifiers = [[NSApp currentEvent] modifierFlags];
> -
> -    if (modifiers & NSCommandKeyMask) {
> +    if ([aSender keyEquivalentModifierMask] & NSCommandKeyMask) {
>        BOOL loadInTab = [[PreferenceManager sharedInstance]
> getBooleanPref:"browser.tabs.opentabfor.middleclick" withSuccess:NULL];
>        BOOL loadInBG  = [[PreferenceManager sharedInstance]
> getBooleanPref:"browser.tabs.loadInBackground" withSuccess:NULL];
> -      if (modifiers & NSShiftKeyMask)
> +      if ([aSender keyEquivalentModifierMask] & NSShiftKeyMask)
> 
> Why are you changing this around?
> 

Because that's how we fix the bug.  [[NSApp currentEvent] modifierFlags] is what's causing the problem.  Using alternates (and checking the menu item's keyEquivalentModifierMask) is the fix.

Comment 8

[Nick Kreeger]

(In reply to comment #7)
> Created an attachment (id=233916) [edit]
> Patch
> 
> (In reply to comment #6)
> > (From update of attachment 227173 [details] [diff] [review] [edit] [edit])
> >  enum BWCOpenDest {
> >    kDestinationNewWindow = 0,
> >    kDestinationNewTab,
> >    kDestinationCurrentView
> >  };
> > 
> > +enum BWCContextTags {
> > +  kFrameRelatedItemsTag = 100,
> > +  kFrameInapplicableItemsTag = 101,
> > +  kSelectionRelatedItemsTag = 102
> > +};
> > +
> > 
> > Granted you were just following the pattern above, but the appropriate exension
> > for a enum is: "eFrame...".
> 
> Somewwhere in the time between when I wrote that patch and when it was
> reviewed, those were changed (by pink, since it's spellcheck) to constants, so
> I'm leaving them like that for now.
> 

The correct prefix for an enum is 'e' not 'k'.

Comment 9

[froodian (Ian Leue)]

(In reply to comment #8)
>
> The correct prefix for an enum is 'e' not 'k'.
> 

That's all fine and dandy.  If you read the patch, you'd find that I'm not declaring an enum.  They're constant ints, because pink make them that way, and therefore have 'k'.

Comment 10

[Stuart Morgan]

(In reply to comment #7)
> > -    unsigned int modifiers = [[NSApp currentEvent] modifierFlags];
> > -
> > -    if (modifiers & NSCommandKeyMask) {
> > +    if ([aSender keyEquivalentModifierMask] & NSCommandKeyMask) {
> >        BOOL loadInTab = [[PreferenceManager sharedInstance]
> > getBooleanPref:"browser.tabs.opentabfor.middleclick" withSuccess:NULL];
> >        BOOL loadInBG  = [[PreferenceManager sharedInstance]
> > getBooleanPref:"browser.tabs.loadInBackground" withSuccess:NULL];
> > -      if (modifiers & NSShiftKeyMask)
> > +      if ([aSender keyEquivalentModifierMask] & NSShiftKeyMask)
> > 
> > Why are you changing this around?
> > 
> 
> Because that's how we fix the bug.  [[NSApp currentEvent] modifierFlags] is
> what's causing the problem.  Using alternates (and checking the menu item's
> keyEquivalentModifierMask) is the fix.

But why not just change the assignment to modifiers, rather than eliminating it and calling keyEquivalentModifierMask twice?

Comment 11

[froodian (Ian Leue)]

Attachment: Patch

Forgot the .h file, sorry.  Also addresses comment 10.

Comment 12

[Nick Kreeger]

+  menuItem = [[NSMenuItem alloc] initWithTitle:NSLocalizedString(@"", nil)
+                                        action:@selector(viewOnlyThisImage:)
+                                 keyEquivalent:@""];
+  if ([[PreferenceManager sharedInstance] getBooleanPref:"browser.tabs.opentabfor.middleclick" withSuccess:NULL])
+    [menuItem setTitle:NSLocalizedString(@"View Image in New Tab", nil)];
+  else
+    [menuItem setTitle:NSLocalizedString(@"View Image in New Window", nil)];
+  [menuItem setTarget:self];
+  [menuItem setKeyEquivalentModifierMask:NSCommandKeyMask];
+  [menuItem setAlternate:YES];
+  [mImageMenu addItem:menuItem];
+  [menuItem release];
+
+  menuItem = [[NSMenuItem alloc] initWithTitle:NSLocalizedString(@"", nil)
+                                        action:@selector(viewOnlyThisImage:)
+                                 keyEquivalent:@""];
+  if ([[PreferenceManager sharedInstance] getBooleanPref:"browser.tabs.opentabfor.middleclick" withSuccess:NULL])
+    [menuItem setTitle:NSLocalizedString(@"View Image in New Tab", nil)];
+  else
+    [menuItem setTitle:NSLocalizedString(@"View Image in New Window", nil)];
+  [menuItem setTarget:self];
+  [menuItem setKeyEquivalentModifierMask:NSCommandKeyMask | NSShiftKeyMask];
+  [menuItem setAlternate:YES];
+  [mImageMenu addItem:menuItem];
+  [menuItem release];

Trying to understand the logic behind this, are you inserting two of the same menu items because you want command-key-mask and shift-key-mask? (Also you can wire these up in IB). 

Comment 13

[froodian (Ian Leue)]

> Trying to understand the logic behind this, are you inserting two of the same
> menu items because you want command-key-mask and shift-key-mask? (Also you can
> wire these up in IB). 

Exactly (well, actually, one item with command-key-mask, and one item with command-key-shift-key-mask).  That way we can have menu items to "Open in new foo" and "Open in new foo in background".

I agree that it'd be nicer to do this in IB, but I'm not sure it's possible.I tried to do it in IB initially, but the problem is that it doesn't recognize shift-key as a modifier unless there's also a key equivalent.  To test this out, make any menu item with no key equivalent, and click the "shift" modifier button.  It'll enable momentarily, then immediately disable itself.  So I ported the menu to code (this also allows us to use "window" or "tab" based on the pref).

Sorry if it's unclear, I know it's a long list of code that seems redundant, but I think (conceptually at least) that it's best.

Comment 14

[Samuel Sidler (old account; do not CC)]

Comment on attachment 234096 [details] [diff] [review]
Patch

Håkan, do you have time to review this?

Comment 15

[Håkan Waara]

Comment on attachment 234096 [details] [diff] [review]
Patch

Whenever you start typing the same thing over and over, it's time to start thinking about how to minimize the repetition. :)

* You should cache the value of the middle click pref

* I suggest you create a NSMenu category (look at our other categories in the extensions/ folder in the source), with a method that goes something like:

- (NSMenuItem*)alternateMenuItemWithTitle:action:target:modifiers:

Then you will be able to reuse this in all places where we create alternate menuitems, with a minimum of repetition.

Comment 16

[Håkan Waara]

A correction: the category method will need to be a class method.

Comment 17

[froodian (Ian Leue)]

Attachment: Patch

In addition to your comments, I took a slightly different approach here which cuts down a lot of extraneous code and is extendable (for when we decide that other context menu items need alternates too).  What's new:

- Implemented the NSMenu class method alternateMenuItemWithTitle:action:target:modifiers:
- Used that method whenever possible for the changes that landed in bug 333765
- introduced a kItemsNeedingAlternatesTag for items needing cmd and shift alternates.  We then create those alternates programmatically in a manner that can be extended to other menu items.
- Has NEW STRINGS:

"NewWindowSuffix" = " in New Window";
"NewTabSuffix" = " in New Tab";

Comment 18

[froodian (Ian Leue)]

Attachment: New BrowserWindow.nib

This nib gives the following three menu items a tag of 104 and unchecks their command key modifier:

"View Image" in the ImageCMenu
"View Image" in the ImageMailToLinkCMenu
"View Image" in the ImageLinkCMenu1

Comment 19

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

Comment on attachment 236564 [details]
New BrowserWindow.nib

Given the type of changes being made in this nib, I'm not the right person to do the review ;)

That said, I tested this and the patch and it works.

However, as I mentioned on irc, you need to use full strings, not suffixes, for these strings so that l10n can change word order if necessary.

Comment 20

[froodian (Ian Leue)]

Attachment: Uses a formatted string for l10n

This solves the l10n problem by passing the original name (the "View Image") part as a parameter for the title.  So, the string changes are:

"Action in New Tab" = "%@ in New Tab";
"Action in New Window" = "%@ in New Window";

Comment 21

[Håkan Waara]

Comment on attachment 236655 [details] [diff] [review]
Uses a formatted string for l10n

>@@ -175,16 +175,26 @@
> 
> // because there's no way to map back from a MenuRef to a Cocoa NSMenu, we have
> // to let receivers of the notification do the test by calling this method.
> - (BOOL)isTargetOfMenuDisplayNotification:(id)inObject
> {
>   return ([inObject pointerValue] == _NSGetCarbonMenu(self));
> }
> 
>++ (id<NSMenuItem>)alternateMenuItemWithTitle:(NSString *)title action:(SEL)action target:(id)target modifiers:(int)modifiers
>+{

Why not NSMenuItem* as return type? Only use id<foo> is if |foo| is a formal objective c protocol.

>+    while (menuItem = [enumerator nextObject]) {
>+      if ([menuItem tag] == kItemsNeedingAlternatesTag) {
>+        NSString* altMenuItemTitle;
>+        if ([[PreferenceManager sharedInstance] getBooleanPref:"browser.tabs.opentabfor.middleclick" withSuccess:NULL])

You don't want to read the pref file every iteration of the loop, please do it once.

>+          altMenuItemTitle = [NSString stringWithFormat:NSLocalizedString(@"Action in New Tab", nil), [menuItem title]];
>+        else
>+          altMenuItemTitle = [NSString stringWithFormat:NSLocalizedString(@"Action in New Window", nil), [menuItem title]];
>+
>+        SEL action = [menuItem action];
>+        id target = [menuItem target];
>+        int initialIndex = [result indexOfItem:menuItem];

I think you can just put these messages directly as arguments to alternateMenuItemWithTitle:. Actually, some compilers optimize that case so they don't have to create/destroy a temp var, so I prefer that for simple arguments like these.

>+
>+        NSMenuItem* cmdMenuItem = [NSMenu alternateMenuItemWithTitle:altMenuItemTitle
>+                                                              action:action
>+                                                              target:target
>+                                                           modifiers:NSCommandKeyMask];
>+        [result insertItem:cmdMenuItem atIndex:initialIndex + 1];
>+
>+        NSMenuItem* cmdShiftMenuItem = [NSMenu alternateMenuItemWithTitle:altMenuItemTitle
>+                                                                   action:action
>+                                                                   target:target
>+                                                                modifiers:NSCommandKeyMask | NSShiftKeyMask];
>+        [result insertItem:cmdShiftMenuItem atIndex:initialIndex + 2];
>+      }
>+    }
>+  }
>+

Given the fact that you need the position/index of the current menuitem, I think it's better for you to use a for-loop instead of a while-loop.

Comment 22

[Håkan Waara]

Comment on attachment 236564 [details]
New BrowserWindow.nib

I trust that the said changes have been made.

As a fun side note, there's a little utility called nibtool that can actually export and import nibs as XML (plists).

So you could create a diff of a nib by diffing the original nib's plist, to the new one.

I leave it as an exercise for the reader how to do that on a given Foo.nib.original and Foo.nib.new using only one command (with unix pipes, and what-not). :)

Comment 23

[froodian (Ian Leue)]

Attachment: Addresses hwaara's comments

Comment 24

[Håkan Waara]

Comment on attachment 236841 [details] [diff] [review]
Addresses hwaara's comments

Some small comments over IRC, address them at will and r=me.

Comment 25

[froodian (Ian Leue)]

Attachment: r=hwaara patch

This patch

- Implements the NSMenu class method |alternateMenuItemWithTitle| to allow for better code-sharing
- Slims down a lot of the code changes from bug 333765 by using said method
- Introduces a way to insert cmd and shift alternates for BWC context menu items, and uses it on the three places we have "View Image" menu items

The reason we don't just define them in the nib is that IB won't accept shift as a lone modifier (ie if there's no key equivalent too)

Comment 26

[froodian (Ian Leue)]

Attachment: This is the bitrot police

Comment 27

[froodian (Ian Leue)]

Attachment: New BrowserWindow.nib

Comment 28

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 238469 [details] [diff] [review]
This is the bitrot police

sr=pink

Comment 29

[froodian (Ian Leue)]

Checked in on trunk and 1.8branch.