Chat Refactor [DRAFT] #33858

SlamBamActionman · 2024-12-14T16:19:33Z

About the PR

This is a refactor of the chat system! It is heavily in development and is not merge-ready; this PR is currently acting as a way for contributors and maintainers to preview the new system and provide feedback in areas where work has been done.

The goal of the chat refactor has been to achieve three things:

Modularity. It should be easy to add new behaviors to chat messages, whether that be modifying text or evaluating conditions.
Linearity: While chatcode may still be complex, it should be easy to follow it from start to finish, without any offshoots.
Condensed: Chatcode shouldn't have copy/pasted behavior spread out over multiple systems. Instead it should be concentrated into a single pipeline, and content should be reused where possible.

Due to the sheer scope of this PR and the many files changed just for the code to compile, below I will highlight the files and functions which are of importance in the refactor.

If you want to dive into the code and find the most important changes, I recommend:
ChatManager.cs (server and client)
CommunicationChannelPrototype.cs
SessionChatCondition.cs
EntityChatCondition.cs
ChatModifier.cs
IContentMarkupTag.cs
ChatUIController.cs
ISharedContentMarkupManager.cs
Note that I have built a lot of the code parallel to current chatcode, which means there remains a lot of unused old functions in the classes that will eventually be removed.

So where do we start? What's the core of this refactor?

CommunicationChannelPrototype

While the idea of having distinct channels (local speech, radio, OOC, adminchat) is nothing new in the codebase, previously their behaviors were largely spread out over multiple functions and classes that modified how they behaved. The new prototype CommunicationChannelPrototype aims to solve this.

A CommunicationChannel contains all information relevant for how a message in a channel should be treated; it contains checks for who may send and receive the message, how the message should be modified and if the message should be passed on to any other channels. This is all defined via .yaml, making it very easy to add to and modify the behaviors of the channels, as well as include inheritance for similar channels (i.e. radios).

The CommunicationChannel prototype contains the following:

PublishSessionChatConditions
This is a list of conditions that a player session must meet to be able to send a message over this channel. These conditions are defined via the new class SessionChatCondition.cs, and can be things like "Is this an admin?", "Is the OOC CCvar enabled?" or "Is the session a ghost?". SessionChatConditions can have other conditions as subconditions, creating an AND/OR tree that filters out any session that shouldn't be able to send to the channel.
Additionally, SessionChatConditions can have EntityChatConditions as subconditions, which check the player entity as well (e.g. "Is the player entity alive?").

Note: Any message sent by the server skips evaluating these conditions, as it is assumed the server is always allowed to publish to a channel.
ConsumeCollections
A ConsumeCollection is a collection of conditions that defines a group of sessions and entities that may receive messages on the channel, and how that message should be modified for them. It does this via three lists:
- ConsumeSessionChatConditions
  Acts very similarly to PublishSessionChatConditions, in that you have a list of conditions that are evaluated against.
- ConsumeEntityChatConditions
  This list contains purely EntityChatConditions. While the ConsumeSessionChatConditions list may optionally evaluate player entities against these, the primary use of this list is to determine what non-player entities may react to messages on the channel. This could be defining a range for speech, or requiring a radio receiver on the entity.
- CollectionChatModifiers
  The final list contains ChatModifiers, which take a message as an input and outputs a modified version of that message. This could be anything from accents to coloring texts. The reason why these are defined in the ConsumeCollection is because you might want different modifications based on different consumers, e.g. whispering should only have the text obfuscated for consumers at a certain range from the speaker (Hello World vs. Hel~~ Wo~ld).
ClientChatModifiers
This list contains ChatModifiers that are applied clientside. Previously nearly all message modifications were being on the server, which limited the ability to provide options where necessary. Having modifiers apply on the client opens up the possibility to give custom chat colors, accent accessibility options and other stuff like that.
AlwaysChildCommunicationChannels & BackupChildCommunicationChannels
These are lists of channels that will also receive the message and attempt to use it. This could be in the case of Deadchat, where attempting to speak in local chat always fails and instead gets redirected to deadchat instead. Another usecase would be radio usage, where you both speak in the radio channel and whisper channel at the same time.

