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

Fixed the issue with last character handling for cases when the length of the message is 4001 characters #1680

Merged
merged 3 commits into from
Jun 23, 2021

Conversation

maxtrap
Copy link
Contributor

@maxtrap maxtrap commented Jun 20, 2021

Pull Request Etiquette

Changes

  • Internal code
  • Library interface (affecting end-user code)
  • Documentation
  • Other: _____

Closes Issue: NaN

Description

This fixes the issue that happens when MessageBuilder#buildAll is called on a message with 2001+ characters. Once the message is split, the last character of the last message ends up being cut off. The reason this happens is because the endIndex of the substring is set as builder.length()-1, even though a substring's end index is exclusive. As a result, the last char in the StringBuilder of the message ends up being cut off.

Tested

With the following sample code

        StringBuilder db = new StringBuilder();
        for (int i = 0; i < 4000; i++) {
            if (i != 1999 && i != 3999)
                db.append('a');
            else db.append('b');
        }
        db.append('c');

        new MessageBuilder(db.toString())
                .buildAll(MessageBuilder.SplitPolicy.ANYWHERE)
                .forEach(replyMessage -> getServer().getTextChannelById(CHANNEL_ID).sendMessage(replyMessage.getContentRaw()).queue());

Existing implementation loses last 'c' letter, by generating 2 messages
image

And proposed implementation correctly generates 3 messages
image

@DManstrator
Copy link
Contributor

DManstrator commented Jun 20, 2021

Good catch!

Small suggestion: Use the constant from the Message interface (Message.MAX_CONTENT_LENGTH) instead of the 2000.

Edit: Same for line 1277.

Copy link
Collaborator

@Almighty-Alpaca Almighty-Alpaca left a comment

Choose a reason for hiding this comment

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

I agree with the comment from @DManstrator but otherwise lgtm

@maxtrap
Copy link
Contributor Author

maxtrap commented Jun 20, 2021

Thanks for review.
Addressed comment by replacing hardcoded number with constant.
Please take a look again.

@DManstrator
Copy link
Contributor

otherwise lgtm

lgtm means "looks good to me". So it is fine now. 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants