Skip to content

Terminal tab progress reporting issue #13072 #13395

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

annatasio
Copy link

No description provided.

@psf-chronographer psf-chronographer bot added the bot:chronographer:provided (automation) changelog entry is part of PR label Apr 25, 2025
Copy link
Member

@RonnyPfannschmidt RonnyPfannschmidt left a comment

Choose a reason for hiding this comment

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

Thanks for starting this.

This feature needs a test and we may need to validate if the terminal supports the codes

@annatasio
Copy link
Author

Thanks for starting this.

This feature needs a test and we may need to validate if the terminal supports the codes

Thanks for your immidiate response! I will check these soon.

Copy link
Member

@nicoddemus nicoddemus left a comment

Choose a reason for hiding this comment

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

This is great!

Left some minor styling suggestions, but as mentioned by @RonnyPfannschmidt , we need some tests for this.

Comment on lines +634 to +637
total_items = 0
if hasattr(self, "_session") and self._session is not None:
if hasattr(self._session, "items"):
total_items = len(self._session.items)
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we need the hasattr checks given _session is set during __init__ (same for Session.items:

Suggested change
total_items = 0
if hasattr(self, "_session") and self._session is not None:
if hasattr(self._session, "items"):
total_items = len(self._session.items)
total_items = len(self._session.items) if self._session is not None else 0

# 1 = set progress value, followed by percentage (0-100)
progress_sequence = f"\033]9;4;1;{progress}\033\\"
# Only write the sequence if we're in a terminal
if hasattr(self._tw, "_file") and self._tw._file.isatty():
Copy link
Member

@nicoddemus nicoddemus Apr 26, 2025

Choose a reason for hiding this comment

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

I think this if should be done together with if self._show_progress_info, given if this is False we don't need to compute the progress at all. Also we can drop hasattr, as TerminalWriter is always set to a TextIO.

@bluetech
Copy link
Member

Hopefully I can review this some time this week. But I did try this PR out and it seems to work as expected (well, except that the progress stuck around for me after ctrl-c). Here's a video using the Ptyxis terminal:

Screencast.From.2025-04-27.00-59-24.mp4

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bot:chronographer:provided (automation) changelog entry is part of PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants