Created attachment 334643 [details] [review]
roomList: Work around imprecise CONNECTING status

This needs some investigation, but for some reason CONNECTING is
used as the initial state, even before we ask to connect in the
first place. I suspect a telepathy-idle issue, but until we figure
it out, this works ...

Created attachment 334644 [details] [review]
telepathyClient: Disconnect accounts when network is unavailable

We currently only request when a connection when network is available,
to avoid showing busy spinners for pointless connection attempts.
This works when we start offline and the network becomes available,
however there's no point either in trying to re-establish a connection
when the network goes away, so disconnect explicitly in that case.

Created attachment 334645 [details] [review]
telepathyClient: Disable (re)connect actions when offline

The actions obviously won't work when networking isn't available, so
disable them accordingly.

Created attachment 334646 [details] [review]
telepathyClient: Use connectivity instead of network-available

GNetworkMonitor:network-available uses a very generous definition of
"available", which doesn't necessarily imply that we can actually
establish a connection to any server. GNetworkMonitor:connectivity
gives us a more fine-grained view on the current network state,
and only considering FULL connectivity as online is much more
appropriate for most networks.
It does mean that we consider servers on the local network or VPN
unreachable when there's no access to the public internet though,
so we may want to consider checking the actual server address in
the future if this turns out to be a problem.

Created attachment 334647 [details] [review]
roomList: Use dedicated disconnected icon

We indicate both connection attempts and failures in the room list
headers, but we currently don't differentiate between the online and
(non-error) offline case. As the functionality in the latter case is
fairly limited though, it makes sense to have a clearer distinction
than making entries insensitive, so use an appropriate icon in the
room list header.

Review of attachment 334643 [details] [review]:

::: src/roomList.js
@@ -198,3 @@
             this._account.connect('notify::connection-status',
                                   Lang.bind(this, this._onConnectionStatusChanged));
-        this._onConnectionStatusChanged();

why do you remove this here?

@@ +297,3 @@
         this._popoverConnect.visible = offline;
         this._popoverReconnect.visible = !offline;
+        this._onConnectionStatusChanged();

..and add this here?

Review of attachment 334644 [details] [review]:

s/when a connection when network is available/when network is available/

