Overhaul IRC in preparation for the global chat UI #9586

pchote · 2015-10-10T13:57:34Z

The existing code didn't scale to multiple panels, so I have reorganized and simplified it in preparation for the lobby and ingame integration.

This mainly focuses on the backend code, but there are a few visible changes:

Polished topic and op/voice status indicators.
Correctly handles kicks and errors.
Distinguishes between chat and notifications.

pchote · 2015-10-10T15:04:08Z

The dispose commit will need to be redone. It currently disposes even when a window is temporarily hidden, which then breaks the chat when it is added to the game lobby (doesn't affect testing in the server brwoser, though).

Mailaender · 2015-10-10T15:21:58Z

OpenRA.Mods.Common/Widgets/Logic/ServerBrowserLogic.cs

-			{
-				Log.Write("debug", "Failed to load server browser IRC chrome layout");
-			}
+			Game.LoadWidget(null, "GLOBALCHAT_PANEL", panel.Get("GLOBALCHAT_ROOT"), new WidgetArgs());


This will make the IRC feature mandatory for every mod. Attention @phrohdoh this will break your RA2 branch.

I don't think we should be making any widget mandatory, and we shouldn't force (our!) IRC on thirdparty mods.

Mods that don't want this can define a dummy GLOBALCHAT_PANEL widget to disable it. The try/catch makes it impossible to debug any errors in the IRC logic, and leaves a bogus widget state when it does die.

pchote · 2015-10-10T15:24:19Z

Fixed the disposing logic.

Mailaender · 2015-10-10T15:42:27Z

OpenRA.Mods.Common/Widgets/Logic/GlobalChatLogic.cs

+			var font = Game.Renderer.Fonts[widget.Font];
+
+			var color = message.Type == ChatMessageType.Notification ?
+				Color.LightGray : Color.White;


I like the subtle color differentiation, but I guess this wants to live in metrics.yaml instead.

pchote · 2015-10-11T12:15:47Z

This can crash if the IRC thread adds a message to the History or Users collections while the UI thread is enumerating them. I'll need to ensure that all dictionary accesses are done on the main thread - running just the widget updates in a RunAfterTick is not enough.

RoosterDragon · 2015-10-12T18:48:29Z

OpenRA.Game/GlobalChat.cs

+		}
+	}
+
+	public sealed class ChatUserComparer : IComparer<string>


Use one of the static members on System.StringComparer and save yourself this class.

pchote · 2015-10-12T18:56:13Z

Fixed all above comments.

RoosterDragon · 2015-10-12T19:01:45Z

OpenRA.Game/GlobalChat.cs

