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

Status: RESOLVED FIXED

(Firefox Graveyard :: RSS Discovery and Preview, defect)

Description

[Jeff Walden [:Waldo]]

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.

Comment 1

[Jeff Walden [:Waldo]]

Attachment: Patch

Comment 2

[Jeff Walden [:Waldo]]

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.

Comment 3

[Jeff Walden [:Waldo]]

Attachment: 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.

Comment 4

[Jeff Walden [:Waldo]]

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...

Comment 5

[Jeff Walden [:Waldo]]

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.

Comment 6

[Jeff Walden [:Waldo]]

Attachment: 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.

Comment 7

[Ben Goodger (use ben at mozilla dot org for email)]

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.

Comment 8

[Jeff Walden [:Waldo]]

(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).

Comment 9

[Jeff Walden [:Waldo]]

(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.

Comment 10

[Ben Goodger (use ben at mozilla dot org for email)]

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. 

Comment 11

[Jeff Walden [:Waldo]]

Attachment: Change constant names, as suggested

Any other comments?

Comment 12

[Ben Goodger (use ben at mozilla dot org for email)]

Comment on attachment 231024 [details] [diff] [review]
Change constant names, as suggested

r=[e-mail]

Comment 13

[Jeff Walden [:Waldo]]

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...

Comment 14

[Mike Connor [:mconnor]]

Jeff, I can hook you up with a query for blockers that aren't fixed on branch yet, poke me when you get online.

Comment 15

[Jeff Walden [:Waldo]]

Attachment: 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.

Comment 16

[Mike Schroepfer]

Comment on attachment 231181 [details] [diff] [review]
Branch patch

approved by schrep for drivers

Comment 17

[Jeff Walden [:Waldo]]

Fix checked in on branch.