716706 - Fix and enhance the feed Subscriptions dialog.

Status: RESOLVED FIXED

(MailNews Core :: Feed Reader, defect)

Description

[alta88]

Attachment: patch.

this patch attempts the following:
1. fix several tree view bugs; anything deeper than 1 level didn't work, nor did things like selection when toggling containers, etc.
2. remove the add/edit dialog and move those properties to the inline panel.  make buttons enabled when relevant.
3. add top level feed accounts to the view, to make multiple account mgmt easier.
4. enable moving feed subscriptions between accounts (dnd).
5. enable updating of feed title and url.
6. add a Check Subscriptions link when a dnd or pasted feed url fails to validate, and point it to the w3c validator service.  so, you know, there's proof where the problem lies...
7. add some more feedback messages on actions completed/failed.
8. remove redundant css and use icons as per the folder pane definitions.
9. remove unnecessary files.

note: this bug doesn't touch import/export opml other than cosmetically; but i think i will address the nested folder problem in a followup.

Comment 1

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 587169 [details] [diff] [review]
patch.

Trying to add my first Feed, I get:
JavaScript error: chrome://messenger-newsblog/content/feed-subscriptions.js, lin
e 807: this.mView.selection is null
So I'm going to say ui-r- for that.

I'm also a little confused as to how I should test this patch, since there are no steps to reproduce, so I'm left with testing the whole feed subscription dialog, which is kinda painful...  (I'll do it on the next version if I have to, but it's going to be slow.)

Thanks,
Blake.

Comment 2

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

alta88: by any chance, do you have an Hg branch on which you've committed these changes individually?  It'd definitely help me provide feedback to be able to look at a set of commits representing individual changes!

Comment 3

[alta88]

(In reply to Blake Winton (:bwinton - Thunderbird UX) from comment #1)
> Comment on attachment 587169 [details] [diff] [review]
> patch.
> 
> Trying to add my first Feed, I get:
> JavaScript error: chrome://messenger-newsblog/content/feed-subscriptions.js,
> lin
> e 807: this.mView.selection is null
> So I'm going to say ui-r- for that.
> 
> I'm also a little confused as to how I should test this patch, since there
> are no steps to reproduce, so I'm left with testing the whole feed
> subscription dialog, which is kinda painful...  (I'll do it on the next
> version if I have to, but it's going to be slow.)
> 
> Thanks,
> Blake.

how did you try to subscribe, dnd a link or paste/add?  the dialog treeview always has an item selected, whether opened by rt click on folder Subscribe or in account management.  the add button shouldn't even be enabled unless a folder is selected..  could there could have been a prior error or the patch didn't apply fully?  this bug depends on some imports from bug 711173.

i know it's a bit large, and i've tried to pound on it prior to submission.  as for a simple script:
1) drag a real feed link/drag a non feed link and drop on a dialog folder.
2) paste a real feed/non feed, and Add.
3) change Title, URL, Folder and Update.
4) add a second NewsBlog account.
5) dnd feeds between and within accounts.
6) in folder pane, create a 2 or 3 level deep folder structure; open the dialog and toggle the tree, do some dnd moves, use the folder picker to move a feed.

Comment 4

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 587169 [details] [diff] [review]
patch.

Type the url, then click "add".  I'll try again and make sure I've applied the patch from bug 711173 first…

Comment 5

[alta88]

