Last Comment Bug 343767 - Implement Force Reload and Force Reload all in Contextual Menus
: Implement Force Reload and Force Reload all in Contextual Menus
Status: RESOLVED FIXED
[good first bug]
: fixed1.8.1.1
Product: Camino Graveyard
Classification: Graveyard
Component: Toolbars & Menus (show other bugs)
: Trunk
: PowerPC Mac OS X
-- minor (vote)
: Camino1.5
Assigned To: froodian (Ian Leue)
:
:
Mentors:
Depends on: 342780
Blocks:
  Show dependency treegraph
 
Reported: 2006-07-06 10:20 PDT by froodian (Ian Leue)
Modified: 2006-10-27 17:48 PDT (History)
1 user (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Patch (9.24 KB, patch)
2006-10-09 19:22 PDT, froodian (Ian Leue)
stuart.morgan+bugzilla: review-
Details | Diff | Splinter Review
New BrowserWindow.nib (24.95 KB, application/zip)
2006-10-09 19:23 PDT, froodian (Ian Leue)
stuart.morgan+bugzilla: review-
Details
Patch (12.09 KB, patch)
2006-10-15 15:54 PDT, froodian (Ian Leue)
stuart.morgan+bugzilla: review+
Details | Diff | Splinter Review
New BrowserWindow.nib (25.08 KB, application/zip)
2006-10-15 15:54 PDT, froodian (Ian Leue)
stuart.morgan+bugzilla: review+
Details
r=smorgan patch (12.06 KB, patch)
2006-10-21 23:32 PDT, froodian (Ian Leue)
stuart.morgan+bugzilla: review-
Details | Diff | Splinter Review
Works on toolbar button and menu items (12.45 KB, patch)
2006-10-22 14:02 PDT, froodian (Ian Leue)
stuart.morgan+bugzilla: review-
Details | Diff | Splinter Review
uses respondsToSelector (12.46 KB, patch)
2006-10-22 14:09 PDT, froodian (Ian Leue)
stuart.morgan+bugzilla: review+
Details | Diff | Splinter Review
r=smorgan patch (12.82 KB, patch)
2006-10-22 20:23 PDT, froodian (Ian Leue)
mikepinkerton: superreview+
Details | Diff | Splinter Review

Description User image froodian (Ian Leue) 2006-07-06 10:20:16 PDT
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 User image froodian (Ian Leue) 2006-07-09 09:54:47 PDT
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 User image froodian (Ian Leue) 2006-07-30 10:43:37 PDT
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 User image froodian (Ian Leue) 2006-10-09 19:22:44 PDT
Created attachment 241783 [details] [diff] [review]
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 User image froodian (Ian Leue) 2006-10-09 19:23:54 PDT
Created attachment 241785 [details]
New BrowserWindow.nib

This just gives the reload menu items the correct tag.
Comment 5 User image Stuart Morgan 2006-10-14 14:51:34 PDT
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 User image Stuart Morgan 2006-10-14 14:52:02 PDT
Comment on attachment 241785 [details]
New BrowserWindow.nib

r-, since this will need adjustment for the tab bar alternate.
Comment 7 User image froodian (Ian Leue) 2006-10-15 15:54:02 PDT
Created attachment 242350 [details] [diff] [review]
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 User image froodian (Ian Leue) 2006-10-15 15:54:29 PDT
Created attachment 242351 [details]
New BrowserWindow.nib
Comment 9 User image Stuart Morgan 2006-10-20 23:19:12 PDT
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 User image froodian (Ian Leue) 2006-10-21 23:32:57 PDT
Created attachment 243072 [details] [diff] [review]
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 User image Stuart Morgan 2006-10-22 07:45:03 PDT
(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 User image Stuart Morgan 2006-10-22 07:56:55 PDT
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 User image froodian (Ian Leue) 2006-10-22 14:02:31 PDT
Created attachment 243129 [details] [diff] [review]
Works on toolbar button and menu items

Nice catch.
Comment 14 User image froodian (Ian Leue) 2006-10-22 14:09:45 PDT
Created attachment 243130 [details] [diff] [review]
uses respondsToSelector

Oh hey, you'd given a solution for fixing this using respondsToSelector.  This patch does that.  Take your pick. ;)
Comment 15 User image Stuart Morgan 2006-10-22 15:39:31 PDT
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 User image Stuart Morgan 2006-10-22 15:47:38 PDT
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 User image froodian (Ian Leue) 2006-10-22 19:32:43 PDT
(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 User image Stuart Morgan 2006-10-22 20:09:57 PDT
Ah, good point.  So just fix the indentation on the second |reloadFlags =|, copy it all to reloadAllTabs, and call it r=me.
Comment 19 User image froodian (Ian Leue) 2006-10-22 20:23:48 PDT
Created attachment 243154 [details] [diff] [review]
r=smorgan patch

r=smorgan, for real (hopefully) this time. ;)
Comment 20 User image Mike Pinkerton (not reading bugmail) 2006-10-27 07:03:04 PDT
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 User image froodian (Ian Leue) 2006-10-27 09:48:41 PDT
Checked in on 1.8branch and trunk.

Note You need to log in before you can comment on or make changes to this bug.