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 highlight #649

Merged
merged 1 commit into from
Aug 10, 2024
Merged

Fix off by one error in highlight #649

merged 1 commit into from
Aug 10, 2024

Conversation

vesdev
Copy link
Contributor

@vesdev vesdev commented Aug 3, 2024

before
image

after
image

it does not account for the newlines properly

shoutout to mostlymaxi's chat for noticing this

@vesdev
Copy link
Contributor Author

vesdev commented Aug 3, 2024

also updated minor version of ahash as 0.8.3 does not compile for me, did not bother separating it into a different commit as its a very small change

@vesdev
Copy link
Contributor Author

vesdev commented Aug 3, 2024

Looking more into it there is something funky with the wrapping. start_index is not correct after the line is built for the first_line_msg

@vesdev vesdev marked this pull request as draft August 3, 2024 02:48
@pull-request-size pull-request-size bot added size/S and removed size/XS labels Aug 3, 2024
@vesdev vesdev force-pushed the main branch 2 times, most recently from 2aeec9e to bd57967 Compare August 3, 2024 03:38
@vesdev vesdev marked this pull request as ready for review August 3, 2024 03:42
Copy link

codecov bot commented Aug 3, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 17.87%. Comparing base (31bcc1c) to head (46248bc).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #649      +/-   ##
==========================================
+ Coverage   17.86%   17.87%   +0.01%     
==========================================
  Files          42       42              
  Lines        5430     5431       +1     
==========================================
+ Hits          970      971       +1     
  Misses       4460     4460              

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

@Nogesma
Copy link
Contributor

Nogesma commented Aug 5, 2024

I think I know what the issue is. When finding the indices to highlight, the message contains a newline character, which is then removed when splitting the message into multiple lines. This leads to the start_index not matching the highlight index.

A solution might be to just strip newlines from the message when trying to find the sections to highlight, or maybe just do a +1 to the start index only when the current line contained a newline.

Your solution, if I'm right, would give us an off by one highlight when the message spans more than one line without containing a newline.

Let me know if you need any more help with the PR.

@vesdev
Copy link
Contributor Author

vesdev commented Aug 5, 2024

I think I know what the issue is. When finding the indices to highlight, the message contains a newline character, which is then removed when splitting the message into multiple lines. This leads to the start_index not matching the highlight index.

A solution might be to just strip newlines from the message when trying to find the sections to highlight, or maybe just do a +1 to the start index only when the current line contained a newline.

Your solution, if I'm right, would give us an off by one highlight when the message spans more than one line without containing a newline.

Let me know if you need any more help with the PR.

Looking at textwrap docs, wrap() should not include a trailing newline char.
https://docs.rs/textwrap/latest/textwrap/fn.wrap.html

@Xithrius
Copy link
Owner

Xithrius commented Aug 5, 2024

Sorry I haven't replied yet, busy with IRL stuff. I'll take a look at this when I can.

@Xithrius
Copy link
Owner

Tested between branches, this fix looks to work perfectly. Thanks for the PR!

@Xithrius Xithrius merged commit 62f53e4 into Xithrius:main Aug 10, 2024
8 checks passed
@Nogesma
Copy link
Contributor

Nogesma commented Aug 10, 2024

I'm afraid this PR breaks the case where the line just overflows onto the next line, without a line break.

This is from current git main branch:
image

The full solution needs to check where the newlines are in the message, then increment the start_index at the right time.

I can make a PR that fixes this, unless @vesdev wants to do it?

Edit: Fixed bad image

@Xithrius
Copy link
Owner

Ah hell I thought I tested that, my bad. Good thing I didn't do a release for it 😅

@vesdev
Copy link
Contributor Author

vesdev commented Aug 11, 2024

I'm afraid this PR breaks the case where the line just overflows onto the next line, without a line break.

This is from current git main branch: image

The full solution needs to check where the newlines are in the message, then increment the start_index at the right time.

I can make a PR that fixes this, unless @vesdev wants to do it?

Edit: Fixed bad image

Yea feel free to, the proper fix I think would be using the textwrap output for getting the highlight indices instead of the message payload. Didn't think about break_word 🤔

Nogesma added a commit to Nogesma/twitch-tui that referenced this pull request Aug 13, 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