(In reply to Myk Melez [:myk] [@mykmelez] from comment #2)
> alta88: by any chance, do you have an Hg branch on which you've committed
> these changes individually?  It'd definitely help me provide feedback to be
> able to look at a set of commits representing individual changes!

do you mean at moz?  no, i just have them in my local repo, i had pulled the comm-central tree anew and applied this patch and 711173 ok previously.  but if there's a process to create or push to a branch let me know.  not sure about auth (from the snowl days :)).

Comment 6

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

(In reply to alta88 from comment #5)
> do you mean at moz?  no, i just have them in my local repo, i had pulled the
> comm-central tree anew and applied this patch and 711173 ok previously.  but
> if there's a process to create or push to a branch let me know.

hg.mozilla.org only provides user repos (a.k.a. topic/feature branches) for users with commit privileges, which I don't think you have, but the branch doesn't have to be on that server; it can be anywhere, like on bitbucket.org!  As long as it is publicly accessible, and the changes were committed individually, I can clone the repo and browse its change graph to get a better understanding of the changes.

Alternately, you could make the local repo available as a file archive that I could download and expand locally.  Or even just export the changes as a set of patches that I can apply to my own local clone of comm-central via `hg export` (which I then import as individual changes via `hg import`).

Comment 7

[alta88]

so you'd like each file touched exported as an individual diff (.patch) rather than one big diff?  all of which could then be zipped and attached here?

Comment 8

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

The best thing to do is to export each change (a.k.a. commit, revision) rather than each file changed.  So use hg export with the list of change IDs to export those changes:

hg export change-id-1 change-id-2 ... > changes.diff

Alternately, specify a range of changes (see `hg help multirevs` for more info on specifying a range of changes):

hg export change-id-1:change-id-12 > changes.diff

Note: if you didn't actually commit each individual change you made as you were making the changes, then obviously this is not possible.  But in that case, per-file diffs aren't more helpful, and I'll just give feedback on the existing patch.  Just let me know!

Comment 9

[alta88]

ah i see.  no, i didn't commit each change locally to the 'wip' incrementally.  it's really just one big delta at this point.  it's more possible with feed code since bit rot chances are low, and there's only been the one opml import patch that had to be merged since i started.

the current trunk with this and bug 711173 applied reflects my local repo.

for future, if there's some way to make largish patches easier for reviewers, please let me know.

Comment 10

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 587169 [details] [diff] [review]
patch.

Okay, I've played around with it a little, and here's what I've found.

I typed "weblog.latte.ca" in, and hit "Add", and it reported "The Feed URL is not a valid feed." (which is kinda correct, but I think we could assume "http://" at the start, which would make it a little better), but when I clicked the "Check Validation." link, the page that opened reported that it was a valid Atom 1.0 feed (presumably because they're using feed autodiscovery).

I can only seem to drop a url onto the top half of the folder.

There seems to be no way to remove a folder, even though I can remove the only feed in it.

When I deleted the "Blog-O!" folder from the 3-pane window, and then tried to add "http://weblog.latte.ca/index.xml", it didn't add, and gave the error message:
JavaScript error: , line 0: uncaught exception: [Exception... "Component returne
d failure code: 0x80550006 [nsIMsgLocalMailFolder.addMessage]"  nsresult: "0x805
50006 (<unknown>)"  location: "JS frame :: chrome://messenger-newsblog/content/F
eedItem.js :: <TOP_LEVEL> :: line 369"  data: no]
(The properties dialog also didn't update to remove the "Blog-O!" folder.)
And now it all seems kinda hosed.  :(

Finally, switching the "Show the article summary" checkbox didn't seem to change anything when I went back to look at the blog.

So, all in all, I like it, but I want to see those things fixed before I give it the ui-r+.

Thanks,
Blake.

Comment 11

[alta88]

> Comment on attachment 587169 [details] [diff] [review]
> patch.
> 
> Okay, I've played around with it a little, and here's what I've found.
> 
> I typed "weblog.latte.ca" in, and hit "Add", and it reported "The Feed URL
> is not a valid feed." (which is kinda correct,

well, it's actually 100% correct ;)

 but I think we could assume
> "http://" at the start, which would make it a little better), but when I
> clicked the "Check Validation." link, the page that opened reported that it
> was a valid Atom 1.0 feed (presumably because they're using feed
> autodiscovery).

yes, and the purpose of the validation page is to do that, give the actual specific url that does validate or tell you if it doesn't (or there are none on best guess).  the url is a key and needs to be specific.

note that almost the entire usage of the Add function in this Dialog is users pasting feed urls.  it necessarily must edit check, but, as i've mentioned elsewhere, a feed autodiscovery UI for random typings that could be urls is a different feature and design.  a random typing can be:
1. invalid site
2. valid site/homepage with no feeds
3. valid site with one or more feeds that are invalid
4. valid site with one or more feeds that are valid (pass parsing validation).
5. items 3 & 4 are mixed; user must make a choice.

and there are additional complexities.  in Fx, a random typing in the urlbar may take you to an isp search page, which may have feeds, but isn't at all what the user meant.

if you mean this sort of feature to be inline, without viewing an actual web page for context, it should be a different bug.

the current patch language could be more clear, ie 'Check validation and retrieve valid feed url'.

> 
> I can only seem to drop a url onto the top half of the folder.
> 

could you elaborate?  the drop target, if valid (ie a folder and not a feed subscription) will highlight indicating the target is a valid drop.

> There seems to be no way to remove a folder, even though I can remove the
> only feed in it.

correct.  the existing code specifically disallows dnd on folders, and does not allow folder removal (auto or otherwise).  the reason is that 'unsubscribe' does not mean 'also delete the folder', as the folder may contain articles.  the existing code does not do folder management in the Dialog, and this patch doesn't attempt to address that.  i happen to think a smart app would allow folder management in the Dialog (mirroring that of folder pane), but this is an additional feature that would be a separate bug.

> 
> When I deleted the "Blog-O!" folder from the 3-pane window, and then tried
> to add "http://weblog.latte.ca/index.xml", it didn't add, and gave the error
> message:
> JavaScript error: , line 0: uncaught exception: [Exception... "Component
> returne
> d failure code: 0x80550006 [nsIMsgLocalMailFolder.addMessage]"  nsresult:
> "0x805
> 50006 (<unknown>)"  location: "JS frame ::
> chrome://messenger-newsblog/content/F
> eedItem.js :: <TOP_LEVEL> :: line 369"  data: no]
> (The properties dialog also didn't update to remove the "Blog-O!" folder.)
> And now it all seems kinda hosed.  :(
> 

that will be fixed.  the existing Dialog code doesn't auto refresh on folder pane changes, but assumes a Dialog restart to get the new folder tree state.

> Finally, switching the "Show the article summary" checkbox didn't seem to
> change anything when I went back to look at the blog.
> 

with a feed message selected, do you have the global override View->Feed Message Body As->Default Format?

> So, all in all, I like it, but I want to see those things fixed before I
> give it the ui-r+.
> 

this patch attempts really the following scope:
1) fix the several treeview problems.
2) remove the add/edit popup to be inline.

much more is feature creep, which i'd rather do as followups as this is already a big patch. ;)


> Thanks,
> Blake.

Comment 12

[alta88]

Attachment: patch

this patch makes any folderpane change (add/del/movecopy/rename) in an rss account immediately reflect in an open subscriptions window tree, while maintaining selection state where feasible.

Comment 13

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 589541 [details] [diff] [review]
patch

(In reply to alta88 from comment #11)
> > Comment on attachment 587169 [details] [diff] [review]
> > I typed "weblog.latte.ca" in, and hit "Add", and it reported "The Feed URL
> > is not a valid feed." (which is kinda correct,
> well, it's actually 100% correct ;)

It may be technically correct, but it's not useful, particularly when recent versions of Firefox are hiding the "http://" at the start.

> > but I think we could assume
> > "http://" at the start, which would make it a little better),

Have you implemented this part?

> > but when I
> > clicked the "Check Validation." link, the page that opened reported that it
> > was a valid Atom 1.0 feed (presumably because they're using feed
> > autodiscovery).
> 
> yes, and the purpose of the validation page is to do that, give the actual
> specific url that does validate or tell you if it doesn't (or there are none
> on best guess).  the url is a key and needs to be specific.

That's both hidden and irrelevant to our users…  The experience of "This is invalid, click here to find out why…" followed by "It's valid!" is confusing.

> note that almost the entire usage of the Add function in this Dialog is
> users pasting feed urls.

I'm very interested to hear where you got the data that shows that!

> it necessarily must edit check, but, as i've
> mentioned elsewhere, a feed autodiscovery UI for random typings that could
> be urls is a different feature and design.  a random typing can be:
> 1. invalid site
> 2. valid site/homepage with no feeds
> 3. valid site with one or more feeds that are invalid
> 4. valid site with one or more feeds that are valid (pass parsing
> validation).
> 5. items 3 & 4 are mixed; user must make a choice.

I'm certainly not asking for you to read people's minds and fix their typos, but there do seem to be a couple of obvious things (add "http://" if it's missing, and check the page for autodiscovery urls) we can do to make the experience much better in many cases.

> the current patch language could be more clear, ie 'Check validation and
> retrieve valid feed url'.

Is there any way we could get the feed url back from that page?

> > I can only seem to drop a url onto the top half of the folder.
> could you elaborate?  the drop target, if valid (ie a folder and not a feed
> subscription) will highlight indicating the target is a valid drop.

It did, but only when my mouse hovered over the top half of the target.  (This might be a platform bug…)

> > There seems to be no way to remove a folder, even though I can remove the
> > only feed in it.
> correct.  the existing code specifically disallows dnd on folders, and does
> not allow folder removal (auto or otherwise).  the reason is that
> 'unsubscribe' does not mean 'also delete the folder', as the folder may
> contain articles.  the existing code does not do folder management in the
> Dialog, and this patch doesn't attempt to address that.  i happen to think a
> smart app would allow folder management in the Dialog (mirroring that of
> folder pane), but this is an additional feature that would be a separate bug.

Okay.

> > When I deleted the "Blog-O!" folder from the 3-pane window, and then tried
> > to add "http://weblog.latte.ca/index.xml", it didn't add, and gave the error
> > message:
> > JavaScript error: , line 0: uncaught exception: [Exception... "Component
> > returne
> > d failure code: 0x80550006 [nsIMsgLocalMailFolder.addMessage]"  nsresult:
> > "0x805
> > 50006 (<unknown>)"  location: "JS frame ::
> > chrome://messenger-newsblog/content/F
> > eedItem.js :: <TOP_LEVEL> :: line 369"  data: no]
> > (The properties dialog also didn't update to remove the "Blog-O!" folder.)
> > And now it all seems kinda hosed.  :(
> that will be fixed.  the existing Dialog code doesn't auto refresh on folder
> pane changes, but assumes a Dialog restart to get the new folder tree state.

Cool.

> > Finally, switching the "Show the article summary" checkbox didn't seem to
> > change anything when I went back to look at the blog.
> with a feed message selected, do you have the global override View->Feed
> Message Body As->Default Format?

I'm not sure, is that the default?  (And if not, why not?)

> > So, all in all, I like it, but I want to see those things fixed before I
> > give it the ui-r+.
> this patch attempts really the following scope:
> 1) fix the several treeview problems.
> 2) remove the add/edit popup to be inline.
> 
> much more is feature creep, which i'd rather do as followups as this is
> already a big patch. ;)

I still feel that at a minimum we need to check for and add "http://" at the start, before I give this a ui-r+.  (And I would also still like feed autodiscovery, but I would accept that in a new bug, albeit under duress.)

Thanks,
Blake.

Comment 14

[alta88]

i suggest rereading the random typing autodiscovery nuances enumerated above to better understand the issues in designing a proper feedback UI.  you have not anywhere described how each of those cases of autodiscovery should look, other than demanding it.

adding http:// is a silly hack that will not even resolve your own use case, unless it also guesses to postpend '/index.xml' as well.  finally, the "It's valid!" very obviously (really) applies to the url found by the validation page and not the random typing in the Dialog.

if you wish to block the fixes here by mandating an out of scope and ill designed additional feature that's your prerogative.

however, i sent you an email regarding the scope and design of this bug.  the time to say "you can't fix the treeview and popup unless you add autodiscovery" would have been then.

Comment 15

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 589541 [details] [diff] [review]
patch

Autodiscovery did seem like a larger feature, which is why I backed off on it in my previous comment.

If you're absolutely against fixing non-urls so they are urls in this bug, then I guess I have no choice but to take or reject the patch as is.  And in that case, this seems like an improvement, even if it's not as much of an improvement as I would like.  So ui-r=me, I guess.

Comment 16

[Ludovic Hirlimann [:Usul]]

Now that the ui is plussed it's time for review and feedback :-)

Comment 17

[David :Bienvenu]

When I run with this patch applied to the trunk, the feeds subscribe UI seems fairly broken, and there are a lot of errors about MailServices and Services not being defined.

Comment 18

[alta88]

it depends on the imports in bug 711173 ;)

Comment 19

[David :Bienvenu]

(In reply to alta88 from comment #18)
> it depends on the imports in bug 711173 ;)

ok, thx, I imported that, and the subscribe UI does show my subscribed feeds. But drag drop of a feed from Firefox onto the news&blogs line in the folder pane doesn't work (no feedback or error or anything), while the same drag drop works in the trunk.

While I was looking through the code, I noted that this will return undefined in the error case, not null - is that intentional or does it not matter?

+      let item;
+      if (currentSelectionIndex)
+        item = this.getItemAtIndex(currentSelectionIndex);
+
+      return item;
+

Comment 20

[alta88]

(In reply to David :Bienvenu from comment #19)
> (In reply to alta88 from comment #18)
> > it depends on the imports in bug 711173 ;)
> 
> ok, thx, I imported that, and the subscribe UI does show my subscribed
> feeds. But drag drop of a feed from Firefox onto the news&blogs line in the
> folder pane doesn't work (no feedback or error or anything), while the same
> drag drop works in the trunk.

actually, i should have said that the patch in bug 711173 should also be applied (not sure if you did that) to get the changes in utils.js part of which are the imports.

the subscribe that is done via a folder pane dnd doesn't use any code in the subscribe dialog, other than sharing utils.js but rather the subscribeToFeed() in the interface from newsblog.js, so this leads me to think the bug 711173 patch may not have applied.

also, there is a known (well to me) issue of a url dnd onto a folder pane feed account; the feed url string gets appended with garbage and depending on the url, it may succeed or fail.  this patch doesn't address folder pane.

if you copy/paste the same url in the subscribe dialog, it will likely work if valid.  if not, could you list the str so i can try to reproduce..

> 
> While I was looking through the code, I noted that this will return
> undefined in the error case, not null - is that intentional or does it not
> matter?
> 
> +      let item;
> +      if (currentSelectionIndex)
> +        item = this.getItemAtIndex(currentSelectionIndex);
> +
> +      return item;
> +

the return value from that getter is always checked, and since both null and undefined evaluate to false, i don't think it matters.

Comment 21

[alta88]

david, the folderpane dnd bug is Bug 517509, it's a one line patch if you could perhaps take a look.  (i guess there weren't enough managers on it back then ;)).

Comment 22

[David :Bienvenu]

Comment on attachment 589541 [details] [diff] [review]
patch

In general, I think this is an improvement over the existing subscribe UI, and I don't want to block it on getting drag drop to work correctly. I would like the issue with the top level container fixed, however, but other than that, r=me.

Comment 23

[alta88]

dnd onto any Subscribe dialog folder (root or other) wfm from:
http://sportsillustrated.cnn.com/services/rss/
https://addons.mozilla.org/en-US/thunderbird/extensions/?sort=featured
http://www.wired.com/about/rss_feeds/

if you can file a followup with str, i'll take a look.

Comment 24

[alta88]

the folderpane dnd issue is fixed in bug 517509.

Comment 25

[Mark Banner (:standard8)]

Unfortunately the patch doesn't apply cleanly, can you look at updating it please?

Comment 26

[alta88]

it depends on bug 711173, which must also be applied along with this patch.  if you had bug 711173 applied, then some more details are necessary.

Comment 27

[Mark Banner (:standard8)]

Comment on attachment 589541 [details] [diff] [review]
patch

Cancelling feedback request as it doesn't seem required now.

Comment 28

[Mark Banner (:standard8)]

Checked in: http://hg.mozilla.org/comm-central/rev/71e65bd6dbd5

Comment 29

[:aceman]

The dialog is fine now. Feeds are even downloading now (probably unrelated to this bug). But how do I activate the Import/Export buttons?