410176 - Select current folder in subscription dialog and add feed dialog

Status: RESOLVED FIXED

(MailNews Core :: Feed Reader, enhancement)

Description

[Christian Schmidt]

Attachment: Patch

This is a suggestion for improving the user-friendliness of the feed subscriptions dialog.

Currently the subscription dialog always opens with the first folder selected, even when the dialog is invoked from another folder. When selecting Subscribe... from the context menu of a feed folder in the folder pane or File > Subscribe... when a feed folder is selected, the subscription dialog should open with the current folder selected.

When selecting a folder inside the subscription dialog and pressing Add for adding a feed, the new dialog for entering the feed URL by default has the root folder selected in the folder menulist. Instead the default folder should be the folder that was selected in the subscription dialog. If the subscription dialog was not opened from a specific folder, no folder should be selected in the dialog (currently the first folder is selected), and the folder menulist should default to the server root (if this is selected, a new folder is created for the feed).

When a feed has been added, the feed list in the subscription dialog is reloaded and the first folder is selected. It would be better to open the folder that the feed was just added to and select the feed that was just added.

This patch implements these suggestions.

The patch also fixes a regression that was introduced with revision 1.26 of toolkit/content/widgets/menulist.xml: When pressing Edit on a feed, the folder menulist in the feed properties dialog isn't properly initialized.

Comment 1

[Phil Ringnalda (:philor)]

Comment on attachment 294833 [details] [diff] [review]
Patch

Sorry, unacceptably slow review, even by my horrible standards.

In general, it's lovely, and turns the UI into something that actually makes sense. Thanks!

A few long-line and trailing whitespace nits (see http://beaufour.dk/jst-review/, ignoring the bogus complaint about QueryInterface while heeding the long-line warning for that line that's hidden by it).

The one real problem I saw was that you forgot to change the openSubscriptionsDialog call in mailnews/base/resources/content/msgAccountCentral.js to pass a folder (hmm, what folder?) instead of a server, so opening that was doesn't set mRSSServer, at which point things start to break.

Comment 2

[Christian Schmidt]

Attachment: Patch, v. 2

>Sorry, unacceptably slow review, even by my horrible standards.
No worries, I am used to waiting far longer :-)

Thanks for the review. I think this patch addresses your comments. In the future I'll make sure to run my patches through the JST tool.

Comment 3

[Phil Ringnalda (:philor)]

Comment on attachment 298319 [details] [diff] [review]
Patch, v. 2

New profile, create a News & Blogs account, Manage subscriptions from account central (which opens quite happily, thanks), Add a feed

>Index: mail/extensions/newsblog/content/feed-subscriptions.js
>+    // unless another folder is specified, default to currently selected
>+    if (aRootFolderURI) {
>+      feedProperties.folderURI = aRootFolderURI;
>+    } else {
>+      var index = this.mTree.view.selection.currentIndex;

since there isn't anything to be a selection, -1

>+      var item = this.mView.getItemAtIndex(index);

getItemAtIndex(-1) makes mView very, very angry (well, okay, it's a warning, not a very, very angry, but you are testing with javascript.options.strict set to true, aren't you?).

Comment 4

[Phil Ringnalda (:philor)]

New profile+account, Manage subscriptions, Add http://www.mozilla.org/news.rdf, close the subscription dialog.

Now that you have a folder in the account, right click the Mozilla Dot Org folder, Subscribe, Add. Change the "where to store" select from Mozilla Dot Org to News & Blogs, and subscribe to http://blog.mozilla.com/feed/.

Expected: items wind up in a folder named The Mozilla Blog (well, maybe I expect them loose under the server, but I don't want to expect that), or if that's not allowed since I came in from right clicking Mozilla Dot Org, no option to think I'll get that.

Actual: items wind up in the Mozilla Dot Org folder even though I changed away from it.

Comment 5

[Christian Schmidt]

Attachment: Patch, v. 3

Yes, I do run with strict warnings, but I must admit that I didn't test the bootstrapping case that well.

getItemAtIndex() now does a bounds check and allows any integer parameter without barfing (assuming that there are no "holes" in the array, i.e. array keys below length that are undefined, but AFAICS this is always the case). The upper bound check may not be necessary, but I added it for consistency with the -1 case.

I fixed the issue in comment 4 by getting the URI from menuitem.value instead of menuitem.id. Those RDF-based menulists are mostly black magic to me, and I am not sure about when to use .id, .value or .uri, but this appears to work.

Comment 6

[Phil Ringnalda (:philor)]

Comment on attachment 300524 [details] [diff] [review]
Patch, v. 3

r=philringnalda, and thank you!

I'm just a little worried that I'm looking at it with stars in my eyes, since it makes things so much saner, so let's have Magnus take a look, too (fortunately, he's not nearly as slothful as me).

Comment 7

[Magnus Melin [:mkmelin]]

Comment on attachment 300524 [details] [diff] [review]
Patch, v. 3

Very nice indeed, Christian! 

One nit: remove angel brackts for the if since it's one row only. (For other cases we generally have else { on a row of its own.)
 
+    if (aRootFolderURI) {
+      feedProperties.folderURI = aRootFolderURI;
+    } else {

Comment 8

[Phil Ringnalda (:philor)]

mail/extensions/newsblog/content/feed-subscriptions.js 1.22
mail/extensions/newsblog/content/feed-properties.js 1.14
mail/extensions/newsblog/content/toolbar-icon.xul 1.9
mail/base/content/mailWindowOverlay.js 1.183
mailnews/base/resources/content/msgAccountCentral.js 1.23

Comment 9

[Martin Stubenschrott]

Did that commit maybe break adding new feeds in a new folder?

I added 3 Feeds in my "News & Blogs" Account previously and they all went up into a new folder (like expected). Now when I right-click the "News & Blogs" server, and click "Subscribe..." a window "RSS Subscription" comes up where i can "Add" a feed. However, even since i don't select a folder in the "RSS Subscriptions" dialog, after pressing "Add" the dialog "Feed properties" which comes up has the first folder selected. Ok, I thought, no big deal and selected "News & Blogs" in the "Feed Properties" dialog.

But when finally adding the feed, the feed comes up in the first folder, instead of creating a new folder just under "News & Blogs" as expected.

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9b4pre) Gecko/2008021103 Thunderbird/3.0a1pre

Comment 10

[Christian Schmidt]

I cannot reproduce the issue described in comment 9. I have tried using a nightly build for Windows and using my own build from CVS on Linux.

Martin, do you see any errors in the Error Console?

Comment 11

[Christian Schmidt]

Attachment: Additional patch

Ah, sorry, there was a file missing in my patch :-/

Comment 12

[Phil Ringnalda (:philor)]

Comment on attachment 302687 [details] [diff] [review]
Additional patch

Grr, bad me, I'm pretty sure I saw that testing one of the first versions, but didn't figure out that the key in the steps to reproduce is "1. add a feed, 2. add a second feed 3. when you add a third feed..." and last time around I must not have added a third feed from having the server selected.

And then I almost tried to pin the bug 68174 rewrite of PickedMsgFolder on you ;)

Comment 13

[Phil Ringnalda (:philor)]

mail/extensions/newsblog/content/feed-properties.xul 1.15