796234 - Renovate feeds account management issues

Status: RESOLVED WORKSFORME

(MailNews Core :: Account Manager, defect)

Description

[alta88]

1. remove feeds from the 'old' account manager (bug 732109), and give it its own very simple wizard.
2. use the FeedUtils method to create a feeds account, already used for commandline and import subscribes with no account yet.
3. add Feeds menuitem in Account Manager - Account Actions, and File - New.  this means News would be the only 'other' account and should get its own too.
4. fix bug 529461 and bug 780110.

Comment 1

[:aceman]

Do you have confirmation from bwinton this will be accepted?

Comment 2

[alta88]

Attachment: non working wip

so in addition to the 2 menuitem changes, the feeds wizard would be a 2 page affair, with the first page looking like the 2nd page of the current wizard if blogs radio is chosen, and it may have the summary or biff minutes prefs available.  the second page would be confirm/finish.

this will allow chipping away at bug 732109, have one (fixed up and simple) codepath to feed account creation, possibly allow removal of the rdf backing file - and more.

Comment 3

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

(In reply to :aceman from comment #1)
> Do you have confirmation from bwinton this will be accepted?

He does now.  ;)

(Killing the old account manager does seem to be a good idea, for the reasons presented.  One change I would suggest is to make "File » New » Other Accounts" into a popup list, and perhaps put "Chat Account" in there, along with RSS and News.)

Thanks,
Blake.

Comment 4

[alta88]

(In reply to Blake Winton (:bwinton - Thunderbird UX) from comment #3)
> One change I would suggest is to make "File » New »
> Other Accounts" into a popup list, and perhaps put "Chat Account" in there,
> along with RSS and News.)

do you feel strongly about this?  isn't there a rule about number of flyouts before it's too many..  i don't think it will kill the appmenu either as it just increases by 1.

but moreso, it seems that this minimizes non mail functions and reduces their discoverability.  account central 'Create a new account' being mail only is actually a lot worse.

i lean toward flatter but it's up to you.

Comment 5

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

Not too strongly, no.

Comment 6

[alta88]

Attachment: patch

bwinton for /mail and ui
rkent for /mailnews
neil for /suite

much of the patch is to make the suite account manager work while also enabling rss.rdf removal and bug 732109 for Tb.

so for Tb, newsgroups are the only item left in Account Manager.

Comment 7

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

