Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Chat Refactor [DRAFT] #33858

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 34 additions & 12 deletions Content.Client/Chat/Managers/ChatManager.cs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,10 @@
using Content.Client.Ghost;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

using Content.Shared.Administration;
using Content.Shared.Chat;
using Content.Shared.Chat.Prototypes;
using Content.Shared.Radio;
using Robust.Client.Console;
using Robust.Shared.Prototypes;
using Robust.Shared.Utility;

namespace Content.Client.Chat.Managers;
Expand All @@ -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!;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


private ISawmill _sawmill = default!;

[ValidatePrototypeId<RadioChannelPrototype>]
private const string DefaultRadioChannel = "Common";

public void Initialize()
{
_sawmill = Logger.GetSawmill("chat");
_sawmill.Level = LogLevel.Info;
}

public void SendAdminAlert(string message)
{
// See server-side manager. This just exists for shared code.
}

public void SendAdminAlert(EntityUid player, string message)
{
// See server-side manager. This just exists for shared code.
}

public void SendMessage(string text, ChatSelectChannel channel)
public void SendMessage(string text, ChatSelectChannel channel, RadioChannelPrototype? radioChannel)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

{
var str = text.ToString();
switch (channel)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

Expand Down Expand Up @@ -67,8 +64,18 @@ public void SendMessage(string text, ChatSelectChannel channel)
_sawmill.Warning("Tried to speak on deadchat without being ghost or admin.");
break;

// TODO sepearate radio and say into separate commands.
case ChatSelectChannel.Radio:
if (radioChannel != null)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

{
SendMessage(str, "Radio" + radioChannel.ID);
}
else
{
radioChannel = _prototypeManager.Index<RadioChannelPrototype>(DefaultRadioChannel);
SendMessage(str, "Radio" + radioChannel.ID);
}
break;

case ChatSelectChannel.Local:
_consoleHost.ExecuteCommand($"say \"{CommandParsing.Escape(str)}\"");
break;
Expand All @@ -81,4 +88,19 @@ public void SendMessage(string text, ChatSelectChannel channel)
throw new ArgumentOutOfRangeException(nameof(channel), channel, null);
}
}

public void SendMessage(string text, string channel)
{
if (_prototypeManager.TryIndex(channel, out CommunicationChannelPrototype? channelPrototype))
{
SendMessage(text, channelPrototype);
}
}

public void SendMessage(string text, CommunicationChannelPrototype channel)
{
var str = text.ToString();

_consoleHost.ExecuteCommand($"chat {channel.ID} \"{CommandParsing.Escape(str)}\"");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure, valid. i agree.

}
}
17 changes: 17 additions & 0 deletions Content.Client/Chat/Managers/ContentMarkupTagManager.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
using System.Linq;
using Content.Client.Chat.MarkupTags;
using Content.Shared.Chat;
using Content.Shared.Chat.ContentMarkupTags;

namespace Content.Client.Chat.Managers;

public sealed class ContentMarkupTagManager : ISharedContentMarkupTagManager
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

{
public Dictionary<string, IContentMarkupTag> ContentMarkupTagTypes => new IContentMarkupTag[]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

{
new ColorValueContentTag(),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

new EntityNameHeaderContentTag(),
new SessionNameHeaderContentTag(),
new SpeechVerbContentTag(),
}.ToDictionary(x => x.Name, x => x);
}
8 changes: 7 additions & 1 deletion Content.Client/Chat/Managers/IChatManager.cs
Original file line number Diff line number Diff line change
@@ -1,9 +1,15 @@
using Content.Shared.Chat;
using Content.Shared.Chat.Prototypes;
using Content.Shared.Radio;

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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...


public void SendMessage(string text, string channel);

public void SendMessage(string text, CommunicationChannelPrototype channel);
}
}
62 changes: 62 additions & 0 deletions Content.Client/Chat/MarkupTags/ColorValueContentTag.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
using System.Diagnostics;
using Content.Shared.Chat.ContentMarkupTags;
using Robust.Shared.Utility;

namespace Content.Client.Chat.MarkupTags;

