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

Display Download Progress of Individual Tracks #57

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

vtdiep
Copy link

@vtdiep vtdiep commented May 11, 2021

As stated and shown below:
bdp

Partially addresses #56
It can be reassuring (or frustrating) to see progress when dealing with large file sizes and/or slow connections.

@obskyr
Copy link
Owner

obskyr commented May 11, 2021

Very nice! Could you make it so that this is only activated if sys.stdout.isatty()? I'd like these \r prints not to appear if you're redirecting output to, for example, a file.

Download progress will not be included when output is redirected/piped.
@vtdiep
Copy link
Author

vtdiep commented May 12, 2021

Ok, just pushed!
Regular Output:
image
Redirected Output:
image

Notes (for Windows users):
Interestingly, I ran into an issue with isatty() where it wouldn't work properly unless the command was prefaced with winpty. Apparently this is one of the known issues of Git-for-Windows(MinTTY).
If using Git-for-Windows, enabling the support for Windows-native pseudo consoles will also solve the issue without needing winpty.

@opusforlife2
Copy link

This looks great! Thank you!

Would it be possible to remove each progress indicator once it reaches 100%? It's not needed at that point, after all, and is just bloating the list, doubling its length.

@vtdiep
Copy link
Author

vtdiep commented May 20, 2021

It is possible to remove the progress once it reaches 100% and I think it would be ideal to do so, but I've run into an issue of compatibility across environments.

The simplest method would leave the last progress message on the screen, which could be overwritten with whitespace (or other text). Potential side effects would be an extra line of whitespace at the bottom of terminal output. There is also the possibility that any print statement after the last progress message will have some extra whitespace added to it.
This approach is simple, but the code would have to be divided between different parts, which goes against separation of concerns. It also has undesirable side effects, although that would be extra trailing whitespace and certainly something that could be lived with.

Then, the next solution might be to use ASCII control codes, and this works well, except they don't work properly out of the box when using Windows cmd. So, if using ASCII control codes, we could

  • not support native Windows cmd [not ideal, but it is an option]
  • use a cursor library [maybe overkill for such a small feature and adds a dependency]
  • enable ascii codes on Windows cmd by checking the OS and running the appropriate commands

I like the cleanliness of the last option. It's just a few lines to enable, but it does introduce OS-specific code. Eg

if os.name == 'nt': # Only if we are running on Windows
    from ctypes import windll
    k = windll.kernel32
    k.SetConsoleMode(k.GetStdHandle(-11), 7)

@obskyr Thoughts?

@obskyr
Copy link
Owner

obskyr commented May 20, 2021

@vtdiep I'm thinking I eventually want to redo this – unless you'd like to – to put the progress indicator on the same line as the "Downloading [x]..." line – that way, \r can quite simply be used, and you don't get cluttered output. Digging into OS-specific APIs is a bit fraught.

@opusforlife2
Copy link

Are you thinking "Downloading x/y (z%): track-name" ?

@vtdiep
Copy link
Author

vtdiep commented May 22, 2021

@obskyr I don't mind working on putting the progress indicator on the same line; its great to be able to contribute! I was wondering though, on how you would approach it. For lack of a better word, I've been trying to keep changes minimally invasive, but if you're open to slightly bigger changes, then the more options there are.

I was tinkering and maybe yielding the progress to the calling function so that downloading X and progress could be printed on the same line could work. I don't know if you like the idea of yielding the progress though, lol
I've just updated the PR branch.

Anyways, let me know what you think. If you think this approach is a good base to work off, then yeah, I'd be happy to continue to work on it. Or if you want to go in a different direction or maybe take care of it with any other changes you're planning, that's good as well.

Currently, the format is "Downloading x/y: track-name (z%)" but switching the ordering and placement would be a quick change.

@opusforlife2
Copy link

@obskyr ^

@opusforlife2
Copy link

Friendly ping, @obskyr.

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.

None yet

3 participants