395250 - Web Feed shows me a preview, option says to use Live Bookmark

Status: VERIFIED FIXED

(Firefox :: File Handling, defect, P1)

Description

[Steve England [:stevee]]

Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a8pre) Gecko/2007090602 Minefield/3.0a8pre ID:2007090602

1. New profile, start firefox
2. Go to http://news.bbc.co.uk
3. Click on RSS feed. Note you get a preview.
4. Tools > Options > Applications
5. Scroll down to Web Feed. Note that it says "Show me a preview and ask me.."
6. Click on Web Feed.

Expected:
- Drop down selection box should have "Show me a preview and ask me.." selected

Actual:
- Drop down selection box has "Live Bookmarks" as its selection.

Comment 1

[Myk Melez [:myk] [@mykmelez]]

I see this on Linux and Mac as well.  Investigating.

Comment 2

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v1: fixes problem

The problem is that rebuildActionsMenu doesn't check alwaysAskBeforeHandling before selecting the item in the menu corresponding to preferredAction.

alwaysAskBeforeHandling overrides preferredAction, so even if preferredAction is set to "Live Bookmarks" (as it is by default), when alwaysAskBeforeHandling is set to true (as it is by default), then we always ask the user what they want to do (which for feeds means showing them the preview page), so we should select the alwaysAskBeforeHandling item in the menu (which for feeds is the "Show me a preview..." item).

In this patch, I make sure to always check alwaysAskBeforeHandling before selecting a menu item based on the value of preferredAction.  Note that this doesn't yet matter for other types, but it will matter once we fix bug 395138 and start showing other types for which we always ask.

Gavin: a better fix might be to create all the menu items first, then select the appropriate item at the end based on the values of alwaysAskBeforeHandling and preferredAction.  That shouldn't be too hard to do, and it would make the code simpler overall.  What do you think?

Comment 3

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #2)
> Gavin: a better fix might be to create all the menu items first, then select
> the appropriate item at the end based on the values of alwaysAskBeforeHandling
> and preferredAction.  That shouldn't be too hard to do, and it would make the
> code simpler overall.  What do you think?

I agree, setting the selected item at the end rather than as you're adding items to the menu would be clearer.

Comment 4

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v2: selects item after creating them all

> I agree, setting the selected item at the end rather than as you're adding
> items to the menu would be clearer.

Ok, here's a patch that does it like that.

Comment 5

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v3: select from created menuItem objects

Gavin suggested on IRC that this would be better if we selected the selected item from the menuitem objects we created in the process of building the menu rather than by going back to the DOM to retrieve the object.

Here's a patch that does it that way.

Comment 6

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v4: unrotted

Comment 7

[Myk Melez [:myk] [@mykmelez]]

Requesting blocking-firefox3 for this Applications prefpane regression fix.

Comment 8

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v5: unrotted

Comment 9

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v6: unrotted

It's easy to rot this patch, but fortunately the conflicts are simple to resolve (there aren't any functionality conflicts, just code being modified in the vicinity of the changes in this patch).  Here's an up-to-date version that is the version I'll check in.

Comment 10

[Myk Melez [:myk] [@mykmelez]]

Checking in browser/components/preferences/applications.js;
/cvsroot/mozilla/browser/components/preferences/applications.js,v  <--  applications.js
new revision: 1.17; previous revision: 1.16
done

Comment 11

[Al Billings [:abillings - ex-MoCo]]

Long fixed but verified in Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9pre) Gecko/2008050904 Minefield/3.0pre.