Created attachment 320868 [details] [review]
connections: Set ConnectionDetails account after creation

Not having to pass the account at construction time makes it possible to
use ConnectionDetails directly in .ui files.

Created attachment 320869 [details] [review]
connections: Use template for properties dialog

We are about to move some UI from connection details into the properties
dialog, so this seems like a good moment to start using GtkBuilder.
Also templates are great.

Created attachment 320870 [details] [review]
connections: Move error information into properties dialog

The ConnectionDetails widget is used both for creating new connections
in the join dialog and for editing existing ones from the properties
dialog, but only in the latter case we may show the error information.
However it still imposes spacing on the details grid even when hidden,
which makes it harder to adapt the grid to the different parents in the
future - just move the error information into the dialog that uses it.

Created attachment 320871 [details] [review]
Add NetworksManager to manage a list of predefined networks

Picking a predefined entry instead of having to enter a hostname/IP and
possibly a port will be a big usability improvement for users. And for us,
knowing which network the users wants to connect to will open the door for
future improvements, like using encrypted connections where available or
having a list of fallback servers to try on connection failures.

The list itself is mostly irc-networks.xml from telepathy-account-widgets
converted to JSON (because gjs' XML story sucks) - there's a fair chance
that entries are as outdated as the GNOME ones were, so take it with a
grain of salt ...

Created attachment 320872 [details] [review]
connections: Add ConnectionsList

This is a widget that displays predefined networks as a scrollable
list and creates a new account with default parameters when an entry
is activated.

Created attachment 320873 [details] [review]
joinDialog: Overhaul connection page

Use the newly added ConnectionsList to allow users to create a new
connection with a single click on a predefined network. For cases
where the server is not included in the list or more control is
needed, the previous UI for creating a connection manually can
still be accessed through a "Custom Connection" toggle.

Created attachment 320874 [details] [review]
joinDialog: Allow filtering predefined connections

The list of connections is quite extensive, which makes it hard to
navigate. Address this by adding a filter entry which follows the
preferred search pattern: The searched text is split into words and
only items that match all terms are displayed, which allows to
quickly narrow down the number of items.

Created attachment 320875 [details] [review]
connections: Hide server settings for predefined networks

The whole point of providing a list of networks is so users needn't
bother with server addresses and ports. In addition, exposing those
settings blurs the line between predefined and custom connections,
which is problematic for features like using encrypted connections
where possible.

So just hide server settings for accounts created from a predefined
network - in the end, users who are unhappy with the picks we make
can still use a custom connection.

*** Bug 760036 has been marked as a duplicate of this bug. ***

Created attachment 321367 [details] [review]
connections: Hide server settings for predefined networks

Bastian encountered a problem where server settings were not hidden for predefined connections the first time the dialog was opened - the updated patch fixes that.

Review of attachment 320872 [details] [review]:

To my eye looks good.

::: src/connections.js
@@ +4,3 @@
 const Tp = imports.gi.TelepathyGLib;
 
+const AccountsMonitor = imports.accountsMonitor;

Styleguide suggests imports divided to environment and application blocks.

@@ +24,3 @@
+
+        this._id = params.id;
+        delete params.id;

This is probably just me being new around here, but why do you modify the argument passed in to this function?

@@ +134,3 @@
+            function(r, res) {
+                let account = req.create_account_finish(res);
+                if (account) // TODO: Handle errors

Could a minimal error handling be to just log something?

::: src/networksManager.js
@@ +74,3 @@
+            throw new Error('No servers for network ' + id);
+
+        let sslServers = network.servers.filter(function(s) { return s.ssl; });

I think arrow functions would be quite nice here and in other places, but it's a style question and not used elsewhere in the code base yet so you might or might not want to mix styles. In any case, the function could be (s) => s.ssl, or s => s.ssl.

@@ +81,3 @@
+            'server': new GLib.Variant('s', server.address),
+            'port': new GLib.Variant('u', server.port),
+            'use-ssl': new GLib.Variant('b', server.ssl),

Trailing comma is discouraged in the style guide

(In reply to Daniel Landau from comment #11)
> @@ +4,3 @@
>  const Tp = imports.gi.TelepathyGLib;
>  
> +const AccountsMonitor = imports.accountsMonitor;
> 
> Styleguide suggests imports divided to environment and application blocks.

Mmh yeah, could do a clean-up patch to move Lang and Signals to the right place ...
(Feel free to do that if you want; otherwise I'll try to remember that myself)


> @@ +24,3 @@
> +
> +        this._id = params.id;
> +        delete params.id;
> 
> This is probably just me being new around here, but why do you modify the
> argument passed in to this function?

Because we handle the params over to the parent constructor, and must not contain properties GtkListBoxRow doesn't know about. The alternative would be to pass the id separately, e.g.
  let row = new Row(foo, { bar: true, baz: 42 });


> @@ +134,3 @@
> +            function(r, res) {
> +                let account = req.create_account_finish(res);
> +                if (account) // TODO: Handle errors
> 
> Could a minimal error handling be to just log something?

Sure, but would it be useful? We can log("Failed to create new account"), but then the account already doesn't show up in the room list (which pretty much says the same without checking out the journal)


> ::: src/networksManager.js
> I think arrow functions would be quite nice here and in other places, but
> it's a style question and not used elsewhere in the code base yet

I agree that they are nice, and the patch in bug 761290 will be the first to start using them - so yeah, I like the syntax and we should use it more :-)


> Trailing comma is discouraged in the style guide

Fixed locally, thanks for the catch!

Created attachment 321553 [details] [review]
connections: Reorganize imports

Follow the style guide and organize imports to environment and
application blocks.

(In reply to Daniel Landau from comment #13)
> Follow the style guide and organize imports to environment and
> application blocks.

Eh, style is also to keep imports in each block in alphabetical order ;-)

Fixed up and pushed, thanks!


Attachment 320868 [details] pushed as 511f393 - connections: Set ConnectionDetails account after creation
Attachment 320869 [details] pushed as af07be4 - connections: Use template for properties dialog
Attachment 320870 [details] pushed as c335c0e - connections: Move error information into properties dialog
Attachment 320871 [details] pushed as 1e99137 - Add NetworksManager to manage a list of predefined networks
Attachment 320872 [details] pushed as edb3f6c - connections: Add ConnectionsList
Attachment 320873 [details] pushed as ec42c4c - joinDialog: Overhaul connection page
Attachment 320874 [details] pushed as 3698680 - joinDialog: Allow filtering predefined connections
Attachment 321367 [details] pushed as b1eec99 - connections: Hide server settings for predefined networks
Attachment 321553 [details] pushed as d2458cc - connections: Reorganize imports