That's most of the things in CommunicationChannelPrototype; if you want to see all properties, check out its class file. So how is this used? Well...

SendChannelMessage

If you want to send a message, either as part of a game system or as a session, you will now be using SendChannelMessage. This acts as a single pipeline for all messages being sent out to the player clients, as well as any message server systems should listen and react to. The basic use of SendChannelMessage is very simple. You include the message you want to send, the communication channel you want to target, and optionally the *sender session and sender entity:

SendChannelMessage(string message, CommunicationChannelPrototype communicationChannel, ICommonSession? senderSession, EntityUid? senderEntity)

And that should be it! SendChannelMessage linearly goes through the prototype's conditions to see if you may send the message, gets a list of consumers, modifies the message and sends it out, all as defined in the chosen CommunicationChannelPrototype.

Some ChatConditions and ChatModifiers take in data from the optional Dictionary<enum, object>? channelParameters, which can be useful if you need granularity in the message you're sending, e.g. custom coloring of an announcement message. You can also supply a limited selection of sessions via HashSet<ICommonSession>? targetSessions, if you need granularity for who should receive the message (useful for admin messages that should only go out to a certain subset of admins). ChatFormattedMessageToHashset is used as the output function and interfaces with the net manager.

All old say, dsay, me, ooc commands and similar have been converted to use SendChannelMessage, targetting the relevant channel.

ContentMarkupTags

As part of the refactor, messages are now being sent over the network as FormattedMessages instead of plain strings. This makes it easier to distinguish between what's written text and what's markup. FormattedMessages contain a collection of MarkupNodes, which are either text nodes or markup nodes, with optional parameters. They're written out as [ExampleNode].

Previously these nodes were only used for actual markup formatting, such as the [bold] or [color="red"] tags, but they are now being leveraged in the form of ContentMarkupTags. These tags are set via ChatModifiers and resolved on the client, and can be used to give more control over how a message is formatted. An example of this is the [EntityNameHeader="Urist McHands"] tag, which is set on the server (allowing things like voicemasks to secretly modify the name) and then turned into plaintext Urist McHands on the client.

Other ChatModifiers may use this tag to provide finer formatting. BoldTagChatModifier is a clientsided chat modifier which looks for the EntityNameHeader tag and wraps it in a [bold] tag, which is suitable for bolding names in the chatbox.

Below is a commented example of the ICSpeech channel showing how the ChatModifiers apply the ContentMarkupTags, and how the conditions are evaluated.

- type: communicationChannel
  id: ICSpeech
  publishSessionChatConditions: //These are the conditions that must be fulfilled to use IC speech:
  - !type:AllSessionChatCondition //All sessions are checked
    subconditions:
    - !type:IsGhostSessionChatCondition //AND you can't speak if you're a ghost! That's why the condition is inverted on the next line!
      inverted: true
      entityChatConditions:
      - !type:IsAboveCritEntityChatCondition //AND you have to be above crit to speak.
        subconditions:
        - !type:HasComponentEntityChatCondition //AND you can't be asleep either.
          component: Sleeping
          inverted: true
  consumeCollections: #These are the conditions that must be fulfilled to hear IC speech
  - consumeSessionChatConditions:
    - !type:IsGhostSessionChatCondition //All ghosts may check local chat
    - !type:AllSessionChatCondition //OR we check all remaining sessions, this time using entity chat conditions.
      useEntityChatConditions: true //We want to use the same chat conditions as non-player consumers use, so we set this to true...
    entityChatConditions:
    - !type:RangeEntityChatCondition //...Which in this case is anything within 10 tiles range!
      maximumRange: 10
    collectionChatModifiers: //Time to modify the message!
    - !type:MainMessageChatModifier //This as a [MainMessage] tag wrapped, which helps with bubble speech later.
    - !type:EntityNameHeaderChatModifier //This adds the entity's name as an [EntityNameHeader="UristMcHands"] tag.
    - !type:SpeechVerbChatModifier //This adds a speech verb tag, e.g. [SpeechVerb="lizard"]
    - !type:ColorFulltextChatModifier //And this adds a color tag, [ColorValue="Speech"]. 
      colorKey: Speech
  clientChatModifiers: //These are chat modifiers used on the client.
  - !type:SpaceMainMessageChatModifier //This adds a space in front of the [MainMessage] tag.
  - !type:QuoteMainMessageChatModifier //And this adds quotation marks around the [MainMessage] tag.
  - !type:BubbleProviderChatModifier //This adds the [BubbleHeader] and [BubbleMessage] tags, which are used to create chat bubbles.
    speechType: Say
  - !type:BoldTagChatModifier //This bolds the [EntityNameHeader] tag.
    targetNode: EntityNameHeader
  - !type:ColorEntityNameHeaderChatModifier //And finally, this optionally colors the name, if that's in your client's settings.
  backupChildCommunicationChannels:
  - Dead //If the chat conditions failed, you're probably dead, so this should attempt the Dead channel
  chatChannels:
  - Local //You're speaking in the local space

