[findings] findings from multi-progress

[cylewitruk]

Here's a few gotchas I've run into so far -- will keep updating this issue as I run into things ;) I'm happy to tackle some of them as time allows, but we can start with talking about them :)

1. The progress/download bars work just fine without having called start() - they will just be default indicatif styling (ugly). Could be easy for a user to miss and not understand why the styling isn't working.

2. Some weird UI behavior that I haven't figured out yet (the only "Bitcoin Core" is the title of the multi_progress: This is a simple multi-progress with download + spinner + spinner being added sequentially, with start and stop being called on them before creating the next bar.

EDIT: This seems to have something to do with the stop() method on either the progressbar or multibar -- you can see in the image in pt. 5 that when I don't call stop() on the multi_progress that this duplicate doesn't happen.

3. Would be better if with_ methods returned &mut Self, otherwise there's move issues in closures.

4. There should be a get_theme() method to return the current theme. I'm trying to "swap" the template of a progress bar from a spinner ("Preparing to download...") to a download bar ("Downloading...") and it's proving very difficult. "Preparing to download" is retrieving headers and getting the total size, and "Downloading" is when the size is known and the progress bar can start.

What I'd like to ideally have is a spinner while preparing, and switch to download once I know the size.

Here's a little example from the "download" step in the image from pt. 2:

let multi = cliclack::multi_progress("Bitcoin Core");
let dl = multi.add(progress_bar(100).with_spinner_template());
dl.bar().set_message("Download starting...");

let mut total_size = 0;
let mut progress = 0;

let bitcoin_core_archive = download_file(
    &bitcoin_url,
    &ctx.tmp_dir,
    |size| {
        total_size = size.clone();
        // `dl` MOVED HERE
        let dl = dl.with_download_template();
        dl.bar().set_message("Downloading Bitcoin Core...");
        dl.bar().set_length(total_size);
    },
    |chunk, _| {
        // `dl` has been moved...
        dl.bar().inc(chunk);
        progress += chunk;
    },
)
.await?;

dl.stop(format!(
    " {} {}",
    style("✔").green(),
    "Download Bitcoin Core"
));

5. If one forgets to call stop() on the multi_progress then the grouping fails: Note that until multi.stop() is called, this formatting is applied (i.e. it's very obvious during longer steps like downloading).

6. I seem to be getting extra padding before the outro()? I didn't see this in your screenshot :thinking:

[fadeevab]

@cylewitruk Progress bars is a rabbit hole.

Alright, pt.2: It's fixable, I can fix it. pt.5: cliclack is literally a printer, if you forget to notify that the multi-bar is stopped, it doesn't become stopped. As an option, I have to implement a mediator pattern where the progress bars hold the reference to the multi-bar and they notify they're finished. pt.6: the padding is noticeable if the multi-progress block is at the end of the list before the outro. There is always a gap to keep spacing before the next element.

[cylewitruk]

@cylewitruk Progress bars is a rabbit hole.

Haha 🐰 It's a little fun, admit it 😉 They look pretty cool, anyway :)

pt.2: It's fixable, I can fix it.

Ok!

pt.5: cliclack is literally a printer, if you forget to notify that the multi-bar is stopped, it doesn't become stopped. As an option, I have to implement a mediator pattern where the progress bars hold the reference to the multi-bar and they notify they're finished.

I had this working in my PR branch, guessing I did something similar. Afk right now but iirc I had logic to print the left bar based on the bar's position in the multibar, so only the last one ever printed with the S_BAR_END which removed the flickering. Let me know if you want me to come with a suggestion-PR?

pt.6: the padding is noticeable if the multi-progress block is at the end of the list before the outro. There is always a gap to keep spacing before the next element.

~Aha, okay -- in this particular case it feels a little weird visually to me - what do you think? My feeling is that it feels that it makes the "finish" more connected to the following element than the current.~ Realized that an outro shouldn't be used here in the middle of the interaction :)

[fadeevab]

@cylewitruk

I had logic to print the left bar based on the bar's position in the multibar

I have the same logic.

What I'd like to ideally have is a spinner while preparing, and switch to download once I know the size.

Keep it simple: run the spinner, wait, show awaiting is done, and at the next step show a progress bar.

There should be a get_theme() method to return the current theme.

When you implement your theme it inherits ALL the methods of the default theme, you can retrieve any string from those methods.

For now, use 0.2.1 with fixes.

[fadeevab]

@cylewitruk and I still find it super buggy... More fixes to come

[cylewitruk]

@cylewitruk and I still find it super buggy... More fixes to come

Yeah there's still some issues, but not all that bad imo -- stuff I'm putting in this ticket are just findings/thoughts, nothing that is a blocker so far, so no stress (well, pt. 2 is a bit annoying :sweat_smile:) :) Feel free to tack-on things you find here as well so the info is consolidated -- I figure we can create issues from things here that should be changed/fixed instead of issue-spamming.

