Created attachment 280168 [details] [review]
Restore saved channels when an account is reconnected

Review of attachment 280168 [details] [review]:

::: src/chatroomManager.js
@@ +124,3 @@
     },
 
+    restoreSavedChannels: function(account) {

I'd prefer to keep the way channels are serialized an implementation detail, so I'd prefer a TpAccount instead of the object path here

::: src/mainWindow.js
@@ +193,3 @@
             function() {
                 app.unmark_busy();
+                ChatroomManager.getDefault().restoreSavedChannels(account.object_path);

Not a fan to be honest - restoring saved channels when a particular notification is closed looks random, e.g. what if we decide to add a close button to in-app notifications so users don't have to wait for them to be closed automatically?

Created attachment 280183 [details] [review]
accountsMonitor: Make primary owner of accountManager

Currently, both ChatroomManager and AccountsMonitor take a reference
on AccountsManager, with the former setting up the required features,
so it needs to be initialized first. Instead, handle AccountManager
initialization in AccountsMonitor and port ChatroomManager to use
it, so we can start to make use of AccountsMonitor's account tracking
in the future.


So I buried up an old local patch of mine that might be useful here - the original patch had this chunk in chatroomManager.js:

+        this._accountsMonitor.connect('account-status-changed', Lang.bind(this,
+            function(mon, account) {
+                if (account.connection_status == Tp.ConnectionStatus.CONNECTED)
+                    this._restoreSavedChannels();
+            }));

I've taken it out as your approach of restoring channels for a particular account looks reasonable to me, so what do you think of rebasing your fix on top of this patch?

Review of attachment 280183 [details] [review]:

::: src/accountsMonitor.js
@@ +42,3 @@
+            am.prepare_finish(res);
+        } catch(e) {
+            this._app.release(); // no point in carrying on

return?

Created attachment 281003 [details] [review]
accountsMonitor: Make primary owner of accountManager

Yeah, makes sense to me ...

Created attachment 281004 [details] [review]
Restore saved channels when an account is reconnected

If an account is reconnected after a transient network failure
(that is not picked up by GNetworkMonitor), rejoin all its channels,
otherwise we end up connected but outside all rooms.

And this is the rebased version.

Review of attachment 281004 [details] [review]:

::: src/chatroomManager.js
@@ +106,3 @@
 
+    _onAccountEnabled: function(am, account) {
+        this._restoreSavedChannels(account.object_path);

You need to pass the account, not its object path

::: src/mainWindow.js
@@ +193,3 @@
             function() {
                 app.unmark_busy();
+                delete account._connectingNotification;

This drive-by fix is a bit odd now that you don't touch any actual code here - can you push this separately please?

Comment on attachment 281003 [details] [review]
accountsMonitor: Make primary owner of accountManager

Attachment 281003 [details] pushed as 5bfc81c - accountsMonitor: Make primary owner of accountManager

Pushed with the noted fixes.

Attachment 281004 [details] pushed as d490384 - Restore saved channels when an account is reconnected