Fix bar-less text output

[spoutn1k]

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.

[djc]

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

[djc]

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

[djc]

Why does this happen?

[spoutn1k]

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]

Thanks! Force pushing is fine IME.

[spoutn1k]

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]

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]

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]

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]

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]

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]

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

[djc]

Thanks, this is a lot easier to review!

[spoutn1k]

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]

Alright, thanks for all your work on this!

[spoutn1k]

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