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 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.
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.
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...
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.
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.
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?
Comment on attachment 231024 [details] [diff] [review] Change constant names, as suggested email@example.com
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.
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.
Comment on attachment 231181 [details] [diff] [review] Branch patch approved by schrep for drivers
Fix checked in on branch.