ListenerEntitySystem & ListenerComponent

While player sessions are given messages via ChatFormattedMessageToHashset, entities (i.e. NPCs and items) are given the messages via events. To subscribe to these, any system aiming to utilize chat messages need to inherit ListenerEntitySystem, with the component inheriting ListenerComponent. This sets up the boilerplate to ensure you only get messages sent via the appropriate chat channel type (e.g. local, whispering, radio, all auditory channels and so on). Override OnListenerMessageReceived to use the message in whatever way you see fit!

That's a lot of words. Can I please have an image that explains the whole process better?

Yes, soon.

Areas of concern

Any big refactor is going to have some issues that will need to be resolved. In this case, I would ask reviewers to pay extra attention to the following areas, as they are places I am not very knowledgeable in:

IoC injection & simulation separation.
Due to ChatConditions/ChatModifiers not inheriting from any entity system, they are manually resolving system and manager dependencies. However, unlike a system, these classes are being instantiated seemingly every time a message is sent. I don't know how this would impact performance, but it seems like it should be better to somehow cache them? If so, how would one do that? I'm also concerned that this could trigger an entity check when outside the simulation and causing it to throw errors.
FormattedMessage over the network
SS14 is a very message-heavy game, and I am uncertain of the impact changing from sending just a simple string to sending a whole FormattedMessage has. If it ends up slowing down the game too much it might be worth considering going back to sending strings, and instead doing FormattedMessage parsing on the client instead.
Repeated iterations over FormattedMessage
Due to understandable protection limitations of FormattedMessage, a way to add new nodes and markup is to iterate over it and rebuild the FormattedMessage with the changes. This could possibly be unnecessary unoptimized work. Presumedly an engine change would allow smoother operations here, at the cost of safety cushions.
Prototype IDs and strings
I don't know if it's necessary to have a statically defined ProtoId variable in each system that intends to use SendChatMessage, or if it's fine to supply a normal string and let the ChatManager handle it. Let me know what the best practice is.

Missing features

Media

Requirements

I have read and am following the Pull Request and Changelog Guidelines.
I have added media to this PR or it does not require an ingame showcase.

Breaking changes

So what does this chat refactor mean for me as a fork?

radio_channels.yml remains untouched, however if you have custom radio channels you will need to create a matching communication channel prototype. Ideally this would be resolved automatically, but the chat refactor must have some scope constraints. To do this, you can look at communication_channels.yml in the radio section and copy an existing radio channel; of importance is that the prototype has the id: Radio[YourRadioPrototypelIdHere] and the DefaultChannelParameters.RadioChannel value be the radio prototype ID.

You will also have to convert accents to Shared, but since I haven't done that part of the refactor yet, I can't explain how to do it. Note to self: remember to edit this part before merging, or it will be embarrassing.

Changelog

🆑

tweak: There has been a major overhaul to how chat is handled in the game; please report any new chat bugs you may encounter.

github-actions · 2024-12-14T16:33:55Z

This pull request has conflicts, please resolve those before we can evaluate the pull request.

FairlySadPanda · 2024-12-14T17:59:17Z

I am here

it is time

to review

FairlySadPanda · 2024-12-14T20:37:52Z

First things first here's a shorter writeup of Slam's PR summary for those with Subway Surfer brains:


