Make ProgressBar::index public and clarify meaning#782
Conversation
|
@djc If this looks OK, I can rebase everything on main. |
| /// Index in the `MultiState` | ||
| /// Opaque index in the `MultiState`. | ||
| /// | ||
| /// This value is an implementation detail and does not necessarily correspond to visual |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Let's try to engage one of their maintainers to figure out what it is they actually need?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 :)
3 participants
No description provided.