343767 - Implement Force Reload and Force Reload all in Contextual Menus

Status: RESOLVED FIXED

(Camino Graveyard :: Toolbars & Menus, defect)

Description

[froodian (Ian Leue)]

It'd be nice to have "Force Reload" and "Force Reload All" in the tab context menus, and "Force Reload" in the page context menu.  This requires moving these context menus from the nib to code (since IB can't handle just having shift as a modifier).

Comment 1

[froodian (Ian Leue)]

We should also do force reload all tabs for the tab bar context menu.  (Implementationwise this is very similar to bug 342780, btw)

Comment 2

[froodian (Ian Leue)]

Whenever somebody does this (who am I kidding?  When bug 342780 lands and I do this), we should make sure we get rid of the event key modifier detection for the reload and force reload methods, and use the menu items' modifier keys instead.

Comment 3

[froodian (Ian Leue)]

Attachment: Patch

This implements force reload for all remaining instances of Reload except for the tab bar.  I'm not quite sure how to go about doing that (since it's not in BWC, and not made programmatically), so if it's not immediately describable, let's leave that last instance for a followup bug.

Note that this also fixes the event key modifiers bug for existing force reload menu items.

This requires the following new string:
"Force Reload Format" = "Force %@";

Comment 4

[froodian (Ian Leue)]

Attachment: New BrowserWindow.nib

This just gives the reload menu items the correct tag.

Comment 5

[Stuart Morgan]

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

(In reply to comment #3)
> I'm not quite sure how to go about doing that (since it's not in
> BWC

Yes it is; it's the top-level item "Tab bar context menu" in the BWC nib.  You just need to add an outlet for it so you can get to it programatically.

At a high level, I'm wondering why you are doing this on every tab load, rather than adjusting the menu once in windowDidLoad.


>+const int kItemsNeedingAlternatesTag = 104;
>+const int kItemsNeedingForceReloadTag = 105;

kItemsNeedingForceReloadTag isn't really accurate (that would imply it's a tag on the top-level menu item); it's really kItemsNeedingForceAlternatesTag.  There's also now ambiguity with kItemsNeedingAlternatesTag, so that will need a more precise name like kItemsNeedingOpenTypeAlternatesTag

>   NSMenu* contextMenu = [mTabMenu copy];
>+  contextMenu = [self insertForceReloadAlternateIntoMenu:contextMenu];

This would leak if the menu it returned weren't always the same as the menu passed in (and as mentioned below, there should be no return/assignment anyway).

>+  BOOL needsForceReload = NO;

What's the purpose of this variable? I'm not clear on why the presence or absence of a menu item with the appropriate tag isn't enough information.

>+    result = [self insertForceReloadAlternateIntoMenu:result];

Again, no assignment.

>+- (NSMenu *)insertForceReloadAlternateIntoMenu:(NSMenu *)inMenu

This should return void.

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

Declare i inline.

