357516 - Favicons (site icons) on alternates in menus don't update

Status: RESOLVED FIXED

(Camino Graveyard :: Toolbars & Menus, defect)

Description

[Stuart Morgan]

Favicons on menu items don't update in all the alternates, so it's easy to get into a situation where the favicons on menu items change as you press Command or Command-Shift.  Right now it's just bookmarks, but it will presumably affect history as well once the history-with-alternates stuff lands.

Comment 1

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

[4:45pm] smorgan: IIRC, it only affects things that have changed since you launched

STR:

1. Go to a site in your bookmarks which you know has a favicon but which currently is not cached (e.g., manually add http://live.gnome.org/Epiphany/MozillaBugs to your bookmarks, then visit it)
2. Open your bookmarks menu and hold down Cmd or Cmd-Shift and watch the icon disappear

Comment 2

[Håkan Waara]

When creating the alternate items, the menu icon might need to be set. Or are they actually removed from the main menuitem as well, and for good?

Comment 4

[Christopher Henderson]

Attachment: Fix v1.0

Updates the bookmark and the next two NSMenuItems (for Cmd and Cmd+Shift). This also fixes titles for alternates not being updated. Looks like this 2 year old bug *will* get fixed.

Comment 5

[Chris Lawson (gone)]

Comment on attachment 351673 [details] [diff] [review]
Fix v1.0

Looks good to me. Minor nit: periods at the end of the comments, since they're complete sentences. r=me with that addressed.

Comment 6

[Stuart Morgan]

Comment on attachment 351673 [details] [diff] [review]
Fix v1.0

>+    for (unsigned int i = 0; i < 3; i++)

I really don't like having the number of alternates hard-coded like this. Instead, make an updateCommandKeyAlternatesForMenuItem: method in NSMenu+Utils and walk forward as long as the subsequent items have the same key equivalent but different key modifiers. That gets the alternates in a generic way.

> Looks like this 2 year old bug *will* get fixed.

This isn't actually the bug people are unhappy about, BTW; the link in that post was to the wrong bug.

Comment 7

[Christopher Henderson]

Attachment: Fix v1.1

Moves the updating to our NSMenu category. Will now update all the alternates that have the same represented object.

Comment 8

[Chris Lawson (gone)]

Comment on attachment 353491 [details] [diff] [review]
Fix v1.1

>Index: camino/src/extensions/NSMenu+Utils.h
>===================================================================

>+// Updates the command and shift-command alternate menu items for a given menu
>+// item. Returns the number of alternates updated.

Is there a reason we would care about the number of alternates we just updated? I can see how it's useful for the method you copied from, but I don't see the point here. Seems like a (void) would be better.

I'd also reword the comment to "Update the Command and Command-Shift alternate menu items for a given menu item." If there's a reason to have it return the number of alternates, replace the period with a comma, and continue, "and return the number of alternates updated."

>+  if (itemIndex == -1) return 0;

It might be useful to explain, in a comment, why this should return early, since it's not quite as self-documenting as Objective-C is supposed to be :-p (It may also be that I don't understand how alternates work as well as I should.)

>+  return altIndex-itemIndex-1;

If there's a reason to keep this as an (int) method, this would be clearer if expressed as:

return (altIndex - itemIndex - 1);

I'm going to r- this for now, pending an answer to the (int) vs (void) question.

Comment 9

[Christopher Henderson]

Attachment: Fix v1.2

Yeah, good point about the int vs void thing. This simplifies the method greatly. The -1 thing will probably never happen, but indexOfItem does return that value if the item can't be found, and it may just prevent a bug in the future.

Comment 10

[Chris Lawson (gone)]

Comment on attachment 353633 [details] [diff] [review]
Fix v1.2

>Index: camino/src/extensions/NSMenu+Utils.m
>===================================================================
>RCS file: /cvsroot/mozilla/camino/src/extensions/NSMenu+Utils.m,v
>retrieving revision 1.21
>diff -u -8 -r1.21 NSMenu+Utils.m
>--- camino/src/extensions/NSMenu+Utils.m	11 Jun 2008 17:57:57 -0000	1.21
>+++ camino/src/extensions/NSMenu+Utils.m	18 Dec 2008 07:00:13 -0000
>@@ -337,16 +337,35 @@
>   [altMenuItem setRepresentedObject:representedObject];
>   [altMenuItem setImage:image];
>   [self insertItem:altMenuItem atIndex:(itemIndex + 2)];
>   [altMenuItem release];
> 
>   return 2;
> }
> 
>+- (void)updateCommandKeyAlternatesForMenuItem:(NSMenuItem *)inMenuItem
>+{
>+  int itemIndex = [self indexOfItem:inMenuItem];
>+  // indexOfItem will return -1 if the item is not in the menu
>+  if (itemIndex == -1) return;

How about this:

// If the item is not in this menu, there's nothing to update.
if (itemIndex == -1)
  return;


r=me with or without the return on a separate line; I don't think we have an established style for that, but I personally prefer it on a separate line as it's easier to read that way.

Comment 11

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 353633 [details] [diff] [review]
Fix v1.2

sr=pink

Comment 12

[Christopher Henderson]

Attachment: Patch, for checkin

Patch with cl's comments incorporated.

Comment 13

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

Checked in on cvs trunk; thanks!