+
+	public class GlobalChat : IDisposable
+	{
+		volatile IrcClient client;


volatile is pointless here.

This was carried over from the earlier code, where I believe you asked for it yourself :P.
But you're right - this is only ever accessed from the worker thread now.

RoosterDragon · 2015-10-12T19:26:50Z

OpenRA.Game/GlobalChat.cs

+		{
+			// Set a random default nick
+			if (Game.Settings.Chat.Nickname == new ChatSettings().Nickname)
+				Game.Settings.Chat.Nickname += Game.CosmeticRandom.Next(100, 999);


Modifying globals from a random static constructor is just asking for trouble with initialization order. You should move this somewhere more deterministic.

pchote · 2015-10-12T20:34:52Z

Updated.

RoosterDragon · 2015-10-12T20:38:03Z

OpenRA.Game/GlobalChat.cs

+			connectionStatus = ChatConnectionStatus.Connected;
+
+			// Guard against settings.yaml modification
+			Game.Settings.Chat.Nickname = SanitizedName(Game.Settings.Chat.Nickname);


This write isn't thread-safe (since we usually only muck with globals on the UI thread).

pchote · 2015-10-12T21:19:27Z

Fixed some more. Also fixed an inherited jank when first connecting to the IRC sever.

RoosterDragon · 2015-10-14T19:06:51Z

OpenRA.Game/GlobalChat.cs

+					Game.RunAfterTick(() => Log.Write("irc", e.ToString()));
+				}
+
+				client.Listen();


If the connect fails, do we still want to listen?

abcdefg30 · 2015-10-16T22:18:39Z

Chat still works. ✅ / 👍

* Simplified UI plumbing. * Improves handling of errors and kicks. * Persists chat history between session. * Fixes leaks of the old widget tree when exiting. * A few small UI polish improvements.

pchote · 2015-10-17T21:13:40Z

Updated.

obrakmann · 2015-10-18T16:32:46Z

OpenRA.Game/GlobalChat.cs

+			foreach (DictionaryEntry user in channel.Users)
+			{
+				var u = (ChannelUser)user.Value;
+				Game.RunAfterTick(() => Users.Add(u.Nick, new ChatUser(u.Nick, u.IsOp, u.IsVoice)));


I guess this is supposed to sync the server's and client's state, right? If so, wouldn't you need to use Users.Clear() before repopulating the list? Or does this only happen once when joining?

I haven't read the docs on this callback, but @Mailaender set it up as if it were used exclusively on first join to the channel. If it may be called multiple times, it would indeed be necessary to clear the dict.

Looks like it only happens that once. At least it doesn't happen when users join or leave. Could be that it's run once every hour or so, but I didn't test that long.

obrakmann · 2015-10-18T17:31:03Z

Looks and works fine. 👍

Overhaul IRC in preparation for the global chat UI

obrakmann · 2015-10-18T17:32:55Z

Changelog

pchote added this to the Next release milestone Oct 10, 2015

pchote force-pushed the irc-common branch from b681d97 to 644f386 Compare October 10, 2015 13:59

pchote added the PR: Changes requested label Oct 10, 2015

Mailaender reviewed Oct 10, 2015
View reviewed changes

pchote force-pushed the irc-common branch from 644f386 to 8cdb90a Compare October 10, 2015 15:23

pchote removed the PR: Changes requested label Oct 10, 2015

Mailaender reviewed Oct 10, 2015
View reviewed changes

pchote added the PR: Changes requested label Oct 11, 2015

pchote force-pushed the irc-common branch from 8cdb90a to 6480596 Compare October 12, 2015 18:47

RoosterDragon reviewed Oct 12, 2015
View reviewed changes

pchote force-pushed the irc-common branch 3 times, most recently from 9c96722 to 189033c Compare October 12, 2015 18:55

pchote removed the PR: Changes requested label Oct 12, 2015

RoosterDragon reviewed Oct 12, 2015
View reviewed changes

pchote force-pushed the irc-common branch from 189033c to 16a3af6 Compare October 12, 2015 19:02

RoosterDragon reviewed Oct 12, 2015
View reviewed changes

pchote force-pushed the irc-common branch from 16a3af6 to 5307483 Compare October 12, 2015 20:32

RoosterDragon reviewed Oct 12, 2015
View reviewed changes

pchote force-pushed the irc-common branch 5 times, most recently from d6cf33f to 0464afa Compare October 12, 2015 21:19

pchote force-pushed the irc-common branch from 0464afa to 3a8dc3f Compare October 12, 2015 21:25

RoosterDragon reviewed Oct 14, 2015
View reviewed changes

abcdefg30 added the PR: Needs +2 label Oct 16, 2015

pchote added 3 commits October 17, 2015 22:13

Add Dispose support to widget logic objects.

4ba78f6

Implement ObservableList collection.

386f252

Overhaul the IRC implementation.

a3fe2b3

* Simplified UI plumbing. * Improves handling of errors and kicks. * Persists chat history between session. * Fixes leaks of the old widget tree when exiting. * A few small UI polish improvements.

pchote force-pushed the irc-common branch from 3a8dc3f to a3fe2b3 Compare October 17, 2015 21:13

obrakmann reviewed Oct 18, 2015
View reviewed changes

obrakmann added a commit that referenced this pull request Oct 18, 2015

Merge pull request #9586 from pchote/irc-common

c1dda97

Overhaul IRC in preparation for the global chat UI

obrakmann merged commit c1dda97 into OpenRA:bleed Oct 18, 2015

pchote deleted the irc-common branch October 21, 2015 20:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Overhaul IRC in preparation for the global chat UI #9586

Overhaul IRC in preparation for the global chat UI #9586

pchote commented Oct 10, 2015

pchote commented Oct 10, 2015

Mailaender Oct 10, 2015

penev92 Oct 10, 2015

pchote Oct 10, 2015

pchote commented Oct 10, 2015

Mailaender Oct 10, 2015

pchote commented Oct 11, 2015

RoosterDragon Oct 12, 2015

pchote commented Oct 12, 2015

RoosterDragon Oct 12, 2015

pchote Oct 12, 2015

RoosterDragon Oct 12, 2015

pchote commented Oct 12, 2015

RoosterDragon Oct 12, 2015

pchote commented Oct 12, 2015

RoosterDragon Oct 14, 2015

abcdefg30 commented Oct 16, 2015

pchote commented Oct 17, 2015

obrakmann Oct 18, 2015

pchote Oct 18, 2015

obrakmann Oct 18, 2015

obrakmann commented Oct 18, 2015

obrakmann commented Oct 18, 2015

Overhaul IRC in preparation for the global chat UI #9586

Overhaul IRC in preparation for the global chat UI #9586

Conversation

pchote commented Oct 10, 2015

pchote commented Oct 10, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pchote commented Oct 10, 2015

Choose a reason for hiding this comment

pchote commented Oct 11, 2015

Choose a reason for hiding this comment

pchote commented Oct 12, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pchote commented Oct 12, 2015

Choose a reason for hiding this comment

pchote commented Oct 12, 2015

Choose a reason for hiding this comment

abcdefg30 commented Oct 16, 2015

pchote commented Oct 17, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

obrakmann commented Oct 18, 2015

obrakmann commented Oct 18, 2015