I have the same logic.

Hmm, weird...

When you implement your theme it inherits ALL the methods of the default theme, you can retrieve any string from those methods.

Sure, that'll work. My thought here is simply to avoid making users create a theme just for the sake of getting access to defaults.

[cylewitruk]

The multiprogress module is private, so the MultiProgress (not re-exported) can't be passed around (nice to be able to do when isolating different steps to smaller methods).

[fadeevab]

What I'd like to ideally have is a spinner while preparing, and switch to download once I know the size.

@cylewitruk Check the version 0.2.2, I added a clear() method, so you can now add the spinner, clear it, and continue with a progress bar!

Warning: I also had to stop exposing indicatif via the bar() method but it made rendering easier to implement.

P.S.: MultiProgress is public: https://docs.rs/cliclack/latest/cliclack/struct.MultiProgress.html

[cylewitruk]

What I'd like to ideally have is a spinner while preparing, and switch to download once I know the size.

@cylewitruk Check the version 0.2.2, I added a clear() method, so you can now add the spinner, clear it, and continue with a progress bar!

Great! Thanks :)

Warning: I also had to stop exposing indicatif via the bar() method but it made rendering easier to implement.

Hm, how do I set_message(), set_length() and inc() now?

P.S.: MultiProgress is public: docs.rs/cliclack/latest/cliclack/struct.MultiProgress.html

It worked now after a cargo update :+1: (I'm using github directly atm, so no need to publish to crates.io yet if it's troublesome)

[fadeevab]

Hm, how do I set_message(), set_length() and inc() now?

I have to add it to API :) Honestly, indicatif's finish_and_clear and other similar methods are out of control, corner cases only.

[cylewitruk]

I have to add it to API :)

Hehe okay, wasn't sure if I was missing something :sweat_smile:

Honestly, indicatif's finish_and_clear and other similar methods are out of control, corner cases only.

A lot of the stuff in indicatif is slightly questionable imo :rofl: That's where cliclack has a real strength in abstracting away a lot of the complexity and giving reasonable defaults :)

But two frameworks fighting for the terminal doesn't make things easier when it comes to implementing this stuff :sweat_smile:

[fadeevab]

@cylewitruk Check the main branch

[cylewitruk]

@cylewitruk Check the main branch

@fadeevab theere we go, it builds again! :dancers: :champagne: thanks :)

[cylewitruk]

@fadeevab aaahhh, now we're looking pretty nice :)

[fadeevab]

@cylewitruk Woow, noooice :)

[cylewitruk]

Just so I don't forget, there's an extra space after the spinner multibar: Current workaround is to lead the text with a \u8

[fadeevab]

@cylewitruk To be sure I understand: do you mean an empty line under the cyan end bar?

[cylewitruk]

@cylewitruk To be sure I understand: do you mean an empty line under the cyan end bar?

No, alignment of "preparing" vs. "Clean"

[fadeevab]

@cylewitruk I can suggest redefining the theme: format_progress_start, it sets the 2 spaces.

[fadeevab]

@cylewitruk FYI, the patch is simple, however, 2 spaces look good as well. PR https://github.com/fadeevab/cliclack/pull/34

[fadeevab]

@cylewitruk Let's close this issue, everything seems fixed now. Let's start a new discussion if it's indeed necessary.