Created attachment 325277 [details] [review]
networksManager + connections: Networks can now be prioritized

Added the 'prioritized' property to the network objects located in data/resources/networks.json and added a sorting function to the connections list so that prioritized networks are shown on top. The json objects can either have a 'prioritized' property set to true/false, or this property can be absent, in which case its value defaults to false.

I have some small coding style issues. I will fix them in the next patch :)

Created attachment 325331 [details] [review]
networksManager + connections: Networks can now be prioritized

Added the 'prioritized' property to the network objects located in data/resources/networks.json and added a sorting function to the connections list so that prioritized networks are shown on top. The json objects can either have a 'prioritized' property set to true/false, or this property can be absent, in which case its value defaults to false.

Review of attachment 325331 [details] [review]:

We need to actually come up with a list of prioritized networks (as you mentioned yourself on IRC). Regarding the commit message:
  - only use a single prefix for the subject - either "connections" (to point to the main module),
    "connectionsList" (to point to the actual component that holds the main change) or "data" (to
    stress the changes to networks.json instead) are fine
  - git doesn't do automatic line-wrapping, so you need to add manual line breaks to the body
  - use present tense ("Add a 'prioritized' property ...")
  - try to focus on the reason for the change more than the change itself - it's fairly clear
    from reading the code that there's a 'prioritized' property that is used for sorting, but
    it's not immediately obvious *why* some networks should appear before others

::: src/connections.js
@@ +71,3 @@
         this._list.connect('row-activated',
                            Lang.bind(this, this._onRowActivated));
+

Gratuitous whitespace change

@@ +173,3 @@
+
+    _sort: function(row1, row2) {
+        let hasPriority1 = this._networksManager.getNetworkPriority(row1._id);

Don't access private properties from outside the class itself - should be row1.id instead.

@@ +178,3 @@
+        if (hasPriority1 != hasPriority2) {
+            return hasPriority1 ? -1 : 1;
+        }

Style nit: no braces

@@ +183,3 @@
+        let name2 = this._networksManager.getNetworkName(row2._id);
+
+        return name1.localeCompare(name2);

OK as-is, but if you want, you can split this out in a separate patch:

  connections: Sort networks list

  We currently list networks in the same order they appear in the data, which
  happens to be sorted alphabetically. Stop relying on that and sort the list
  explicitly by name instead.

::: src/networksManager.js
@@ +69,3 @@
     },
 
