Fix animation of long lines #247

itsayellow · 2019-10-21T04:42:20Z

This fixes a problem with the text animation during install. With a long --spec, the animation during install of a pipx package can be corrupted, resulting in a page or more of spurious identical messages being printed. This is because the \r carriage return does not work if the printed text is so long that it moves the cursor to the next line. Under these circumstances, \r only moves to the beginning of this terminal line, leaving the previous part of the printed text still printed above it.

The patch truncates the animated printed text so it does not run over to the next line of the terminal. It uses shutil.get_terminal_size(). The fallback in case shutil.get_terminal_size() cannot determine the terminal size is 9999 columns, essentially "infinity", making sure that we revert to the original behavior if we cannot determine the actual terminal size (or we are in some circumstance like a pipe, etc. where it wouldn't matter anyway.)

Current behavior:

birchtree:pipx$ pipx install --spec git+https://github.com/itsayellow/pytivometa pytivometa
⣷ installing package 'git+https://github.com/itsayellow/pytivometa#egg=pytivomet⣯ installing package 'git+https://github.com/itsayellow/pytivometa#egg=pytivomet⣟ installing package 'git+https://github.com/itsayellow/pytivometa#egg=pytivomet⡿ installing package 'git+https://github.com/itsayellow/pytivometa#egg=pytivomet⢿ installing package 'git+https://github.com/itsayellow/pytivometa#egg=pytivomet⣻ installing package 'git+https://github.com/itsayellow/pytivometa#egg=pytivomet⣽ installing package 'git+https://github.com/itsayellow/pytivometa#egg=pytivomet⣾ installing package 'git+https://github.com/itsayellow/pytivometa#egg=pytivomet⣷ installing package 'git+https://github.com/itsayellow/pytivometa#egg=pytivomet⣯ installing package 'git+https://github.com/itsayellow/pytivometa#egg=pytivomet⣟ installing package 'git+https://github.com/itsayellow/pytivometa#egg=pytivomet⡿ installing package 'git+https://github.com/itsayellow/pytivometa#egg=pytivomet⢿ installing package 'git+https://github.com/itsayellow/pytivometa#egg=pytivomet⣻ installing package 'git+https://github.com/itsayellow/pytivometa#egg=pytivomet⣽ installing package 'git+https://github.com/itsayellow/pytivometa#egg=pytivomet⣾ installing package 'git+https://github.com/itsayellow/pytivometa#egg=pytivomet⣷ installing package 'git+https://github.com/itsayellow/pytivometa#egg=pytivomet⣯ installing package 'git+https://github.com/itsayellow/pytivometa#egg=pytivomet⣟ installing package 'git+https://github.com/itsayellow/pytivometa#egg=pytivomet⡿ installing package 'git+https://github.com/itsayellow/pytivometa#egg=pytivomet⢿ installing package 'git+https://github.com/itsayellow/pytivometa#egg=pytivomet⣻ installing package 'git+https://github.com/itsayellow/pytivometa#egg=pytivomet⣽ installing package 'git+https://github.com/itsayellow/pytivometa#egg=pytivomet⣾ installing package 'git+https://github.com/itsayellow/pytivometa#egg=pytivomet⣷ installing package 'git+https://github.com/itsayellow/pytivometa#egg=pytivomet⣯ installing package 'git+https://github.com/itsayellow/pytivometa#egg=pytivomet⣟ installing package 'git+https://github.com/itsayellow/pytivometa#egg=pytivomet⡿ installing package 'git+https://github.com/itsayellow/pytivometa#egg=pytivomet⢿ installing package 'git+https://github.com/itsayellow/pytivometa#egg=pytivomet⣻ installing package 'git+https://github.com/itsayellow/pytivometa#egg=pytivomet⣽ installing package 'git+https://github.com/itsayellow/pytivometa#egg=pytivomet⣾ installing package 'git+https://github.com/itsayellow/pytivometa#egg=pytivomet⣷ installing package 'git+https://github.com/itsayellow/pytivometa#egg=pytivomet⣯ installing package 'git+https://github.com/itsayellow/pytivometa#egg=pytivomet⣟ installing package 'git+https://github.com/itsayellow/pytivometa#egg=pytivomet⡿ installing package 'git+https://github.com/itsayellow/pytivometa#egg=pytivomet  installed package pytivometa 0.2.0, Python 3.7.4
  These apps are now globally available
    - pytivometa
done! ✨ 🌟 ✨
birchtree:pipx$

(Note, in my Terminal this wraps so it looks like many lines.)

With this pull request:

During:

birchtree:pipx$ pipx install --spec git+https://github.com/itsayellow/pytivometa pytivometa
⣽ installing package 'git+https://github.com/itsayellow/pytivometa#egg=pytiv...

Afterwards:

birchtree:pipx$ pipx install --spec git+https://github.com/itsayellow/pytivometa pytivometa
  installed package pytivometa 0.2.0, Python 3.7.4
  These apps are now globally available
    - pytivometa
done! ✨ 🌟 ✨
birchtree:pipx$

cs01 · 2019-10-21T04:47:15Z

Nice!

Should the size be retrieved more frequently since there is no guarantee it will remain the same during the entire session?

itsayellow · 2019-10-21T06:19:10Z

Should the size be retrieved more frequently since there is no guarantee it will remain the same during the entire session?

I'm trying to think of the ways that might happen--would it only happen if the user resized the terminal while a pipx command was running?

I wasn't worrying about it, but get_terminal_size could easily be put inside of print_animation() or even more real-time, inside of the for s in symbols: loop. I don't know if there's any cost to this, but I'm guessing not much.

itsayellow · 2019-10-21T06:21:24Z

I think something like reinstall-all is probably the longest-running command, and that takes long enough a user could easily resize the terminal, which would screw things up as it stands now.

itsayellow · 2019-10-21T06:27:43Z

OK, I moved get_terminal_size() inside of animate() which seemed pretty obviously a good thing.

Do you think it needs to be in the for s in symbols: loop?

pipx/animate.py

gaborbernat · 2019-10-21T12:18:19Z

pipx/animate.py

-                cur_line = f"{message}{s}"
+                if len(message) <= term_cols - 4:
+                    cur_line = f"{message}{s}"
+                else:


Also I think for this to not suffer easily regressions going ahead we should add some tests that validate the change.

Agree, but I'm frankly not sure how to do that with animated (transient) text. Anybody have a pointer or ideas?

when the pytest capsys is attached the override does not happen, allowing you to check byte-wise; see https://github.com/tox-dev/tox/blob/master/tests/unit/util/test_spinner.py#L60-L81 for example

Nice. And the test above it is testing with animation.

Another tidbit I just figured out, is that by setting env variables COLUMNS and ROWS you can make shutil.get_terminal_size() return values at your command.

pipx/animate.py

cs01 · 2019-10-27T18:16:56Z

tests/test_animate.py

+HIDE_CURSOR = "\033[?25l"
+SHOW_CURSOR = "\033[?25h"
+CLEAR_LINE = "\033[K"


These should be defined in animate.py and imported here

cs01 · 2019-10-27T18:18:19Z

tests/test_animate.py

+
+        frame_strings = [
+            f"{CLEAR_LINE}\r{x} {expected_message[i]}"
+            for x in ["⣷", "⣯", "⣟", "⡿", "⢿", "⣻", "⣽", "⣾"]


the animation list should be imported from animate.py

cs01 · 2019-10-27T18:24:59Z

tests/test_animate.py

+    pipx.animate.stderr_is_tty = True
+    pipx.animate.emoji_support = True


Recommend defining a fixture to ensure pipx.animate.stderr_is_tty, pipx.animate.emoji_support values are stored at entry to the fixture, then yield to let the test run, then reset the values to their orignal value in the fixture. We should not assume what they were at the time they were computed on first pass.

https://docs.pytest.org/en/latest/fixture.html#fixture-finalization-executing-teardown-code

Thanks, for the hint.
Done.

Used @gaborbernat 's tip about monkeypatch instead, but if things get more complicated in the future I am happy to know how to do teardown.

cs01 · 2019-10-27T18:26:07Z

tests/test_animate.py

+    frames_to_test = 4
+    # matches animate.py
+    frame_period = 0.1


Recommend making these values importable from animate.py and using them directly

cs01 · 2019-10-27T18:29:13Z

tests/test_animate.py

@@ -0,0 +1,93 @@
+#!/usr/bin/env python3
+import time


This is great! Thanks for adding these tests.

cs01 · 2019-10-27T18:29:42Z

tests/test_animate.py

@@ -0,0 +1,93 @@
+#!/usr/bin/env python3


This is probably a copy+paste thing, but in general pipx has a bunch of these at the top of files that should be removed. It can be removed here.

Ack, yes. Force of habit!
Done.

…ate.py.

gaborbernat · 2019-10-27T23:15:15Z

tests/test_animate.py

+    captured = capsys.readouterr()
+
+    # print for debug help if fail
+    print("expected_string:")


adding '-rvva --showlocals`` achieves the same, not?

Thanks, didn't know about that.

gaborbernat · 2019-10-27T23:15:55Z

tests/test_animate.py

+def test_line_lengths_no_emoji(capsys, monkeypatch):
+    # emoji_support and stderr_is_tty is set only at import animate.py
+    # since we are already after that, we must override both here
+    pipx.animate.stderr_is_tty = True


use monkeypatch instead of direct set to ensure their correctly reset in case tests fails for the next test

Thanks for the tip!

cs01 · 2019-10-27T23:41:14Z

pipx/animate.py

+EMOJI_ANIMATION_FRAMES = ["⣷", "⣯", "⣟", "⡿", "⢿", "⣻", "⣽", "⣾"]
+NONEMOJI_ANIMATION_FRAMES = ["", ".", "..", "..."]


…animate_long_lines

itsayellow · 2019-10-29T05:47:19Z

Am I clear to merge this now? I think I've addressed all the comments.

cs01 · 2019-10-29T18:33:05Z

I believe this is the first PR that I have not approved+merged! Thank you and congrats! 🎉

In the future please squash commits to avoid polluting commits on the master branch.

itsayellow · 2019-10-29T21:46:14Z

In the future please squash commits to avoid polluting commits on the master branch.

Ah ok! Is the idea one commit per PR?

gaborbernat · 2019-10-29T22:40:31Z

I think the idea is more one commit per feature/bug fix.

cs01 · 2019-10-29T23:04:07Z

The idea is as many commits on a PR as you want, but when you click the "merge" button, there are a couple options.

One is "merge and squash commits into a single commit" and the other is "merge all commits" (the exact text is not this, but this is the idea). We should always opt for the "squash" option.

If you look at the commit history, you generally see a single commit with the pull request number in parentheses, such as

Add printed summary after successful injection. (#226)

this is what we want, so you can look through commits on master and get a basic idea of all the PRs that went into the codebase.

If you don't mind, I will do some work with git and do a force push so the master branch looks like this so there is a single commit for this PR.

itsayellow · 2019-10-29T23:44:47Z

I don't mind at all, thanks for offering to clean it up. Thanks for the tip.

itsayellow added 4 commits October 20, 2019 11:26

Truncate long lines with animation, to prevent multi-line.

a26b2bb

Remove unused TERM_ROWS.

0fefbf2

Tailor truncation depending on animate_at_beginning_of_line.

4bcce89

Make if test correspond to string format precision.

6bfe12c

itsayellow changed the title ~~Animate long lines~~ Fix animation of long lines Oct 21, 2019

Move get_terminal_size() inside of animate().

c7ef9d8

Fix bug from last commit.

4f31d46

gaborbernat reviewed Oct 21, 2019

View reviewed changes

Make max message length method a bit clearer.

58cbefb

gaborbernat reviewed Oct 22, 2019

View reviewed changes

pipx/animate.py Outdated Show resolved Hide resolved

itsayellow added 2 commits October 24, 2019 14:48

Add tests for message truncation.

9fdf83f

Deleted unused import sys.

aec4c98

cs01 reviewed Oct 27, 2019

View reviewed changes

itsayellow added 4 commits October 27, 2019 15:44

Add HIDE_CURSOR, SHOW_CURSOR, CLEAR_LINE global strings to animate.py.

9e1eb97

Add EMOJI_ANIMATION_FRAMES, NONEMOJI_ANIMATION_FRAMES globals to anim…

29a058a

…ate.py.

Add EMOJI_ANIMATION_PERIOD, NONEMOJI_ANIMATION_PERIOD globals to anim…

f3aa77a

…ate.py.

Remove python shebang.

c9be0a7

gaborbernat reviewed Oct 27, 2019

View reviewed changes

cs01 reviewed Oct 27, 2019

View reviewed changes

Implement teardown to re-do original terminal state.

73b861f

itsayellow added 5 commits October 27, 2019 23:08

Use monkeypatch instead of terminal_state fixture.

49228e5

Refactor and remove unused import pytest.

9f60dc5

Increase sleep time to ensure all frames print.

443b621

Merge remote-tracking branch 'upstream/master' into animate_long_lines

f60e310

Merge branch 'animate_long_lines' of github.com:itsayellow/pipx into …

8f89f97

…animate_long_lines

gaborbernat approved these changes Oct 29, 2019

View reviewed changes

itsayellow merged commit 33813d1 into pypa:master Oct 29, 2019

itsayellow deleted the animate_long_lines branch October 29, 2019 17:00

cs01 pushed a commit that referenced this pull request Nov 2, 2019

Merge pull request #247 from itsayellow/animate_long_line

4daf652

cs01 pushed a commit that referenced this pull request Nov 2, 2019

Truncate long lines with animation, to prevent multi-line (#247)

18eb430

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix animation of long lines #247

Fix animation of long lines #247

itsayellow commented Oct 21, 2019

cs01 commented Oct 21, 2019

itsayellow commented Oct 21, 2019

itsayellow commented Oct 21, 2019

itsayellow commented Oct 21, 2019

gaborbernat Oct 21, 2019

itsayellow Oct 22, 2019

gaborbernat Oct 22, 2019

itsayellow Oct 22, 2019

itsayellow Oct 24, 2019

cs01 Oct 27, 2019

itsayellow Oct 27, 2019

cs01 Oct 27, 2019 •

edited

Loading

itsayellow Oct 27, 2019

cs01 Oct 27, 2019 •

edited

Loading

itsayellow Oct 28, 2019

itsayellow Oct 28, 2019 •

edited

Loading

cs01 Oct 27, 2019

itsayellow Oct 27, 2019

cs01 Oct 27, 2019

cs01 Oct 27, 2019

itsayellow Oct 27, 2019

gaborbernat Oct 27, 2019

itsayellow Oct 28, 2019

gaborbernat Oct 27, 2019

itsayellow Oct 28, 2019

cs01 Oct 27, 2019

itsayellow commented Oct 29, 2019

cs01 commented Oct 29, 2019

itsayellow commented Oct 29, 2019

gaborbernat commented Oct 29, 2019

cs01 commented Oct 29, 2019 •

edited

Loading

itsayellow commented Oct 29, 2019

		pipx.animate.stderr_is_tty = True
		pipx.animate.emoji_support = True

		EMOJI_ANIMATION_FRAMES = ["⣷", "⣯", "⣟", "⡿", "⢿", "⣻", "⣽", "⣾"]
		NONEMOJI_ANIMATION_FRAMES = ["", ".", "..", "..."]

Fix animation of long lines #247

Fix animation of long lines #247

Conversation

itsayellow commented Oct 21, 2019

cs01 commented Oct 21, 2019

itsayellow commented Oct 21, 2019

itsayellow commented Oct 21, 2019

itsayellow commented Oct 21, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cs01 Oct 27, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cs01 Oct 27, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

itsayellow Oct 28, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

itsayellow commented Oct 29, 2019

cs01 commented Oct 29, 2019

itsayellow commented Oct 29, 2019

gaborbernat commented Oct 29, 2019

cs01 commented Oct 29, 2019 • edited Loading

itsayellow commented Oct 29, 2019

cs01 Oct 27, 2019 •

edited

Loading

cs01 Oct 27, 2019 •

edited

Loading

itsayellow Oct 28, 2019 •

edited

Loading

cs01 commented Oct 29, 2019 •

edited

Loading