public sealed class ColorValueContentTag : IContentMarkupTag
{
public string Name => "ColorValue";

// TODO: These values should probably be retrieved via yaml, and be customizable in options! This solution works for now!
private Dictionary<string, Color> _colors = new Dictionary<string, Color>()
{
["Base"] = Color.White,
["Speech"] = Color.LightGray,
["Emote"] = Color.LightGray,
["Whisper"] = Color.DarkGray,
["LOOC"] = Color.MediumTurquoise,
["OOC"] = Color.LightSkyBlue,
["Dead"] = Color.MediumPurple,
["IngameAnnouncement"] = Color.Yellow,
["IngameAlert"] = Color.Red,
["Server"] = Color.Orange,
["AdminChat"] = Color.HotPink,
["AdminAlert"] = Color.Red,
["Radio.Common"] = Color.FromHex("#2cdb2c"),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Obviously this is temp code but doc these if this is gonna get merged so folks know what the colour is without their IDE having a plugin or whatever.

["Radio.Security"] = Color.FromHex("#ff4242"),
["Radio.Supply"] = Color.FromHex("#b48b57"),
["Radio.Command"] = Color.FromHex("#fcdf03"),
["Radio.Service"] = Color.FromHex("#539c00"),
["Radio.Science"] = Color.FromHex("#cd7ccd"),
["Radio.Engineering"] = Color.FromHex("#ff733c"),
["Radio.Medical"] = Color.FromHex("#57b8f0"),
["Radio.Syndicate"] = Color.FromHex("#8f4a4b"),
["Radio.Freelance"] = Color.FromHex("#f6ce64"),
["Radio.Service"] = Color.FromHex("#539c00"),
["Radio.Binary"] = Color.FromHex("#2ed2fd"),
["Radio.CentCom"] = Color.FromHex("#2681a5"),
["Radio.Handheld"] = Color.FromHex("#967101"),
};

public List<MarkupNode>? OpenerProcessing(MarkupNode node)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Methods should be named with active verbs; this should be ProcessOpener for example.

{
return new List<MarkupNode>() { new MarkupNode("color", new MarkupParameter(ColorValueLookup(node)), null) };
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

}

public List<MarkupNode>? CloserProcessing(MarkupNode node)
{
return new List<MarkupNode>() { new MarkupNode("color", null, null, true) };
}

public Color ColorValueLookup(MarkupNode node)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Getters like these should be GetColor or somesuch.

{
var markupColor = node.Value.StringValue;
if (markupColor != null && _colors.TryGetValue(markupColor, out var color))
return color;

// CHAT-TODO: Log erroneous color attempt.
Logger.Debug("This should not run!");
return Color.White;
}
}
31 changes: 31 additions & 0 deletions Content.Client/Chat/MarkupTags/EntityNameHeaderContentTag.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
using System.Diagnostics;
using System.Linq;
using Content.Shared.CCVar;
using Content.Shared.Chat.ContentMarkupTags;
using Content.Shared.Decals;
using FastAccessors;
using Robust.Shared.Configuration;
using Robust.Shared.GameObjects.Components.Localization;
using Robust.Shared.Prototypes;
using Robust.Shared.Utility;

namespace Content.Client.Chat.MarkupTags;

public sealed class EntityNameHeaderContentTag : IContentMarkupTag
{
public string Name => "EntityNameHeader";

public List<MarkupNode>? OpenerProcessing(MarkupNode node)
{

var list = new List<MarkupNode>();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

var name = node.Value.StringValue;

if (name == null)
return null;

list.Add(new MarkupNode(name));

return list;
}
}
31 changes: 31 additions & 0 deletions Content.Client/Chat/MarkupTags/SessionNameHeaderContentTag.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
using System.Diagnostics;
using System.Linq;
using Content.Shared.CCVar;
using Content.Shared.Chat.ContentMarkupTags;
using Content.Shared.Decals;
using FastAccessors;
using Robust.Shared.Configuration;
using Robust.Shared.GameObjects.Components.Localization;
using Robust.Shared.Prototypes;
using Robust.Shared.Utility;

namespace Content.Client.Chat.MarkupTags;

public sealed class SessionNameHeaderContentTag : IContentMarkupTag
{
public string Name => "SessionNameHeader";

public List<MarkupNode>? OpenerProcessing(MarkupNode node)
{

var list = new List<MarkupNode>();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same

var name = node.Value.StringValue;

if (name == null)
return null;

list.Add(new MarkupNode(name));

return list;
}
}
41 changes: 23 additions & 18 deletions Content.Client/Chat/UI/SpeechBubble.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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!;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

[Dependency] protected readonly IConfigurationManager ConfigManager = default!;
private readonly SharedTransformSystem _transformSystem;

Expand Down Expand Up @@ -73,14 +74,14 @@ public static SpeechBubble CreateSpeechBubble(SpeechType type, ChatMessage messa
return new FancyTextSpeechBubble(message, senderEntity, "whisperBox");

case SpeechType.Looc:
return new TextSpeechBubble(message, senderEntity, "emoteBox", Color.FromHex("#48d1cc"));
return new TextSpeechBubble(message, senderEntity, "emoteBox");

default:
throw new ArgumentOutOfRangeException();
}
}

