Created attachment 332960 [details] [review]
mainWindow: Manage active room via an :active-room property

The ChatroomManager class hasn't aged very well, and has become
increasingly muddy with regard to what it actually does and how
it relates to other components (in particular the application):

 - ChatroomManager manages joined rooms, except updating
   which rooms should be joined is done by the application

 - ChatroomManager handles incoming channel requests from
   mission-control, but our own channel requests are made
   by the application

 - managing the list of rooms means we want it available as
   early as possible to other components; being a Telepathy
   handler and observer means we only want to start it when
   we should establish network connections

We will untangle that mess over the next couple of commits, starting
with something simple: managing the active room. Code-wise it would
be convenient to keep using an always available singleton for this,
with application being the obvious choice. However conceptually, the
window is better suited to manage the active room, as the whole concept
is tied to some visual representation - being "active" really means
being the currently selected room in the window. If we are ever going
to support more than a single main window, we surely don't want all
windows to show the same room. Similarly, if we allow running in the
background without any windows, the idea that any of the joined rooms
was active seems silly. So move tracking of the active room from the
ChatroomManager into the window.

Created attachment 332961 [details] [review]
mainWindow: Focus the first added room

Now that the active room is a property of the window, it makes sense
to move initialization there as well rather than leaving it in the
chatroom manager.

Created attachment 332962 [details] [review]
mainWindow: Track last selected room

Equally, the tracking of the last selected room is now easier done by
the window. For now, we still rely on the correct room being restored
first, but we will soon handle this in the window as well.

Created attachment 332963 [details] [review]
chatroomManager: Handle channel requests

ChatroomManager already handles most connection-related actions - handling
and observing channels and (re)connecting accounts. It makes sense to assume
control over the remaining bit as well, and handle requesting new channels
as well.

Created attachment 332964 [details] [review]
application: Initialize actions as early as possible

Until recently, some of the action create hooks were using some of the
singletons that are initialized on startup, so we had to create those
first before adding the actions. Now that this is no longer the case,
we can reorder the code so that all application actions are always
available, including to the singletons created on startup.

Created attachment 332965 [details] [review]
chatroomManager: Split out room management

Originally rooms were tightly coupled with channels - a room was created
and added when we observed a channel, and removed when the channel was
invalidated - so doing room management in the telepathy observer/handler
made a lot of sense. But nowadays where we show disconnected rooms, the
only point where room management overlaps with channel handling is where
we match a channel to a (usually existing) room. It is time to split out
room management into its own class, including maintaining the list of
saved channels (where updating the list got split from restoring rooms
when the code moved into the application).

Created attachment 332966 [details] [review]
chatroomManager: Rename to TelepathyClient

With room management now moved to its own module, the existing name
becomes a relict that doesn't make sense anymore. As the primary
functions are now registering as a channel observer and handler with
and requesting channels from mission control, TelepathyClient looks like
a reasonable pick.

Created attachment 332967 [details] [review]
telepathyClient: Stop using the singleton pattern

The pattern made a lot of sense while many components needed to access it
for its room management, but now that it has been reduced to its function
as telepathy client, it is cleaner to make it a regular class that is
instantiated by the application when its functionality is actually
needed.
This also allows us to implement Telepathy.BaseClient directly instead of
using a helper class, and to get rid of the awkward lateInit() split.

Review of attachment 332960 [details] [review]:

some comments and questions. :-)

::: src/application.js
@@ +536,2 @@
     _onLeaveCurrentRoom: function() {
+        let room = this._window.active_room;

question, what is the difference between this.active_window.active_room; and this.window.active_room? should this.active_window.active_room; be used here?

::: src/chatroomManager.js
@@ +476,3 @@
     },
 
+    _onActiveRoomChanged: function(window) {

in roomList, roomStack and userList it's called just "_activeRoomChanged", maybe we want to be consistent and do that here too?

::: src/mainWindow.js
@@ +115,3 @@
+                                                'active-room',
+                                                GObject.ParamFlags.READWRITE,
+                                                Polari.Room.$gtype)

mmmh, can you have dollarmarks in variable names? im just wondering in which cases you are supposed to do that.

@@ +118,2 @@
     },
+    Signals: { 'active-room-state-changed': {} },

I can see that this signal is connected to inside application.js, but to me it looks like it's emitted the same time as notify::active-room (and I perceive the meaning of the name to be the same). Can you elaborate a bit why this is necessary to have in addition to that?

@@ +127,3 @@
+        this._displayNameChangedId = 0;
+        this._topicChangedId = 0;
+        this._membersChangedId = 0;

