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

Fix off by one error in hightlight #650

Merged
merged 2 commits into from
Aug 17, 2024

Conversation

Nogesma
Copy link
Contributor

@Nogesma Nogesma commented Aug 13, 2024

It's a quick fix but should work.
I don't know if it would be more efficient to maybe get the highlights indices after wrapping the message, which is the core of the issue here.
edit: I just realized it would break highlight of words spanning multiple lines so I'm not sure what a better solution would look like.

I couldn't manage to find how to send a message with newlines either with the twitch website or with twt so I just added a test case for it, but maybe @vesdev can confirm this fixes it?

Oh and I reverted the PR completely, which downgrades ahash, let me know if you want me to update it back.

Copy link

codecov bot commented Aug 13, 2024

Codecov Report

Attention: Patch coverage is 96.96970% with 2 lines in your changes missing coverage. Please review.

Project coverage is 23.10%. Comparing base (62f53e4) to head (cd3570c).
Report is 1 commits behind head on main.

Files Patch % Lines
src/handlers/data.rs 96.96% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #650      +/-   ##
==========================================
+ Coverage   17.87%   23.10%   +5.22%     
==========================================
  Files          42       42              
  Lines        5431     5492      +61     
==========================================
+ Hits          971     1269     +298     
+ Misses       4460     4223     -237     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@vesdev
Copy link
Contributor

vesdev commented Aug 13, 2024

I can still reproduce the issue
image

@Nogesma
Copy link
Contributor Author

Nogesma commented Aug 13, 2024

Hmm, what client are you using that inserts newlines in the message? So I can test this myself.

Also did you make sure the PR was applied?

@vesdev
Copy link
Contributor

vesdev commented Aug 13, 2024

Hmm, what client are you using that inserts newlines in the message? So I can test this myself.

Also did you make sure the PR was applied?

Just twt, there are no newlines. Only line breaks are done by textwrap afaik. And yes the pr is applied

@Nogesma
Copy link
Contributor Author

Nogesma commented Aug 13, 2024

Ah my bad then I didn't understand the issue right, let me fix this

@Nogesma Nogesma force-pushed the fix/highlight_newline branch from 2f14c71 to cd3570c Compare August 15, 2024 02:33
@Nogesma
Copy link
Contributor Author

Nogesma commented Aug 15, 2024

This should now be fixed, let me know if it works!

@vesdev
Copy link
Contributor

vesdev commented Aug 15, 2024

This looks like the correct fix and works 👍
Still doesn't build without updating ahash, but that could be a separate pr

@Xithrius Xithrius merged commit cd0210e into Xithrius:main Aug 17, 2024
8 checks passed
@Xithrius
Copy link
Owner

Ok this time I'm pretty sure I tested it properly. I'll update the required dependencies in a new commit.

@Xithrius
Copy link
Owner

Xithrius commented Aug 17, 2024

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.

3 participants