Want tweaks to feed preference code to better support the new prefwindow

RESOLVED FIXED in Firefox 2 beta2

Status

()

Firefox
RSS Discovery and Preview
--
major
RESOLVED FIXED
12 years ago
12 years ago

People

(Reporter: Waldo, Assigned: Waldo)

Tracking

({fixed1.8.1})

Trunk
Firefox 2 beta2
fixed1.8.1
Points:
---
Dependency tree / graph
Bug Flags:
blocking-firefox2 +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [has patch][baking])

Attachments

(2 attachments, 3 obsolete attachments)

The current preferences don't map well to the new UI, and patches which implement it now must go to great effort to correctly handle and save the represented preference settings.  (Such patches are also likely to be broken, as I discovered attachment 228386 [details] [diff] [review] was when I was not under pressure to make a b1 deadline within the day :-\ .)  The new UI (see also bug 340677) contains:

- a menulist listing all the user's web readers and client readers
- a three-option radiogroup titled "When I click on a web feed:" with the options:
  - "Ask me what to use"
  - "Always subscribe with a Live Bookmark"
  - "Always subscribe using the Feed Reader"

This doesn't map well to the current prefs, where the options are not-present ("ask"), "bookmarks" (live bookmark), and "client" and "web" (feed reader).  Representing "ask" as not-present is particularly problematic given how the preference bindings work.

The patch I will post shortly makes the following changes:

- add a fourth value to browser.feeds.handler, "ask"
- add a preference used to store a non-default reader choice (e.g., when "ask"
  is set and the user makes a for-this-time-only reader choice), with possible
  values "web", "client", and "bookmarks"

The "ask" value will be exposed in UI as the inverse of a new checkbox "use this as default reader" in the subscription dialog and as "ask every time" (or not-bookmarks and not-reader) in the prefwindow.  The second preference is for internal use only for communicating the selected reader between the selection dialog and the code which processes the subscription request after the user accepts the dialog.
Created attachment 229222 [details] [diff] [review]
Patch
Attachment #229222 - Flags: review?(bugs)
(Assignee)

Updated

12 years ago
Status: NEW → ASSIGNED
Flags: blocking-firefox2?
Comment on attachment 229222 [details] [diff] [review]
Patch

Actually, after using this and writing some pref UI I think it's still wrong.  Next plan of attack:

Preferences:
- rename browser.feeds.handler.one_shot to browser.feeds.handler.default, with possible values "web" and "client" -- determines identity of default feed reader
- remove "web" and "client" from the values for browser.feeds.handler and replace it with "reader" (thus the final list is "ask", "bookmarks", and "reader") -- when "ask", always displays reader selection UI, when "bookmarks", uses a live bookmark, when "reader", uses the reader ID'd by browser.feeds.handler.default

UI:
- radiogroup corresponds directly to browser.feeds.handler
- menulist corresponds to possible readers, with the one indicated by browser.feeds.handler.default as the selected default (if none selected, uses first alphabetically)

There's nothing especially wrong with the previous methods, but they don't expose a default reader well enough for preferences, and if the one_shot pref is used to represent that value, information is stored redundantly in prefs.
Attachment #229222 - Attachment is obsolete: true
Attachment #229222 - Flags: review?(bugs)
Created attachment 229628 [details] [diff] [review]
Better patch

...as described in the previous comment.  I'm at 99% on the prefwindow, so this bug is extremely (extremely) close to being a practical blocker in addition to a theoretical blocker of bug 340677.
Attachment #229628 - Flags: review?(bugs)

Updated

12 years ago
Flags: blocking-firefox2? → blocking-firefox2+
Whiteboard: [has patch][needs review ben]
Comment on attachment 229628 [details] [diff] [review]
Better patch

On hold for a sec until I can finish up the feed-fu changes requested during the prefwindow meeting on Tuesday...
Attachment #229628 - Flags: review?(bugs)
I have a patch, but without a fix for bug 341407 I can't test that it works correctly, and I'm fairly certain that fix will conflict with my patch here.
Depends on: 341407
Created attachment 231008 [details] [diff] [review]
Patch, no conflicts with trunk