should this._channelChangedId also be declared here?

this._channelChangedId = 0;

(In reply to Bastian Ilsø from comment #9)
> ::: src/application.js
> @@ +536,2 @@
>      _onLeaveCurrentRoom: function() {
> +        let room = this._window.active_room;
> 
> question, what is the difference between this.active_window.active_room; and
> this.window.active_room? should this.active_window.active_room; be used here?

GtkApplication:active-window refers to the most recently focused app window. this._window is a simple propery we use to track the (single) main window we create. As long as we are a single-window application, there's no difference between the two. There's a patch to get rid of the _window property on the wip/window-experiments branch that I guess I could apply, as this._window is
 - wrong if we want to support multiple window
 - pointless if we don't (because it's just the same as
   GtkApplication:active-window)

I'm indifferent about this, but if we kill off the separate _window property now, it should be in a separate patch anyway ...



> ::: src/chatroomManager.js
> @@ +476,3 @@
>      },
>  
> +    _onActiveRoomChanged: function(window) {
> 
> in roomList, roomStack and userList it's called just "_activeRoomChanged",
> maybe we want to be consistent and do that here too?

On the other hand, we usually use _onSignalName (or _onPropertyNameChanged for ::notify), so arguably _activeRoomChanged() is wrong :-)

I don't mind changing it either way *shrug*


> ::: src/mainWindow.js
> @@ +115,3 @@
> +                                                'active-room',
> +                                               
> GObject.ParamFlags.READWRITE,
> +                                                Polari.Room.$gtype)
> 
> mmmh, can you have dollarmarks in variable names? im just wondering in which
> cases you are supposed to do that.

It's not a random variable name though - it's added by gjs to refer to the prototype's actual GType. I haven't been involved in that, but I figure the "uncommon" name was used to make conflicts with regular code less likely ...


> @@ +118,2 @@
>      },
> +    Signals: { 'active-room-state-changed': {} },
> 
> I can see that this signal is connected to inside application.js, but to me
> it looks like it's emitted the same time as notify::active-room (and I
> perceive the meaning of the name to be the same). Can you elaborate a bit
> why this is necessary to have in addition to that?

It is not necessary, just a bit more convenient - it's not *only* emitted on room changes, but also when the active room's channel changes. We use that to track if the currently visible room is connected (i.e. whether the userList should be available etc.). See the ::active-state-changed signal that used to be in ChatroomManager that this signal replaces.


> @@ +127,3 @@
> +        this._displayNameChangedId = 0;
> +        this._topicChangedId = 0;
> +        this._membersChangedId = 0;
> 
> should this._channelChangedId also be declared here?
> 
> this._channelChangedId = 0;

Yup.

Review of attachment 332960 [details] [review]:

looks good to me (with Bastian's observation about this._channelChangedId = 0 initialization).

Review of attachment 332961 [details] [review]:

looks good to me.

Review of attachment 332961 [details] [review]:

isn't the initialization still happening in chatroom manager, technically speaking? i mean, _onRoomAdded sets the active_room label but only if it was already set which the chatroom manager does? Would it make sense to have this.active_room = null in init?

Review of attachment 332962 [details] [review]:

looks good to me

Review of attachment 332962 [details] [review]:

looks good to me.

Review of attachment 332963 [details] [review]:

looks good to me.

Review of attachment 332964 [details] [review]:

looks good to me.

Review of attachment 332966 [details] [review]:

looks good to me.

Review of attachment 332967 [details] [review]:

looks good to me.

Review of attachment 332965 [details] [review]:

looks good to me.

Created attachment 334359 [details] [review]
chatroomManager: Split out room management

Fix parameter unpacking for 'message-user' action (in _onQueryActivated): the (unused) 'message' parameter is positioned before the 'time' parameter, not after.

Attachment 332960 [details] pushed as 59daae7 - mainWindow: Manage active room via an :active-room property
Attachment 332961 [details] pushed as 2723a61 - mainWindow: Focus the first added room
Attachment 332962 [details] pushed as 4a55bb3 - mainWindow: Track last selected room
Attachment 332963 [details] pushed as 9b1497e - chatroomManager: Handle channel requests
Attachment 332964 [details] pushed as a615198 - application: Initialize actions as early as possible
Attachment 332966 [details] pushed as 7bbb220 - chatroomManager: Rename to TelepathyClient
Attachment 332967 [details] pushed as 3b0eae3 - telepathyClient: Stop using the singleton pattern
Attachment 334359 [details] pushed as 7da6b54 - chatroomManager: Split out room management