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 and rename real_len #608

Merged
merged 3 commits into from Dec 12, 2023
Merged

fix and rename real_len #608

merged 3 commits into from Dec 12, 2023

Conversation

tgolsson
Copy link
Contributor

@tgolsson tgolsson commented Dec 1, 2023

Fixes #606 -- see added test cases. This logic matches what happens draw_target.rs as well. It'll now also correctly handle lines with only ANSI codes.

let diff = if line.is_empty() {
// Empty line are new line
1
} else {
// Calculate real length based on terminal width
// This take in account linewrap from terminal
let terminal_len = (line_width as f64 / term_width as f64).ceil() as usize;
// If the line is effectively empty (for example when it consists
// solely of ANSI color code sequences, count it the same as a
// new line. If the line is measured to be len = 0, we will
// subtract with overflow later.
usize::max(terminal_len, 1)
};

@djc
Copy link
Collaborator

djc commented Dec 7, 2023

Thanks for working on this!

Can we share the code with draw_target somehow, so that it is more obviously using the same algorithm?

If we're going to move this I'd like to see two separate commits, one that moves/renames the code and one that changes the algorithm. Ideally tests that work with the old algorithm are introduced before this, and then the fixing commit adds tests that didn't work previously -- this will make it easy for me to review. (Please squash linting changes etc.)

@tgolsson
Copy link
Contributor Author

tgolsson commented Dec 7, 2023

Can we share the code with draw_target somehow, so that it is more obviously using the same algorithm?

Doesn't seem like a worthwhile change seeing as that is counting lines while drawing, while this does it ahead of time.

If we're going to move this I'd like to see two separate commits, one that moves/renames the code and one that changes the algorithm. Ideally tests that work with the old algorithm are introduced before this, and then the fixing commit adds tests that didn't work previously -- this will make it easy for me to review. (Please squash linting changes etc.)

Please see new commits -- first commit moves + add working tests, second commit adds the failing tests, third commit fixes the failing tests.

Copy link
Collaborator

@djc djc left a comment

Choose a reason for hiding this comment

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

Thanks, that is better. A few nits to address...

src/multi.rs Outdated Show resolved Hide resolved
src/multi.rs Show resolved Hide resolved
src/multi.rs Outdated Show resolved Hide resolved
@tgolsson
Copy link
Contributor Author

CI seems like a flake, can't repro at all locally.

Copy link
Collaborator

@djc djc left a comment

Choose a reason for hiding this comment

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

Yup, the CI failures seemed to go away on rerunning. This is looking great, thanks!

@djc djc merged commit 8941d5e into console-rs:main Dec 12, 2023
10 checks passed
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.

Empty lines in MultiProgress::println leads to lost output
2 participants