The only problem I've noticed with this patch is that if bookmarks are the default reader (no preview page), the resultant bookmark has an empty title, at least on <http://planet.mozilla.org/atom.xml>.  I looked at the code, and this looks like a Places problem, not a feeds problem, so I'm ignoring it.
Attachment #229628 - Attachment is obsolete: true
Attachment #231008 - Flags: review?(bugs)
Comment on attachment 231008 [details] [diff] [review]
Patch, no conflicts with trunk

>Index: mozilla/browser/components/feeds/content/options.js
>+    // only save selected value to preferences if the user checked the
>+    // "use as default" checkbox
>+    var useAsDefault = document.getElementById("useAsDefault").checked;
>+    prefs.setCharPref(PREF_SELECTED_HANDLER,
>+                      useAsDefault ? selectedHandler : "ask");

Why? I want the list box to remember the choice I picked last, even if I have it ask me every time.
(In reply to comment #7)
> Why? I want the list box to remember the choice I picked last, even if I have
> it ask me every time.

It does -- there are two separate preferences here.  The PREF_SELECTED_HANDLER one has values "ask", "bookmarks", and "reader".  The PREF_DEFAULT_READER one has values "bookmarks", "client", and "web".

Here's how it works:

Action                   |  PREF_SELECTED_HANDLER  | PREF_DEFAULT_READER
-------------------------------------------------------------------------
web reader, not default  |        ask              |        web
bookmarks, not default   |        ask              |       bookmarks
app, not default         |        ask              |        client
web reader, default      |        reader           |        web
bookmarks, default       |       bookmarks         |      bookmarks
app, default             |        reader           |       client

The code you quote is determining the value of PREF_SELECTED_HANDLER, which determines what action happens.  However, the selected reader in the subscribe dialog isn't determined using that preference -- it's determined using PREF_DEFAULT_READER, which, as you can see, is set a few lines later to whatever option was chosen (with "reader" converted into "client"/"web" as necessary).
(In reply to comment #8)
> It does -- there are two separate preferences here.  The PREF_SELECTED_HANDLER
> one has values "ask", "bookmarks", and "reader".  The PREF_DEFAULT_READER one
> has values "bookmarks", "client", and "web".

Actually, now that I think about it, these names aren't that great.  Maybe the constant names should be PREF_DEFAULT_HANDLER and PREF_SELECTED_READER, respectively?  The concepts are fairly alike, and I think I'm having trouble coming up with accurate yet cognitively distinct names.
PREF_SELECTED_ACTION?

e.g. action is to ask or to use the default reader

then use PREF_SELECTED_READER (change from default) to identify which reader is chosen. 

_ACTION controls defaultness
_READER controls reader, independent of whether or not the user is being asked. This means reader state is saved across invocations of the dialog, even if the user does not change the default/not default setting. 
Created attachment 231024 [details] [diff] [review]
Change constant names, as suggested

Any other comments?
Attachment #231008 - Attachment is obsolete: true
Attachment #231024 - Flags: review?(bugs)
Attachment #231008 - Flags: review?(bugs)
Comment on attachment 231024 [details] [diff] [review]
Change constant names, as suggested

r=ben@mozilla.org
Attachment #231024 - Flags: review?(bugs) → review+
Patch in on trunk; will give it a day to bake before requesting branch approval.  Leaving open in the interval so I don't forget this...
Jeff, I can hook you up with a query for blockers that aren't fixed on branch yet, poke me when you get online.
Whiteboard: [has patch][needs review ben] → [has patch][baking]
Created attachment 231181 [details] [diff] [review]
Branch patch

I've given this some testing on trunk and haven't found any new issues with it; consequently I'd like it to go in on branch.  This patch is necessary for feed preferences in the new preference window to work correctly 100% of the time.
Attachment #231181 - Flags: approval1.8.1?

Comment 16

12 years ago
Comment on attachment 231181 [details] [diff] [review]
Branch patch

approved by schrep for drivers
Attachment #231181 - Flags: approval1.8.1? → approval1.8.1+
Fix checked in on branch.
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Keywords: fixed1.8.1
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.