Opened 9 years ago

Closed 8 years ago

Last modified 5 years ago

#3396 closed enhancement (fixed)

[PATCH] Lobby - Clear chat command / button

Reported by: elexis Owned by: ruiRanger
Priority: Nice to Have Milestone: Alpha 20
Component: Multiplayer lobby Keywords: patch
Cc: Patch:

Description (last modified by elexis)

It would be nice if we could blank the chat history by typing /clear or hitting a button.

This should be especially easy to implement if #3241 is commited, as this provides a way to process local commands.

Attachments (3)

clearChat.patch (1.8 KB ) - added by ruiRanger 8 years ago.
clearChat_v2.patch (3.3 KB ) - added by ruiRanger 8 years ago.
clearChat_v3.patch (1.5 KB ) - added by ruiRanger 8 years ago.
clear Chat without hotkeys and clearChatMessages function not duplicated

Download all attachments as: .zip

Change History (14)

comment:1 by scythetwirler, 8 years ago

Keywords: simple added

by ruiRanger, 8 years ago

Attachment: clearChat.patch added

comment:2 by Stan, 8 years ago

Keywords: patch added
Milestone: BacklogAlpha 20
Summary: Lobby - Clear chat command / button[PATCH] Lobby - Clear chat command / button

Hey thanks for the patch.

Can you replace "Blank the chat history" by "Clear the chat history" ? also document functions in both places, or better,unify them to use the same code.

Next time add the review and patch keywords and edit the title. Please read http://trac.wildfiregames.com/wiki/SubmittingPatches

If you plan to continue working on this, please modify ticket, and click accept so it's assigned to you.

in reply to:  2 comment:3 by elexis, 8 years ago

With regards to the code:

Replying to stanislas69:

Can you replace "Blank the chat history" by "Clear the chat history" ?

Comments are useful if they add information that is not already contained in the function name.

unify them to use the same code.

As we currently don't have a suitable file that contains common chat functions, we should do this later in #2640

It would be nice to have a hotkey (maybe ctrl+l) to clear the chat without typing.

by ruiRanger, 8 years ago

Attachment: clearChat_v2.patch added

comment:4 by ruiRanger, 8 years ago

Keywords: review added
Owner: set to ruiRanger
Status: newassigned

Changed the code as suggested by elexis. Added the ctrl+L hotkey.

comment:5 by Josh, 8 years ago

I would argue against a hotkey, as that would be inconsistent with the rest of the lobby commands.

in reply to:  5 ; comment:6 by elexis, 8 years ago

The patch works and it could be committed (without the hotkey part).

The only thing that should be updated is that you define the exact same function in two places (clearChatMessages in gamesetup.js and lobby.js). Duplicate code must be avoided. you can define the function in functions_global_object.js or functions_utility.js (and would be moved later to a new functions_chat.js).

With regards to hotkeys, we could exclude them for this ticket and maybe create a specific ticket for menu hotkeys (if there isn't one already).

by ruiRanger, 8 years ago

Attachment: clearChat_v3.patch added

clear Chat without hotkeys and clearChatMessages function not duplicated

comment:7 by elexis, 8 years ago

Resolution: fixed
Status: assignedclosed

In 17465:

Add a chat-command to clear all messages. Works for lobby, gamesetup and session. Patch by ruiRanger, fixes #3396.

comment:8 by elexis, 8 years ago

In 17467:

Clear g_ChatTimers too in case the new /clear command is used from the session. Refs #3396.

comment:9 by elexis, 8 years ago

Keywords: simple review removed

Thanks for the patch.

As r17464 renames chatMessages to g_ChatMessages, your command doesn't throw errors when used for the session. The timers needed to be cleared too in that case. I committed this separately as it was not in the scope of this ticket.

in reply to:  6 comment:10 by elexis, 5 years ago

Description: modified (diff)

Replying to elexis:

Duplicate code must be avoided. you can define the function in functions_global_object.js

It should be avoided, but not at the cost of refering to page-specific globals in gui/common/. The issue is related to #2640 which aims at unifying chat parsing, but this never really works out conceptually, since the three chat pages have different features, different variables, different presentation. If deduplicating / unifying the chat, it would probably be cleaner to introduce a prototype about ChatHandling that pleases features of all three pages without becoming odd, without refering to page-specific globals or names (but receiving those as arguments) (#5387).

(Also the patch has turned out to be useful in some situations, thanks again)

comment:11 by elexis, 5 years ago

In 22654:

Duplicate clearChatMessages since that ends up being cleaner
than having a stray function in gui/common/ that refers to page specific names and logic with a try-catch block to shield against reference errors, refs #3396, rP17465, rP17467.

Incidentally fixes ESLint no-empty hint triggered for empty catch blocks, refs #5524, D2146.

Note: See TracTickets for help on using tickets.