-
Notifications
You must be signed in to change notification settings - Fork 242
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.
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
Implement multi-line progress message support #443
Implement multi-line progress message support #443
Conversation
If
I'm honestly not sure I 100% understand them, myself. @chris-laplante do you? I think they're used to keep track of how many lines we drew in the previous iteration, so that we can correct if we're drawing fewer lines in the current iteration.
Would you mind filing a separate issue about this? |
Could you provide an example of this happening? It would probably be possible to render completely on a line-by-line basis, since the render target is not really aware of the content of the lines and just redraws everything every time. I'm not sure I understand the issue you're referring to here though.
From what I could glean from the code, it's about cleaning up old lines when the progress is dropped, as well as when lines are printed manually by the user and lines that were moved up as a result need to be forgotten. So I defaulted to counting multiline strings in there as well, since they could potentially contain multiline messages set by the user. Not sure if this is necessary though.
Sure, will do! |
The code is a little hard to follow, but what I understand is that |
Cool, feel free to make changes or add suggestions on my PR. |
Will do, but BTW I meant that the code in indicatif currently is a little hard to follow, not your code :) |
Haha, don't worry about it, that's not how I took it. I didn't add enough code for it to be too complicated anyways :-P |
Is there anything I can help with to move this along? I'm depending on my fork with my own projects for now, but of course I'd prefer if I can go back to upstream. I don't mind this taking longer, but I'd be happy to help if I can speed up the process :-) |
I'm still wondering if there's a good reason not to have one element per line in |
Just had a more thorough look at this. From what I can see, the different parts of the status (msg, prefix and so on) are set separately, and then they are appended to This already introduces overhead ( I think my approach is not optimal, but it makes the rendering of newlines in any part of the progress state a part of the rendering process, which allows any amount of newlines in any part of the template or message. Maybe I misunderstood you and I'm missing an easier solution here... What do you think? |
Hmm, maybe it would be better to split stuff inside |
Alright, that turned out to be a great idea! I'm now splitting the buffer right before any new lines are appended, and running the same code as before if no newlines are contained. This should handle all cases, even when the newlines are split over different parts of the template. This also enabled me to add some tests for the cases I added. Have a look and tell me if there's anything else I can add. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking pretty good! I'd like it if you squashed all your changes into two commits: one that extracts the push_line()
method and one that adds the new functionality and the test. (I usually squash using git rebase -i
.)
src/style.rs
Outdated
|
||
// If cur doesn't contain any newlines now, we can add | ||
// the line as is. | ||
if !expanded.contains('\n') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is somewhat less efficient than it could be. I think what we want here is to skip this extra if
and instead, in the loop below, add enumerate()
and check on i == 0 && line.len() == expanded().len()
to avoid the allocation in that case.
examples/multi-log.rs
Outdated
@@ -0,0 +1,81 @@ | |||
use std::thread; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer not to add a separate example for this.
02588ff
to
ff61137
Compare
Alright, all done. Please let me know if there's anything else I can do. |
ff61137
to
64126e2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me! I think there is some potential for optimization, but this will do as a first take. @chris-laplante any thoughts?
It looks good to me too and some light testing on my own code gives correct results :). The only thing I'm wondering is whether we've tested what happens when you change the number of newlines in messages using |
What's the status on this? Can I do anything in addition to help this land faster? |
You could submit some tests for the case Chris mentioned in his last comment, I think? |
This pulls the part of format_style that appends a new line to the progress into a separate function, which can be used to transform the expanded line content before it is added.
During format_style, we now split up expanded lines before they are added, accounting for the fact that there could be additional newlines in the line we're adding.
64126e2
to
fc29f46
Compare
bc99b50
to
cb314ac
Compare
Alright, I've added additional tests that render a Please tell me if these tests cover the cases you were thinking about. |
Thanks for adding the tests! This looks good to me. @djc alright to merge? |
Great, thanks! |
This PR adds support for multi-line progress messages, following #442.
The changes mostly update the drawing functions to split progress messages before counting them, so that the correct number of lines are cleared when they are redrawn. I'm not sure I 100% understood how orphan lines work, so I'd appreciate if someone could take a closer look at how I handle them.
The existing examples all run as they did previously, and I've added a new example (
multi-log
) showing the style I was trying to implement when I opened the issue.One thing I noticed is that calling
clear
on a multiline progress doesn't always seem to clear all progress messages, a random amount stays on the screen even when a message is printed afterwards. I first thought that I introduced that bug, but switching back to main it still appeared in the examples I tried (yarnish
andmulti
). Is this a known bug?