Created attachment 323226 [details] [review]
joinDialog: removes the password entry

Created attachment 323228 [details] [review]
joinDialog: implements room list on join dialog

A list containing the rooms of a selected account should be available
to the user. This patch implements the rooms list which also enables
the functionality to select and join many rooms at once.

Created attachment 323229 [details] [review]
joinDialog: implements room list on join dialog

A list containing the rooms of a selected account should be available
to the user. This patch implements the rooms list which also enables
the functionality to select and join many rooms at once.

Review of attachment 323226 [details] [review]:

Code looks good, the commit message could do a bit better:

joinDialog: Remove the password entry

The password entry in the join dialog has never been hooked
up, and is now getting in the way of a list of existing rooms
we are about to add to make joining channels easier; just
remove it.

Review of attachment 323229 [details] [review]:

This is moving in the right direction, but will need a bit more work:
  - I think it makes sense to split out the list after all - while there's
    still a lot of functionality missing, it's already adding quite a bit
    to the joinDialog (which is already fairly complex, with nested stacks
    and whatnot)

  - we are clearly missing a loading indicator

  - the entry is currently really bad - it doesn't do filtering yet, but can no
    longer be used to directly join a channel either; I think in a first step,
    it makes sense to keep the entry as-is and only change it when filtering
    is implemented (including adding the current filter terms as row)

::: data/resources/join-room-dialog.ui
@@ +111,3 @@
+                    <property name="spacing">18</property>
+                    <child>
+                      <object class="GtkStack" id="roomStack">

Why a stack?

::: src/joinDialog.js
@@ +119,3 @@
         this._connectionCombo.sensitive = false;
 
