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

RESOLVED FIXED in Camino1.5

Status

Camino Graveyard
Preferences
RESOLVED FIXED
11 years ago
11 years ago

People

(Reporter: Stuart Morgan, Assigned: Nick Kreeger)

Tracking

({fixed1.8.1})

Trunk
Camino1.5
PowerPC
Mac OS X
fixed1.8.1

Details

Attachments

(1 attachment, 2 obsolete attachments)

3.05 KB, patch
Stuart Morgan
: review+
Mike Pinkerton (not reading bugmail)
: superreview+
Details | Diff | Splinter Review
(Reporter)

Description

11 years ago
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.
Target Milestone: --- → Camino1.1
(Assignee)

Comment 1

11 years ago
Created attachment 237832 [details] [diff] [review]
Patch 1

Here is a proposed patch.
Attachment #237832 - Flags: review?(stuart.morgan)
(Reporter)

Comment 2

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

Comment 3

11 years ago
Created attachment 237889 [details] [diff] [review]
Patch 2

Round 2...
Attachment #237832 - Attachment is obsolete: true
Attachment #237889 - Flags: review?(stuart.morgan)
(Assignee)

Comment 4

11 years ago
Created attachment 237890 [details] [diff] [review]
Patch 2.1

Oops, missed the comment
Attachment #237889 - Attachment is obsolete: true
Attachment #237890 - Flags: review?(stuart.morgan)
Attachment #237889 - Flags: review?(stuart.morgan)
(Reporter)

Comment 5

11 years ago
Comment on attachment 237890 [details] [diff] [review]
Patch 2.1

r=me
Attachment #237890 - Flags: superreview?(mikepinkerton)
Attachment #237890 - Flags: review?(stuart.morgan)
Attachment #237890 - Flags: review+
Comment on attachment 237890 [details] [diff] [review]
Patch 2.1

sr=pink
Attachment #237890 - Flags: superreview?(mikepinkerton) → superreview+
(Assignee)

Comment 7

11 years ago
Checked in trunk and MOZILLA_1_8_BRANCH.
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
(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.