fix and rename `real_len` by tgolsson · Pull Request #608 · console-rs/indicatif

tgolsson · 2023-12-01T17:47:09Z

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.

indicatif/src/draw_target.rs

Lines 505 to 518 in 81aa4c6

    
           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 · 2023-12-07T08:07:09Z

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 · 2023-12-07T21:25:55Z

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.

djc

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

src/multi.rs

tgolsson · 2023-12-11T21:07:17Z

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

djc

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

These dependencies were using a git dependency due to unmerged bug-fixes. These fixes have now been merged and are available in the latest releases: - console-rs/console#186 - console-rs/indicatif#608 In addition, I think the commits in question may've disappeared? E.g. the fresh build on the new CI platform in had this error: #20997 ``` Updating git repository `https://github.com/tgolsson/console.git` error: failed to load source for dependency `console` Caused by: Unable to update https://github.com/tgolsson/console.git?rev=5483880905f384679d322e83c37180f122951995#54838809 Caused by: revspec '5483880905f384679d322e83c37180f122951995' not found; class=Reference (4); code=NotFound (-3) ``` Thus, this is marked for cherry-picking back to our active branches.

These dependencies were using a git dependency due to unmerged bug-fixes. These fixes have now been merged and are available in the latest releases: - console-rs/console#186 - console-rs/indicatif#608 In addition, I think the commits in question may've disappeared? E.g. the fresh build on the new CI platform in had this error: #20997 ``` Updating git repository `https://github.com/tgolsson/console.git` error: failed to load source for dependency `console` Caused by: Unable to update https://github.com/tgolsson/console.git?rev=5483880905f384679d322e83c37180f122951995#54838809 Caused by: revspec '5483880905f384679d322e83c37180f122951995' not found; class=Reference (4); code=NotFound (-3) ``` Thus, this is marked for cherry-picking back to our active branches. Co-authored-by: Huon Wilson <huon@exoflare.io>

jparris mentioned this pull request Dec 5, 2023

Progress Bars over 80 characters move up the screen when stdout is redirected #609

Closed

kaos mentioned this pull request Dec 6, 2023

Use fork of indicatif to work around output truncation pantsbuild/pants#20269

Merged

tgolsson force-pushed the ts/real-len branch from 31890f0 to b268761 Compare December 7, 2023 21:24

djc requested a review from chris-laplante December 8, 2023 08:56

djc reviewed Dec 8, 2023

View reviewed changes

src/multi.rs Outdated Show resolved Hide resolved

src/multi.rs Show resolved Hide resolved

src/multi.rs Outdated Show resolved Hide resolved

tgolsson force-pushed the ts/real-len branch from b268761 to 06f082e Compare December 8, 2023 22:59

tgolsson added 3 commits December 9, 2023 00:01

move and rename real_len

e3c60d5

handle empty lines properly

b6d05c3

simplify from feedback

9169539

tgolsson force-pushed the ts/real-len branch from 06f082e to 9169539 Compare December 8, 2023 23:02

tgolsson requested a review from djc December 8, 2023 23:05

djc approved these changes Dec 11, 2023

View reviewed changes

djc mentioned this pull request Dec 12, 2023

indicatif crashes with assertion failed: self.orphan_lines_count <= self.lines.len() if number of terminal columns is less than 135 #612

Closed

djc approved these changes Dec 12, 2023

View reviewed changes

djc merged commit 8941d5e into console-rs:main Dec 12, 2023

smoelius mentioned this pull request Dec 25, 2023

Add VisualLines newtype wrapper #616

Merged

huonw mentioned this pull request Jun 4, 2024

Update console and indicatif deps pantsbuild/pants#20998

Merged

This was referenced Jun 4, 2024

Update console and indicatif deps (Cherry-pick of #20998) pantsbuild/pants#21001

Merged

Update console and indicatif deps (Cherry-pick of #20998) pantsbuild/pants#21002

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix and rename `real_len`#608

fix and rename `real_len`#608
djc merged 3 commits intoconsole-rs:mainfrom
tgolsson:ts/real-len

tgolsson commented Dec 1, 2023

Uh oh!

djc commented Dec 7, 2023 •

edited

Loading

Uh oh!

tgolsson commented Dec 7, 2023 •

edited

Loading

Uh oh!

djc left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tgolsson commented Dec 11, 2023

Uh oh!

djc left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

	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)
	};

Conversation

tgolsson commented Dec 1, 2023

Uh oh!

djc commented Dec 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tgolsson commented Dec 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

djc left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tgolsson commented Dec 11, 2023

Uh oh!

djc left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

djc commented Dec 7, 2023 •

edited

Loading

tgolsson commented Dec 7, 2023 •

edited

Loading