Created attachment 332863 [details] [review]
app: Fix restored account names

When a connection fails because the used account is already connected,
we retry the connection up to three times appending '_' to the account
name on each attempt, and restore the original nick after either
connecting successfully or giving up. However the nickname property we
currently use to restore the account name is ill-suited for that
purpose, as it doesn't necessarily match the account parameter, but
rather the nickname that was active when last connected (which may
be a temporary nick picked by the user like 'nick-afk', or indeed
have the underscore suffix we want to revert).
We want to restore the original account parameter, so actually use that
parameter rather than a property that may or may not match ...

Created attachment 332864 [details] [review]
app: Handle errors on connection status changes

We try to recover from failed connection attempts by trying alternative
servers or using different account names, but only when handling the
'join-room' and 'message-user' actions - if the attempt was initiated
by any other mean, for instance the (re)connect items in the room list,
our error handling will be sidestepped completely.
So rather than handling exceptions that occur in the ensure_channels()
call, handle errors when the connection status changes from CONNECTING
to DISCONNECTED, which works independently from how the connection
was initiated.

This is part of a bigger patchset, so probably won't apply to master directly. To make it easy to try without hunting dependencies in bugs, I've pushed it as part of https://git.gnome.org/browse/polari/log/?h=wip%2Ffmuellner%2Fmisc-fixes

Review of attachment 332863 [details] [review]:

looks good to me.

Created attachment 333042 [details] [review]
app: Split _clearRetryData()

We want to restore the account name immediately after we either succeeded
to connect or gave up retrying. But on the other hand, we only want to
reset the data when we actually want to start another try-retry cycle.

Does this fix the infinite recursion we saw last night?

Created attachment 333054 [details] [review]
app: Handle errors on connection status changes

Squashed with attachment 333042 [details] [review].

Review of attachment 333054 [details] [review]:

i have some questions out of curiosity, otherwise looks good to me now.

::: src/application.js
@@ +392,3 @@
+        let data = this._retryData.get(account.object_path);
+        if (!data || !data.retry || !data.originalNick)
+            return;

If i read this right, restoreAccountName() is called by onConnectAccount() and we check this._retryData, however only ensureRetryData() populates this._retryData with data. Then onConnectAccount deletes data for the given account path which wont exist unless ensureRetryData() has been called. So in effect onConnectAccount() only "does" something if ensureRetryData() was called at some point earlier?

I was wondering if we want to try removing underscores in the case where the original nickname contains it, ie sometimes my original nickname somehow ended up being "bastianilso__". Probably needs to go in a different patch i guess though.


and when you use "restoreAccountName" "AccountName" refers to "originalNick" from data? would it make sense to call the function "restoreOriginalNick" or rename the retryData.originalNick to originalAccountName ?

Review of attachment 332863 [details] [review]:

looks good to me as well.

(In reply to Bastian Ilsø from comment #7)
> Review of attachment 333054 [details] [review] [review]:
> 
> i have some questions out of curiosity, otherwise looks good to me now.
> 
> ::: src/application.js
> @@ +392,3 @@
> +        let data = this._retryData.get(account.object_path);
> +        if (!data || !data.retry || !data.originalNick)
> +            return;
> 
> If i read this right, restoreAccountName() is called by onConnectAccount()
> and we check this._retryData, however only ensureRetryData() populates
> this._retryData with data. Then onConnectAccount deletes data for the given
> account path which wont exist unless ensureRetryData() has been called. So
> in effect onConnectAccount() only "does" something if ensureRetryData() was
> called at some point earlier?

Yup. The "real" (re)connect-account actions are handled in chatroomManager/telepathyClient, the handling here does indeed just clear the retry data from a previous attempt (if it exists).

The issue is that retryData holds data that is relevant of whether there'll be retries in the first place ('retry' for appending '_' to nicknames, and the list of alternate servers to try), so we need to clear it at some point if we want to start another series of attempts after we failed once. Doing that immediately after we failed is obviously a bad idea, as it was sending you into an infinite loop :-)


> I was wondering if we want to try removing underscores in the case where the
> original nickname contains it, ie sometimes my original nickname somehow
> ended up being "bastianilso__". Probably needs to go in a different patch i
> guess though.

You had a chat with csoriano_____? The cause of the underscores-in-original-nick is what the first patch should address, but it doesn't strip any existing underscores from the existing name. It may be worth considering, but that would indeed be a different patch/issue.


> and when you use "restoreAccountName" "AccountName" refers to "originalNick"
> from data? would it make sense to call the function "restoreOriginalNick" or
> rename the retryData.originalNick to originalAccountName ?

Mmmh, yeah. "originalNick" did use the nickname originally (sic!), but that's actually wrong - see the first patch and the trailing underscores in your nick ;-)

On the other hand I'm not too happy with the confusion between "account == Tp.Account object" and "account == parameter of a Tp.Account object". originalAccountName is probably the most precise name though - I'll change the name in the first patch, which is where the value changes from nickname to the account parameter.

Created attachment 333098 [details] [review]
app: Fix restored account names

s/originalNick/originalAccountName/

Created attachment 333099 [details] [review]
app: Handle errors on connection status changes

Adjusted for the changes in the previous patch.

Review of attachment 333098 [details] [review]:

looks good to me.

Review of attachment 333099 [details] [review]:

looks good to me. Do you think we also want to rename "RetryNickRequest" to be completely consistent or?

Attachment 333098 [details] pushed as a7e9fc4 - app: Fix restored account names
Attachment 333099 [details] pushed as bc8f10e - app: Handle errors on connection status changes