350806 - Make Go menu history items respect the cmd/shift modifiers properly

Status: VERIFIED FIXED

(Camino Graveyard :: Toolbars & Menus, defect)

Description

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

Go menu history items currently sort-of respects the cmd modifier (new foo)--only if it's held down when before the menu drops.  The shift toggle is currently ignored entirely.

We need to port that to alternates to fix the Go menu's version of bug 333765 and to enable shift.

We should also keep an eye on perf here, since smfr had to do a bunch of work to make the Go menu performant with >4 days of history.  Hopefully if we only create alternates out of the items that are actually displayed (100 per folder, iirc) it won't suck.

Comment 1

[froodian (Ian Leue)]

Attachment: Patch

The following patch:
- Adds alternates for Go menu, using a new method in NSMenu+Utils
- Re-factors the code that makes the bookmark bar alternates in BookmarkMenu.  Note that to do this, I had to move the |setRepresentedObject| call out of the ifs so that menuItem would have a representedObject to give the new alternates.

Comment 2

[Stuart Morgan]

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

>+  NSMenuItem *menuItem = [[NSMenuItem alloc] initWithTitle:title action:NULL keyEquivalent:@""];

I'm not sure I agree with the refactoring in this method; why do the extra work in the separator case? It's not particularly tricky code.

>+  [menuItem setRepresentedObject:inItem];
...
>     else    //separator
>-    {
>-      menuItem = [[NSMenuItem separatorItem] retain];
>-      [self addItem:menuItem];
>-    }
>+      [self addItem:[NSMenuItem separatorItem]];

This I definitely don't agree with, as it actually changes behavior. I don't see any reason a separator shouldn't be linked with it's corresponding bookmark item.

>     else // group
>     { 
>-      menuItem = [[NSMenuItem alloc] initWithTitle:title action: NULL keyEquivalent: @""];
>       [menuItem setTarget:[NSApp delegate]];
>       [menuItem setAction:@selector(openMenuBookmark:)];
>       [menuItem setImage:[inItem icon]];      
>     }
> 
>     [self addItem:menuItem];

Groups should have menu alternates too.  I guess that was missed in the other bug?  I'm not sure what extra plumbing would be needed if any to support it in the opening logic; if it's as easy as adding a call to your new method here, lets do it now, otherwise we'll need a follow-up bug.

>+      if ((modifiers & NSCommandKeyMask) != 0)
>+        if ((modifiers & NSShiftKeyMask) != 0)

Lose the |!= 0|

>+- (void)insertInvisibleCommandAndShiftAlternatesForMenuItem:(NSMenuItem *)inMenuItem;

This is adding, not inserting, Invisible is redundant with alternate, and CommandAndShift could be a bit misleading since there's no shift alternate.  Call it addCommandKeyAlternatesForMenuItem:

