Skip to content
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

MultiProgressBar now buffers the output text to a single write #1825

Merged
merged 2 commits into from
Nov 8, 2024

Conversation

Giedriusj1
Copy link
Contributor

This is to avoid multiple small writes to the terminal, which can cause flickering.

On my machine (Fedora 41 GNOME) I can see that the new concurrent download status bar flickers quite a bit. I've tested this on gnome-terminal/xfce4-terminal/uterm and kitty. Only Kitty does not flicker. The flickering happens regardless if Wayland/X11 is used.

The old behaviour would issue multiple clear_line writes, followed by multiple writes to "draw" the new status bars.

The new behaviour buffers all of the screen updates for a single "frame" and then writes all of that out with a single syscall.

I've tested the code changes using gnome-terminal/xfce4-terminal/uterm and kitty. Wayland and X11 sessions. It works flawlessly for me now.

I've also tested resizing the terminal window during download, and that works as before (w/o flickering now)

I believe this also fixes #1263

@kontura kontura requested review from jrohel and removed request for jrohel November 5, 2024 11:44
@kontura kontura self-assigned this Nov 5, 2024
This is to avoid multiple small writes to the terminal, which can cause flickering.
@psimonyi
Copy link

psimonyi commented Nov 7, 2024

I was just looking at the same problem and want to suggest also simplifying the command sequences (around line 102).

Currently it repeats the cursor-up-one, clear-line control sequences for as many lines as need to be cleared. But the control sequences can tell the terminal how much to move/clear in a single command. No loop required; just fill in n:

  • ESC [ n A — move cursor up n lines
  • \r — move cursor to column 1
  • ESC [J — clear screen from cursor to the end

@Giedriusj1
Copy link
Contributor Author

@psimonyi thanks for your feedback!

I've implemented your suggestions. In fact, I was able to remove the clear line tty commands altogether!

I figured that it was enough to move the cursor up, and simply let the next progress update write over the previous terminal contents.

I've tested this with multiple terminals while resizing them during download and it all works beautifully 😄

for (std::size_t i = 1; i < mbar.num_of_lines_to_clear; i++) {
text_buffer << tty::cursor_up << tty::clear_line;
if (mbar.num_of_lines_to_clear > 1) {
text_buffer << tty::cursor_up_multiple_lines(mbar.num_of_lines_to_clear - 1);
Copy link
Contributor

Choose a reason for hiding this comment

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

I like the approach, but please don't do it like this via a new public class.

Perhaps just inline it with a comment what the sequences mean.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done! Let me know if you are happy with how it looks now :)

Copy link
Contributor

Choose a reason for hiding this comment

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

It is much better and simpler, thank you.

Instead of sending multiple cursor_up and clear_line sequences,
send a single command to move the cursor up multiple lines.

This makes the tty::clear_line redundant, because the next update
will overwrite the previous lines anyway.
Copy link
Contributor

@kontura kontura left a comment

Choose a reason for hiding this comment

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

Thank you for the contribution!

@kontura kontura added this pull request to the merge queue Nov 8, 2024
Merged via the queue into rpm-software-management:main with commit 8fbbeb2 Nov 8, 2024
20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The download progress of multiple packages is flickering and not smooth
3 participants