Opened 17 months ago

Closed 14 months ago

Last modified 5 months ago

#4069 closed enhancement (fixed)

[PATCH] Ingame chat history

Reported by: elexis Owned by: elexis
Priority: Should Have Milestone: Alpha 21
Component: UI & Simulation Keywords: patch
Cc: andy011973@… Patch:

Description (last modified by Lionkanzen)

Since chat is only shown for 30 seconds, sometimes chat messages are not read while they might have contained some important message. Thus it should be possible to browse through prior chat messages.

It might be nice to query the chat history for only relevant messages, for example only team chat or hiding observer spam, refs #3380.

Notice if the "extended" checkbox is unchecked, it will keep the old style.

Attachments (9)

screenshot1.jpg (198.1 KB) - added by elexis 17 months ago.
screenshot2.jpg (193.0 KB) - added by elexis 17 months ago.
chathistory_wip1.patch (13.6 KB) - added by elexis 17 months ago.
Filtering logic not implemented yet. Gamesetup chat should be preserved too.
4069_chat_history_v1.patch (8.2 KB) - added by Imarok 15 months ago.
reused some parts from elexis patch. Final patch
4069_chat_history_v1.1.patch (8.8 KB) - added by Imarok 15 months ago.
Disable the filtering for Observers, as it makes no sense there
4069_chat_history_v1.2.patch (12.8 KB) - added by Imarok 14 months ago.
always center(with offset) the window. move the filterdropdown to the top. rename lobby.chattimestamp to chat.timestamp
4069_chat_history_v1.3.patch (13.1 KB) - added by Imarok 14 months ago.
fixed everything besides the style.xml thing
diplomacy_screen_chatPanel_fix.patch (5.3 KB) - added by elexis 14 months ago.
Note to oneself: commit after #3934 is merged.
4069_chat_history_v3.patch (27.7 KB) - added by elexis 14 months ago.
Rewrote and added a number of things

Download all attachments as: .zip

Change History (27)

Changed 17 months ago by elexis

Attachment: screenshot1.jpg added

Changed 17 months ago by elexis

Attachment: screenshot2.jpg added

Changed 17 months ago by elexis

Attachment: chathistory_wip1.patch added

Filtering logic not implemented yet. Gamesetup chat should be preserved too.

comment:1 Changed 17 months ago by elexis

Description: modified (diff)

comment:2 Changed 17 months ago by elexis

Keywords: rfc added

A call to rfc!

Does it make sense to show the bottom panel (addressee, text, send button) also when having that other tab "Filter" opened? To me it seems more natural to have that always show up.

comment:3 Changed 17 months ago by stanislas69

I think so. Now what you could do is switch back to the other tab after sending a message.

Also, can you make the tabs look more like the summary screen ? https://wildfiregames.com/forum/index.php?/topic/20963-epic-battles/

Last edited 17 months ago by stanislas69 (previous) (diff)

comment:4 Changed 16 months ago by Andy A.

Cc: andy011973@… added

comment:5 Changed 15 months ago by Imarok

Owner: set to Imarok

I'll give it a try

Changed 15 months ago by Imarok

Attachment: 4069_chat_history_v1.patch added

reused some parts from elexis patch. Final patch

Changed 15 months ago by Imarok

Disable the filtering for Observers, as it makes no sense there

comment:6 Changed 14 months ago by elexis

About your patch:

  • What's wrong about using lobby.chattimestamp in session/?
  • When changing the dropdown filter, only new messages are affected. In my version, one could change the filter ingame and see all messages sent at any time with the currently specified filters.
  • Not sure about the window size, windows ought to be vertically centered I think, or slightly biased towards the top, but likely not that much
  • The filter dropdown will only be confusing above the other two. It should rather go above the chat window, thus also hint that changing the filter dropdown will change the text below it.
  • It's probably best to allow both hosts to mute clients globally (#3380) and in this ticket only implement the local muting.

About my patch: Indeed it was silly to add one checkbox for every client connected. An individual muting system could be a two step process:

  • first a dropdown to select a target to mute ("everything", "connect/disconnect notifications", "game notifications", "observers", "allies", "enemies", "players", "one player", ...)
  • then if "one player" was selected, a dropdown box to pick some client.

Thus avoiding the pointless checkboxes.

It's perhaps not needed to have a client specific filtering at all. There were thoughts about adding a bunch of specific optional game notifications (for example if an ally reached city phase, built a civic center or a market #3512).