>+  NSMenuItem *altMenuItem  = [NSMenu alternateMenuItemWithTitle:title
>+                                                         action:action
>+                                                         target:target
...
>+  altMenuItem = [NSMenu alternateMenuItemWithTitle:title
>+                                            action:action
>+                                            target:target
>+                                         modifiers:(NSCommandKeyMask | NSShiftKeyMask)];

Since we make lots of menu items in loops, and this isn't the kind of method that's ever going to have early returns, I think there's a strong case for using non-autoreleased menu items here.

Comment 3

[froodian (Ian Leue)]

Attachment: Patch

I reworked the refactoring in a way that should hopefully make everything a bit cleaner and still ensure that menuItem has a representedObject by the time we make the call to the NSMenu+Utils method.

> >     else // group
> >     { 
> >-      menuItem = [[NSMenuItem alloc] initWithTitle:title action: NULL keyEquivalent: @""];
> >       [menuItem setTarget:[NSApp delegate]];
> >       [menuItem setAction:@selector(openMenuBookmark:)];
> >       [menuItem setImage:[inItem icon]];      
> >     }
> > 
> >     [self addItem:menuItem];
> 
> Groups should have menu alternates too.  I guess that was missed in the other
> bug?  I'm not sure what extra plumbing would be needed if any to support it in
> the opening logic; if it's as easy as adding a call to your new method here,
> lets do it now, otherwise we'll need a follow-up bug.

Yeah, we just missed this in the original bug.  Done.

> >+  NSMenuItem *altMenuItem  = [NSMenu alternateMenuItemWithTitle:title
> >+                                                         action:action
> >+                                                         target:target
> ...
> >+  altMenuItem = [NSMenu alternateMenuItemWithTitle:title
> >+                                            action:action
> >+                                            target:target
> >+                                         modifiers:(NSCommandKeyMask | NSShiftKeyMask)];
> 
> Since we make lots of menu items in loops, and this isn't the kind of method
> that's ever going to have early returns, I think there's a strong case for
> using non-autoreleased menu items here.
> 

Sure, yeah.  Especially since it's a pretty low-level method, I think we can justify code duplication for the sake of non-autoreleasedness.  Is what I'm doing the best way to do this, or should I make it once, copy it, and just change the keyEquivalentModifierMask?

Comment 4

[Stuart Morgan]

(In reply to comment #3)
> Is what I'm doing the best way to do this, or should I make it once, copy it,
> and just change the keyEquivalentModifierMask?

Make an initAlternateWithTitle:..., use that, and reimplement alternateMenuItemWithTitle:... on top of it.

Comment 5

[froodian (Ian Leue)]

Attachment: Patch

Does that.

Comment 6

[Stuart Morgan]

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

This doesn't apply cleanly anymore, so it'll need a respin for testing. A few notes though

>+    [menuItem setKeyEquivalentModifierMask:0]; //Needed since by default NSMenuItems have NSCommandKeyMask

The addCommandKeyAlternatesForMenuItem: should do this, since it will always need to be done for those items.

>         // Create the alternates and insert them into the two places after the menu item
>-        NSMenuItem* cmdMenuItem = [NSMenu alternateMenuItemWithTitle:altMenuItemTitle
>-                                                              action:action
>-                                                              target:target
>-                                                           modifiers:NSCommandKeyMask];
>+        NSMenuItem* cmdMenuItem = [NSMenuItem alternateMenuItemWithTitle:altMenuItemTitle
>+                                                                  action:action
>+                                                                  target:target
>+                                                               modifiers:NSCommandKeyMask];
>         [result insertItem:cmdMenuItem atIndex:i + 1];
>-        NSMenuItem* cmdShiftMenuItem = [NSMenu alternateMenuItemWithTitle:altMenuItemTitle
>-                                                                   action:action
>-                                                                   target:target
>-                                                                modifiers:(NSCommandKeyMask | NSShiftKeyMask)];
>+        NSMenuItem* cmdShiftMenuItem = [NSMenuItem alternateMenuItemWithTitle:altMenuItemTitle
>+                                                                       action:action
>+                                                                       target:target
>+                                                                    modifiers:(NSCommandKeyMask | NSShiftKeyMask)];
>         [result insertItem:cmdShiftMenuItem atIndex:i + 2];

This is basically a duplication of addCommandKeyAlternatesForMenuItem:, so we should find a way to re-use code.  One would be to always find the index of the menu item in addCommandKeyAlternatesForMenuItem: and then do the right thing, but if we are processing all of history and all the bookmarks, that's probably not a good idea.  Maybe an insert... version, with a shared implementation.

Also, I missed this in the bug where it landed, but the code in this section is a sketchy; looping forward over the array while inserting into it isn't a good idea.  This turns out to be safe since the items it inserts don't have the tag, and it recomputes the count every time through the loop, but it makes me nervous.  Make the loop run from end to start rather than start to end.

>+  NSMenuItem* altMenuItem = [[[NSMenuItem alloc] initAlternateWithTitle:title
>+                                                                 action:action
>+                                                                 target:target
>+                                                              modifiers:modifiers] autorelease];
>+  return altMenuItem;

Just
  return [[[NSMenuItem alloc]...

Comment 7

[froodian (Ian Leue)]

Attachment: Patch

(In reply to comment #6)
> (From update of attachment 243514 [details] [diff] [review] [edit])
> This doesn't apply cleanly anymore, so it'll need a respin for testing.

Part of the bitrot was the landing of BWC's |shouldLoadInBackground|, which uses current event modifier flags.  In order to avoid the "have to hold modifier before opening the menu bug," I made that method take a sender param.  For now, I made all other calls except the one concerned with this bug send in NULL to |shouldLoadInBackground|, essentially preserving the same behavior they have now.  A lot the other calls will have (already have) the same problem, and will need to get alternate menu items and send |shouldLoadInBackground| their sender as a param.  That can be covered in a followup bug.

> This is basically a duplication of addCommandKeyAlternatesForMenuItem:, so we
> should find a way to re-use code.  One would be to always find the index of the
> menu item in addCommandKeyAlternatesForMenuItem: and then do the right thing,
> but if we are processing all of history and all the bookmarks, that's probably
> not a good idea.  Maybe an insert... version, with a shared implementation.

Per IRC, since we also use custom titles (in addition to insert vs add), I'm just leaving this code as it is.

> Also, I missed this in the bug where it landed, but the code in this section is
> a sketchy; looping forward over the array while inserting into it isn't a good
> idea.  This turns out to be safe since the items it inserts don't have the tag,
> and it recomputes the count every time through the loop, but it makes me
> nervous.  Make the loop run from end to start rather than start to end.

Ew, yeah.

Comment 8

[Stuart Morgan]

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

>+  if ([aSender isKindOfClass:[NSMenuItem class]]) {
>+    if([aSender keyEquivalentModifierMask] & NSShiftKeyMask)
>+      loadInBackground = !loadInBackground;
>+  }

respondsToSelector:?

>+      BOOL backgroundLoad = [BrowserWindowController shouldLoadInBackground:NULL];

NULL is a null pointer. id's use nil.

>+    // Iterate end to start so we don't end up examining items we just inserted

No need for the comment here

>+    for (unsigned int i = [menuArray count] - 1; i >= 0; i--) {

This needs to be a signed int, or empty menus will make it very sad.

>+- (id)initAlternateWithTitle:(NSString *)title action:(SEL)action target:(id)target modifiers:(int)modifiers
>+{
>+  [self initWithTitle:title action:action keyEquivalent:@""];
>+  [self setTarget:target];
>+  [self setKeyEquivalentModifierMask:modifiers];
>+  [self setAlternate:YES];
>+
>+  return self;

Should be
  if ((self = [self initWithTitle:...])) {
    ...
  }
  return self;

Comment 9

[froodian (Ian Leue)]

Attachment: Patch

Comment 10

[Stuart Morgan]

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

>+        NSMenuItem* cmdMenuItem = [NSMenuItem alternateMenuItemWithTitle:altMenuItemTitle
>+                                                                  action:action
>+                                                                  target:target
>+                                                               modifiers:NSCommandKeyMask];

Fix the colon alignment here (and anywhere else that it may need fixing from the NSMenu -> NSMenuItem change).

> // Get the load-in-background pref
>-+ (BOOL)shouldLoadInBackground;
>++ (BOOL)shouldLoadInBackground:(id)aSender;

Comment about how aSender is used to get the modifiers and what happens if you use |nil|, so the behavior is clear from the header.

>+  if (self = [self initWithTitle:title action:action keyEquivalent:@""]) {

if ((self = [...])) {
(double parens for assignment used as equality test)


There's also been a bit of bitrot, both in a few sections not applying and in a few more places now using shouldLoadInBackground and needing a change (sorry this sat for so long).

r=me with all that fixed up though.

Comment 11

[froodian (Ian Leue)]

Attachment: r=smorgan patch

Comment 12

[Mike Pinkerton (not reading bugmail)]

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

sr=pink

Comment 13

[froodian (Ian Leue)]

Checked in on 1.8branch and trunk.