The default bug view has changed. See this FAQ.

Implement Force Reload and Force Reload all in Contextual Menus

RESOLVED FIXED in Camino1.5

Status

Camino Graveyard
Toolbars & Menus
--
minor
RESOLVED FIXED
11 years ago
11 years ago

People

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

Tracking

({fixed1.8.1.1})

Trunk
Camino1.5
PowerPC
Mac OS X
fixed1.8.1.1

Details

(Whiteboard: [good first bug])

Attachments

(2 attachments, 6 obsolete attachments)

(Assignee)

Description

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

Updated

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

Comment 1

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

Comment 2

11 years ago
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.
Depends on: 342780
(Assignee)

Updated

11 years ago
Assignee: nobody → stridey
(Assignee)

Comment 3

11 years ago
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 %@";
Attachment #241783 - Flags: review?(stuart.morgan)
(Assignee)

Comment 4

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

This just gives the reload menu items the correct tag.
Attachment #241785 - Flags: review?(stuart.morgan)

Comment 5

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

Comment 6

11 years ago
Comment on attachment 241785 [details]
New BrowserWindow.nib

r-, since this will need adjustment for the tab bar alternate.
Attachment #241785 - Flags: review?(stuart.morgan) → review-
(Assignee)

Comment 7

11 years ago
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.
Attachment #241783 - Attachment is obsolete: true
Attachment #242350 - Flags: review?(stuart.morgan)
(Assignee)

Comment 8

11 years ago
Created attachment 242351 [details]
New BrowserWindow.nib
Attachment #241785 - Attachment is obsolete: true
Attachment #242351 - Flags: review?(stuart.morgan)

Updated

11 years ago
Attachment #242351 - Flags: review?(stuart.morgan) → review+

Comment 9

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

Comment 10

11 years ago
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
Attachment #242350 - Attachment is obsolete: true
Attachment #243072 - Flags: superreview?(mikepinkerton)

Comment 11

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

11 years ago
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.
Attachment #243072 - Flags: superreview?(mikepinkerton) → review-
(Assignee)

Comment 13

11 years ago
Created attachment 243129 [details] [diff] [review]
Works on toolbar button and menu items

Nice catch.
Attachment #243072 - Attachment is obsolete: true
Attachment #243129 - Flags: review?(stuart.morgan)
(Assignee)

Comment 14

11 years ago
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. ;)
Attachment #243130 - Flags: review?(stuart.morgan)

Comment 15

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

Comment 16

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

Comment 17

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

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

Comment 19

11 years ago
Created attachment 243154 [details] [diff] [review]
r=smorgan patch

r=smorgan, for real (hopefully) this time. ;)
Attachment #243129 - Attachment is obsolete: true
Attachment #243130 - Attachment is obsolete: true
Attachment #243154 - Flags: superreview?(mikepinkerton)
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.
Attachment #243154 - Flags: superreview?(mikepinkerton) → superreview+
(Assignee)

Comment 21

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

Updated

11 years ago
Keywords: fixed1.8.1.1
(Assignee)

Updated

11 years ago
Keywords: fixed1.8.1
You need to log in before you can comment on or make changes to this bug.