Created attachment 319618 [details] [review]
roomList: Adds at the connection header a popover with connection status and actions

Review of attachment 319618 [details] [review]:

Thanks for the update! It's looking pretty good, but as it's the first version, there are tons of comments - don't let that discourage you.

Before jumping into details, some general observations:
 - several places add trailing whitespace - by default, "git log -p" and similar
   highlight that in red, so it should be easy to catch
 - some coding style violations (indentation, missing spaces) - I pointed some out
   below, but it's always a good idea to double-check because I may have overlooked
   some
 - I have one mockup with "Properties" and another one with "Preferences" - would be
   good to ask for clarification there
 - "Reconnect" not doing anything when already connected is confusing. This wasn't
   an issue before as it was only used in the error case, but now should either
   make it do something (disconnect + reconnect) or disable it when it would do nothing
   (in any case it's a separate patch, I'm just mentioning it to not forget it later)
 - the commit message could do a lot better:
   - on a formal level, no line should be longer than ~75 characters - git doesn't
     do automatic line wrapping, so newlines need to be added manually
   - in the content, there's way too much detail about what the patch does - which is
     already in the code itself - and none at all about the reasoning (which can *not*
     be easily understood by just reading the code)
     I've come up with this:

     roomList: Use header popover for connection management
    
     We want to move away from a separate connection editor in favor of more direct
     connection management. Creating new connections directly from the join dialog
     has been possible for a while now, and the room list's headers provide a good
     place for exposing connection editing/removal, so rework the existing error
     popover to be usable for general connection management in non-error cases
     as well.

::: data/resources/application.css
@@ +54,3 @@
+    font-size: smaller;
+    border-bottom: 1px solid alpha(@borders, 0.55);
+    padding: 0px 0px 6px;

I really don't like this - if we want to separate children from each other, we generally use a GtkSeparator. Doing the same as other popovers makes it much easier to be consistent (including adding 6px bottom/top margins to the separator).

Also the small font-size doesn't work well with the bold error label - the mockup uses the normal font style there, which works much better IMHO. So this class should only be applied for the actual status (i.e. at the same time as 'dim-label'). Yet another thing that's easier with a separator :-)

(Personally I'd also use markup for sup/bold instead of adding/removing style classes, but that's just a style preference - both options should work out fine)

::: data/resources/room-list-header.ui
@@ +2,3 @@
 <interface>
   <template class="Gjs_RoomListHeader" parent="GtkMenuButton">
+    <property name="popover">connectionPopover</property>

I think the header is too heavy now - it used to be grayed out most of the time (because it was insensitive except for the error case), now it really pops out. I'd add 'dim-label' to the style classes below.

@@ +70,1 @@
         <property name="spacing">3</property>

The spacing is now inconsistent with other popover menus (e.g. those created with new_from_model()). I don't think the new design needs it, so should be fine to remove.

@@ +71,3 @@
         <property name="visible">True</property>
         <child>
+          <object class="GtkLabel" id="popoverServername">

The name is a bit awkward in the error case where the label is used for something completely different. "popoverTitle" maybe?

@@ +74,2 @@
             <property name="wrap">True</property>
             <property name="max-width-chars">30</property>

With the error popover, we only had to worry about the maximum width. Now I think we need to set a minimum size as well, in case of short server names (see the "irc.gnome.org (irg.gnome.org)" example later). Something like
  <property name="width-chars">15</property>

::: src/roomList.js
@@ +181,2 @@
         this.parent(params);
+        this._connectionPopover.relative_to = this;

This is already the default for GtkMenuButton, so you don't need this (it's also the only use of _connectionPopover, so you don't need that either)

@@ +184,1 @@
+        this._popoverReconnect.action_target = new GLib.Variant('o', this._account.get_object_path());

You can share the variant:
let target = new GLib.Variant('o', this._account.get_object_path());
this._popoverReconnect.action_target = target;
this._popoverRemove.action_target = target;

@@ +232,3 @@
+        
+        this._updateConnectionPopover();
+        this.sensitive = true;

Widgets are sensitive by default

