ChatService refactoring

[Katharsas]

Just letting you guys know that I am currently working on a refactoring that basically touches all of PircBotXChatService. This is the current IRC implementation for ChatService.

I decided to try and split that class into stuff that is related to PircBotX and stuff that is not (IRC independent). Stuff that isn't calls the new PircBotXChatService through an IRC independant interface (which may get the name ChatConnector or something). This way ONLY the actual IRC stuff is behind ChatConnector, while the stuff that would be needed for any chat implementation is inside the new class ChatServiceImpl.

I may also make it more clear what the connected channels SHOULD be (vs which may be connected at a certain moment), maybe this allows me to ge to get rid of some awkward flags and make it easy to get from bad states/disconnect to proper proper channel state better.

There are no functionality changes. but this should for example make it easy to instantly join channels when editing saved auto channels or similar fun stuff in the future. Also should make it easier to swap out the IRC implementation by reimplementing only ChatConnector instead of entire ChatService.

This refactoring will not be able to hide IRC entirly, because the ChatService interface would need to change for that (because it exposes IRC specific stuff like whois() method) and i am not changing that interface for now.

[Katharsas]

Branch for this:
https://github.com/FAForever/downlords-faf-client/tree/refactor/%231336-irc-chatservice

[Katharsas]

Ok, i looked into the ChatService (which is the "problem") and the ChatController (which seems to be the owner/"root" of all Chat related things) and here are some more observations:

For some parts of the chat, UI state is held directly inside ChatController, while other state is held inside Service classes (PircBotXChatService), specifically:

• ChatController contains the UI state for private message (pm) channels
• PircBotXChatService (implements ChatService) contains the UI state for room channels

While this for itself could be reasonable, since ChatController owns/has dependency on PircBotXChatService, it seems like PircBotXChatService is actually supposed to be the IRC specific stuff (i infer that from the fact that "ChatService" is a pretty generic name while it deals with all the IRC stuff under the hood).
So, this ChatService implementation is not just there way to deal with room state, it also currently fills the role of providing ALL (abstracted) access to the underlying IRC functionality, which was probably its primary intended role.

These two roles of the current ChatService implementation need to be separated imo. So we probably end up with something like:

• ChatRoomService (class): holds and manages all non-IRC-specific state about chat rooms. Specifically, the current UI state for all chat rooms and the names of the chat rooms that that should always survive (joined on start or on reconnet). This class is mostly just an extension of the ChatController.
• ChatConnector (interface): abstraction to hide away IRC specific stuff
• PircBotXConnector: implementation of ChatConnector for IRC

However im pretty sure we still end up with a bunch of stuff that is

• not IRC specific (so doesn't belong into ChatConnector implementation)
• not chat room specific, but simply necessary for both rooms and pms (so doesn't belong into ChatRoomService)
  So i propose that thi stuff that is left over will become the new ChatService (so ChatService will not be an interface anymore). Alternativly, this could go directly into ChatController or a differently named class.

Any thoughts on this?

[Katharsas]

Ok, i started implementing this and have a pretty good feeling about the proposed changes.

[Sheikah45]

Stale