The default bug view has changed. See this FAQ.

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

VERIFIED FIXED in Camino1.5

Status

Camino Graveyard
Toolbars & Menus
VERIFIED FIXED
11 years ago
11 years ago

People

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

Tracking

({fixed1.8.1})

unspecified
Camino1.5
PowerPC
Mac OS X
fixed1.8.1
Dependency tree / graph

Details

(Whiteboard: [good first bug] [NEW STRINGS in comment 20])

Attachments

(2 attachments, 11 obsolete attachments)

(Assignee)

Description

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

Updated

11 years ago
Whiteboard: [good first bug]
(Assignee)

Comment 1

11 years ago
Created attachment 227168 [details] [diff] [review]
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"
Assignee: nobody → stridey
Status: NEW → ASSIGNED
Attachment #227168 - Flags: review?(mozilla)
(Assignee)

Comment 2

11 years ago
Created attachment 227169 [details]
new BrowserWindow.nib

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

Updated

11 years ago
Depends on: 181978
(Assignee)

Comment 3

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

Comment 4

11 years ago
Created attachment 227173 [details] [diff] [review]
Uses correct capitalization of strings
Attachment #227168 - Attachment is obsolete: true
Attachment #227173 - Flags: review?(mozilla)
Attachment #227168 - Flags: review?(mozilla)
(Assignee)

Comment 5

11 years ago
Created attachment 228303 [details]
New BrowserWindow.nib

This is the bitrot police.
Attachment #227169 - Attachment is obsolete: true
(Assignee)

Updated

11 years ago
Blocks: 331745
(Assignee)

Updated

11 years ago
Attachment #227173 - Flags: review?(mozilla) → review?(nick.kreeger)
Blocks: 343767

Comment 6

11 years ago
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?
Attachment #227173 - Flags: review?(nick.kreeger) → review-
(Assignee)

Comment 7

11 years ago
Created attachment 233916 [details] [diff] [review]
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.
Attachment #227173 - Attachment is obsolete: true
Attachment #233916 - Flags: review?(nick.kreeger)

Comment 8

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

Comment 9

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

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

Comment 11

11 years ago
Created attachment 234096 [details] [diff] [review]
Patch

Forgot the .h file, sorry.  Also addresses comment 10.
Attachment #233916 - Attachment is obsolete: true
Attachment #234096 - Flags: review?(nick.kreeger)
Attachment #233916 - Flags: review?(nick.kreeger)

Comment 12

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

Comment 13

11 years ago
> 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 on attachment 234096 [details] [diff] [review]
Patch

Håkan, do you have time to review this?
Attachment #234096 - Flags: review?(nick.kreeger) → review?(hwaara)

Comment 15

11 years ago
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.
Attachment #234096 - Flags: review?(hwaara) → review-

Comment 16

11 years ago
A correction: the category method will need to be a class method.
(Assignee)

Comment 17

11 years ago
Created attachment 236563 [details] [diff] [review]
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";
Attachment #234096 - Attachment is obsolete: true
Attachment #236563 - Flags: review?(hwaara)
(Assignee)

Comment 18

11 years ago
Created attachment 236564 [details]
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
Attachment #228303 - Attachment is obsolete: true
Attachment #236564 - Flags: review?(alqahira)
(Assignee)

Updated

11 years ago
Whiteboard: [good first bug] → [good first bug] [NEW STRINGS in comment 17]
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.
Attachment #236564 - Flags: review?(alqahira) → review?(hwaara)
(Assignee)

Updated

11 years ago
Blocks: 350806
(Assignee)

Comment 20

11 years ago
Created attachment 236655 [details] [diff] [review]
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";
Attachment #236563 - Attachment is obsolete: true
Attachment #236655 - Flags: review?(hwaara)
Attachment #236563 - Flags: review?(hwaara)
(Assignee)

Updated

11 years ago
Whiteboard: [good first bug] [NEW STRINGS in comment 17] → [good first bug] [NEW STRINGS in comment 20]

Comment 21

11 years ago
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.
Attachment #236655 - Flags: review?(hwaara) → review-

Comment 22

11 years ago
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). :)
Attachment #236564 - Flags: review?(hwaara) → review+
(Assignee)

Comment 23

11 years ago
Created attachment 236841 [details] [diff] [review]
Addresses hwaara's comments
Attachment #236655 - Attachment is obsolete: true
Attachment #236841 - Flags: review?(hwaara)

Comment 24

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

Some small comments over IRC, address them at will and r=me.
Attachment #236841 - Flags: review?(hwaara) → review+
(Assignee)

Comment 25

11 years ago
Created attachment 236888 [details] [diff] [review]
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)
Attachment #236841 - Attachment is obsolete: true
Attachment #236888 - Flags: superreview?(mikepinkerton)
(Assignee)

Updated

11 years ago
Blocks: 290212
(Assignee)

Comment 26

11 years ago
Created attachment 238469 [details] [diff] [review]
This is the bitrot police
Attachment #236888 - Attachment is obsolete: true
Attachment #238469 - Flags: superreview?(mikepinkerton)
Attachment #236888 - Flags: superreview?(mikepinkerton)
(Assignee)

Comment 27

11 years ago
Created attachment 238470 [details]
New BrowserWindow.nib
Attachment #236564 - Attachment is obsolete: true
Comment on attachment 238469 [details] [diff] [review]
This is the bitrot police

sr=pink
Attachment #238469 - Flags: superreview?(mikepinkerton) → superreview+
(Assignee)

Comment 29

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

Updated

11 years ago
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.