+    getNetworkPriority: function(id) {

"priority" doesn't suggest a boolean. If we stick with "prioritized", getNetworkIsPrioritized() is a better name.

@@ +73,3 @@
+
+        if(network.hasOwnProperty("prioritized")) {
+            return network['prioritized'];

I don't like 'prioritized' too much - maybe "important" or "favorite"?

@@ +87,3 @@
         let server = sslServers.length > 0 ? sslServers[0]
                                            : network.servers[0];
+

Gratuitous whitespace change

Created attachment 325667 [details] [review]
networksManager: Add 'favorite' property to the networks

Using the 'favorite' property enables us to flag specific networks as
important. So far Polari displays networks alphabetically, but having
this property will make it possible for us to implement network
prioritization

Created attachment 325668 [details] [review]
connections: Sort networks by importance and alphabetically

Polari displays networks as they appear in the networks.json file.
In order for the favorite networks to appear first in the list, a
sorting function is necessary. Important networks now appear first,
ordered alphabetically, then all other networks are displayed
alphabetically.

Review of attachment 325667 [details] [review]:

The code of the two patches looks good to me, but the split is a bit odd. IMHO, the two main things the patches do are:
 - sort the list explicitly instead of relying
   on the order of networks in the data
 - prioritize "important" networks

As said before, it would be fine to use a single commit as before, but with the above split, we can provide separate answers for:
  - why we add a sort() function instead of reordering the networks
  - why we want to prioritize some networks

Created attachment 326045 [details] [review]
connections: Sort networks by importance and alphabetically

Polari displays networks as they appear in the networks.json file.
This order is unreliable, therefore we cannot always count on the
fact that the networks are already ordered alphabetically. Moreover
we want to be able to dynamicaly select favorite networks, rather
than reordering the json file every time a new network becomes
a favorite one. The same applies for adding a new network to the
list.

A sorting function will fix this issue. The favorite networks are
displayed on the top of the list in order to make it easier for
new users to quickly setup their connections.

(In reply to Florian Müllner from comment #7)
> Review of attachment 325667 [details] [review] [review]:
> 
> The code of the two patches looks good to me, but the split is a bit odd.
> IMHO, the two main things the patches do are:
>  - sort the list explicitly instead of relying
>    on the order of networks in the data
>  - prioritize "important" networks
> 
> As said before, it would be fine to use a single commit as before, but with
> the above split, we can provide separate answers for:
>   - why we add a sort() function instead of reordering the networks
>   - why we want to prioritize some networks

Questions answered!

(In reply to Rares Visalom from comment #9)
> Questions answered!

Sorry, but what I was saying was: Either address those questions in a squashed patch, or make the split match the questions.

Created attachment 326098 [details] [review]
connections: Sort the connections list alphabetically

At the moment, Polari displays the networks in the exact order
that they appear in the networks.json file.

We cannot always rely on the fact that those networks will
be sorted, so we need to sort them using a function.
That way, we don't need to reorder the file every time we add
a new network to it.

Created attachment 326099 [details] [review]
networksManager: Add "favorite" property to the networks and sort them accordingly

Using the "favorite" property enables us to flag specific
networks as important. We want this property because we need
to display favorite networks first in the connetions list,
so that new users could be aided in the process of setting up
their first connection.

Moreover, the sorting function implemented in patch 9dcb30e
is updated in order to consider the "favorite" property in
the sorting process.

Review of attachment 326098 [details] [review]:

OK as-is, but I'll add some random thoughts on the commit message anyway (feel free to ignore):

 - the subject isn't wrong, but it sounds like the list
   wasn't sorted alphabetically before; something like
   "connections: Explicitly sort connections list" is slightly
   clearer (and maybe use "networks" instead of "connections list"
   to avoid word repetition)

 - "we cannot rely" isn't quite correct, as we are in control
   of the data - similar, we don't need to reorder the file when
   making additions, but be careful where we add new entries. My
   suggestion:

   "At the moment, Polari displays the networks in the exact order
    as they appear in the networks.json file. As a result, any future
    change to the list may break the order if not done carefully.
    Avoid that risk by setting an appropriate sort function."

Review of attachment 326099 [details] [review]:

Some nits with the commit message:
 - the summary is too long
   (see the recommendations in [0]); I'd suggest
   a simple "networksManager: Prioritize some networks"

 - the commit hash in the last paragraph refers to the
   commit in your local tree, so you would need to update
   the commit message after pushing the first patch;
   you can avoid this by saying "last patch" instead, though
   in this case I'd just drop the whole sentence: "Change the
   sort function to change the ordering" is kinda obvious ;-)

 - the commit message is still a bit vague - what this is fixing
   is that the list is unwieldily long, so it makes sense to put
   the most likely picks on top to not overwhelm the user

 - optionally, you could mention that the current designs for
   initial setup sport a very reduced list of networks, so this
   will be useful there as well

[0] https://wiki.gnome.org/Git/CommitMessages

::: src/networksManager.js
@@ +69,3 @@
     },
 
+    getIsFavorite: function(id) {

The other functions have "Network" in the name to make it clear that the function doesn't refer to the manager object (i.e. "getNetworkName()" instead of "getName()"), so "getNetworkIsFavorite" would be more consistent.

@@ +72,3 @@
+        let network = this._lookupNetwork(id);
+
+        if(network.hasOwnProperty("favorite")) {

Nits: Space after if, no braces

Created attachment 326190 [details] [review]
connections: Sort the connections list alphabetically

At the moment, Polari displays the networks in the exact order
that they appear in the networks.json file.

We cannot always rely on the fact that those networks will
be sorted, so we need to sort them using a function.
That way, we don't need to reorder the file every time we add
a new network to it.

Created attachment 326191 [details] [review]
networksManager: Prioritize favorite networks

Using the "favorite" property enables us to flag specific
networks as important. We want this property because the
networks list is long and naturally, we want to put the
favorite ones on top of the list. Furthermore, the current
designs for initial setup use a reduced list of networks,
thus prioritization would make it easier to display the new
list.

Moreover, the sorting function is changed in order to account
for the favorite proprty first, and then the alphabetic order.

I still have to edit the first commit message. I'll do that right away

Created attachment 326192 [details] [review]
connections: Explicitly sort connections list alphabetically

At the moment, Polari displays the networks in the exact order
that they appear in the networks.json file. As a result, any
future change to the list may break the order if not done
carefully. Avoid that risk by setting an appropriate
sort function.

Done

Review of attachment 326192 [details] [review]:

LGTM

Review of attachment 326191 [details] [review]:

One more style nit - just fix that up before pushing, no need for another review round

::: src/networksManager.js
@@ +72,3 @@
+        let network = this._lookupNetwork(id);
+
+        if (network.hasOwnProperty("favorite"))

Sorry I overlooked that earlier:
By convention, double quotes are used for translatable string, single quotes otherwise.

Created attachment 326209 [details] [review]
networksManager: Prioritize favorite networks

Using the "favorite" property enables us to flag specific
networks as important. We want this property because the
networks list is long and naturally, we want to put the
favorite ones on top of the list. Furthermore, the current
designs for initial setup use a reduced list of networks,
thus prioritization would make it easier to display the new
list.

Moreover, the sorting function is changed in order to account
for the favorite proprty first, and then the alphabetic order.

Fixed

Review of attachment 326209 [details] [review]:

Gah, I said no more review, push it already :-p

I don't think i have rights for that :D

Oh, OK. I'll push the patches when I get home later then.

Meanwhile, feel free to ask for git access[0].

[0] https://wiki.gnome.org/AccountsTeam/NewAccounts

On it!

Attachment 326192 [details] pushed as c73741c - connections: Explicitly sort connections list alphabetically
Attachment 326209 [details] pushed as 3bea70e - networksManager: Prioritize favorite networks