Fix bar-less text output by spoutn1k · Pull Request #659 · console-rs/indicatif

spoutn1k · 2024-09-26T18:50:44Z

This is my attempt at fixing #447.

The code in draw_to_term is a bit of mess, with double definitions, complex branching, and I tried cleaning it up. I introduce a LineType enum to differentiate between text, bars and empty lines. The variables names have been renamed to reflect better what they represent. Some code has been shuffled around. Tests pass and I ran examples at random with no issues.

A sample program to understand what this PR wants to fix:

static LOREM_SMALL: &str = "Lorem ipsum dolor sit amet.";
static LOREM_BIG: &str= "Lorem ipsum dolor sit amet, consectetur adipiscing elit. Donec nec viverra massa. Nunc nisl lectus,
auctor in lorem eu, maximus elementum est.";

fn generate_logs(manager: &indicatif::MultiProgress, i: u32) {
    if i % 20 == 0 {
        let _ = manager.println(LOREM_BIG);
    } else if i % 10 == 0 {
        let _ = manager.println(LOREM_SMALL);
    }
}

fn main() {
    let display_manager = indicatif::MultiProgress::new();
    let bar = display_manager.add(indicatif::ProgressBar::new(100));

    for i in 0..100 {
        generate_logs(&display_manager, i);
        std::thread::sleep(std::time::Duration::from_millis(10));
    }

    for i in 0..100 {
        bar.inc(1);
        generate_logs(&display_manager, i);
        std::thread::sleep(std::time::Duration::from_millis(100));
    }
}

This is the output of the above before the PR:

djc

Thanks for trying to improve this code. Especially because this is pretty gnarly code, it would be good to split these changes into smaller commits that make minimal (logical) changes -- for example, might introduce the LineType enum while representing only the types of lines that are currently in lines, and then later introduce the other variants.

src/draw_target.rs

djc · 2024-10-07T09:27:30Z

src/draw_target.rs

+                    .filter(|l| matches!(l, LineType::Text(_)))
+                    .map(|l| String::from(l.as_ref())),


Suggest we use filter_map() instead of filter().map(). Also prefer to_owned() instead of String::from() here.

djc · 2024-10-07T09:28:05Z

src/draw_target.rs


 const MAX_BURST: u8 = 20;

+#[derive(Debug, Clone, PartialEq)]


We have a top-down order here, this should move down below any types that rely on it (DrawState at least).

djc · 2024-10-07T09:29:02Z

