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

ui: very short progress bar #2487

Closed
Suor opened this issue Sep 11, 2019 · 13 comments · Fixed by #2857
Closed

ui: very short progress bar #2487

Suor opened this issue Sep 11, 2019 · 13 comments · Fixed by #2857
Assignees
Labels
bug Did we break something? ui user interface / interaction

Comments

@Suor
Copy link
Contributor

Suor commented Sep 11, 2019

On master branch.

Steps to reproduce:

  • create a dvc repo
  • create a directory of 150 moderately large files, say 10mb
  • dvc add them

The progress bar looks like 10 chars in width.

@efiop
Copy link
Member

efiop commented Sep 12, 2019

@Suor something like a screenshot would be helpful to assess the severity. 🙂

@pared
Copy link
Contributor

pared commented Sep 12, 2019

I had the same issue with my latest test, and its not only add.

https://asciinema.org/a/267948

@shcheklein shcheklein added the ui user interface / interaction label Sep 12, 2019
@shcheklein shcheklein added the bug Did we break something? label Sep 12, 2019
@casperdcl
Copy link
Contributor

yes this was a design introduced in #2436, specifically https://github.com/iterative/dvc/pull/2436/files#diff-b4d487abdc2dca03a19e8e4a7bceccf2R39

This was included due to the ugly looking misalignment of progress bars on top of each other (i.e. one bar with width 20 followed by another width 10 don't align nicely visually).

@efiop perhaps we don't need this any more since the bars are being cleared away so we never see them on top of each other any more?

@efiop
Copy link
Member

efiop commented Sep 13, 2019

Thank you @pared 🙏

@casperdcl We might bring some of those back, though, right? Maybe we'll decide to simply increase the percentage of the bar length later. In any case, it is functional right now and doesn't look bad, so I suggest we don't jump the gun again right now and just deal with it during the upcoming UI reevaluation for this command, when we'll have a bigger picture.

@Suor
Copy link
Contributor Author

Suor commented Sep 13, 2019

I would say it urgent, 10 char progress bar:

  • is ugly
  • has limited functionality (poorly shows progress)

So I would say either revert to misaligned ones or put text on top of the bar body and invert text/background color when it is filled.

@efiop
Copy link
Member

efiop commented Sep 19, 2019

@Suor

is ugly

Less ugly then it was originally without tqdm. I dont think this one counts as urgent.

has limited functionality (poorly shows progress)

Not really, since it has the right part to it where you get more granular N/M thingy. Again, not urgent.

So I would say either revert to misaligned ones or put text on top of the bar body and invert text/background color when it is filled.

Reverting would be problematic for this one, I would rather not do that. What do you mean by inverting? Not sure I follow that one.

@casperdcl
Copy link
Contributor

I'm assuming inverting means black text on white background where necessary (to have description drawing on top of the bar).

Not sure if there's a reliable cross-platform way to display such a thing. Also wouldn't work well on non-unicode envs which will currently display hashes for the bar.

@shcheklein
Copy link
Member

Is there a specific reason to keep so small? Can we do a quick fix to make it 3x wider?

I agree with Ruslan, the whole output might change soon, so I would not put too much effort into fixing this, just a cosmetic minor quick stuff if it's possible.

@casperdcl
Copy link
Contributor

the problem with fixing to say width 30 is increased chance of overflow (on narrow terminals or long descriptions)

@shcheklein
Copy link
Member

@casperdcl yeah, totally, I was just assuming probably that we can calculate in advance or tqdm supports this - and specify something like this min(30, number of symbols to the description)

@Suor
Copy link
Contributor Author

Suor commented Sep 24, 2019

I am against fixed pbars, these look bad.

@Suor
Copy link
Contributor Author

Suor commented Sep 24, 2019

BTW, the reason behind fixed with bars is several of those dancing when we perform some work in several threads. So if we unite those into one pbar there would no need to bother about fixed size pbars at all.

And now we shouldn't used fixed size pbar outside multithreaded situation.

@Suor
Copy link
Contributor Author

Suor commented Nov 26, 2019

This shows how tqdm looks by default and in dvc: https://asciinema.org/a/283771
It looks pathetic in dvc. May we fix it?

casperdcl added a commit to casperdcl/dvc that referenced this issue Nov 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Did we break something? ui user interface / interaction
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants