Closed Bug 352760 Opened 13 years ago Closed 13 years ago
.3] Safari no longer special-cased (hidden) when default reader
21.83 KB, image/jpeg
22.21 KB, image/jpeg
3.29 KB, image/jpeg
4.12 KB, image/jpeg
1.54 KB, patch
|Details | Diff | Splinter Review|
On 10.3, Safari is the default feed reader even though it can't handle feeds--unless the user has installed and set another feed reader to be default. Previously, we excluded Safari from the list of installed feed readers (and "ignored" it if set to the default reader. Now we blithely accept Safari as the default on 10.3, even though it can't load feeds. (The fact that Apple spent the effort to add a sheet specifically prompting you to upgrade to 10.4 to get feed support when passing a feed to Safari 1.3.x irks me...that effort could have been better spent on removing "feed" from the supported protocols in Safari's plist on 10.3 :/ ) Is there a way we can fix this regression without regressing bug 352069 again? if 10.3 and default feed app = Safari then display grey localized string "No Feed Readers Installed" instead of "Safari" or something?
To be clear: we exclude it from the list of feed readers; we just don't special-case it when it's set as default (which will be the case for most of our userbase on 10.3, I'd reckon)
Summary: [10.3] Safari no longer excluded from the → [10.3] Safari no longer excluded from the feed readers list when default
Hoping 3rd time's the charm for a decent summary :P
Summary: [10.3] Safari no longer excluded from the feed readers list when default → [10.3] Safari no longer special-cased (hidden) when default reader
Argh. Yes, there is, and I specifically mentioned the need to do it when I filed bug 352078, but then I forgot by the time I did the review: > 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). There's no need for a special string; once that's fixed it will just show a blank entry like it normally would for the no-current-reader case.
(In reply to comment #3) > There's no need for a special string; once that's fixed it will just show a > blank entry like it normally would for the no-current-reader case. And that will properly collapse the separator now (post-bug 352078)? I.e., on a "stock" 10.3 install, the only thing that will appear in that menu will be "Select…", right?
Yes, it should show only a blank item, a separator, and Select…
This should do it. Smokey (or anyone else with 10.3), could you verify that this does the right thing?
Assignee: nobody → stuart.morgan
Status: NEW → ASSIGNED
Attachment #239835 - Flags: review?(alqahira)
Comment on attachment 239835 [details] [diff] [review] Ignore Safari on 10.3 It does what you describe in comment 5, although I find that still looks odd...and is a bit problematic in a really edge case with the "first-time" feed sheet. I.e., in the feed sheet, you can hit "OK" and we'll try to pass the feed to the "blank" app. (We have working error checking there; about to file on issues with that...), but it'd be better if we never got in that state. IOW, I'd prefer, on the "stock 10.3" case, that this menu only showed "Select…", not the (checked!) blank line and separator as well. If there's not a good/easy way to do that, then r=me on this patch, because it fixes the Safari issue.
Comment on attachment 239835 [details] [diff] [review] Ignore Safari on 10.3 Having a blank makes it visually clear that you have nothing selected, which is good, and I'm not sure about the idea of the user having to select the thing that's already selected to cause something to happen; that seems unintuitive. My preference would be to make the "OK" button disabled as long as nothing is selected (that has to be done either way or the user could press it while "Select…" is showing, which would behave just as badly). Lets just check this in, since Safari on 10.3 needs to be special-cased to behave like no option regardless of how the no-feed case looks, and then tackle the rest in a new bug.
Attachment #239835 - Flags: superreview?(sfraser_bugs)
Comment on attachment 239835 [details] [diff] [review] Ignore Safari on 10.3 Sure.
Attachment #239835 - Flags: review?(alqahira) → review+
Attachment #239835 - Flags: superreview?(sfraser_bugs) → superreview-
Comment on attachment 239835 [details] [diff] [review] Ignore Safari on 10.3 Sorry, fumbled there.
Attachment #239835 - Flags: superreview- → superreview+
Fixed trunk and MOZILLA_1_8_BRANCH, thanks Stuart!
You need to log in before you can comment on or make changes to this bug.