src/draw_target.rs

 impl DrawState {
    fn draw_to_term(
-        &mut self,
+        &self,


Why does this happen?

spoutn1k · 2024-10-07T16:29:57Z

Thank you for taking the time to review this. My tests omitted the --all-features flag and it shows. It did not help that this PR is the result of a caffeine overdose and subsequent insomnia. Comments duly noted this PR will either be force pushed or a new one will be opened with working tests and better commits. Thanks again !

djc · 2024-10-07T16:38:20Z

Thanks! Force pushing is fine IME.

spoutn1k · 2024-10-10T05:23:12Z

indicatif/src/draw_target.rs

Lines 554 to 556 in 5396704

    
           // Keep the cursor on the right terminal side 
        
           // So that next user writes/prints will happen on the next line 
        
           last_line_filler = term_width.saturating_sub(line_width);

I think my main issue stems here. For wrapping lines, this calculation is not accurate. However the concept itself seems strange to me. Why fill the line with whitespace and expect the terminal to wrap instead of moving the cursor down when needed ? This means when resizing the terminal the formatting will fall apart.

Any objections to me investigating changing this behaviour ?

spoutn1k · 2024-10-10T18:56:46Z

The new commit passes all the tests but two, and that is because the filler is not only printed when needed (removing one variable definition). On my machine:

---- multi_progress_many_bars stdout ----
thread 'multi_progress_many_bars' panicked at tests/render.rs:1230:5:
assertion failed: `(left == right)`
                                                                                                                                                                                 
Diff < left / right > :
[...]
 Str("")
 NewLine
 Str("⠁ 2")
>Str("") <---------------------------------- Those useless prints are avoided
 Flush

djc · 2024-10-11T08:16:29Z

I think my main issue stems here. For wrapping lines, this calculation is not accurate. However the concept itself seems strange to me. Why fill the line with whitespace and expect the terminal to wrap instead of moving the cursor down when needed ? This means when resizing the terminal the formatting will fall apart.

Any objections to me investigating changing this behaviour ?

I never have objections to other people investigating things. 😄 Suggest reading git blame to figure out why this change was made, pretty sure it wasn't just for funsies.

djc · 2024-10-11T08:16:54Z

The new commit passes all the tests but two, and that is because the filler is not only printed when needed (removing one variable definition).

Removing the empty writes from the test seems fine to me.

spoutn1k · 2024-10-11T12:44:16Z

Suggest reading git blame to figure out why this change was made, pretty sure it wasn't just for funsies.

I believe this is working in tandem with this check:

indicatif/src/draw_target.rs

Lines 549 to 551 in 5396704

    
           if idx != 0 { 
        
               term.write_line("")?; 
        
           }

This means that for everyline line other than the first one, a newline is added. The first is expected to wrap automatically due to the filler set in my previous comment.

spoutn1k · 2024-10-11T12:53:21Z

This effectively fixes #447. I deeply want to keep going, because there is a lot of further refactoring that can be done, and the current solution of padding the last line will break any resizing. This is, for future reference, the watermark to go back to if the PR spins out of control

spoutn1k · 2024-10-11T14:33:02Z

In a rare moment of lucidity I will not change the line padding in this PR. It is now ready for review again @djc !

src/draw_target.rs

djc

Thanks, this is a lot easier to review!

src/draw_target.rs

tests/render.rs

src/draw_target.rs

tests/render.rs

spoutn1k · 2024-11-12T13:00:23Z

Resolved most of the comments. For the separate commits, I am very sorry but I do not have infinite time to alter commits of code that gets optimized out in the full PR. I do not think it alters readability either.

djc · 2024-11-15T11:57:07Z

Alright, thanks for all your work on this!

spoutn1k · 2024-11-15T12:51:57Z

Thank you, it was a pleasure, I just am travelling a lot at the moment. Cheers !

djc reviewed Oct 7, 2024

View reviewed changes

spoutn1k force-pushed the linetype-enum branch 2 times, most recently from 506bb01 to 1b29cf0 Compare October 10, 2024 18:55

spoutn1k force-pushed the linetype-enum branch from c1b6c0f to 8c7038e Compare October 11, 2024 14:16

djc reviewed Oct 20, 2024

View reviewed changes

src/draw_target.rs Outdated Show resolved Hide resolved

spoutn1k force-pushed the linetype-enum branch 3 times, most recently from 62a7eea to 1433812 Compare November 11, 2024 16:10

djc approved these changes Nov 12, 2024

View reviewed changes

spoutn1k added 12 commits November 12, 2024 16:53

Remove empty prints

88600b7

Add test for console-rs#447

fa6587a

Do not create useless variable len

616caf4

Do not create useless variable term_height

6a5af10

Factor height calculations to avoid repetitions

8bda72c

Rename visual_lines to full_height

a221f50

Rename last_line_count to bar_count

eae7eeb

Rename orphan_visual_line_count (WTF) to text_height

f6ee030

Remove variable in unnecessary scope

0734118

Rename real_len to real_height

fc2c136

Introduce LineType

c598100

Move all height calculation to LineType; add comments

73b5b37

spoutn1k added 3 commits November 12, 2024 16:57

Sunset confusingly named, obsolete external count

fd40626

Un-mystify terminal overflow checking

7b1043c

Purge debug assert and all lines making it up

c7001af

spoutn1k force-pushed the linetype-enum branch from 1433812 to c7001af Compare November 12, 2024 12:57

djc merged commit 632989d into console-rs:main Nov 15, 2024

		.filter(\|l\| matches!(l, LineType::Text(_)))
		.map(\|l\| String::from(l.as_ref())),

Conversation

spoutn1k commented Sep 26, 2024 • 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!

djc Oct 7, 2024

Choose a reason for hiding this comment

Uh oh!

djc Oct 7, 2024

Choose a reason for hiding this comment

Uh oh!

djc Oct 7, 2024

Choose a reason for hiding this comment

Uh oh!

spoutn1k commented Oct 7, 2024

Uh oh!

djc commented Oct 7, 2024

Uh oh!

spoutn1k commented Oct 10, 2024

Uh oh!

spoutn1k commented Oct 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

djc commented Oct 11, 2024

Uh oh!

djc commented Oct 11, 2024

Uh oh!

spoutn1k commented Oct 11, 2024

Uh oh!

spoutn1k commented Oct 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

spoutn1k commented Oct 11, 2024

Uh oh!

Uh oh!

djc left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

spoutn1k commented Nov 12, 2024

Uh oh!

djc commented Nov 15, 2024

Uh oh!

spoutn1k commented Nov 15, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

spoutn1k commented Sep 26, 2024 •

edited

Loading

spoutn1k commented Oct 10, 2024 •

edited

Loading

spoutn1k commented Oct 11, 2024 •

edited

Loading