@@ +237,3 @@
+    },
+    
+    _updateConnectionPopover: function(){

Mmmh, I think I see where this restructuring is coming from - messing around with the popover from a function called updateConnectionStatusIcon() is weird. But then you're still doing that now via updateConnectionPopover(). After commenting on the remaining patch, I will suggest a slightly different split that I think is a bit more readable.

(Also style nit - space before braces!)

@@ +238,3 @@
+    
+    _updateConnectionPopover: function(){
+        let servername = this._account.dup_parameters_vardict().deep_unpack().server.deep_unpack();

IMHO splitting this into two lines is more readable:
  let params = this._account.dup_parameters_vardict().deep_unpack();
  let servername = params['server'].deep_unpack();

@@ +239,3 @@
+    _updateConnectionPopover: function(){
+        let servername = this._account.dup_parameters_vardict().deep_unpack().server.deep_unpack();
+        let defaultLabel =  this._label.label + ' (' + servername + ')';

this._label.label is not very descriptive - please use this._account.display_name instead.

Also: We re-use the server as account name when no description has been set[0], in which case we get something like "irc.gnome.org (irc.gnome.org)" - it makes sense to only use the account name in that case.

[0] https://git.gnome.org/browse/polari/tree/src/connections.js#n255

@@ +242,3 @@
+        this._popoverServername.get_style_context().remove_class('connection-popover-error-label');
+        this._popoverStatus.get_style_context().add_class('dim-label');
+        switch (this._account.connection_status){

Space before braces

@@ +244,3 @@
+        switch (this._account.connection_status){
+            case Tp.ConnectionStatus.CONNECTED: {
+                this._popoverServername.label = _(defaultLabel);

Both account name and servername aren't translatable (neither we nor the translators can know what values the users will use when they configure polari). What we can do is allow translators to adapt the format (swap servername and account name, use '[' instead of '(' or whatever):

/* Translators: this is an account name followed by a server address,
   e.g. "GNOME (irc.gnome.org)" */
let fullTitle = _("%s (%s)").format(accountName, serverName);
let defaultLabel = (accountName == server) ? accountName : fullTitle;

@@ +260,3 @@
+                } else{
+                    this._popoverServername.label = _("Offline");
+                    this._popoverStatus.label = _("Could not connect to " + this._label.label);

Those are wrong - "Offline" should be the status label, and the servername the defaultLabel.

@@ +272,2 @@
                 case Tp.error_get_dbus_name(Tp.Error.CONNECTION_REFUSED):
                 case Tp.error_get_dbus_name(Tp.Error.NETWORK_ERROR): {

The indentation no longer fits the surrounding code

@@ +273,3 @@
                 case Tp.error_get_dbus_name(Tp.Error.NETWORK_ERROR): {
+                    this._popoverServername.label = _("Please check your connection details.");
+                    this._popoverStatus.label = _("Could not connect to " + this._label.label);

I don't think this is working very well - IMHO the bold title should be short and concise, then the status label can provide some details. The default case looks good to me, in the other cases I'd use "Connection Problem" as title and your servername label for the status label.

@@ +291,3 @@
                 case Tp.error_get_dbus_name(Tp.Error.CERT_SELF_SIGNED): {
+                    this._popoverServername.label = _("Could not make connection in a safe way."); 
+                    this._popoverStatus.label = _("Could not connect to " + this._label.label);

This doesn't work. If the account name is "GNOME", then we will look for a translation for "Could not connect to GNOME". In other words: translators would need to translate that string for all possible account names, which obviously doesn't work :-)

This would kind of work:
  _("Could not connect to ") + this._account.display_name;

Except if in some language the phrase needs to be structured differently, for instance "Could not connect to the GNOME server" - with the above we cannot do that and would get "Could not connect to the server GNOME".

So this is what you should use:
 _("Could not connect to %s").format(this._account.display_name);

@@ -288,3 @@
-        this.sensitive = isError;
-        this._iconStack.visible_child_name = child;
-        this._spinner.active = (child == 'connecting');

So as promised, here's my suggestion for an alternative code split:

  - rename _updateConnectionStatusIcon() to _onConnectionStatusChanged()
    (it does more than setting the icon after all)

  - at the end of the method, append something like:

        this._popoverTitle.use_markup = isError;
        this._popoverStatus.use_markup = !isError;

        if (!isError) {
            let styleContext = this._popoverStatus.get_style_context();
            styleContext.add_class('dim-label');

            let params = this._account.dup_parameters_vardict().deep_unpack();
            let server = params['server'].deep_unpack();
            let accountName = this._account.display_name;

            /* Translators: This is an account name followed by a
               server address, e.g. "GNOME (irc.gnome.org)" */
            let fullTitle = _("%s (%s)").format(accountName, server);
            this._popoverTitle.label = (accountName == server) ? accountName
                                                               : fullTitle;
            this._popoverStatus.label = '<sup>' + this._getStatusLabel() + '</sup>';
        } else {
            let styleContext = this._popoverStatus.get_style_context();
            styleContext.remove_class('dim-label');

            this._popoverTitle.label = '<b>' + _("Connection Problem") + '</b>';
            this._popoverStatus.label = this._getErrorLabel();
        }

  - rename _updateConnectionPopover() to _getStatusLabel(), remove everyhing except for
    the switch statement and strip it down to just return the matching label:

       switch (this._account.connection_status) {
           case Tp.ConnectionStatus.CONNECTED:
               return _("Connected");
           case ...

  - do the same for _updateConnectionErrorPopover() => _getErrorLabel()

Created attachment 319955 [details] [review]
roomList: Use header popover for connection management

We want to move away from a separate connection editor in favor of more direct
connection management. Creating new connections directly from the join dialog
has been possible for a while now, and the room list's headers provide a good
place for exposing connection editing/removal, so rework the existing error
popover to be usable for general connection management in non-error cases
as well.

Review of attachment 319955 [details] [review]:

Yay, getting there!

::: data/resources/room-list-header.ui
@@ +69,3 @@
         <property name="orientation">vertical</property>
         <property name="margin">12</property>
+        <property name="margin-left">6</property>

Why have different margins on the left/right?

@@ +78,3 @@
             <property name="xalign">0</property>
             <property name="visible">True</property>
+            <property name="margin-left">6</property>

If we want a margin here, it should probaby be:
  <property name="margin-start">6</property>
  <property name="margin-end">6</property>

(or in other words: use consistent margins on either side and use start/end instead of left/right for LTR/RTL handling)

@@ +87,3 @@
             <property name="xalign">0</property>
             <property name="visible">True</property>
+            <property name="margin-left">6</property>

Dto.

@@ +94,3 @@
             <property name="visible">True</property>
+            <property name="margin-top">6</property>
+            <property name="margin-left">6</property>

margin-bottom

::: src/roomList.js
@@ +228,2 @@
         }
+        this._getStatusLabel();

???

@@ +266,3 @@
+                    return this._getErrorLabel();
+            default:
+                return _("Offline");

I would make that:

  case Tp.ConnectionStatus.DISCONNECTED:
    return _("Offline");
  default:
    return _("Unknown");

That way, _getStatusLabel() has a clearly defined purpose (return a user-readable string for the current connection status) with no knowledge what other parts of the program consider an error. (Note how the method isn't called in the error case above)

@@ +275,3 @@
+            case Tp.error_get_dbus_name(Tp.Error.CONNECTION_REFUSED):
+            case Tp.error_get_dbus_name(Tp.Error.NETWORK_ERROR):
+                return _("Could not connect to %s").format(this._account.display_name);

If we use the same label as the default case, we don't need to list those separately.

Also: We should probably finish every label with a full-stop (in particular where we have two phrases: "Could not connect to GNOME. The server is busy" looks odd without the final '.')

(In reply to Florian Müllner from comment #2)
>  - I have one mockup with "Properties" and another one with "Preferences" -
> would be
>    good to ask for clarification there

Just chiming in and clarifying that "properties" sounds like the right thing to me.

Created attachment 319972 [details] [review]
roomList: Use header popover for connection management

We want to move away from a separate connection editor in favor of more direct
connection management. Creating new connections directly from the join dialog
has been possible for a while now, and the room list's headers provide a good
place for exposing connection editing/removal, so rework the existing error
popover to be usable for general connection management in non-error cases
as well.

(In reply to Bastian Ilsø from comment #5)
> (In reply to Florian Müllner from comment #2)
> >  - I have one mockup with "Properties" and another one with "Preferences" -
> > would be
> >    good to ask for clarification there
> 
> Just chiming in and clarifying that "properties" sounds like the right thing
> to me.

I agree.

Review of attachment 319972 [details] [review]:

needs-work because the copy-paste error pointed out below breaks the error case, otherwise just a couple of style nits

::: data/resources/room-list-header.ui
@@ +78,3 @@
             <property name="visible">True</property>
+            <property name="margin_start">6</property>
+            <property name="margin_end">6</property>

We're already inconsistent wrt dash vs. underscore, but let's try to settle on the former (it's what the actual property name uses, plus it's already more common)

@@ +88,3 @@
             <property name="visible">True</property>
+            <property name="margin_start">6</property>
+            <property name="margin_end">6</property>

Dto.

::: src/roomList.js
@@ +183,3 @@
+        this._popoverRemove.action_target = target;
+        this._popoverProperties.action_target = target;
+        

Trailing whitespace

@@ +263,3 @@
+            case Tp.ConnectionStatus.DISCONNECTED: {
+                if (this._account.connection_status_reason != Tp.ConnectionStatusReason.REQUESTED)
+                    return this._getErrorLabel();

I still don't like this - the condition for showing a "normal" status label and the error label is now in two places. Just always return "Offline" here, because that's the status label that corresponds to the DISCONNECTED state - in the case where we want to show an error instead of the status label, we simply don't call _getStatusLabel(), so the method shouldn't care.

@@ +270,3 @@
+        }
+    },
+    

Trailing whitespace

@@ +281,3 @@
+            case Tp.error_get_dbus_name(Tp.Error.CERT_NOT_PROVIDED):
+            case Tp.error_get_dbus_name(Tp.Error.ENCRYPTION_NOT_AVAILABLE):
+            case Tp.error_get_dbus_name(Tpl.Error.CERT_UNTRUSTED):

Copy+paste error: This throws a warning about Tpl being undefined, should be Tp.Error

Created attachment 319975 [details] [review]
roomList: Use header popover for connection management

We want to move away from a separate connection editor in favor of more direct
connection management. Creating new connections directly from the join dialog
has been possible for a while now, and the room list's headers provide a good
place for exposing connection editing/removal, so rework the existing error
popover to be usable for general connection management in non-error cases
as well.

Review of attachment 319975 [details] [review]:

Good to push with the whitespace errors below fixed - do you have git access, or should I push the patch for you?

::: src/roomList.js
@@ +261,3 @@
+            case Tp.ConnectionStatus.CONNECTING:
+                return _("Connecting...");
+            case Tp.ConnectionStatus.DISCONNECTED: 

trailing whitespace

@@ +267,3 @@
+        }
+    },
+    

Dto.

Created attachment 319997 [details] [review]
roomList: Use header popover for connection management

We want to move away from a separate connection editor in favor of more direct
connection management. Creating new connections directly from the join dialog
has been possible for a while now, and the room list's headers provide a good
place for exposing connection editing/removal, so rework the existing error
popover to be usable for general connection management in non-error cases
as well.

Attachment 319997 [details] pushed as 6ef2e57 - roomList: Use header popover for connection management

(In reply to Florian Müllner from comment #2)
>  - "Reconnect" not doing anything when already connected is confusing. This
> wasn't
>    an issue before as it was only used in the error case, but now should
> either
>    make it do something (disconnect + reconnect) or disable it when it would
> do nothing
>    (in any case it's a separate patch, I'm just mentioning it to not forget
> it later)

Just a note that I opened a separate bug for this: Bug 761276