352078 - RSS selection menu looks weird when there's no default feed app inserted

Status: RESOLVED FIXED

(Camino Graveyard :: Preferences, defect)

Description

[Stuart Morgan]

The popup doesn't do what we wanted when the default app either doesn't exist, or isn't in the main list. It's a simple thing mistake we both missed:

+  // The user selected an application that is not registered for "feed://".
+  if (!insertedDefaultApp) {
+    NSURL* defaultFeedAppURL = [[NSWorkspace sharedWorkspace] urlOfApplicationWithIdentifier:defaultFeedViewerID];
+    if (defaultFeedAppURL) {
+      NSMenuItem* menuItem = [self menuItemForAppURL:defaultFeedAppURL
+                                        withBundleID:defaultFeedViewerID
+                                           andAction:nil 
+                                           andTarget:inTarget];
+      [[inPopupButton menu] insertItem:menuItem atIndex:0];
+    }
+    // Since we couldn't find a default application, add a blank menu item.
+    else {
+      NSMenuItem* dummyItem = [[NSMenuItem alloc] init];
+      [dummyItem setTitle:@""];
+      [[inPopupButton menu] insertItem:dummyItem atIndex:0];
+      [dummyItem release];
+    }
+  }

Both of these add to [inPopupButton menu] (before it's set) rather than menu, so they don't show up.

The |if (defaultFeedAppURL)| should also check that the app in question isn't Safari if it's 10.3 (arguably it should show if it's 10.4 and that already is their default).

+  // allow the user to select a feed application
+  [menu addItem:[NSMenuItem separatorItem]];

This should only be added if at least one app was actually added to the list in the main loop, to prevent double separators.

Comment 1

[Nick Kreeger]

Attachment: Patch 1

Here is a proposed patch.

Comment 2

[Stuart Morgan]

Comment on attachment 237832 [details] [diff] [review]
Patch 1

>+  BOOL shouldInsertSeperatorAtEnd = NO;  // determine if we need to add a seperator above the select item.

Remove the comment; its name and use self-document.

>     if (defaultFeedViewerID && [curBundleID isEqualToString:defaultFeedViewerID]) {
>       [menu insertItem:menuItem atIndex:0];
>       insertedDefaultApp = YES;
>     }
>     else
>       [menu addItem:menuItem];
>+    
>+    shouldInsertSeperatorAtEnd = YES;

The |shouldInsertSeperatorAtEnd = YES;| needs to be inside the else, otherwise if the only installed app is the default app there will be two dividers with nothing between them.

Comment 3

[Nick Kreeger]

Attachment: Patch 2

Round 2...

Comment 4

[Nick Kreeger]

Attachment: Patch 2.1

Oops, missed the comment

Comment 5

[Stuart Morgan]

Comment on attachment 237890 [details] [diff] [review]
Patch 2.1

r=me

Comment 6

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 237890 [details] [diff] [review]
Patch 2.1

sr=pink

Comment 7

[Nick Kreeger]

Checked in trunk and MOZILLA_1_8_BRANCH.