Skip to content
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

Clean up chat messages of special line characters prior to sending #3312

Merged
merged 4 commits into from
Oct 30, 2021

Conversation

Infinitay
Copy link
Contributor

@Infinitay Infinitay commented Oct 27, 2021

Pull request checklist:

  • CHANGELOG.md was updated, if applicable

Description

If you add a command that contains a new line in the message, it does not correctly send the message. It will only send the first line of the command. The issue does not occur if you manually input the message with new lines into the chat box and hit send. This behavior only seems to occur with commands.

image

For example, if you have a command set to the following (including the new line):

This line will be sent.
This line should not send.

The solution I found was to remove the new line characters from the message. Instead of removing it from the command itself when users add or update a command, it might be best to do so prior to sending the message as done in this PR. Also, I wasn't sure if messages would have a difference between \n and \r\n depending on their OS, so I found QString::simplified to be useful as it replaces escape characters with a space. This way ascii arts shouldn't be affected either, and text messages would be more readable too (no added space versus an added space).

@Infinitay Infinitay changed the title Parse chat message with QString::simplified prior to sending Parse chat message to remove new lines and other special line characters prior to sending Oct 27, 2021
@Felanbird Felanbird requested review from zneix and Mm2PL October 27, 2021 17:47
Copy link
Collaborator

@Felanbird Felanbird left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a thought on the changelog but otherwise the functionality change fixes the issue you have described.

Tested it by adding a command with new lines - without this PR only asd1 sent, with the PR all 3 sent on a single line.

asd1
asd2
asd3

CHANGELOG.md Outdated Show resolved Hide resolved
@Felanbird
Copy link
Collaborator

As a note from some testing in chat, this issue persists for Channel Joining/Renaming, as well as Nicknames. Maybe there's a better approach to fixing?
image
image

@Infinitay
Copy link
Contributor Author

Infinitay commented Oct 27, 2021

As a note from some testing in chat, this issue persists for Channel Joining/Renaming, as well as Nicknames. Maybe there's a better approach to fixing? image image

Didn't consider these since I came across this when dealing with ASCII art. I suppose we would have to start parsing all user input. Is there an extended window class with text input somewhere that we can parse for UI? Perhaps a respectful one for field boxes for dealing with settings as in commands or nicknames?

If we don't want to create some sort of wrapper for these windows, I suppose we have to modify them each individually. The later is the easiest of course. The following list of places come to mind that prompts for user text input. Can you think of anything else?

  • Joining a channel
  • Renaming tabs
  • Most settings using EditableModelView (commands, highlights, ignores, nicknames, and filters)

Of course, there's also the the other input fields such as for general settings if you want to bother parsing there in addition to Accounts -> Add -> Advanced

@TETYYS
Copy link
Contributor

TETYYS commented Oct 27, 2021

I want that my newlines would be converted to spaces when sending, as it its now. What will I do after this PR?

@Mm2PL
Copy link
Collaborator

Mm2PL commented Oct 27, 2021

user input:

  • Timestamps
  • Moderation Buttons, kinda. This could actually stay as it could be a 'feature'
  • Image Uploader settings (Image link, Deletion link)

@Infinitay
Copy link
Contributor Author

I want that my newlines would be converted to spaces when sending, as it its now

Sending a message in chat? Remains the same, feel free to test it yourself if you'd like. Actually that is one thing that I am confused on. Current if you paste and send a message in chat that does have new lines, it behaves as you said. The message is sent and replaces the new lines with spaces. However, when you do the same but instead as a command, it will only send the first line. To be honest I'm not sure why. I looked through it all again too and can't figure out why that's the case.

My original thought was to make this change within the Command class. However, I noticed that the change only affects adding new commands or modifying existing ones.

@Mm2PL
Copy link
Collaborator

Mm2PL commented Oct 27, 2021

Turns out this sends the text as raw irc.