Much like "newmed" let's call this something like newchat.

PR information:

1. This isn't done yet
2. This means reviewing for code quality is pointless
3. Slam has prioritized:
-> Modularity, by which he means he want it to be extensible.
-> Linearity, meaning it is generally a consistent point to point process, not a tree.
-> Condensed. No copypasta, one pipeline, KISS.

CommunicationsChannelPrototype
    -> PublishSessionChatConditions
        Lists conditions that a session must fulfil to send on this channel
    -> EntityChatConditions
        A subset of the above that covers entity conditions.
    
The server as auth does not need these auth checks.

ConsumeCollections 
    -> A collection of conditions for receiving on a channel.
        -> Session conditions
        -> Entity conditions
        -> Condition modifiers (i.e. mutators)

ClientChatModifiers
    -> Client-side mods of chat, for accessibility etc.

AlwaysChildCommunicationChannels
BackupChildCommunicationChannels
    -> Subscribers to other channels. e.g. deadchat is a backup to local if you're dead.

SendChannelMessage
    This is the core event that encapsulates a message.
    Contains a message, the channel, and opts of the session and entity sending.
    Some chat conds and mods lean on a context object called ChannelParameters which is a dict of enum to obj.
    Messages can be given specific target sessions.
    
All console chat messages use newchat.

Messages are formatted messages when sent over the network. Formatted messages have a message and markup for that message. Markup uses square brackets - `[]` - for tags. Examples of use are color or entity name tags.

ListenerEntitySystem and ListenerComponent
    -> Non-players get messages via events. These are subbed to by extensions of ListenerComponent. 


TODOs

-> IOC Injection, sim separation
    Some concerns about perf and redundant instantiation.
-> Concerns about weight of chat messages OTA
-> FormattedMessages haven't had a optimization pass
-> Debate over if it's better to have a ProtoID var for senders of SendChatMessage or if strings are ok?