>+    NSMenuItem* menuItem = [menuArray objectAtIndex:i];
>+
>+    if ([menuItem tag] == kItemsNeedingForceReloadTag) {
>+      NSString* title = [NSString stringWithFormat:NSLocalizedString(@"Force Reload Format", nil), [menuItem title]];

@"Force Format"; there's nothing reload-specific about it.

>+      [inMenu insertItem:forceReloadItem atIndex:i + 1];

Parenthesize |i + 1|

Comment 6

[Stuart Morgan]

Comment on attachment 241785 [details]
New BrowserWindow.nib

r-, since this will need adjustment for the tab bar alternate.

Comment 7

[froodian (Ian Leue)]

Attachment: Patch

(In reply to comment #5)
> (From update of attachment 241783 [details] [diff] [review] [edit])
> (In reply to comment #3)
> > I'm not quite sure how to go about doing that (since it's not in
> > BWC
> 
> Yes it is; it's the top-level item "Tab bar context menu" in the BWC nib.  You
> just need to add an outlet for it so you can get to it programatically.

Oops, yeah

> At a high level, I'm wondering why you are doing this on every tab load, rather
> than adjusting the menu once in windowDidLoad.

Just not thinking - all the other context menu stuff is done in |contextMenu|, but since it's not really context based (and therefore doesn't need to be done for each tab), it should definitely be in |windowDidLoad|.  Fixed.

> 
> Parenthesize |i + 1|
> 

Done.  Also fixed the code where I first did this too.

All other comments addressed.

Comment 8

[froodian (Ian Leue)]

Attachment: New BrowserWindow.nib

Comment 9

[Stuart Morgan]

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

>+const int kItemsNeedingForceReloadTag = 105;

This still needs to be changed to kItemsNeedingForceAlternateTag

>+- (void)insertForceReloadAlternateIntoMenu:(NSMenu *)inMenu;

Since the method is generic (and could potentially add multiple items in a menu), call it insertForceAlternatesIntoMenu:

>+  if ([[aSender keyEquivalent] isEqualToString:@"R"] || ([aSender keyEquivalentModifierMask] & NSShiftKeyMask) != 0)
...
>+      if ([[sender keyEquivalent] isEqualToString:@"R"] || ([sender keyEquivalentModifierMask] & NSShiftKeyMask) != 0)
...
>+  if ([[sender keyEquivalent] isEqualToString:@"R"] || ([sender keyEquivalentModifierMask] & NSShiftKeyMask) != 0)

Lose the |!= 0|

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

This still needs to be declared inline

>+      NSString* title = [NSString stringWithFormat:NSLocalizedString(@"Force Format", nil), [menuItem title]];

For localized format strings, put the English version in the comment param so that it's clear what the arguments are doing.


r=me with those changes.

Comment 10

[froodian (Ian Leue)]

Attachment: r=smorgan patch

So all in all this patch:

- Creates force reload alternates for
 - "Reload Tab" and "Reload all Tabs" in the tab context menus
 - "Reload All Tabs" in the tab bar context menu
 - "Reload" in the page context menu
- Allows us to lose the currentEvent keyModifiers in the reload methods, since any time we want force reload, it's now done with an alternate

Comment 11

[Stuart Morgan]

(In reply to comment #10)
> - Allows us to lose the currentEvent keyModifiers in the reload methods, since
> any time we want force reload, it's now done with an alternate

Wait, how does that work with the reload toolbar button?

Comment 12

[Stuart Morgan]

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

Yeah, this not only breaks force-reload with the toolbar button, but the entire toolbar button, because sending keyEquivalent to an NSToolbarItem throws an exception.  Sorry for missing that in review.

You'll have to break it down by whether the sender responds to that, and check the old way if not.

Comment 13

[froodian (Ian Leue)]

Attachment: Works on toolbar button and menu items

Nice catch.

Comment 14

[froodian (Ian Leue)]

Attachment: uses respondsToSelector

Oh hey, you'd given a solution for fixing this using respondsToSelector.  This patch does that.  Take your pick. ;)

Comment 15

[Stuart Morgan]

Comment on attachment 243129 [details] [diff] [review]
Works on toolbar button and menu items

respondsToSelector is definitely the preferred way of finding out if something responds to a selector.

Comment 16

[Stuart Morgan]

Comment on attachment 243130 [details] [diff] [review]
uses respondsToSelector

>+  if ([aSender respondsToSelector:@selector(keyEquivalent:)]) {
>+    // Capital R tests for shift when there's a keyEquivalent, keyEquivalentModifierMask tests when there isn't
>+    if ([[aSender keyEquivalent] isEqualToString:@"R"] || ([aSender keyEquivalentModifierMask] & NSShiftKeyMask))
>+      reloadFlags = NSLoadFlagsBypassCacheAndProxy;
>+  }
>+  // It's a toolbar button, so we test for shift using modifierFlags
>+  else if ([[NSApp currentEvent] modifierFlags] & NSShiftKeyMask)
>+      reloadFlags = NSLoadFlagsBypassCacheAndProxy;

Make this:

  if (([aSender respondsToSelector:@selector(keyEquivalent:)] &&
       ([[aSender keyEquivalent] isEqualToString:@"R"] ||
        ([aSender keyEquivalentModifierMask] & NSShiftKeyMask))) ||
      ([[NSApp currentEvent] modifierFlags] & NSShiftKeyMask))
    {
      reloadFlags = NSLoadFlagsBypassCacheAndProxy;
    }

(with an appropriate comment before it) to remove the flag code duplication, and do this in reloadAllTabs: in case that's ever called from something other than a menu item.

r=me with those changes.

Comment 17

[froodian (Ian Leue)]

(In reply to comment #16)
> Make this:
> 
>   if (([aSender respondsToSelector:@selector(keyEquivalent:)] &&
>        ([[aSender keyEquivalent] isEqualToString:@"R"] ||
>         ([aSender keyEquivalentModifierMask] & NSShiftKeyMask))) ||
>       ([[NSApp currentEvent] modifierFlags] & NSShiftKeyMask))
>     {
>       reloadFlags = NSLoadFlagsBypassCacheAndProxy;
>     }

This actually changes behavior, and won't necessarily work.  Say you hit shift, open the menu, release shift, choose reload.  It'll force reload.  ie, the "else" part of the "else if" in my patch was important.  We could add a ![aSender respondsToSelector:@selector(keyEquivalent:)] to the second superclause, but that really hurts readability.  I'm not sure what the best solution (keeping in mind both code recycling and readability) is.

Comment 18

[Stuart Morgan]

Ah, good point.  So just fix the indentation on the second |reloadFlags =|, copy it all to reloadAllTabs, and call it r=me.

Comment 19

[froodian (Ian Leue)]

Attachment: r=smorgan patch

r=smorgan, for real (hopefully) this time. ;)

Comment 20

[Mike Pinkerton (not reading bugmail)]

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

+      NSString* title = [NSString stringWithFormat:NSLocalizedString(@"Force Format", "Force %@"), [menuItem title]];

omit the 2nd param on NSLocalizedString (make it nil). We don't use them to any effect in camino.

Comment 21

[froodian (Ian Leue)]

Checked in on 1.8branch and trunk.