Implement multi-line progress message support #443

happenslol · 2022-06-13T18:58:40Z

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 and multi). Is this a known bug?

djc · 2022-06-14T08:31:00Z

If lines does not contain one element per line, that seems like a problem to me. Should we split up multi-line messages earlier in the process to deal with this?

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.

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.

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 and multi). Is this a known bug?

Would you mind filing a separate issue about this?

happenslol · 2022-06-14T11:55:41Z

If lines does not contain one element per line, that seems like a problem to me. Should we split up multi-line messages earlier in the process to deal with 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.

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.

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.

Would you mind filing a separate issue about this?

Sure, will do!

chris-laplante · 2022-06-15T17:49:41Z

The code is a little hard to follow, but what I understand is that orphan_lines are only used to implement println. I'll try to go through and add some comments and/or clean up the code to make it easier to understand.

happenslol · 2022-06-15T19:29:20Z

Cool, feel free to make changes or add suggestions on my PR.

chris-laplante · 2022-06-16T14:04:34Z

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 :)

happenslol · 2022-06-16T15:05:28Z

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

happenslol · 2022-07-02T23:07:47Z

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 :-)

djc · 2022-07-04T10:06:48Z

I'm still wondering if there's a good reason not to have one element per line in DrawState::lines, rather than potentially multiple lines per element due to nested \n. IMO we should deal with that in a different way, by splitting up the message in set_message().

happenslol · 2022-07-09T20:57:42Z

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 DrawState::lines in ProgressStyle::format_state. This means that if we want to already split multiline messages in set_message, we would need to have Vecs for all of the parts of the status, since both prefix and message (and maybe even the bar and template?) might have multiple lines.

This already introduces overhead (Vec instead of String for every part that can contain multiple lines), but also gets very complicated since the last line of each part needs to be joined together with the first line of the next part. Also, having newlines in the template is not even possible like this.

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?

djc · 2022-07-25T09:53:03Z

Hmm, maybe it would be better to split stuff inside ProgressStyle::format_state()?

happenslol · 2022-07-31T00:15:58Z

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.

djc

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.)

djc · 2022-07-31T20:12:52Z

src/style.rs

+
+        // If cur doesn't contain any newlines now, we can add
+        // the line as is.
+        if !expanded.contains('\n') {


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.

djc · 2022-07-31T20:13:41Z

examples/multi-log.rs

@@ -0,0 +1,81 @@
+use std::thread;


I'd prefer not to add a separate example for this.

happenslol · 2022-08-02T20:16:13Z

Alright, all done. Please let me know if there's anything else I can do.

djc

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?

chris-laplante · 2022-08-03T14:53:01Z

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 ProgressBar::set_message. Especially when part of MultiProgress. I want to make sure it doesn't under/over-erase lines.

happenslol · 2022-09-15T19:21:04Z

What's the status on this? Can I do anything in addition to help this land faster?

djc · 2022-09-15T20:30:33Z

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.

happenslol · 2022-09-17T22:07:45Z

Alright, I've added additional tests that render a MultiProgress, set multiline messages and then single line messages again using set_message, increase the progress and then finish the progress using finish_with_message.

Please tell me if these tests cover the cases you were thinking about.

chris-laplante · 2022-09-22T15:44:25Z

Thanks for adding the tests! This looks good to me. @djc alright to merge?

djc · 2022-09-22T19:49:23Z

Great, thanks!

happenslol mentioned this pull request Jun 13, 2022

Support for multi-line progress messages #442

Closed

djc mentioned this pull request Jul 26, 2022

0.17.0 release planning #318

Closed

djc reviewed Jul 31, 2022

View reviewed changes

happenslol force-pushed the improve-multiline-progress-support branch from 02588ff to ff61137 Compare August 2, 2022 20:14

happenslol force-pushed the improve-multiline-progress-support branch from ff61137 to 64126e2 Compare August 2, 2022 20:18

djc approved these changes Aug 2, 2022

View reviewed changes

chris-laplante mentioned this pull request Aug 17, 2022

finish_with_message method does not end message with newline #461

Closed

happenslol added 2 commits September 17, 2022 23:11

Pull line push into separate function

271ebbd

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.

Improve multiline support in format_style

fc29f46

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.

happenslol force-pushed the improve-multiline-progress-support branch from 64126e2 to fc29f46 Compare September 17, 2022 21:11

Add additional tests for multi-progress multiline rendering

cb314ac

happenslol force-pushed the improve-multiline-progress-support branch from bc99b50 to cb314ac Compare September 17, 2022 22:07

djc approved these changes Sep 22, 2022

View reviewed changes

djc merged commit 222df5b into console-rs:main Sep 22, 2022

happenslol deleted the improve-multiline-progress-support branch September 23, 2022 21:18

djc mentioned this pull request Oct 20, 2022

0.7.2 release planning #488

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement multi-line progress message support #443

Implement multi-line progress message support #443

happenslol commented Jun 13, 2022

djc commented Jun 14, 2022

happenslol commented Jun 14, 2022

chris-laplante commented Jun 15, 2022

happenslol commented Jun 15, 2022

chris-laplante commented Jun 16, 2022

happenslol commented Jun 16, 2022

happenslol commented Jul 2, 2022

djc commented Jul 4, 2022

happenslol commented Jul 9, 2022 •

edited

djc commented Jul 25, 2022

happenslol commented Jul 31, 2022

djc left a comment

djc Jul 31, 2022

djc Jul 31, 2022

happenslol commented Aug 2, 2022

djc left a comment

chris-laplante commented Aug 3, 2022

happenslol commented Sep 15, 2022

djc commented Sep 15, 2022

happenslol commented Sep 17, 2022

chris-laplante commented Sep 22, 2022

djc commented Sep 22, 2022

Implement multi-line progress message support #443

Implement multi-line progress message support #443

Conversation

happenslol commented Jun 13, 2022

djc commented Jun 14, 2022

happenslol commented Jun 14, 2022

chris-laplante commented Jun 15, 2022

happenslol commented Jun 15, 2022

chris-laplante commented Jun 16, 2022

happenslol commented Jun 16, 2022

happenslol commented Jul 2, 2022

djc commented Jul 4, 2022

happenslol commented Jul 9, 2022 • edited

djc commented Jul 25, 2022

happenslol commented Jul 31, 2022

djc left a comment

Choose a reason for hiding this comment

djc Jul 31, 2022

Choose a reason for hiding this comment

djc Jul 31, 2022

Choose a reason for hiding this comment

happenslol commented Aug 2, 2022

djc left a comment

Choose a reason for hiding this comment

chris-laplante commented Aug 3, 2022

happenslol commented Sep 15, 2022

djc commented Sep 15, 2022

happenslol commented Sep 17, 2022

chris-laplante commented Sep 22, 2022

djc commented Sep 22, 2022

happenslol commented Jul 9, 2022 •

edited