Skip to content

Make ProgressBar::index public and clarify meaning #782

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
chris-laplante wants to merge 2 commits into
base: main
Choose a base branch
from
+9 −2
Open

Make ProgressBar::index public and clarify meaning #782

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f2dc688
Make ProgressBar::index public
strowk Jul 27, 2025
ae505a8
progress_bar: clarify ProgressBar::index meaning and make it doc(hidden)
chris-laplante Apr 1, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 9 additions & 2 deletions src/progress_bar.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -618,8 +618,15 @@ impl ProgressBar {
self.state().state.elapsed()
}

/// Index in the `MultiState`
pub(crate) fn index(&self) -> Option<usize> {
/// Opaque index in the `MultiState`.
///
/// This value is an implementation detail and does not necessarily correspond to visual
Copy link
Copy Markdown
Member

@djc djc Apr 1, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So why might want one to use it, and how does one do so without breaking things? Can you talk more about any invariants that must be upheld?

Copy link
Copy Markdown
Collaborator Author

@chris-laplante chris-laplante Apr 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It sounds like it will be useful to the tracing-indicatif crate. Although now that I write that, I wonder if they understand the limitation I mentioned ("does not necessarily correspond to visual order"). So perhaps the better method for me to add would be ProgressBar::visual_index...

Copy link
Copy Markdown
Collaborator Author

@chris-laplante chris-laplante Apr 1, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead, I'd propose (1) leave ProgressBar::index private but (2) add the doc comment, and (3) add a ProgressBar::visual_index method to give people something actually useful.

Copy link
Copy Markdown
Member

@djc djc Apr 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's try to engage one of their maintainers to figure out what it is they actually need?

cc @strowk @emersonford

Copy link
Copy Markdown

@strowk strowk Apr 1, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey, so I needed access to this so that spans in tracing indicatiff could be correctly ordered. This is how fix looked like:

emersonford/tracing-indicatif@main...strowk:tracing-indicatif:fix/children-spans-order

Without knowing that index result is that spans are shown in reverse order (span simply injected after trace bar..), which looked off to me, so I fixed it in the above fork (but never upstreamed since it depends on being able to read this index field). I admit.. I do not understand the 'does not correspond to visual order' enough to reason about consequences. It worked ok in the fork I made

Copy link
Copy Markdown
Collaborator Author

@chris-laplante chris-laplante Apr 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The issue with index would be if you remove a progress bar from the MultiProgress (from the middle, lets say) and then add a new ProgressBar (at the end, say). The index from the removed pb will be reused for the one you just added. What you really want to get at is the MultiState::ordering. That maps indices to the visual order on the screen.

Copy link
Copy Markdown

@strowk strowk Apr 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm my code would break in following way - span 1 starts, span 2 starts, progress now has two entries, span 1 finishes and span 3 starts, takes index of span 1, then when another span 3.a starts as child of span 3, it would probably be inserted correctly after span 3 (offset would be 0 and I simply call .add(/index-of-span-3/)), but then another span 3.b would be added as .add(/index-of-span-3/+1) which will not be after span 3.a , but rather after span 2. I never seen this happening due to tool where I use indicatiff tracing never getting this exact state, but it seems that my naive use of index is incorrect and you should ignore it.. :) sorry

Copy link
Copy Markdown
Collaborator Author

@chris-laplante chris-laplante Apr 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No worries :). I think unless @djc objects, I'll just go with my plan from here #782 (comment) since it sounds like you really do need the visual index, not the internal index.

Copy link
Copy Markdown

@strowk strowk Apr 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That may be true, but the more I think about this the more I suspect my approach with indexes and offsets is even more flawed than I realized now. I also assumed that span 2 does not have its own children while I'm adding progress bar for span 3.a - this would also break even if I have visual order index of span 3 .. So the right way is probably keeping track of all children there.. Sadly I will unlikely to get to this any time soon :)

/// position on the screen.
///
/// It is an index into `MultiState::members`, which is not in any particular order (due to
/// reclaiming of indices from the `MultiState::free_set`).
#[doc(hidden)]
pub fn index(&self) -> Option<usize> {
self.state().draw_target.remote().map(|(_, idx)| idx)
}

Expand Down