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: multibyte line trucation #162

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

yossizahn
Copy link

@yossizahn yossizahn commented Dec 14, 2024

In some cases of cell content truncation (i.e. when the last line displayed has to be truncated to add ellipses), the library would throw an error if the content contained multi-byte characters
See the added test case and note that it previously failed

@yossizahn
Copy link
Author

I took the lazy path of using the already existing function - split_long_word
Please feel free to edit my code in any way

@yossizahn
Copy link
Author

Note: While I was at it, I fixed an incorrect >= to >

Copy link
Owner

@Nukesor Nukesor left a comment

Choose a reason for hiding this comment

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

Neat! Good fix.

split_long_word does exactly what you want here. However, in contrast to the previous usecase, you're not only splitting words but a string that might contain delimiters.

So if that's ok for you, it would be awesome if you could add a new commit in which you rename split_long_word to split_string and adjust the docstring accordingly. Otherwise I'll do that myself once this is merged :).

Comment on lines 126 to +127
let surplus = (last_line.width() + 3) - width;
last_line.truncate(last_line.width() - surplus);
let split_result = split_long_word(last_line.width() - surplus, &last_line);
Copy link
Owner

Choose a reason for hiding this comment

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

Please do a double check :D

last_line.width() - surplus is effectively last_line.width() - (last_line.width() + 3 - width), which can be simplified to width - 3 right?

Not sure why I did it that complicated before.

@Nukesor
Copy link
Owner

Nukesor commented Dec 15, 2024

It's kinda funny that the exact same lines of code have been touched in #163, which has been opened at around the same time as your MR. Crazy coincidence.

Anyway, since that one got merged more quickly, please do a quick rebase on main :) Thanks!

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.

2 participants