::: src/telepathyClient.js
@@ +230,3 @@
+    _setAccountPresence: function(account, presence) {
+        let statuses = Object.keys(Tp.ConnectionPresenceType).map(s =>
+            s.replace('_', '-', 'g').toLowerCase()

can you elaborate to me what is going on here in the map function?

(In reply to Bastian Ilsø from comment #6)
> this._onConnectionStatusChanged));
> -        this._onConnectionStatusChanged();
> 
> why do you remove this here?

The function is now called as part of _onRequestedPresenceChanged() a couple of lines below, so no need to call it twice on startup.


> @@ +297,3 @@
>          this._popoverConnect.visible = offline;
>          this._popoverReconnect.visible = !offline;
> +        this._onConnectionStatusChanged();
> 
> ..and add this here?

We now use an effective connection status that is a combination of TpAccount:connection-status and TpAccount:requested-presence-type - in other words, the status returned by _getConnectionStatus() may change when either of those properties changes, so to make sure that the UI is updated correctly, we update it in both cases.

Review of attachment 334645 [details] [review]:

I'm actually wondering whether it would be better to leave them enabled as "brute force" controls. Do we feel GNetworkMonitor is reliable enough to report the correct state? These specific actions are mostly used by users with troublesome internet or exotic setups anyway, so maybe it's worth to leaving the actions enabled.

Review of attachment 334646 [details] [review]:

I have a bit cold feet about this at least without testing the behavior over time. For now I'd rather we try to connect too often than assume no connection unless we are absolutely sure (warm thoughts to people with crappy routers, ISPs and network configs).

(In reply to Bastian Ilsø from comment #7)
> ::: src/telepathyClient.js
> @@ +230,3 @@
> +    _setAccountPresence: function(account, presence) {
> +        let statuses = Object.keys(Tp.ConnectionPresenceType).map(s =>
> +            s.replace('_', '-', 'g').toLowerCase()
> 
> can you elaborate to me what is going on here in the map function?

Yeah, that's a bit of black magic, isn't it?

So the first two parameters to request_presence_async() are a numerical presence (from Tp.ConnectionPresenceType) and a string representation of the same status. The string isn't an arbitrary value though, but should match the presence type - if the type is Tp.ConnectionPresenceType.AVAILABLE, the string should be "available", if it's Tp.ConnectionPresenceType.EXTENDED_AWAY it should be "extended-away" and so forth - it's really annoying that the second parameter exists at all (or at least isn't hidden by the telepathy-glib API).

At least we can avoid a big switch statement by creating the presence -> string mapping automatically, which is what the above code does:
  AVAILABLE => "available", BUSY => "busy", EXTENDED_AWAY => "extended-away" etc.

(Admittedly we only use AVAILABLE and OFFLINE in practice, but I had some fun writing that code :-) )

Review of attachment 334647 [details] [review]:

looks good to me.

the commit message sounds a bit like this patch is removing the sensitivity change on the entries, while that's not the case. maybe the "than making entries insensitive" bit could be removed/reworded?

(In reply to Bastian Ilsø from comment #10)
> Review of attachment 334646 [details] [review] [review]:
> 
> I have a bit cold feet about this at least without testing the behavior over
> time. For now I'd rather we try to connect too often than assume no
> connection unless we are absolutely sure (warm thoughts to people with
> crappy routers, ISPs and network configs).

(That said, I think your proposal on checking the actual server address in
the future is a really good idea.)

(In reply to Bastian Ilsø from comment #9)
> I'm actually wondering whether it would be better to leave them enabled as
> "brute force" controls.

Fair enough.


(In reply to Bastian Ilsø from comment #10)
> I have a bit cold feet about this at least without testing the behavior over
> time.

Dito. I'm fine with leaving this out for 3.22.


> For now I'd rather we try to connect too often than assume no
> connection unless we are absolutely sure (warm thoughts to people with
> crappy routers, ISPs and network configs).

That said, trying to connect when the public internet cannot be reached doesn't help with crappy routers, ISPs and network configs :-)

(In reply to Bastian Ilsø from comment #12)
> the commit message sounds a bit like this patch is removing the sensitivity
> change on the entries, while that's not the case. maybe the "than making
> entries insensitive" bit could be removed/reworded?

"than just making entries insensitive" maybe?

(In reply to Florian Müllner from comment #15)
> (In reply to Bastian Ilsø from comment #12)
> > the commit message sounds a bit like this patch is removing the sensitivity
> > change on the entries, while that's not the case. maybe the "than making
> > entries insensitive" bit could be removed/reworded?
> 
> "than just making entries insensitive" maybe?

sounds good to me

Created attachment 335277 [details] [review]
telepathyClient: Disconnect accounts when network is unavailable

I added some debug messages to debug an issue on suspend/resume (patch for that coming), and decided they were worth keeping ...

Created attachment 335278 [details] [review]
telepathyClient: Fix inconsistency on resume

When the :network-available property changes repeatedly in quick
succession (which apparently happens when resuming from suspend),
we can end up in an inconsistent state where an account has a
requested presence of OFFLINE, but is in fact fully connected.
Work around this by applying the same work-around as in the
previous commit.

Review of attachment 335277 [details] [review]:

looks good to me

Review of attachment 335278 [details] [review]:

code looks good to me.

"previous commit" refers to attachment 334643 [details] [review] ?

Yeah, I rearranged the commits a bit locally (after dropping the two commits we decided to leave out for now)

Attachment 334643 [details] pushed as 5ba822e - roomList: Work around imprecise CONNECTING status
Attachment 334647 [details] pushed as 884b38a - roomList: Use dedicated disconnected icon
Attachment 335277 [details] pushed as 5434c3b - telepathyClient: Disconnect accounts when network is unavailable
Attachment 335278 [details] pushed as 741c55b - telepathyClient: Fix inconsistency on resume

Moving to https://gitlab.gnome.org/GNOME/polari/merge_requests/120 now.