(Disclaimer: I haven't tested the patch yet.)

> so for Tb, newsgroups are the only item left in Account Manager.

If this is true, can we skip the Account Manager step (in Thunderbird), and just let people create a newsgroup account directly from the menu?

Thanks,
Blake.

Comment 8

[alta88]

(In reply to Blake Winton (:bwinton - Thunderbird UX) from comment #7)
> (Disclaimer: I haven't tested the patch yet.)
> 
> > so for Tb, newsgroups are the only item left in Account Manager.
> 
> If this is true, can we skip the Account Manager step (in Thunderbird), and
> just let people create a newsgroup account directly from the menu?
> 
> Thanks,
> Blake.

yes, but it's a new bug.  it's conceivable a lot of 'old' Account Manager could then be reduced (for Tb) if newsgroups had their own specialized wizard, or reuse the 'new' mail wizard in ways.

Comment 9

[Joshua Cranmer [:jcranmer]]

(In reply to alta88 from comment #6)
> so for Tb, newsgroups are the only item left in Account Manager.

No, you're forgetting movemail and other potential ISP data users.

Comment 10

[Joshua Cranmer [:jcranmer]]

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

I haven't looked at the patch in great detail, but as I mentioned in the previous comment, ISP data stuff still lurks in the account dialog, including movemail on Unix systems, so calling it "newsgroups" makes the menu item a lie.

Comment 11

[alta88]

(In reply to Joshua Cranmer [:jcranmer] from comment #9)
> (In reply to alta88 from comment #6)
> > so for Tb, newsgroups are the only item left in Account Manager.
> 
> No, you're forgetting movemail and other potential ISP data users.

i should correct the terminology, it's the Account Wizard rather than the Account Manager.

those are then overlaid on the main wizard page, as special config cases, rather than hardcoded.  so many/most will not see them.

so in the case of only showing newsgroups on the first wizard page, blake is right that it makes sense to advance the page.  how to square this with additional items is beyond this bug.

Comment 12

[alta88]

(In reply to Joshua Cranmer [:jcranmer] from comment #10)
> Comment on attachment 667998 [details] [diff] [review]
> patch
> 
> I haven't looked at the patch in great detail, but as I mentioned in the
> previous comment, ISP data stuff still lurks in the account dialog,
> including movemail on Unix systems, so calling it "newsgroups" makes the
> menu item a lie.

then the next patch will go back to calling it 'other accounts'.

Comment 13

[[e-mail]]

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

SeaMonkey still uses the supposed "old" account wizard for all of its accounts.

Comment 14

[alta88]

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

you've misunderstood.  this patch takes that into account.  it both adds a Tb wizard and fixes the code for the existing Sm wizard.  there is no change in ui or usage for Sm.

Comment 15

[alta88]

Attachment: patch

revert to 'Other Accounts' string.

suite r? for newsblog strings change.

Comment 16

[Kent James (:rkent)]

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

Technically I am not a /mail reviewer but a /mailnews reviewer as Neil is. I could probably do the review anyway, but let's be official here and let Magnus try it.

Also, the allmailnews account is not intended as an account for reviews, the [e-mail] account (search for :rkent) is the one to use.

Comment 17

[alta88]

right, your request is for /mailnews, bwinton is for /mail.  magnus is only for /mail (as i've tried to get him to do mailnews/ in the past..).

neil seems to have a full plate with /suite so i usually just ask him for that rather than /mailnews.

Comment 18

[alta88]

Attachment: patch

updated patch.

Comment 19

[alta88]

Attachment: patch

the real update..

Comment 20

[Mike Conley (:mconley) (:⚙️)]

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

Stealing this review...

Comment 21

[[e-mail]]

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

>+  if ("selectServer" in window.opener)
>+    // Opened from Account Manager.
>+    window.opener.setTimeout(window.opener.selectServer, 20, gCurrentAccount.incomingServer);
>+  else if ("gFolderTreeView" in window.opener)
>+    // Opened from 3pane File->New or appbutton New Message menuitem.
>+    window.opener.gFolderTreeView.selectFolder(gCurrentAccount.incomingServer.rootMsgFolder);
Not really part of this bug? Also, code duplication sucks.

>+  <!ENTITY % newsblogDTD SYSTEM "chrome://messenger-newsblog/locale/am-newsblog.dtd" >
You're mixing account wizard and account manager files...

>+        <radio id="feedaccount" value="feedaccount"
>+               label="&feeds.accountName;" accesskey="&feeds.accountName.accesskey;"/>
So, why can't SeaMonkey keep using rss.rdf?

>         if (gCurrentAccountData && gCurrentAccountData.wizardAccountName)
>             accountName = gCurrentAccountData.wizardAccountName;
>         else if (type == "nntp") 
>             accountName = pageData.newsserver.hostname.value;
>+        else if (pageData.accounttype.feedaccount && pageData.accounttype.feedaccount.value)
>+            accountName = document.getElementById("feedaccount").label;
>         else
>             accountName = pageData.identity.email.value;
Why isn't the feed account name in page data?

>+  // Initialize if changing account types.
>+  document.getElementById("prettyName").value = "";
>+  gPageData = null;
What "bug" does this solve?

>+        setDivTextFromForm("server.port", null);
Nice fix.

>+/* Feed account standalone wizard functions */
This belongs in feedAccountWizard.js, not a random account manager file...

>diff --git a/suite/locales/en-US/chrome/mailnews/newsblog/am-newsblog.dtd b/suite/locales/en-US/chrome/mailnews/newsblog/am-newsblog.dtd
This change was impossible to review because of the reformatting.

Comment 22

[alta88]

(In reply to [e-mail] from comment #21)
> Comment on attachment 668645 [details] [diff] [review]
> patch
> 
> >+        <radio id="feedaccount" value="feedaccount"
> >+               label="&feeds.accountName;" accesskey="&feeds.accountName.accesskey;"/>
> So, why can't SeaMonkey keep using rss.rdf?

a major goal of this patch is to remove rss.rdf  i'm not going to spend time figuring out whether your comment is an objection or what.  if you don't see why every opportunity should be taken to remove rdf dependency from the codebase, then close this WONTFIX.

Comment 23

[Mike Conley (:mconley) (:⚙️)]

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

Hm - so, this patch doesn't apply cleanly. I cleaned it up manually remotely, but it also seems that feedAccountWizard.xul wasn't added to the jar.mn of mailnews/extensions/newsblog.

So I added that myself, locally. That seemed to get it working.

I agree with Blake that we're starting to accrue a large number of account types, and it'd be nice to group the less common ones (chat, feeds, newsgroups) into an "Other Accounts" submenu. But we can leave that for another day, I guess.

My only other complaint is that the wizard seems pretty heavy-handed for what we're doing here. Can we not just have a single-pane wizard, whereupon after entering the account name, we just hit Finish and are done?  The second pane seems pretty superfluous (and the "congratulations" at the top a bit obnoxious for my taste).

So that's what I suggest - make it a single pane wizard (not much of a wizard, I guess).

Comment 24

[Mark Banner (:standard8)]

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

Cancelling review requests given the comments in comment 21 and comment 23 as this would need a new patch.

Comment 25

[Mark Banner (:standard8)]

(In reply to alta88 from comment #22)
> (In reply to [e-mail] from comment #21)
> > Comment on attachment 668645 [details] [diff] [review]
> > patch
> > 
> > >+        <radio id="feedaccount" value="feedaccount"
> > >+               label="&feeds.accountName;" accesskey="&feeds.accountName.accesskey;"/>
> > So, why can't SeaMonkey keep using rss.rdf?
> 
> a major goal of this patch is to remove rss.rdf  i'm not going to spend time
> figuring out whether your comment is an objection or what.  if you don't see
> why every opportunity should be taken to remove rdf dependency from the
> codebase, then close this WONTFIX.

If SeaMonkey wants to keep rss.rdf, then I suggest that they branch the code into suite/ and let us move forward (or move the code into suite and we'll have our code in mail) - whilst RDF "works", I'm sure there are better ways to implement what we really need.

Comment 26

[Mark Banner (:standard8)]

This isn't a good first bug, but I'd be willing to help out with some ideas / changes to move this forward.

Comment 27

[:aceman]

I think alta88 knows what to do as long as he is allowed to remove the rdf from the TB version. I think he would be willing to continue with the patch in that case.

Comment 28

[alta88]

fixed in bug 529131 for Tb.