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 subtract with overflow when measuring terminal line length #547

Merged
merged 1 commit into from Jun 3, 2023
Merged

Fix subtract with overflow when measuring terminal line length #547

merged 1 commit into from Jun 3, 2023

Conversation

foresterre
Copy link
Contributor

@foresterre foresterre commented Jun 2, 2023

If console::measure_text_width returns 0, the line is effectively empty, although line.is_empty() may still be false. This can for example happen when there is a line which just consists of ANSI color escape sequences.

I hope I didn't undo the work done in #533.

An alternative would be to do:

// make sure we never subtract with overflow
let real_len = max(real_len, self.orphan_lines_count + shift);

*last_line_count = real_len - self.orphan_lines_count + shift;

closes #546

@djc
Copy link
Collaborator

djc commented Jun 2, 2023

Sorry for the regression, and thanks for the report and fix! I'm going to merge and release this -- it would be great if you are able to add a test that covers this behavior somehow.

@foresterre
Copy link
Contributor Author

foresterre commented Jun 2, 2023

it would be great if you are able to add a test that covers this behavior somehow

Agreed! I did try to create a test case, but didn't get it to fail properly yet, which is weird 🙃. I expected that pb.println("<ansii color escape sequence>"); pb.tick(); would work (as this is what I saw in my program), but somehow it didn't.

I'll have another try tonight.

@@ -12,6 +12,8 @@ fn main() {
};

{
// TODO(clippy false positive): https://github.com/rust-lang/rust-clippy/issues/10577
#[allow(clippy::redundant_clone)]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added this because otherwise the Clippy lint workflow will fail.

@foresterre
Copy link
Contributor Author

I added a test 🙂.

If measure_text_width returns 0, the line is effectively empty, although line.is_empty() may still be false. This can for example happen when there is a line which just consists of ANSI color escape sequences.
@foresterre
Copy link
Contributor Author

I removed the commit which added the same as #549, and rebased on main.

@djc djc merged commit f88ec3b into console-rs:main Jun 3, 2023
9 checks passed
@djc
Copy link
Collaborator

djc commented Jun 3, 2023

Great work, thanks!

@foresterre foresterre deleted the subtract-with-overflow branch June 3, 2023 23:23
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.

Subtract with overflow when measuring line length in 0.17.4
2 participants