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

Function: clearChat #215

Merged
merged 21 commits into from Aug 27, 2018

Conversation

Projects
@CrosRoad95
Contributor

CrosRoad95 commented Jul 2, 2018

Type: shared
function name: clearChatBox
return: true if cleared successfully, otherwise false
arguments :

  1. client has no arguments.
  2. server by default first argument is root, but at first can be player element.

I often see programmers that making serverside script to clear chat in this style:

for i=1,1000 do
  outputChatBox("", root)
end

This function prevent doing stupid laggy code.

Example ( serverside )

outputChatBox("SA-MP better then MTASA serverside")
clearChatBox(getRandomPlayer())
outputChatBox("MTASA better then SA-MP serverside")

Result:

image
( look only on chat, outdate picture )

CrosRoad95 added some commits May 30, 2018

clientside working version
- clearChatBox & clearConsole
@@ -126,6 +126,29 @@ int CLuaFunctionDefs::OOP_OutputChatBox(lua_State* luaVM)
return 1;
}
int CLuaFunctionDefs::ClearChatBox(lua_State* luaVM)
{
CElement* pElement;

This comment has been minimized.

@Citizen01

Citizen01 Jul 2, 2018

Uninitialized pointer ? (I see that the whole file is doing the same but is it safe to keep doing it ?)

@Citizen01

Citizen01 Jul 2, 2018

Uninitialized pointer ? (I see that the whole file is doing the same but is it safe to keep doing it ?)

This comment has been minimized.

@ccw808

ccw808 Jul 2, 2018

Member

Argument values are initialized by CScriptArgReader

@ccw808

ccw808 Jul 2, 2018

Member

Argument values are initialized by CScriptArgReader

This comment has been minimized.

@Citizen01

Citizen01 Jul 2, 2018

ok my knowledge about "uninitialized pointers" was off, thanks for clearing that for me.

@Citizen01

Citizen01 Jul 2, 2018

ok my knowledge about "uninitialized pointers" was off, thanks for clearing that for me.

Show outdated Hide outdated Server/mods/deathmatch/logic/packets/CConsoleEchoPacket.h
Show outdated Hide outdated Server/mods/deathmatch/logic/CStaticFunctionDefinitions.cpp
Show outdated Hide outdated Server/mods/deathmatch/logic/CStaticFunctionDefinitions.cpp
@Citizen01

This comment has been minimized.

Show comment
Hide comment
@Citizen01

Citizen01 Jul 2, 2018

I have one more question:
If you call clearChatBox(root) it will clear the chatbox of all players, right ?

Citizen01 commented Jul 2, 2018

I have one more question:
If you call clearChatBox(root) it will clear the chatbox of all players, right ?

@CrosRoad95

This comment has been minimized.

Show comment
Hide comment
@CrosRoad95

CrosRoad95 Jul 2, 2018

Contributor

yes, and this is by default

Contributor

CrosRoad95 commented Jul 2, 2018

yes, and this is by default

@Pirulax

This comment has been minimized.

Show comment
Hide comment
@Pirulax

Pirulax Jul 2, 2018

Contributor

Will this work on client-side too?

Contributor

Pirulax commented Jul 2, 2018

Will this work on client-side too?

@CrosRoad95

This comment has been minimized.

Show comment
Hide comment
@CrosRoad95

CrosRoad95 Jul 2, 2018

Contributor

yes, client and server side
client:
clearChatBox() will clear chat, clearConsole() will clear console
if you use this serverside, then by default clear chat every player on server but
clearChatBox(getRandomPlayer()) - this clear chat of random player

Contributor

CrosRoad95 commented Jul 2, 2018

yes, client and server side
client:
clearChatBox() will clear chat, clearConsole() will clear console
if you use this serverside, then by default clear chat every player on server but
clearChatBox(getRandomPlayer()) - this clear chat of random player

@Pirulax

This comment has been minimized.

Show comment
Hide comment
@Pirulax

Pirulax Jul 4, 2018

Contributor

You may even do a clearDebugscript function too.

Contributor

Pirulax commented Jul 4, 2018

You may even do a clearDebugscript function too.

@qaisjp

This comment has been minimized.

Show comment
Hide comment
@qaisjp

qaisjp Jul 4, 2018

Member

I don't think we should let servers clear the console (could be abused).

I agree that printing whitespace a hundred times is a common (and laggy) pattern.

Why would you want to clear the console?

Member

qaisjp commented Jul 4, 2018

I don't think we should let servers clear the console (could be abused).

I agree that printing whitespace a hundred times is a common (and laggy) pattern.

Why would you want to clear the console?

@gatno

This comment has been minimized.

Show comment
Hide comment
@gatno

gatno Jul 5, 2018

How could it be abused? I think there is no problem to clear the console.

gatno commented Jul 5, 2018

How could it be abused? I think there is no problem to clear the console.

@Dezash

This comment has been minimized.

Show comment
Hide comment
@Dezash

Dezash Jul 8, 2018

Contributor

The current way of using hundreds of outputChatBox functions "clears" the console anyway. Not clearing the console kinda takes its purpose away as most people use it to hide certain messages. A better solution would be to add a function to delete a specific message, but that would probably be much harder.

Contributor

Dezash commented Jul 8, 2018

The current way of using hundreds of outputChatBox functions "clears" the console anyway. Not clearing the console kinda takes its purpose away as most people use it to hide certain messages. A better solution would be to add a function to delete a specific message, but that would probably be much harder.

@ccw808

This comment has been minimized.

Show comment
Hide comment
@ccw808

ccw808 Jul 8, 2018

Member

What are the reasons for clearing the chatbox?

Member

ccw808 commented Jul 8, 2018

What are the reasons for clearing the chatbox?

@CrosRoad95

This comment has been minimized.

Show comment
Hide comment
@CrosRoad95

CrosRoad95 Jul 8, 2018

Contributor

if someone put advertising on chat -> clear chat
players started spamming, you can clear chat
If you have multi gamemodes, then when you can easily clear chat while changing gamemod

Contributor

CrosRoad95 commented Jul 8, 2018

if someone put advertising on chat -> clear chat
players started spamming, you can clear chat
If you have multi gamemodes, then when you can easily clear chat while changing gamemod

@ccw808

This comment has been minimized.

Show comment
Hide comment
@ccw808

ccw808 Jul 8, 2018

Member

Ok, they seem reasonable.
Why would you want to clear the console?

Member

ccw808 commented Jul 8, 2018

Ok, they seem reasonable.
Why would you want to clear the console?

@CrosRoad95

This comment has been minimized.

Show comment
Hide comment
@CrosRoad95

CrosRoad95 Jul 8, 2018

Contributor

same reasons

Contributor

CrosRoad95 commented Jul 8, 2018

same reasons

@saml1er

This comment has been minimized.

Show comment
Hide comment
@saml1er

saml1er Jul 8, 2018

Member

Good work. I think only clearChatBox should be implemented, because usage of clearConsole isn't too convincing, just my 2 cents.

Member

saml1er commented Jul 8, 2018

Good work. I think only clearChatBox should be implemented, because usage of clearConsole isn't too convincing, just my 2 cents.

@Dezash

This comment has been minimized.

Show comment
Hide comment
@Dezash

Dezash Jul 8, 2018

Contributor

@saml1er Why not?

Contributor

Dezash commented Jul 8, 2018

@saml1er Why not?

@saml1er

This comment has been minimized.

Show comment
Hide comment
@saml1er

saml1er Jul 8, 2018

Member

As qaisjp said, it can be abused. Servers can spam this function and possibly crash the client. It's simply not safe. I support client side clearConsole, but I'm against giving server the ability to clear client console for obvious reasons.

Member

saml1er commented Jul 8, 2018

As qaisjp said, it can be abused. Servers can spam this function and possibly crash the client. It's simply not safe. I support client side clearConsole, but I'm against giving server the ability to clear client console for obvious reasons.

@Dezash

This comment has been minimized.

Show comment
Hide comment
@Dezash

Dezash Jul 8, 2018

Contributor

I'm not sure why would a server want to crash its clients, but there are plenty of ways to do that. Anyway, clientside function would suffice.

Contributor

Dezash commented Jul 8, 2018

I'm not sure why would a server want to crash its clients, but there are plenty of ways to do that. Anyway, clientside function would suffice.

@CrosRoad95

This comment has been minimized.

Show comment
Hide comment
@CrosRoad95

CrosRoad95 Jul 8, 2018

Contributor

if someone want to troll own players, they can do this in 4534563 diferent way.
I can crash any player I want, i can clear chat and console using empty outputChatBox, this pull request is only for performance reason.

Contributor

CrosRoad95 commented Jul 8, 2018

if someone want to troll own players, they can do this in 4534563 diferent way.
I can crash any player I want, i can clear chat and console using empty outputChatBox, this pull request is only for performance reason.

@ccw808

This comment has been minimized.

Show comment
Hide comment
@ccw808

ccw808 Jul 8, 2018

Member

A wise man once told me: "Console is something that lives from server to server and belongs to the client. The chat box is something that belongs to the server. It makes sense to want to clear the chat box but not really the console."

Member

ccw808 commented Jul 8, 2018

A wise man once told me: "Console is something that lives from server to server and belongs to the client. The chat box is something that belongs to the server. It makes sense to want to clear the chat box but not really the console."

@Dezash

This comment has been minimized.

Show comment
Hide comment
@Dezash

Dezash Jul 8, 2018

Contributor

Then why are there functions such as outputConsole? Servers should either have control over the console or not. This functions does essentially the same thing as outputting hundreds of messages, just in an efficient way. It makes no sense clearing the chat, but not clearing the console. To put this into perspective, let's assume somebody accidentally types in ".login John cheesecake123". Staff members would try to use the tools at their disposal to hide such a message. If only the chat is cleared, other people would still be able to easily see that message in the console.

Contributor

Dezash commented Jul 8, 2018

Then why are there functions such as outputConsole? Servers should either have control over the console or not. This functions does essentially the same thing as outputting hundreds of messages, just in an efficient way. It makes no sense clearing the chat, but not clearing the console. To put this into perspective, let's assume somebody accidentally types in ".login John cheesecake123". Staff members would try to use the tools at their disposal to hide such a message. If only the chat is cleared, other people would still be able to easily see that message in the console.

@ccw808

This comment has been minimized.

Show comment
Hide comment
@ccw808

ccw808 Jul 8, 2018

Member

Clearing the console won't stop the password being saved in logs\console.log
Is mistyping /login in chat a frequent problem? We can probably solve that in other ways.

Member

ccw808 commented Jul 8, 2018

Clearing the console won't stop the password being saved in logs\console.log
Is mistyping /login in chat a frequent problem? We can probably solve that in other ways.

@patrikjuvonen

This comment has been minimized.

Show comment
Hide comment
@patrikjuvonen

patrikjuvonen Jul 8, 2018

Collaborator

How often do you find other (multiplayer) games clearing out their (debug) consoles from users? Chatboxes maybe, but not consoles.

Collaborator

patrikjuvonen commented Jul 8, 2018

How often do you find other (multiplayer) games clearing out their (debug) consoles from users? Chatboxes maybe, but not consoles.

@Dezash

This comment has been minimized.

Show comment
Hide comment
@Dezash

Dezash Jul 9, 2018

Contributor

@ccw808 the majority of players do not know about the logs though. In my experience, people tend to mistype the "/msg" command a lot, but I've seen failed "/login" commands a few times as well.

Contributor

Dezash commented Jul 9, 2018

@ccw808 the majority of players do not know about the logs though. In my experience, people tend to mistype the "/msg" command a lot, but I've seen failed "/login" commands a few times as well.

@qaisjp

Thank you very much for your pull request, good work!

We'd be happy to merge clearChatBox, but please remove clearConsole.

@qaisjp

This comment has been minimized.

Show comment
Hide comment
@qaisjp

qaisjp Jul 9, 2018

Member

@Dezash I've been on servers that detect if messages begin with .login, login, 'login, or similar and prevent those messages from being shared with all players.

I think this is something you could implement on your server if it is currently a big problem. We should be happy with accepting a pull request that prevents such messages from ever making the chatbox, and we may work on this at some point otherwise.

Member

qaisjp commented Jul 9, 2018

@Dezash I've been on servers that detect if messages begin with .login, login, 'login, or similar and prevent those messages from being shared with all players.

I think this is something you could implement on your server if it is currently a big problem. We should be happy with accepting a pull request that prevents such messages from ever making the chatbox, and we may work on this at some point otherwise.

say bye bye to clearConsole ;( chlip
i think everything is removed

@CrosRoad95 CrosRoad95 changed the title from Clear chat and console function. to Function: clearChat Jul 11, 2018

CrosRoad95 added some commits Jul 11, 2018

@qaisjp qaisjp dismissed their stale review Jul 11, 2018

Thanks for addressing my requested changes. Note to merger: please squash merge.

@emre1702

This comment has been minimized.

Show comment
Hide comment
@emre1702

emre1702 Jul 13, 2018

Contributor

while (true) do outputConsole("") end
This code can also crash the client and spam the whole console.
I also can "clear" the console like this:
for i=1, 1000 do outputConsole("") end
If I would have used clearConsole instead, it wouldn't result in a huge console.log.

So I would suggest adding clearConsole.

Contributor

emre1702 commented Jul 13, 2018

while (true) do outputConsole("") end
This code can also crash the client and spam the whole console.
I also can "clear" the console like this:
for i=1, 1000 do outputConsole("") end
If I would have used clearConsole instead, it wouldn't result in a huge console.log.

So I would suggest adding clearConsole.

@botder botder removed the review required label Jul 13, 2018

@botder

This comment has been minimized.

Show comment
Hide comment
@botder

botder Jul 13, 2018

Member

I suggest to include clearConsole, but under the circumstances that it will only clear player messages and every message added through outputConsole (scripted).

Member

botder commented Jul 13, 2018

I suggest to include clearConsole, but under the circumstances that it will only clear player messages and every message added through outputConsole (scripted).

@CrosRoad95

This comment has been minimized.

Show comment
Hide comment
@CrosRoad95

CrosRoad95 Jul 13, 2018

Contributor

Okey, in next pull request

Contributor

CrosRoad95 commented Jul 13, 2018

Okey, in next pull request

@Dezash

This comment has been minimized.

Show comment
Hide comment
@Dezash

Dezash Jul 13, 2018

Contributor

Wouldn't it be easier to just delete specific messages if we are going that route? outputConsole and outputChatBox could return the message ID and then there should be a function like deleteChatBoxMessage(ID) deleteConsoleMessage(ID)

Contributor

Dezash commented Jul 13, 2018

Wouldn't it be easier to just delete specific messages if we are going that route? outputConsole and outputChatBox could return the message ID and then there should be a function like deleteChatBoxMessage(ID) deleteConsoleMessage(ID)

@CrosRoad95

This comment has been minimized.

Show comment
Hide comment
@CrosRoad95

CrosRoad95 Jul 13, 2018

Contributor

Dezash, meh, better is to add option to assign id to message insted of return them.

outputChatBox("asdf",player,255,255,255,true,"myid")
deleteChatBoxMessage("myid")
deleteConsoleMessage("myid")

System messages don't have id, and when you print multiple messages, you don't need to store id's,
example player messages can have id "player_PLAYERNAME"
i could rewrite this feature to make options like above

Contributor

CrosRoad95 commented Jul 13, 2018

Dezash, meh, better is to add option to assign id to message insted of return them.

outputChatBox("asdf",player,255,255,255,true,"myid")
deleteChatBoxMessage("myid")
deleteConsoleMessage("myid")

System messages don't have id, and when you print multiple messages, you don't need to store id's,
example player messages can have id "player_PLAYERNAME"
i could rewrite this feature to make options like above

@emre1702

This comment has been minimized.

Show comment
Hide comment
@emre1702

emre1702 Jul 13, 2018

Contributor

That would be a good solution.

Contributor

emre1702 commented Jul 13, 2018

That would be a good solution.

@saml1er

You can also press ctrl+h in VS and replace all (void) with () for the files that you are modifying. Before you push changes to the PR, make sure to run clang-format. I'll test this later.

Show outdated Hide outdated Client/core/CCore.cpp
Show outdated Hide outdated Client/core/CCore.h
Show outdated Hide outdated Server/mods/deathmatch/logic/packets/CChatEchoPacket.h
Show outdated Hide outdated Client/sdk/core/CCoreInterface.h
Show outdated Hide outdated Server/mods/deathmatch/logic/packets/CChatEchoPacket.h

CrosRoad95 added some commits Aug 27, 2018

@CrosRoad95

This comment has been minimized.

Show comment
Hide comment
@CrosRoad95

CrosRoad95 Aug 27, 2018

Contributor

I moved packet to other file.
I also tested this once again with newstest version ( client and server side ) and everything seems to be okey.

Contributor

CrosRoad95 commented Aug 27, 2018

I moved packet to other file.
I also tested this once again with newstest version ( client and server side ) and everything seems to be okey.

@saml1er saml1er merged commit 600de49 into multitheftauto:master Aug 27, 2018

3 checks passed

WIP ready for review
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@qaisjp qaisjp added this to In progress in release/v1.5.6 via automation Aug 27, 2018

@qaisjp qaisjp added this to the 1.5.6 milestone Aug 27, 2018

@qaisjp qaisjp moved this from In progress to Done in release/v1.5.6 Aug 27, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment