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

Fix broken tqdm behavior for Windows and older Unix platforms #4093

Merged
merged 7 commits into from Apr 20, 2023

Conversation

joshuacwnewton
Copy link
Member

@joshuacwnewton joshuacwnewton commented Apr 19, 2023

Description

In many of our calls to sct_progress_bar (a light wrapper for tqdm), we currently force ascii=False, which turns on unicode characters even on terminals that don't support them. This can lead to some buggy-looking output in certain cases:

image

This PR removes the ascii setting from the various calls, then adds alternate default ascii behavior to sct_progress_bar, following the recommendations from #2770 (comment):

normally the default option ascii=None should automatically use the smooth unicode bar if available, and else the ascii bar if not available.

Though, we may want to set ascii=True in the Windows case anyway:

Windows consoles often only partially support unicode and thus often require explicit acii=True

Linked issues

Fixes #2770.

`ascii=False` forces unicode behavior, even on terminals that don't support it.
So, by removing this, we can rely on our newly-added `ascii` settings, which
properly account for terminals that don't fully support unicode.
@joshuacwnewton joshuacwnewton added enhancement category: improves performance/results of an existing feature SCT API: utils.py context: labels Apr 19, 2023
@joshuacwnewton joshuacwnewton added this to the 6.0 milestone Apr 19, 2023
@joshuacwnewton joshuacwnewton self-assigned this Apr 19, 2023
`ascii=False` forces unicode behavior, even on terminals that don't support it.
So, by removing this, we can rely on our newly-added `ascii` settings, which
properly account for terminals that don't fully support unicode.
@joshuacwnewton
Copy link
Member Author

Quick addedum: Multiple callers are setting ascii to either True or False; I've removed each setting so that we can rely on this new platform-specific behavior instead.

`ascii=False` forces unicode behavior, even on terminals that don't support it.
So, by removing this, we can rely on our newly-added `ascii` settings, which
properly account for terminals that don't fully support unicode.
@joshuacwnewton joshuacwnewton force-pushed the jn/2770-fix-tqdm-behavior-on-more-obscure-platforms branch from 9f3c0a4 to 25b0ae2 Compare April 19, 2023 14:14
Copy link
Member

@mguaypaq mguaypaq 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 the fix, and for increasing consistency in the codebase.

@mguaypaq mguaypaq merged commit 3250169 into master Apr 20, 2023
24 checks passed
@mguaypaq mguaypaq deleted the jn/2770-fix-tqdm-behavior-on-more-obscure-platforms branch April 20, 2023 22:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement category: improves performance/results of an existing feature SCT API: utils.py context:
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Weird trailing of tqdm progress bar in some distros
2 participants