Fly by design review based on Marco's screenshots...

http://people.collabora.com/~bari/tmp-goa-empathy/accounts.png

 * Jabber accounts should use user-status-available-symbolic for the icon (since they are generic)
 * Maybe it should say XMPP and not Jabber?
 * Needs more (12px ?) padding above the Chat switch
 * "Use for" and "Chat" should be vertically aligned
 * "Edit Connection Parameters..." > "Connection Settings"
 * "Edit Personal Details..." > "Personal Details"

http://people.collabora.com/~bari/tmp-goa-empathy/edit-connection-parameters.png

 * It would be much better to have these fields 
 * "Edit Connection Parameters" > "Connection Settings"
 * Text fields should be wider (about 280px)
 * Add more padding above the password field (maybe just 6px)
 * "Remember password" > "Remember Password"
 * Do you really need the clear button in the password field? If yes, please use edit-clear-symbolic for the icon
 * I dread to think what's under Advanced Settings ;)
 * Needs extra padding (12px) below Advance Settings

http://people.collabora.com/~bari/tmp-goa-empathy/edit-personal-details.png

 * "Edit Personal Details" > "Personal Details"
 * Would benefit from an explanation at the top: "These details will be shared with other users on this chat network"
 * Use title case for field labels
 * Use edit-clear-symbolic for the clear button icon
 * Needs more padding above the Cancel/OK buttons - add 12px

http://people.collabora.com/~bari/tmp-goa-empathy/add-account1.png

 * I'm worried that we're overloading this list. Will this be revisited next cycle?

http://people.collabora.com/~bari/tmp-goa-empathy/add-account2.png

 * Why is the layout different from connection settings? Lots of alignment issues here - would be fixed by following the connections settings layout.
 * Odd phrasing in labels, eg. "What is your Jabber ID?"
 * "Log In" > "Add"

Just a few quick points.

(In reply to comment #1)
> http://people.collabora.com/~bari/tmp-goa-empathy/accounts.png
> 
>  * Jabber accounts should use user-status-available-symbolic for the icon
> (since they are generic)

But then most of the IM types will share the same icon.

>  * Needs more (12px ?) padding above the Chat switch

That comes directly from g-c-c. It looks better on my installed system than in the stuff I compile from jhbuild, not sure why (maybe some theme problem running stuff in jhbuild).

>  * "Use for" and "Chat" should be vertically aligned

Same as above.

> http://people.collabora.com/~bari/tmp-goa-empathy/add-account1.png
> 
>  * I'm worried that we're overloading this list. Will this be revisited next
> cycle?

At the moment we show all the protocols available from the installed CMs.
You proposed to have a white list (that I haven't implemented yet, but shouldn't take long).
What would you like this to look like exactly? Do you still think we need the white list? I'm a bit worried that distros will mess this up badly or just whitelist everything.

> http://people.collabora.com/~bari/tmp-goa-empathy/add-account2.png
> 
>  * Why is the layout different from connection settings? Lots of alignment
> issues here - would be fixed by following the connections settings layout.

The code creating the 2 dialogs is the same, but it's supposed to simplify the dialog when creating the account.

(In reply to comment #2)
> (In reply to comment #1)
> > http://people.collabora.com/~bari/tmp-goa-empathy/accounts.png
> > 
> >  * Jabber accounts should use user-status-available-symbolic for the icon
> > (since they are generic)
> 
> But then most of the IM types will share the same icon.

How many generic account types will there be?

> >  * Needs more (12px ?) padding above the Chat switch
> 
> That comes directly from g-c-c. It looks better on my installed system than in
> the stuff I compile from jhbuild, not sure why (maybe some theme problem
> running stuff in jhbuild).
>
> >  * "Use for" and "Chat" should be vertically aligned
> 
> Same as above.

Hmm, yes. These are misaligned for existing account types in GOA too. It's fine in my 3.8 install though.
 
> > http://people.collabora.com/~bari/tmp-goa-empathy/add-account1.png
> > 
> >  * I'm worried that we're overloading this list. Will this be revisited next
> > cycle?
> 
> At the moment we show all the protocols available from the installed CMs.
> You proposed to have a white list (that I haven't implemented yet, but
> shouldn't take long).
> What would you like this to look like exactly? Do you still think we need the
> white list? I'm a bit worried that distros will mess this up badly or just
> whitelist everything.

I don't remember the whitelist. My preferred solution was to have accounts be installable as "online account extensions" or something. That said, we don't have the other bits in place for that for 3.10, which is why I'm asking about revisiting it next release.

> > http://people.collabora.com/~bari/tmp-goa-empathy/add-account2.png
> > 
> >  * Why is the layout different from connection settings? Lots of alignment
> > issues here - would be fixed by following the connections settings layout.
> 
> The code creating the 2 dialogs is the same, but it's supposed to simplify the
> dialog when creating the account.

It's pretty inconsistent with what we do everywhere else... I don't think we ever phrase field labels as questions. Either way, there are layout issues with it that need to be fixed (I can detail those if you want to keep the current design, although I'd prefer to reuse the other connection settings dialog).

Created attachment 252521 [details] [review]
online-accounts: handle providers with unknown groups

Created attachment 252522 [details] [review]
online-accounts: handle unbranded chat providers

Created attachment 252523 [details] [review]
online-accounts: make the list boxes in "Other" expand vertically

Created attachment 252524 [details] [review]
online-accounts: use the async function to get all the providers

Review of attachment 252522 [details] [review]:

Looks good.

Review of attachment 252523 [details] [review]:

::: panels/online-accounts/cc-online-accounts-add-account-dialog.c
@@ +218,3 @@
+       * For now we just make list boxes with multiple children expand as
+       * the result is quite similar. */
+      GtkWidget *sw = gtk_widget_get_parent (GTK_WIDGET (list_box));

Could you please split the variable definition and function calls into separate statements?

Review of attachment 252524 [details] [review]:

Looks good.

Review of attachment 252521 [details] [review]:

I am not convinced that we need this.

::: panels/online-accounts/cc-online-accounts-add-account-dialog.c
@@ +457,3 @@
+       * This should not happen unless you have mismatched GOA and g-c-c,
+       * but it's better to show all the available providers than just
+       * silently ignore them. */

Mismatched GOA and g-c-c versions are not meant to be supported.

There are already too many moving pieces in the stack, and I would rather have it crash as early as possible so that testers and distributors can notice it easily, instead of failing later in subtle ways.

Second, we don't add new GoaProviderGroups that frequently, so, in reality, this isn't going to be a problem unless you are running wildly differing versions. If you are then see my previous point.

Even when we do add a new GoaProviderGroup, it would be during one of the unstable releases, and I expect those running them to unbreak their systems. Adding a new group to GOA implies the addition of a corresponding UI in g-c-c, so they should really be encouraged to use the right version.

Created attachment 252608 [details] [review]
online-accounts: make the list boxes in "Other" expand vertically

I removed the patch about the unknown groups and made the change you asked for in comment #9.

Review of attachment 252608 [details] [review]:

Looks good.

Review of attachment 252521 [details] [review]:

Setting it to rejected.

Created attachment 252751 [details] [review]
online-accounts: use the async function to get all the providers

Created attachment 252752 [details] [review]
configure.ac: bump GOA dependency to 3.9.90

Review of attachment 252751 [details] [review]:

Looks good.

Review of attachment 252752 [details] [review]:

Looks good.

Committed to master.
The top of the branch is the commit with ID 4ab5a5d9ad8eb4e17d3a19707844e1d04f184a7f.