Here is an example command: /totally_now_raw -> I made a new command HeyGuys[CR][LF]PRIVMSG #mm2pl :testing! (replace CR and LF with 0x0D and 0x0A characters respectively)
This command will send I made a new command HeyGuys to the current channel and send testing! to my channel.

@Infinitay
Copy link
Contributor Author

Infinitay commented Oct 27, 2021

Turns out this sends the text as raw irc.

Here is an example command: /totally_now_raw -> I made a new command HeyGuys[CR][LF]PRIVMSG #mm2pl :testing! (replace CR and LF with 0x0D and 0x0A characters respectively) This command will send I made a new command HeyGuys to the current channel and send testing! to my channel.

Wasn't aware of this, is this behavior intended?

When I thought I understood the current behavior of sending messages given your example, I'm still uncertain as to why it's behaving as such. My thought as to why is that we know the channel we are in for the first message and sends that fine via the underlying send raw message method. The second line in your example gets sent because it explicitly sends the second line in the raw format as mentioned in #3189 in the first bullet. However, what I'm not grasping is why it's making two individual raw calls when it is one message but with line breaks. I personally couldn't find anywhere that iterates over line breaks and sends a raw call for each.

However going back to the whether or not this is intended behavior, I would imagine the answer is no. Right? Not only because it doesn't seem to make perfect sense as to why it should do so, but also because if you paste your same example into the chatbox rather than a command and send it, it sends it raw as intended.

@Mm2PL
Copy link
Collaborator

Mm2PL commented Oct 27, 2021

The problem is that there is just no validation that removes new lines but they are meaningful in IRC. IRC is a text protocol. An IRC message is defined as follows:

<message> ::= ['@' <tags> <SPACE>] [':' <prefix> <SPACE> ] <command> <params> <crlf>

<crlf> is just a new line

All this mumbo-jumbo means that a normal chat message you'd sent to #pajlada would like this:

PRIVMSG #pajlada :here is some text [line break]

If you were to add a line break into the text (ie here is some text\r\n:)), this would happen:

PRIVMSG #pajlada :here is some text [line break]
:) [line break from original message] <-- IRC server thinks this is a separate message

My example did this:

PRIVMSG #pajlada :I made a new command HeyGuys [line break]
PRIVMSG #mm2pl :testing! [line break from original message]

@Infinitay
Copy link
Contributor Author

Infinitay commented Oct 27, 2021

Ah okay, I had no idea how IRC functions. To tackle no validation, what are your thoughts on using a regex to search the message for IRC commands after a new line? If there is, then leave it alone, but if there isn't then replace the new lines with spaces.

@Mm2PL
Copy link
Collaborator

Mm2PL commented Oct 27, 2021

IMO just not letting new lines sneak through to the TwitchIRCServer should be enough, using regex is an overkill, just remove replace CR and LF with spaces.

@Infinitay
Copy link
Contributor Author

just remove replace CR and LF with spaces.

That's what we are currently doing and what led me to making this change

@pajlada pajlada changed the title Parse chat message to remove new lines and other special line characters prior to sending Clean up chat messages of special line characters prior to sending Oct 30, 2021
@pajlada
Copy link
Member

pajlada commented Oct 30, 2021

Thanks for your contribution @Infinitay! As a first-time contributor, you can now add yourself to the contributors list that's shown inside the application.

To do so, open a new PR where you modify the resources/contributors.txt file and add yourself to the list. Make sure to read the comments at a top for instructions.

@pajlada pajlada enabled auto-merge (squash) October 30, 2021 12:45
@pajlada pajlada merged commit b4b7450 into Chatterino:master Oct 30, 2021
zneix added a commit to SevenTV/chatterino7 that referenced this pull request Oct 30, 2021
Now we're on commit b4b7450; Changes from upstream we've pulled:

- Minor: Clean up chat messages of special line characters prior to sending. (Chatterino#3312)
- Bugfix: Fixed `First Message` scrollbar highlights not being disabled. (Chatterino#3325)
- Dev: Add GitHub action to test builds without precompiled headers enabled. (Chatterino#3327)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants