-
-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Add chat tab to multiplayer/replays ingame menu #13408
Conversation
to free PR queue. |
I would really like this for next + 1. Can you reopen it? Or are you abandoning the feature completely? |
Ok, looked like no interest in this one. |
Interest, but not time |
var chatPanelContainer = widget.Get<ContainerWidget>("CHAT_PANEL"); | ||
var chatTabButton = widget.Get<ButtonWidget>(string.Concat("BUTTON", numTabs.ToString())); | ||
chatTabButton.Text = "Chat"; | ||
chatTabButton.IsVisible = () => true && numTabs > 1 && !hasError; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
true && ...
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm, removed.
Updated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good overall, just two points that I think need more discussion before merging:
|
||
chatScrollPanel = chatChrome.Get<ScrollPanelWidget>("CHAT_SCROLLPANEL"); | ||
chatTemplate = chatScrollPanel.Get<ContainerWidget>("CHAT_TEMPLATE"); | ||
chatScrollPanel.RemoveChildren(); | ||
chatScrollPanel.ScrollToBottom(); | ||
|
||
foreach (var chatLine in orderManager.ChatCache) | ||
foreach (var chatLine in orderManager.ChatCache.Skip(isMenuChat && orderManager.ChatCache.Count() > 100 ? orderManager.ChatCache.Count() - 20 : 0)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems a bit arbitrary: if there are 99 messages you see 99, but if there are 100 you only see 20?
What is the motivation for introducing this for the menu chat, but not for the regular chat?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
iirc ingame chat only adds messages, doesn't have to display all messages from history since match start on each opening while in menu Chat tab, ChatCache contains always all messages (I think). The idea was it is more important to be able to continue chatting in Chat tab than be able to see all history so this was to avoid displaying hundreds of messages.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The ingame chat will show the full history if you open the chat dialog.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but ctor (that foreach) runs once for ingame chat, so it is done only first time (usually history small at this moment) while ctor for Chat tab runs each time you go to Options (or only when Chat tab, I don't remember, anyway more times).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this introduce a noticeable performance hitch in practice?
Game.LoadWidget(world, "CHAT_CONTAINER", chatPanelContainer, new WidgetArgs() { { "isMenuChat", true } }); | ||
|
||
if (activePanel == IngameInfoPanel.AutoSelect || world.IsGameOver) | ||
chatTabButton.OnClick(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mentioned this in IRC right at the start, and it is still a major sticking point for me.
When the game finishes we automatically open the menu to show the objectives screen, as a combined mission accomplished/failed and score screen. Opening the menu automatically to show the chat window is... a bit weird really, and IMO a feature regression.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I will remove it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A separate PR that could offer a good compromise here would be to remember and restore the last active tab when closing/reopening the menu (except for the game-end auto-open case, which should force the objectives tab). This could be done by adding an IngameInfoPanel
argument to the onExit
handler so that MenuButtonsChromeLogic
could save and reuse it in the activePanel
argument.
Updated. |
On game over this tab is opened by default so when game ends, this tab is activated and players can read and write before leaving. The chat tab is hidden in case there is only one human player.
Edit: In the chat tab are also replayed older messages for which we don't store time atm so we could show the time only for new messages so I removed the time for now. If agreed, we could extend chat cache to store also time (sent or received?) and then display it here and/or also in game chat.