Created attachment 290013 [details] [review]
userList: Convert to ui-files

Make UserList, UserListPopover and UserListRow use ui files to
construct their ui.

Created attachment 290014 [details] [review]
userList: Limit height of list popover

There is filtering and a scrollbar. There is no need to grow the list
indefinetly. This patch limits users shown to eight.

Review of attachment 290013 [details] [review]:

Not a big fan - adding an extra file to put a box in a popover (UserListPopover) is just overkill. UserList is not much more complex either, but I could see a point if it was ported to use templates[0]. For ListRow this makes most sense to me, but I would like to see it use templates as well.

[0] www.hadess.net/2014/09/gtk-widget-templates-now-in-javascript.html

::: data/resources/user-list-row.ui
@@ +25,3 @@
+              <object class="GtkArrow" id="user-list-row-arrow">
+                <property name="arrow_type">right</property>
+                <property name="no_show_all">True</property>

If you use UI files, I would prefer to add <property name="visible">True</property> to all the visible-by-default widgets and not stop calling show_all().

Review of attachment 290014 [details] [review]:

::: src/userList.js
@@ +345,3 @@
 
+        let rowHeight = this._getRowHeight();
+        this.widget.min_content_height = NUM_USERS_SHOWN * rowHeight;

MAX_USERS_SHOWN would be better in my opinion, e.g. use Math.min(MAX_USERS_SHOWN, numUsers) - it doesn't really make sense to reserve space for 8 users if we only have 5.

@@ +355,3 @@
+
+        let row = builder.get_object('user-list-row');
+        row.show_all();

Ugh, ugly. You need at the very least destroy the widget when no longer needed.
(You also don't take changes to the row height into account, for instance when the user changes themes)

Review of attachment 290013 [details] [review]:

Sure.

I can look into making them templates. We want the same in Maps.
Right now there is some sort of bug blocking this as one get:
(gjs-console:24118): Gtk-CRITICAL **: Unable to load resource for composite template for type 'Gjs_UserListRow': The resource at '/org/gnome/Polari/resources/user-list-row.ui' does not exist

When you try to add a template resource. Will get back to that when it has been resolved.

Review of attachment 290014 [details] [review]:

Thanks!

::: src/userList.js
@@ +345,3 @@
 
+        let rowHeight = this._getRowHeight();
+        this.widget.min_content_height = NUM_USERS_SHOWN * rowHeight;

Sure!

@@ +355,3 @@
+
+        let row = builder.get_object('user-list-row');
+        row.show_all();

Yeah, you are right. Updated version on its way.

Created attachment 290082 [details] [review]
userList: Limit height of list popover

There is filtering and scrollbar. There is no need to grow the lost
indefinetly. This patch limits users shown to eight.

Created attachment 290083 [details] [review]
userList: Fix filter entry revealing

Do not call _updateEntryVisibility on map since it will
always evaluate to reveal.

Review of attachment 290082 [details] [review]:

::: src/userList.js
@@ +390,3 @@
+
+        for (let i = 0; i < membersShown; i++)
+            height += this._list.get_row_at_index(i).get_preferred_height()[1];

Maybe should be get_allocated_height() ?

Review of attachment 290082 [details] [review]:

The handling of row expansion doesn't work currently (see detailed comment below). Also some typos in the commit message:
 - s/lost/list/
 - s/indefinetly/indefinitely/

::: src/userList.js
@@ +309,3 @@
             this._cancellable = null;
         }
+        this.emit('expand-notify');

I'm not sure we need to account for expansion - the previous patch always used to height of eight *unexpanded* rows, which seemed fine to me.
If we want to take expansion into account, this here is wrong: GtkRevealer:reveal-child means the child *should* be revealed (e.g. the animations is about to start, but nothing has been expanded yet). So this is currently exactly backwards, the popover starts with space for 8 rows, keeps its size when expanded, then grows when unexpanded; expanding it again shrinks the popover.
Using GtkRevealer:child-revealed instead should do what you intend, but the size change would still be jumpy (the row expands, then when it is fully expanded, the popover jumps to the new size). So if we want to grow the popover rather than just using the unexpaned size, I think we should get fancy and animate the size change. Something like this should work:
 - expose 'reveal-child' and 'child-revealed' somehow ('animation-start/end' signals,
   animating property ...)
 - connect to row::size-allocate on animation start and call _updateContentHeight from the handler
 - disconnect the signal on animation end

@@ +371,3 @@
 
