ddnet / ddnet

DDraceNetwork, a free cooperative platformer game
https://ddnet.org
Other
507 stars 383 forks source link

Half working multi line chat support in the current code base #8305

Open ChillerDragon opened 1 week ago

ChillerDragon commented 1 week ago

the client can do newlines the server can not

I assume ~15 years ago matricks planned to support newlines in chat messages. This is why the client has the following loop:

https://github.com/ddnet/ddnet/blob/2f22447d44535c50cc5bdd538bd7741dad0f6787/src/game/client/components/chat.cpp#L722-L734

But even if the client would send a newline character to the server the server replaces it with spaces in the reply.

https://github.com/ddnet/ddnet/blob/2f22447d44535c50cc5bdd538bd7741dad0f6787/datasrc/network.py#L453

https://github.com/ddnet/ddnet/blob/2f22447d44535c50cc5bdd538bd7741dad0f6787/src/base/system.cpp#L2829-L2830

I tested that to verify my theory by removing the outer while(*p) and the chat still worked as expected. And I also modified the server to keep the newlines in the message and the client did display multiple chat messages.

image image

(the tests were performed on a teeworlds server not a ddnet server but it should be the same)

either support it on both ends or nowhere

The chat.cpp AddLine method is quite complicated because of that multi line chat support. And only custom servers that send newline characters ever make use of that. I would propose to remove the newline support in the client to keep the code simpler. And just replace newlines with spaces in the client side or cut the messages on newline characters.

tl;dr

The server sanitizes newlines away. The client still wants to handle newlines.

Robyt3 commented 1 week ago

I would be for removing this code from the client-side, since it would probably allow avoiding the use of const_cast there.

ChillerDragon commented 1 week ago

I now also tested it on ddnet. And seems like that multi line support broke in ddnet at some point anyways. One more reason to fully remove it.

image

heinrich5991 commented 1 week ago

Remove it.