set_move_cursor plays badly with println

[Hawk777]

The documentation for set_move_cursor says:

This can reduce flickering, but do not enable it if you intend to change the number of progress bars.

It does not say anything about other restrictions, only not to use it when changing the number of bars. Consider the following code:

use indicatif::{MultiProgress, ProgressBar};
use std::thread::sleep;

fn main() {
	const SEC: std::time::Duration = std::time::Duration::from_secs(1);
	let pb = ProgressBar::new_spinner();
	let mp = MultiProgress::new();
	mp.add(pb.clone());
	mp.set_move_cursor(true);
	pb.set_message("This is quite a long message");
	sleep(SEC);
	pb.tick();
	sleep(SEC);
	mp.println("foo");
	sleep(SEC);
	pb.tick();
	sleep(SEC);
}

This code does not, at any point, change the number of progress bars—it creates exactly one progress bar that lasts for the program’s whole lifetime, and adds it to the MultiProgress as early as it possibly can, before setting the message or doing anything else with either object. However, the output of this program looks like this, once the println runs:

foohis is quite a long message                                                                                                                                                                
⠉ This is quite a long message                                                                                                                                                                

I’m using urxvt 9.31.

[Hawk777]

Using set_move_cursor(true) also seems to cause MultiProgress::clear to not clear all the lines properly. This code:

use indicatif::{MultiProgress, ProgressBar};
use std::thread::sleep;

fn main() {
	const SEC: std::time::Duration = std::time::Duration::from_secs(1);
	let pb = ProgressBar::new_spinner();
	let pb2 = ProgressBar::new_spinner();
	let pb3 = ProgressBar::new_spinner();
	let pb4 = ProgressBar::new_spinner();
	let mp = MultiProgress::new();
	mp.add(pb.clone());
	mp.add(pb2.clone());
	mp.add(pb3.clone());
	mp.add(pb4.clone());
	mp.set_move_cursor(true);
	pb.set_message("This is quite a long message");
	pb2.set_message("This is quite a long message");
	pb3.set_message("This is quite a long message");
	pb4.set_message("This is quite a long message");
	sleep(SEC);
	pb.tick();
	pb2.tick();
	pb3.tick();
	pb4.tick();
	sleep(SEC);
	mp.clear().unwrap();
}

also doesn’t dynamically change the number of progress bars, but when I run it, my terminal ends up looking like this after the program terminates:

me@place $ cargo run
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.00s
     Running `/home/me/.cache/cargo/target/debug/foo`
me@place $ 
⠁ This is quite a long message                                                                                                                                                                
⠁ This is quite a long message                                                                        

[djc]

So why are you using this method?

[Hawk777]

The documentation says it may reduce flickering. All else being equal, that seems like a good thing; even if I don’t see flickering on my choice of terminal emulator, someone else using a different one might. I read it as basically “this makes things better, but is incompatible with dynamically changing number of progress bars”. Since I’m not dynamically changing number of progress bars, why wouldn’t I turn on the “make things better” flag?

[djc]

Maybe @chris-laplante will have some feedback here, but I'd advise just avoiding the method if you don't need it.

[chris-laplante]

Maybe @chris-laplante will have some feedback here, but I'd advise just avoiding the method if you don't need it.

I'll caveat by saying I'm not very familiar with set_move_cursor. But if I had to guess - conceptually, I'd argue a println is very similar to adding a progress bar (albeit text-only, which we only draw once). So I think we need to expand the "do not enable it if you intend to change the number of progress bars" restriction to also cover println, and probably also clear.

To actually make println work with set_move_cursor, I suspect we'd need to teach MultiProgress::draw about it. Unfortunately I don't time to do this though.

[chris-laplante]

@djc as a workaround, we could temporarily turn off 'move cursor' during a println (possibly also a clear) then re-enable. Something like:

diff --git a/src/multi.rs b/src/multi.rs
index e2f64bc..5256bb3 100644
--- a/src/multi.rs
+++ b/src/multi.rs
@@ -177,7 +177,13 @@ impl MultiProgress {
     /// will not do anything.
     pub fn println<I: AsRef<str>>(&self, msg: I) -> io::Result<()> {
         let mut state = self.state.write().unwrap();
-        state.println(msg, Instant::now())
+
+        state.draw_target.set_move_cursor(false);
+        let ret = state.println(msg, Instant::now());
+
+        // TODO: only enable if it was before - this is just for demo purposes
+        state.draw_target.set_move_cursor(true);
+        ret
     }

I need to test it more, but it seems to do what we want.

I'd argue that the performance impact doesn't matter in this case, since println and clear should be relatively infrequent compared to normal progress updates.

[djc]

I dislike this API anyway; from the docs at least it's not clear at all when people should use it. If it's helpful, should we just enable it for everyone and flush out warts like this?

[chris-laplante]

I dislike this API anyway; from the docs at least it's not clear at all when people should use it. If it's helpful, should we just enable it for everyone and flush out warts like this?

In general I'd be in favor of that, but I worry whether there are terminals that don't properly support the 'move cursor' escape sequences?

[djc]

I dislike this API anyway; from the docs at least it's not clear at all when people should use it. If it's helpful, should we just enable it for everyone and flush out warts like this?

In general I'd be in favor of that, but I worry whether there are terminals that don't properly support the 'move cursor' escape sequences?

Do you have concrete reasons to think this is an issue? If not, maybe we should just try it? Can always roll it back if it breaks too many people.

[chris-laplante]

I dislike this API anyway; from the docs at least it's not clear at all when people should use it. If it's helpful, should we just enable it for everyone and flush out warts like this?

In general I'd be in favor of that, but I worry whether there are terminals that don't properly support the 'move cursor' escape sequences?

Do you have concrete reasons to think this is an issue? If not, maybe we should just try it? Can always roll it back if it breaks too many people.

I do not, so I'm on-board to try it.