-> Message sanitization
-> Accent handling
-> Handheld radios, intercoms and TVs
-> Announcements
-> Admin chat parity
-> Radio jamming
-> Codeword highlighting
-> Removing chatcode
-> Documentation
-> Bug sweep```

Djungelskog2 · 2024-12-15T00:57:04Z

languages, coming to a station near you soon

Fildrance · 2024-12-15T12:57:55Z

Content.Client/Chat/Managers/ChatManager.cs

-    }
-
-    public void SendMessage(string text, ChatSelectChannel channel)
+    public void SendMessage(string text, ChatSelectChannel channel, RadioChannelPrototype? radioChannel)
    {
        var str = text.ToString();
        switch (channel)


this looks like old-old code, is it left there just for backwards compatibility? If so i'd like to propos Proxy or Adapter design pattern here <_< low priority thing ofc...

maybe a little of todo with a link to separate issue for cleanup? even if issue will be placeholder - u would be able to fill it up later :godo:

Fildrance · 2024-12-15T13:00:20Z

Content.Client/Chat/Managers/ChatManager.cs

+    {
+        var str = text.ToString();
+
+        _consoleHost.ExecuteCommand($"chat {channel.ID} \"{CommandParsing.Escape(str)}\"");


i would like to remove this in favour of direct message/event sending, launching some console commands looks ugly Q_Q
It also can cause unexpected results... but thats of none concern as we were living with this for so long...

There is a PR by ElectroSr that does this; it would have some minor merge conflicts, but I agree it would be nice to have.

sure, valid. i agree.

Fildrance · 2024-12-15T13:03:07Z

Content.Client/Chat/Managers/ContentMarkupTagManager.cs

+{
+    public Dictionary<string, IContentMarkupTag> ContentMarkupTagTypes => new IContentMarkupTag[]
+    {
+        new ColorValueContentTag(),


it would be cool if code of DI would just scan assembly for all implementation of IContentMarkupTag and inject them there, and its easy to do! BUT! i am not sure if assembly-scanning is ok for sandboxing, and 2 - i'm not sure if maintainers are okay with that practice.

There is a similar implementation for RobustToolbox's MarkupTagTypes that does it this way, and cites sandboxing as the reason why. So it might be a limitation we have to live with. I agree it's not optimal.

well, i guess we are stuck with this. Well it is readable at least!

Fildrance · 2024-12-15T13:04:54Z

Content.Client/Chat/Managers/ContentMarkupTagManager.cs

+
+public sealed class ContentMarkupTagManager : ISharedContentMarkupTagManager
+{
+    public Dictionary<string, IContentMarkupTag> ContentMarkupTagTypes => new IContentMarkupTag[]


u decalare it as Dictionary, so u are OK with changing it in runtime? I'm not 100% sure what about maintainers policy on that but i'd better declare it as IReadOnlyDictionary if u want it initialized and never changed after creation. Maybe some frozen dictionary for bettere perf but same semantics?

Fildrance · 2024-12-15T13:06:06Z

Content.Client/Chat/Managers/IChatManager.cs


 namespace Content.Client.Chat.Managers
 {
    public interface IChatManager : ISharedChatManager
    {
-        public void SendMessage(string text, ChatSelectChannel channel);
+        public void SendMessage(string text, ChatSelectChannel channel, RadioChannelPrototype? radioChannel);


but this ine is cursed :pepeHands: why do we leave it be and not kill it with fre? it is called RadioChannel but is changeling hiveming radio channel? thats all so confusing so i'd be better off without it... but oh well...

Fildrance · 2024-12-15T13:10:28Z

Content.Client/Chat/MarkupTags/ColorValueContentTag.cs

+
+    public List<MarkupNode>? OpenerProcessing(MarkupNode node)
+    {
+        return new List<MarkupNode>() { new MarkupNode("color", new MarkupParameter(ColorValueLookup(node)), null) };


i'm a little bit afraid of perf for heavily loaded server with all thouse lists - i'm not agitating to do some pre-emptive optimizations but it might be interesting to make all chat code allocation-less... maybe later!

Fildrance · 2024-12-15T13:12:56Z

Content.Client/Chat/MarkupTags/EntityNameHeaderContentTag.cs

+    public List<MarkupNode>? OpenerProcessing(MarkupNode node)
+    {
+
+        var list = new List<MarkupNode>();


dont allocate if u dont use! : D i'm not 100% sure it will be optimized by compiler so better move it down after IF yourself.

Fildrance · 2024-12-15T13:13:05Z

Content.Client/Chat/MarkupTags/SessionNameHeaderContentTag.cs

+    public List<MarkupNode>? OpenerProcessing(MarkupNode node)
+    {
+
+        var list = new List<MarkupNode>();


Fildrance · 2024-12-15T13:18:02Z

Content.Client/Chat/UI/SpeechBubble.cs

@@ -15,6 +15,7 @@ public abstract class SpeechBubble : Control
    {
        [Dependency] private readonly IEyeManager _eyeManager = default!;
        [Dependency] private readonly IEntityManager _entityManager = default!;
+        [Dependency] protected readonly ISharedChatManager _chatManager = default!;


i'd say this one needs updating to adapt usage of our protos >_> so maybe leave todo at least? SpeechType enum is irritating for example...

Fildrance · 2024-12-15T13:28:02Z

Content.Client/Entry/EntryPoint.cs

@@ -72,6 +73,7 @@ public sealed class EntryPoint : GameClient
        [Dependency] private readonly ILogManager _logManager = default!;
        [Dependency] private readonly DebugMonitorManager _debugMonitorManager = default!;
        [Dependency] private readonly TitleWindowManager _titleWindowManager = default!;
+        [Dependency] private readonly ISharedContentMarkupTagManager _contentMarkupTagManager = default!;


<_< what is that? i mean EntryPoint class... and why is manager needed here...

Very good question; I think this is from me trying to get the content markup working, finding another fix and forgetting to remove this line.

Fildrance · 2024-12-15T13:37:16Z

Content.Client/UserInterface/Systems/Chat/Controls/ChannelFilterPopup.xaml.cs

@@ -17,6 +17,7 @@ public sealed partial class ChannelFilterPopup : Popup
        ChatChannel.Emotes,
        ChatChannel.Radio,
        ChatChannel.Notifications,
+        ChatChannel.Announcements,


read from protos too? ;w;

maybe leave todo...

Fildrance · 2024-12-15T13:41:05Z

Content.Server/Administration/Commands/DSay.cs

@@ -33,8 +35,7 @@ public void Execute(IConsoleShell shell, string argStr, string[] args)
            if (string.IsNullOrEmpty(message))
                return;

-            var chat = _e.System<ChatSystem>();
-            chat.TrySendInGameOOCMessage(entity, message, InGameOOCChatType.Dead, false, shell, player);
+            _chat.SendChannelMessage(message, "Dead", player, entity);


no magic strings pls, use protoId validation and maybe separate class for such dedicated constants :3

So maybe do some 'public static class ChatChannels' and declare all of channels that are 'usually required' there (stuff that u do not think people gonna easily remove from protos? : D). Declare them with ProtoIdValidate attribute and add xml-doc that this is just helper list for channel names constants (so ppl won't think they NEED to add const there for it to actually work')

Fildrance · 2024-12-15T14:19:40Z

Content.Server/Chat/Commands/LOOCCommand.cs

@@ -35,7 +35,7 @@ public void Execute(IConsoleShell shell, string argStr, string[] args)
            if (string.IsNullOrEmpty(message))
                return;

-            _e.System<ChatSystem>().TrySendInGameOOCMessage(entity, message, InGameOOCChatType.Looc, false, shell, player);
+            _e.System<ChatSystem>().TrySendInGameOOCMessage(entity, message, player);


wierd.... u have communication way (channel) for ooc but there u are using separate method... is that correct?

It's just a helper method; could very easily be replaced with SendChannelMessage entirely (as soon as I handle the whole magic strings issue)

oh sorry then! if its just helper for usual way - its fine! i was just confused coz its kinda looking like old dirty methods.

Fildrance · 2024-12-15T14:23:39Z

Content.Server/Content.Server.csproj

@@ -26,5 +26,8 @@
    <ProjectReference Include="..\RobustToolbox\Robust.Server\Robust.Server.csproj" />
    <ProjectReference Include="..\Content.Shared\Content.Shared.csproj" />
  </ItemGroup>
+  <ItemGroup>


can be safely removed

Fildrance · 2024-12-15T14:25:30Z

Content.Server/GameTicking/GameTicker.cs

@@ -115,7 +115,7 @@ public override void Shutdown()
        private void SendServerMessage(string message)
        {
            var wrappedMessage = Loc.GetString("chat-manager-server-wrap-message", ("message", message));
-            _chatManager.ChatMessageToAll(ChatChannel.Server, message, wrappedMessage, default, false, true);
+            _chatManager.SendChannelMessage(wrappedMessage, "Server", null, null);


eeeeh, why use overload with exra null parameters? it looks wierd...

Fildrance · 2024-12-15T14:33:29Z

Content.Server/Salvage/SalvageSystem.Runner.cs

-            false,
-            true,
-            null);
+        // CHAT-TODO: This is wrong - should not take the mapUid as an argument and instead it should be baked into the radio prototype


how is it even used! Q_Q

FairlySadPanda

I've left random comments across the files, mostly just flagging magic strings... but the overall review is that this has a few revisions it needs to do:

Avoidable API changes should be avoided for external callers. A substantial amount of the file changes are oneliners that change the API call from the old chat management call to the new system. This repetitive work is pointless, it makes the PR harder to review, and can be remedied by keeping the old API around when possible and adapting the old API to the new one via adaption functions.
There's a few instances of code that's sitting next to old code when it would be better broken into one or more partial classes. This would allow the PR to be largely append-only, minimizing downstream impact and making what you're trying to do clearer.
The "compose channels of specific conditions" design is brittle and prescriptive. Any change to any of the conditions could break the design of any number of channels. You can get the same level of flexibility here by encapsulating the send/receive/mutation rules of a given channel by creating C# classes that control the "rules" that a channel uses, with exposed variables and options to configure that ruleset. This has the advantage of balancing what you want to do - an extensible, modular system - with keeping complexity low.
The process of sending a chat message from emission to reception requires domentation and graphing. As part of this you need to demonstrate how other developers would extend or listen in to parts of the system to customize it via raised events.
Engineering "server-side" and "game-side" chat using the same systems I think is a dead end. They really should be different systems. It's part of the complexity of the blob-like current system.
This really needs an understanding from the admins of what features the admins want. Chat is a key aspect of the game for admins and the current tools for dealing with it are insufficient; sanitization is only one half of the problem. This system needs to support per-message editing and deletion and mass deletion of chat messages. I'd encourage you to sit down with @nikthechampiongr and the rest of the admin team and get the requirements off them.

Overall this is movement in the right direction but it needs reigning in in places. Deffo for the best you didn't make my mistake of finishing an entire end-to-end attempt before asking for feedback.

FairlySadPanda · 2024-12-14T21:24:46Z

Content.Client/Chat/Managers/ChatManager.cs

@@ -12,26 +15,20 @@ internal sealed class ChatManager : IChatManager
    [Dependency] private readonly IClientConsoleHost _consoleHost = default!;
    [Dependency] private readonly IClientAdminManager _adminMgr = default!;
    [Dependency] private readonly IEntitySystemManager _systems = default!;
+    [Dependency] private readonly IPrototypeManager _prototypeManager = default!;


Most important feedback is going to be that additions to the existing systems can be safely contained in a partial for the time being; this makes it easier to extend and review and has lower chance of causing merge conflicts on downstreams.

FairlySadPanda · 2024-12-14T21:38:48Z

Content.Client/Chat/Managers/ChatManager.cs

-    }
-
-    public void SendMessage(string text, ChatSelectChannel channel)
+    public void SendMessage(string text, ChatSelectChannel channel, RadioChannelPrototype? radioChannel)


Given if RadioChannelPrototype is set, it must be a radio message, this optional is superfluous, especially given there is already overloading of SendMessage below. It would make more sense to create an overload for SendMessage specifically for Radio.

public void SendMessage(string text, RadioChannelPrototype radioChannel)

FairlySadPanda · 2024-12-14T21:39:51Z

Content.Client/Chat/Managers/ChatManager.cs

            case ChatSelectChannel.Radio:
+                if (radioChannel != null)


In general avoid nesting ifs inside of switches. Switches are there specifically to avoid cyclo. In this case this should be two cases; however this code should be moved to its own overload.

FairlySadPanda · 2024-12-14T21:41:29Z

Content.Client/Chat/Managers/ChatManager.cs

@@ -2,7 +2,10 @@
 using Content.Client.Ghost;


This file still running console commands for most chat methods is not ideal.

FairlySadPanda · 2024-12-14T21:42:37Z

Content.Client/Chat/Managers/ContentMarkupTagManager.cs

+
+namespace Content.Client.Chat.Managers;
+
+public sealed class ContentMarkupTagManager : ISharedContentMarkupTagManager


Sticking a general comment here and won't repeat it; considering this is greenfield code in a notoriously horrible area of the codebase, ensure that the code has summary docstrings and in general has your intent communicated.

FairlySadPanda · 2024-12-15T22:27:57Z

Content.Server/StationEvents/Events/MeteorSwarmSystem.cs

@@ -32,7 +34,7 @@ protected override void Added(EntityUid uid, MeteorSwarmComponent component, Gam
        Filter allPlayersInGame = Filter.Empty().AddWhere(GameTicker.UserHasJoinedGame);

        if (component.Announcement is { } locId)
-            _chat.DispatchFilteredAnnouncement(allPlayersInGame, Loc.GetString(locId), playSound: false, colorOverride: Color.Gold);
+            _chatManager.SendChannelMessage(Loc.GetString(locId), "GameMessage", null, null);


Magic string needs removing

FairlySadPanda · 2024-12-15T22:28:08Z

Content.Server/StationEvents/Events/StationEventSystem.cs

@@ -46,7 +48,7 @@ protected override void Added(EntityUid uid, T component, GameRuleComponent game
        Filter allPlayersInGame = Filter.Empty().AddWhere(GameTicker.UserHasJoinedGame);

        if (stationEvent.StartAnnouncement != null)
-            ChatSystem.DispatchFilteredAnnouncement(allPlayersInGame, Loc.GetString(stationEvent.StartAnnouncement), playSound: false, colorOverride: stationEvent.StartAnnouncementColor);
+            ChatManager.SendChannelMessage(Loc.GetString(stationEvent.StartAnnouncement), "GameMessage", null, null);


Magic string needs removing

FairlySadPanda · 2024-12-15T22:28:23Z

Content.Server/StationEvents/Events/StationEventSystem.cs

+            ChatManager.SendChannelMessage(Loc.GetString(stationEvent.EndAnnouncement), "GameMessage", null, null);



Magic string needs removing

FairlySadPanda · 2024-12-15T22:32:47Z

Content.Server/Tips/TipsSystem.cs

@@ -198,8 +198,7 @@ private void AnnounceRandomTip()
            RaiseNetworkEvent(ev);
        } else
        {
-            _chat.ChatMessageToManyFiltered(Filter.Broadcast(), ChatChannel.OOC, tip, msg,
-            EntityUid.Invalid, false, false, Color.MediumPurple);
+            _chat.SendChannelMessage(msg, "Server", null, null);


Magic string needs removing

FairlySadPanda · 2024-12-15T22:33:05Z

Content.Server/Voting/Managers/VoteManager.DefaultVotes.cs

@@ -415,7 +415,7 @@ private async void CreateVotekickVote(ICommonSession? initiator, string[]? args)
                {
                    var message = Loc.GetString("ui-vote-votekick-not-enough-eligible", ("voters", eligibleVoterNumber.ToString()), ("requirement", eligibleVoterNumberRequirement.ToString()));
                    var wrappedMessage = Loc.GetString("chat-manager-server-wrap-message", ("message", message));
-                    _chatManager.ChatMessageToOne(ChatChannel.Server, message, wrappedMessage, default, false, initiator.Channel);
+                    _chatManager.SendChannelMessage(wrappedMessage, "Server", null, null, new HashSet<ICommonSession>() { initiator });


Magic string needs removing

sleepyyapril · 2024-12-16T10:17:51Z

Does this currently support a BeforeChatSendEvent or similar? If not, it should!

Kadeo64 · 2024-12-16T12:57:30Z

holy shit praise slambam

SlamBamActionman added 4 commits November 29, 2024 15:26

Stage 1, adding stuff before deleting/changing existing stuff

9942071

Second go, moving stuff to IChatManager

a76e449

Woe whoever has to review this

c4bee23

More changes, MORE!

c5e32e9

SlamBamActionman requested review from metalgearsloth, Jezithyr, TheShuEd, DrSmugleaf and nikthechampiongr as code owners December 14, 2024 16:19

SlamBamActionman marked this pull request as draft December 14, 2024 16:22

github-actions bot added the S: Merge Conflict Status: Needs to resolve merge conflicts before it can be accepted label Dec 14, 2024

chromiumboy mentioned this pull request Dec 14, 2024

Holopads #32711

Open

2 tasks

Fildrance reviewed Dec 15, 2024

View reviewed changes

FairlySadPanda reviewed Dec 15, 2024

View reviewed changes


		namespace Content.Client.Chat.Managers;

		public sealed class ContentMarkupTagManager : ISharedContentMarkupTagManager

		ChatManager.SendChannelMessage(Loc.GetString(stationEvent.EndAnnouncement), "GameMessage", null, null);

Chat Refactor [DRAFT] #33858

Are you sure you want to change the base?

Chat Refactor [DRAFT] #33858

Conversation

SlamBamActionman commented Dec 14, 2024 • edited Loading

About the PR

CommunicationChannelPrototype

SendChannelMessage

ContentMarkupTags

ListenerEntitySystem & ListenerComponent

Areas of concern

Missing features

Media

Requirements

Breaking changes

github-actions bot commented Dec 14, 2024

FairlySadPanda commented Dec 14, 2024

FairlySadPanda commented Dec 14, 2024

Djungelskog2 commented Dec 15, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FairlySadPanda left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sleepyyapril commented Dec 16, 2024

Kadeo64 commented Dec 16, 2024

SlamBamActionman commented Dec 14, 2024 •

edited

Loading

FairlySadPanda left a comment •

edited

Loading