So if the latter would be implemented, the prior one could be done too. Other than that the client-based filtering isn't needed now.

Changed 14 months ago by Imarok

always center(with offset) the window. move the filterdropdown to the top. rename lobby.chattimestamp to chat.timestamp

comment:7 Changed 14 months ago by elexis

Looks good, only some minor optimizations:

  • L718 could use a comment stating that these keys are used for filtering the chat history
  • styles.xml straight copy of the gamesetup ChatPanel? Move it to common/styles.xml until someone decides to use two different formats IMO.
  • translate("All") -> `translateWithContext("chat history filter", "Notifications and Chat")
  • translate("Chat") -> `translateWithContext("chat history filter", "Chat messages")
  • initChatHistoryFilter could use prepareForDropdown to specify the options in a more grouped manner: let options = prepareForDropdown([{"key": foo, "text": translate(...)}, {...}, ...];
  • options.js entry could mention that it's used in the lobby and ingame chat history
  • Being attacked by Gaia throws:
    WARNING: JavaScript warning: gui/session/messages.js line 713
    reference to undefined property msg.guid
    
  • The window could be a bit wider IMO, just so that the proportions are more close to the golden angle.
  • Since the extended checkbox is now only about the history, we can show translateWithContext("chat", "History") there. (That might change in the future when more stuff is added. So perhaps keep the internal "extended" variable naming)
  • Would rename noObserver to playerChat for consistency
  • It seems weird to hide that dropdown to observers. There is a particular use case where some player is sending resources to an ally, thus spamming the chatlog which could be filtered with that dropdown. Since the chat history recalls who was an ally at that time the message was received, it should work without issues in a post defeat state too.

Changed 14 months ago by Imarok

fixed everything besides the style.xml thing

comment:8 in reply to:  7 Changed 14 months ago by Imarok

Replying to elexis:

Looks good, only some minor optimizations:

  • styles.xml straight copy of the gamesetup ChatPanel? Move it to common/styles.xml until someone decides to use two different formats IMO.

Sorry, I don't understand what you mean. I just copied the styles.xml changes from your patch

Changed 14 months ago by elexis

Note to oneself: commit after #3934 is merged.

Changed 14 months ago by elexis

Attachment: 4069_chat_history_v3.patch added

Rewrote and added a number of things

comment:9 Changed 14 months ago by elexis

Added functionality:

  • The filters didn't work they are supposed to. For example the ally filter should display only messages that were sent to allies, i.e. exclude public messages (even if they were sent by an ally).
  • Adding support to filter enemy, observer and private messages
  • Added timestamp functionality to the gamesetup
  • Saving the checkbox state to the user config file, so people don't have to click their preference every gamestart.

Style fixes:

  • let options can become a global g_ChatHistoryFilters right near those other chat objects in messages.js. The logic behind the isChat, isAllyChat keys can be moved to functions of that global, thus becoming more grouped. Renaming "data" to "key".
  • Adding newlines to chat_window.xml where needed to stay under the 80 character limit. Unfortunately we can't have linebreaks for translatableAttribute, since updateTemplates.py will extract the surrounding whitespace:
    public-gui-ingame.pot:"\t\t\t\tSelect chatmessage addressee.\n"
    
  • Reordering XML tags type, name, size, sprite, style, ...
  • Commented out code, really?
  • Removing unused name tags chatHistoryFilterLabel.
  • Don't copy my wip mistakes (1): Removed the new chatpanel style that is a duplicate of the gamesetup one.
  • Don't copy my wip mistakes (2): The ChatPanel theme shouldn't be used for labels. Use ModernText, ModernLeftLabelText or similar instead.
  • The entire diplomacy manager was using the ChatPanel? theme (while clearly not being one).
  • Removed the lobby chatpanel style which is yet another duplicate.
  • Renamed the session chatPanel style to chatPanelOverlay to be more precise and differentiate it from the other ChatPanel style.
  • Removed trailing whitespace
  • Reordering execution: first updating the chat overlay, then updating the chat history
  • Always call updateChatHistory(), even if the window is hidden. It's not really needed.
  • new Date() doesn't require the "now" argument. Unneeded variable time.
  • Adding the user preference loading to initChatHistoryFilter, thus renaming it to initChatWindow.
  • Moving resizeChatWindow below initChatWindow
  • Redistributing updateChatHistory() and resizeChatWindow() so that stuff is called as rarely as possible, as often as needed without introducing checks.

comment:10 Changed 14 months ago by bb

Mostly things said in IRC yesterday (leaving out things I've changed my mind on):

menu.js L242-3 Can't that be merged to Engine.GetGUIObjectByName("extendedChat").checked = Engine.ConfigDB_GetValue("user", "chat.session.extended") == "true";?

L250 // Hide/show the panel


The players messages to enemy's and observers are not shown on filter. On this I don't believe a player is the enemy of itself: messages.js L209-13.

Not all AI ally message are showed due to being "Team" messages cleanup patch in #3102 solves that.

Extended chat just borders the bottom panel at min res: http://imgur.com/a/c09aw

The ceasefire messages (maybe wonder too) are on top of the extended dialog making it look odd: http://imgur.com/a/a57J6

Lobby chat broken because missed file inclusion.

For long player names (AI names) the diplo screen looks broken (it was actually broken already but this patch make it show up): http://imgur.com/a/HWX1C

Last edited 14 months ago by bb (previous) (diff)

comment:11 Changed 14 months ago by Imarok

Owner: changed from Imarok to elexis

comment:12 Changed 14 months ago by elexis

In 18772:

Petra AI: Use ally instead of team chat, since it should eventually be able to use diplomacy. Based on patch by bb, refs #4143, #3102, #4069.

comment:13 in reply to:  10 Changed 14 months ago by elexis

Keywords: rfc removed

Replying to bb:

menu.js L242-3 Can't that be merged to Engine.GetGUIObjectByName("extendedChat").checked = Engine.ConfigDB_GetValue("user", "chat.session.extended") == "true";?

Indeed, Press is only called when the user clicks it. The Press event isn't triggered when the .checked property changes.

The players messages to enemy's and observers are not shown on filter. On this I don't believe a player is the enemy of itself: messages.js L209-13.

Agree, g_Players[Engine.GetPlayerID()].isEnemy[senderID] is false for senderID = Engine.GetPlayerID() afaik (Player component). However since messages that are not addressed to the player are filtered out beforehand, these logic checks are not needed at all.

The /enemies bug was a missing msg.guid != "local" check. That check was missing in another place (that offered the player a dropdown element of himself).

(The chat system is a bit complex:

  • Chat messages sent by the player in singleplayer come with guid == local
  • Chat messages sent by the player in multiplayer come with guid == 16 character hex
  • We need to check for GUIDs preferably since the playerID test fails for observers (every observer has playerID -1)
  • Chat messages sent by the AI or simulation otherwise comes with player ID

)

Not all AI ally message are showed due to being "Team" messages cleanup patch in #3102 solves that.

Fixed that part. I can rebase your patch on request, since I didn't commit it entirely.

Extended chat just borders the bottom panel at min res: http://imgur.com/a/c09aw

Fixed by resizing and chaning topOffset.

The ceasefire messages (maybe wonder too) are on top of the extended dialog making it look odd: http://imgur.com/a/a57J6

Fixed by adding z="90".

Lobby chat broken because missed file inclusion.

Fixed by including the forgotton style.

For long player names (AI names) the diplo screen looks broken (it was actually broken already but this patch make it show up): http://imgur.com/a/HWX1C

Yes, not enough space for long botnames has been there before. The vertical centering is the wrong part.

Thanks for your review(s)!

comment:14 Changed 14 months ago by elexis

In 18774:

Use a custom style for the text in the Diplomacy manager instead of abusing the ChatPanel? style, refs #4069.

comment:15 Changed 14 months ago by elexis

Resolution: fixed
Status: newclosed

In 18775:

Filterable ingame chat history. Based on patch by Imarok, reviewed by bb, fixes #4069.

Rename session chatPanel style to chatPanelOverlay and merge other chatPanel styles in common/.
Fix a singleplayer bug where messages send by the player wouldn't be shown in the chat overlay due to a missing guid != "local" check.
Save the user preference whether to show the history to the user config file.
Remove /enemy /ally and /observer aliases for simplicity.

comment:16 Changed 12 months ago by elexis

Description: modified (diff)

comment:17 Changed 5 months ago by elexis

In 19893:

Chat window size beautification.

Use the entire available screen height for easier chat history reading.
Use the golden ratio for the chat history frame size, so that the ratio looks appealing with all screen size ratios.

Differential Revision: https://code.wildfiregames.com/D713
Refs #4069
Patch By: temple

comment:18 Changed 5 months ago by Lionkanzen

Description: modified (diff)

This feature must be avaible in the GUI as icon.

Note: See TracTickets for help on using tickets.