+        this._roomFilterEntry.connect('changed',

The entry doesn't filter anything, so it doesn't make change to rename it (yet)

@@ +216,3 @@
+        let selectedRooms = this._selectedRooms;
+        selectedRooms.forEach(function(checkbox) {
+            let room = checkbox.label

This is too much magic. Apart from making assumptions about rows (there's a private _checkbox property that uses the room name as label), "selectedRooms" is a very confusing name for something that's not a list of selected rooms (or room names).

@@ +286,3 @@
+    },
+
+    _setupServerRoomList: function() {

"setup" is an odd name for something that is done more than once (i.e. every time the selected account changes)

@@ +299,3 @@
+            }));
+
+        this._serverRoomListId = this._serverRoomList.connect('got-room', 

You are saving a signal handler ID, not an object ID. This naming scheme will break the moment you add another signal handler (let's say 'notify::listing' to control a spinner while the list is loading ;-) ).

@@ +302,3 @@
+                                                               Lang.bind(this, this._onGotRoom));
+
+        this._serverRoomList.hscrollbar_policy = Gtk.PolicyType.NEVER;

TpRoomList doesn't have a hscrollbar-policy property - this is monkey-patching a random property that is never read, so doesn't have any effect

@@ +307,3 @@
+        this._roomList.set_sort_func(Lang.bind(this, this._sortRoomList));
+        this._roomList.connect('row-activated',
+                           Lang.bind(this, this._onRowActivated));

Indentation

@@ +308,3 @@
+        this._roomList.connect('row-activated',
+                           Lang.bind(this, this._onRowActivated));
+        this.add(this._roomList);

The list is already added via the .ui file - if 'this' wasn't a GtkBin that can only hold a single child, it would fail because the list already has a parent

@@ +311,3 @@
+    },
+
+    _destroyRoomList: function(){

'destroy' doesn't sound right - the room list isn't a widget. 'clear' maybe?

Also nit: space before brace.

@@ +312,3 @@
+
+    _destroyRoomList: function(){
+        if (this._serverRoomListId) {

_serverRoomListId is never unset

@@ +321,3 @@
+
+    _onGotRoom: function(roomList, roomInfo) {
+        let sensitive = true; //TODO check if channel is already added

We simply don't know yet how to handle already joined rooms yet, so the patch shouldn't make assumptions about sensitivity.

@@ +324,3 @@
+        let row = new ServerRoomRow({ name: roomInfo.get_name(),
+                                      sensitive: sensitive });
+        row._checkbox.connect('toggled', Lang.bind(this, this._onRowChecked));

Don't access private properties of other objects

@@ +329,3 @@
+
+    _onRowActivated: function(list, row) {
+        row._checkbox.active = !row._checkbox.active;

You should not access private properties of other objects; imho this would be better handled by the row itself:
  vfunc_activate: function() {
      this._checkbox.activate();
  }

@@ +330,3 @@
+    _onRowActivated: function(list, row) {
+        row._checkbox.active = !row._checkbox.active;
+        row._checkbox.emit('toggled');

The checkbox emits the signal when its state changes, you should *not* emit it manually

@@ +334,3 @@
+
+    _onRowChecked: function(checkbox) {
+        //TODO activate row

What is that TODO? You check the row when it is activated, so if you also activate it when checked, you end up with a circular dependency ...

@@ +339,3 @@
+
+    _sortRoomList: function(row1, row2) {
+        return row1._checkbox.label.localeCompare(row2._checkbox.label);

Accessing private properties again. Also: Allan suggested to sort by size instead of name.

@@ +342,3 @@
+    },
+
+    get _selectedRooms() {

We need to come up with something better here:
  - either use _selectedRooms, e.g. return a list of selected rooms
  - or use _selectedRows, e.g. return a list of selected rows

This currently doesn't do either, it returns a list of internal row widgets instead.

@@ +369,3 @@
+
+        this.bind_property('sensitive', this, 'activatable',
+                           GObject.BindingFlags.SYNC_CREATE);

This doesn't make sense unless there actually are insensitive rows

@@ +381,3 @@
+                                              hexpand: true,
+                                              halign: Gtk.Align.END });
+        box.add(insensitiveDesc);

Again, we haven't decided on this yet (and it's not working anyway) - better take this out for now.

@@ +386,3 @@
+        this.show_all();
+
+        this.bind_property('sensitive', insensitiveDesc, 'visible',

Dto.

Created attachment 323338 [details] [review]
joinDialog: Remove the password entry

The password entry in the join dialog has never been hooked
up, and is now getting in the way of a list of existing rooms
we are about to add to make joining channels easier; just
remove it.

Created attachment 323469 [details] [review]
joinDialog: implements room list on join dialog

A list containing the rooms of a selected account should be available
to the user. This patch implements the rooms list which also enables
the functionality to select and join many rooms at once.

Created attachment 323470 [details] [review]
joinDialog: implements room list on join dialog

A list containing the rooms of a selected account should be available
to the user. This patch implements the rooms list which also enables
the functionality to select and join many rooms at once.

Review of attachment 323338 [details] [review]:

OK

Review of attachment 323470 [details] [review]:

Patch doesn't apply

Created attachment 324853 [details] [review]
joinDialog: implements room list on join dialog

A list containing the rooms of a selected account should be available
to the user. This patch implements the rooms list which also enables
the functionality to select and join many rooms at once.

Created attachment 324857 [details] [review]
joinDialog: implements room list on join dialog

A list containing the rooms of a selected account should be available
to the user. This patch implements the rooms list which also enables
the functionality to select and join many rooms at once.

Hey, great to see you back :-)

So given that we missed 3.20, let's try to do it right instead of rushing something in. From trying the patch for a while, here are a couple of observations that I think we should address:

 - loading the list can be really slow,
   so we need some form of caching
 - loading the list for a big network
   (Freenode) blocks the process here :-(
 - no horizontal scrolling! ellipsize long
   room names instead
 - the spacing in the dialog is a bit off:
   IMHO we want to horizontally align the
   room list with the networks list
 - the filter entry needs to be hooked up
 - it's not really obvious that the typed
   text is taken as channel name as well;
   you should probably add it to the list
   (as long as it doesn't match an existing
    room of course)
 - enter/escape don't do anything - we'll have
   to figure out what they *should* do of
   course
 - we need to decide how to treat already
   joined rooms

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

Created attachment 345885 [details] [review]
serverRoomManager: Add class for caching room lists

Room lists tend to be mostly static and may take a while to load,
so it makes sense to cache them rather than loading them every
time a list is requested. For that purpose, add a small class to
manage and cache the room list of all active accounts.

Created attachment 345886 [details] [review]
serverRoomManager: Add ServerRoomList widget

Now that we have a room list cache, we want to present the lists in
the UI, so add a list widget that'll allow users to browse and select
existing rooms in the join dialog.

Created attachment 345887 [details] [review]
serverRoomManager: Load rooms in chunks

On big networks like Freenode, room lists are rather large, so loading
them can take a while. We don't want to keep blocking the UI thread for
longer periods of time, so split up the list into smaller chunks and
load them in an idle handler.

Created attachment 345888 [details] [review]
serverRoomManager: Handle already joined rooms

It doesn't make sense to allow users to select already joined rooms,
or to leave rooms by unchecking them. On the other hand, hiding rooms
that clearly exist would be weird as well, so represent joined rooms
as checked but insensitive.

Created attachment 345889 [details] [review]
serverRoomManager: Use a treeview instead of listbox

For potentially large lists, a treeview is significantly more performant
than a listbox with many composite row widgets. As the ServerRoomList
doesn't require anything that cannot be achieved with a treeview, drop
the listbox for a nice performance gain.

Created attachment 345890 [details] [review]
joinDialog: Remove the password entry

The password entry in the join dialog has never been hooked
up, and is now getting in the way of a list of existing rooms
we are about to add to make joining channels easier; just
remove it.

Created attachment 345891 [details] [review]
joinDialog: Include room list in join dialog

Currently users have to know exactly which rooms they want to join
and type in the exact name (either in the join dialog or as parameter
to the /join command). It is often more convenient to browse the list
of existing rooms and pick from that instead, so include that list
in the join dialog.

Attachment 345885 [details] pushed as ebd3c7c - serverRoomManager: Add class for caching room lists
Attachment 345886 [details] pushed as 84f96b2 - serverRoomManager: Add ServerRoomList widget
Attachment 345887 [details] pushed as 70f6be4 - serverRoomManager: Load rooms in chunks
Attachment 345888 [details] pushed as 9c1d4c3 - serverRoomManager: Handle already joined rooms
Attachment 345889 [details] pushed as 8ad6ecd - serverRoomManager: Use a treeview instead of listbox
Attachment 345890 [details] pushed as e676005 - joinDialog: Remove the password entry
Attachment 345891 [details] pushed as 4aec23a - joinDialog: Include room list in join dialog