public SpeechBubble(ChatMessage message, EntityUid senderEntity, string speechStyleClass, Color? fontColor = null)
public SpeechBubble(ChatMessage message, EntityUid senderEntity, string speechStyleClass)
{
IoCManager.InjectDependencies(this);
_senderEntity = senderEntity;
Expand All @@ -89,7 +90,7 @@ public SpeechBubble(ChatMessage message, EntityUid senderEntity, string speechSt
// Use text clipping so new messages don't overlap old ones being pushed up.
RectClipContent = true;

var bubble = BuildBubble(message, speechStyleClass, fontColor);
var bubble = BuildBubble(message, speechStyleClass);

AddChild(bubble);

Expand All @@ -100,7 +101,7 @@ public SpeechBubble(ChatMessage message, EntityUid senderEntity, string speechSt
_verticalOffsetAchieved = -ContentSize.Y;
}

protected abstract Control BuildBubble(ChatMessage message, string speechStyleClass, Color? fontColor = null);
protected abstract Control BuildBubble(ChatMessage message, string speechStyleClass);

protected override void FrameUpdate(FrameEventArgs args)
{
Expand Down Expand Up @@ -184,28 +185,28 @@ protected FormattedMessage FormatSpeech(string message, Color? fontColor = null)
return msg;
}

protected FormattedMessage ExtractAndFormatSpeechSubstring(ChatMessage message, string tag, Color? fontColor = null)
/*protected FormattedMessage ExtractAndFormatSpeechSubstring(ChatMessage message, string tag, Color? fontColor = null)
{
return FormatSpeech(SharedChatSystem.GetStringInsideTag(message, tag), fontColor);
}
}*/

}

public sealed class TextSpeechBubble : SpeechBubble
{
public TextSpeechBubble(ChatMessage message, EntityUid senderEntity, string speechStyleClass, Color? fontColor = null)
: base(message, senderEntity, speechStyleClass, fontColor)
public TextSpeechBubble(ChatMessage message, EntityUid senderEntity, string speechStyleClass)
: base(message, senderEntity, speechStyleClass)
{
}

protected override Control BuildBubble(ChatMessage message, string speechStyleClass, Color? fontColor = null)
protected override Control BuildBubble(ChatMessage message, string speechStyleClass)
{
var label = new RichTextLabel
{
MaxWidth = SpeechMaxWidth,
};

label.SetMessage(FormatSpeech(message.WrappedMessage, fontColor));
label.SetMessage(message.Message);

var panel = new PanelContainer
{
Expand All @@ -221,12 +222,12 @@ protected override Control BuildBubble(ChatMessage message, string speechStyleCl
public sealed class FancyTextSpeechBubble : SpeechBubble
{

public FancyTextSpeechBubble(ChatMessage message, EntityUid senderEntity, string speechStyleClass, Color? fontColor = null)
: base(message, senderEntity, speechStyleClass, fontColor)
public FancyTextSpeechBubble(ChatMessage message, EntityUid senderEntity, string speechStyleClass)
: base(message, senderEntity, speechStyleClass)
{
}

protected override Control BuildBubble(ChatMessage message, string speechStyleClass, Color? fontColor = null)
protected override Control BuildBubble(ChatMessage message, string speechStyleClass)
{
if (!ConfigManager.GetCVar(CCVars.ChatEnableFancyBubbles))
{
Expand All @@ -235,8 +236,6 @@ protected override Control BuildBubble(ChatMessage message, string speechStyleCl
MaxWidth = SpeechMaxWidth
};

label.SetMessage(ExtractAndFormatSpeechSubstring(message, "BubbleContent", fontColor));

var unfanciedPanel = new PanelContainer
{
StyleClasses = { "speechBox", speechStyleClass },
Expand All @@ -258,9 +257,15 @@ protected override Control BuildBubble(ChatMessage message, string speechStyleCl
StyleClasses = { "bubbleContent" }
};

//We'll be honest. *Yes* this is hacky. Doing this in a cleaner way would require a bottom-up refactor of how saycode handles sending chat messages. -Myr
bubbleHeader.SetMessage(ExtractAndFormatSpeechSubstring(message, "BubbleHeader", fontColor));
bubbleContent.SetMessage(ExtractAndFormatSpeechSubstring(message, "BubbleContent", fontColor));

if (_chatManager.TryGetMessageInsideTag(message.Message, out var bubbleHeaderMsg, "BubbleHeader") &&
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a perennial problem with Wizden code, especially Wizden UI code, but try and avoid magic strings and put this as a STATIC_CONST elsewhere.

_chatManager.TryGetMessageInsideTag(message.Message, out var bubbleMessageMsg, "BubbleMessage"))
{
bubbleHeader.SetMessage(bubbleHeaderMsg);
bubbleContent.SetMessage(bubbleMessageMsg);
}
//bubbleHeader.SetMessage(ExtractAndFormatSpeechSubstring(message, "BubbleHeader", fontColor));
//bubbleContent.SetMessage(ExtractAndFormatSpeechSubstring(message, "BubbleContent", fontColor));

//As for below: Some day this could probably be converted to xaml. But that is not today. -Myr
var mainPanel = new PanelContainer
Expand Down
2 changes: 2 additions & 0 deletions Content.Client/Entry/EntryPoint.cs
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
using Content.Client.Viewport;
using Content.Client.Voting;
using Content.Shared.Ame.Components;
using Content.Shared.Chat;
using Content.Shared.Gravity;
using Content.Shared.Localizations;
using Robust.Client;
Expand Down Expand Up @@ -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!;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


public override void Init()
{
Expand Down
1 change: 1 addition & 0 deletions Content.Client/IoC/ClientContentIoC.cs
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@ public static void Register()
collection.Register<PlayerRateLimitManager>();
collection.Register<SharedPlayerRateLimitManager, PlayerRateLimitManager>();
collection.Register<TitleWindowManager>();
collection.Register<ISharedContentMarkupTagManager, ContentMarkupTagManager>();
}
}
}
Loading
Loading