+    get numRows() {
+        return Object.keys(this._rows).length;

Should this only count visible rows?

If not, there's some existing code in _onMembersChanged and _updateHeader that should be replaced with the new property.

Review of attachment 290083 [details] [review]:

This now just broken differently, no? With the patch, when switching from a room with <8 users to a room with >8, you have to open the popover *twice* to get the search entry. What do you think about just kicking that code out altogether? I think with the new height restriction, lists that don't require scrolling should be rare enough to just always show the filter entry ...

Review of attachment 290082 [details] [review]:

::: src/userList.js
@@ +309,3 @@
             this._cancellable = null;
         }
+        this.emit('expand-notify');

The reason I added it was that it felt annoying that when you click one of the users towards the bottom of the eight you would have to scroll to see the details.

Will look into fixing it up as you say.

@@ +371,3 @@
 
+    get numRows() {
+        return Object.keys(this._rows).length;

Sure those can change as well.

(In reply to comment #12)
> The reason I added it was that it felt annoying that when you click one of the
> users towards the bottom of the eight you would have to scroll to see the
> details.

Ah, good point. Another option here would be to adjust the scrolling so that the currently selected row is visible, but that's probably not any easier than growing the popup.

Something is really fishy, the child-revealed and the reveal-child signals are arriving in the wrong order for me.

I changed the code to this to test:

 this._revealer.connect('notify::child-revealed', function() {
            log('child-revealed');
 });

this._revealer.connect('notify::reveal-child', function() {
            log('reveal-child');
});

And get the output:
Gjs-Message: JS LOG: setting reveal-child to: true
Gjs-Message: JS LOG: child-revealed
Gjs-Message: JS LOG: reveal-child
Gjs-Message: JS LOG: child-revealed
Gjs-Message: JS LOG: reveal-child

Created attachment 290139 [details] [review]
userList: Limit height of list popover

There is filtering and scrollbar. There is no need to grow the list
indefinitely. This patch limits users shown to eight.

Created attachment 290140 [details] [review]
userList: Fix filter entry revealing

Use the numRows property to determine visibility of the filter entry.
This is more reliable than the allocated height.

So I didn't manage to get the animated thing to work. Partly I guess because I received the notifies in the wrong order, see above.

This version instead hooks in to the size-allocate signal of the listbox and when that changes we update the content height.

Not sure if that is the way. Can you reproduce the issue I see above?

(In reply to comment #5)
> Review of attachment 290013 [details] [review]:
> 
> Sure.
> 
> I can look into making them templates. We want the same in Maps.
> Right now there is some sort of bug blocking this as one get:
> (gjs-console:24118): Gtk-CRITICAL **: Unable to load resource for composite
> template for type 'Gjs_UserListRow': The resource at
> '/org/gnome/Polari/resources/user-list-row.ui' does not exist
> 
> When you try to add a template resource. Will get back to that when it has been
> resolved.

Bug here:
https://bugzilla.gnome.org/show_bug.cgi?id=739739

Created attachment 290160 [details] [review]
userList: Fix filter entry revealing

Use the UserList numRows property to determine visibility of the
filter entry. This is more reliable than the allocated height.

Review of attachment 290139 [details] [review]:

This is getting really close now, thanks for working on this!

::: src/userList.js
@@ +387,3 @@
 
+    _updateContentHeight: function() {
+        Mainloop.idle_add(Lang.bind(this, function() {

There are two issues here:
I figure you are doing this to rate-limit min_content_height updates when
receiving many updates in a row (read: GtkWidget::size-allocate signals during
animation). However this does not work properly, as you are *still* running
this once per received signal. You can use the returned source id to check
whether you already queued an update and bail out early (just don't forget to
reset the property in the idle handler!).
Then you should also add an explicit "return GLib.SOURCE_REMOVE" at the end of the
handler - no return value is falsey in JS, so omitting it works (unlike in C,
where it becomes a question of luck whether the source is stopped or not!).
It is bad style nonetheless.

Review of attachment 290160 [details] [review]:

Oh right, now that we have a limit on the number of rows (and know it), we can just use that. I like it!

Created attachment 290303 [details] [review]
userList: Limit height of list popover

There is filtering and scrollbar. There is no need to grow the list
indefinitely. This patch limits users shown to eight.

Created attachment 290304 [details] [review]
userList: Limit height of list popover

There is filtering and scrollbar. There is no need to grow the list
indefinitely. This patch limits users shown to eight.

Review of attachment 290304 [details] [review]:

Looks good to me.

Created attachment 291631 [details] [review]
userList: Use visible rows for content height computation

Currently the content height is calculated based on the top eight
rows, which means that the popover will only adjust its size when
expanding one of the top rows. Base the computation on the visible
rows instead, to get consistent behavior for the whole list.

Comment on attachment 291631 [details] [review]
userList: Use visible rows for content height computation

Attachment 291631 [details] pushed as 1e17a7b - userList: Use visible rows for content height computation