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

Use simpler ‘\r’ instead of cursor manipulation escape sequences #393

Merged
merged 2 commits into from
Dec 8, 2023

Conversation

sergv
Copy link
Contributor

@sergv sergv commented Oct 1, 2023

This primarily benefits displaying progress in Emacs’s shell-mode since it supports \r but doesn’t support ansi escapes other than color manipulation ones.

Checked that it works on Linux and Windows in msys, cygwin and cmd.exe (the last one doesn't display colors correctly though but cursor manipulation is OK).

Copy link
Collaborator

@Bodigrim Bodigrim left a comment

Choose a reason for hiding this comment

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

Thanks, makes sense.

@andreasabel @VictorCMiraldo @martijnbastiaan any Emacs users to test please?

@andreasabel
Copy link
Collaborator

@sergv wrote:

Checked that it works on Linux and Windows in msys, cygwin and cmd.exe (the last one doesn't display colors correctly though but cursor manipulation is OK).

Please detail the procedure how to verify this change. Meaning give the tests in enough detail so I can mechanically reproduce them on my side.

@sergv
Copy link
Contributor Author

sergv commented Oct 22, 2023

@andreasabel

Instructions are as follows:

  1. Clone
$ git clone https://github.com/sergv/tasty-progress-test.git
Cloning into 'tasty-progress-test'...
remote: Enumerating objects: 6, done.
remote: Counting objects: 100% (6/6), done.
remote: Compressing objects: 100% (5/5), done.
remote: Total 6 (delta 0), reused 6 (delta 0), pack-reused 0
Receiving objects: 100% (6/6), done.
$ cd tasty-progress-test/
  1. Validate revision so that you're testing the same thing as I
$ git rev-parse HEAD
5bd507b10b733d68a98da5324e53edd65eebd46a
  1. Run the test from under Emacs's M-x shell (shell-mode) and any other terminal emulator you want to test.

Take special note to observe that progress percentage is shown for currently running test. They're visible for me so you'll probably be able to see them to. If not, please increase the number fo tests.

$ cabal run tasty-test
Cloning into '/c/home/tmp/tasty-progress-test/dist-newstyle/src/tasty-91b4642148b15c74'...
remote: Enumerating objects: 4768, done.
remote: Counting objects: 100% (1226/1226), done.
remote: Compressing objects: 100% (212/212), done.
remote: Total 4768 (delta 1057), reused 1086 (delta 995), pack-reused 3542
Receiving objects: 100% (4768/4768), 912.60 KiB | 1.94 MiB/s, done.
Resolving deltas: 100% (2773/2773), done.
HEAD is now at c1d9588 Use simpler ‘\r’ instead of cursor manipulation escape sequences
Warning: The package list for 'hackage.haskell.org' is 30 days old.
Run 'cabal update' to get the latest list of available packages.
Resolving dependencies...
Build profile: -w ghc-9.6.2 -O1
In order, the following will be built (use -v for more details):
 - tasty-test-0.1 (test:test) (first run)
Configuring test suite 'test' for tasty-test-0.1..
Preprocessing test suite 'test' for tasty-test-0.1..
Building test suite 'test' for tasty-test-0.1..
[1 of 1] Compiling TestMain

test\TestMain.hs:10:1: warning: [-Wunused-imports]
    The import of ‘System.Exit’ is redundant
      except perhaps to import instances from ‘System.Exit’
    To import instances alone, use: import System.Exit()
   |
10 | import System.Exit
   | ^^^^^^^^^^^^^^^^^^

test\TestMain.hs:11:1: warning: [-Wunused-imports]
    The import of ‘System.IO’ is redundant
      except perhaps to import instances from ‘System.IO’
    To import instances alone, use: import System.IO()
   |
11 | import System.IO
   | ^^^^^^^^^^^^^^^^

test\TestMain.hs:24:28: warning: [GHC-40798] [-Woperator-whitespace]
    The tight infix use of a ‘^’ might be repurposed as special syntax
      by a future language extension.
    Suggested fix: Add whitespace around ‘^’.
   |
24 |       \x -> ((x :: Integer)^7 - x) `mod` 7 == 0
   |                            ^

test\TestMain.hs:24:28: warning: [GHC-18042] [-Wtype-defaults]
    • Defaulting the type variable ‘b0’ to type ‘Integer’ in the following constraints
        (Integral b0) arising from a use of ‘^’ at test\TestMain.hs:24:28
        (Num b0) arising from the literal ‘7’ at test\TestMain.hs:24:29
    • In the first argument of ‘(-)’, namely ‘(x :: Integer) ^ 7’
      In the first argument of ‘mod’, namely ‘((x :: Integer) ^ 7 - x)’
      In the first argument of ‘(==)’, namely
        ‘((x :: Integer) ^ 7 - x) `mod` 7’
   |
24 |       \x -> ((x :: Integer)^7 - x) `mod` 7 == 0
   |                            ^
[2 of 2] Linking C:\\home\\tmp\\tasty-progress-test\\dist-newstyle\\build\\x86_64-windows\\ghc-9.6.2\\tasty-test-0.1\\t\\test\\build\\test\\test.exe
QuickCheck
  sort == sort . reverse:  OK (0.95s)
    +++ OK, passed 100000 tests.
  Fermat's little theorem: OK (0.15s)
    +++ OK, passed 100000 tests.

All 2 tests passed (1.10s)

@sergv
Copy link
Contributor Author

sergv commented Nov 5, 2023

@andreasabel Did you get any luck with reproducing the changes?

@sergv
Copy link
Contributor Author

sergv commented Dec 6, 2023

@andreasabel Bump please

sergv added 2 commits December 6, 2023 19:17
This primarily benefits displaying progress in Emacs’s shell-mode
since it supports \r but doesn’t support ansi escapes other than
color manipulation ones.
@andreasabel
Copy link
Collaborator

@sergv Thanks for the testing instructions, and sorry for the delay.

I cloned your PR, but I am failing to run the tests as you describe

$ cabal run tasty-test

I am getting

$ cabal run tasty-test
...
Cannot run the package tasty-test, it is not in this project (either directly or indirectly). If you want to add it to the project then edit the cabal.project file.

@sergv
Copy link
Contributor Author

sergv commented Dec 7, 2023

@andreasabel I've updated https://github.com/sergv/tasty-progress-test.git a bit but since I didn't get your error message I doubt that I fixed it. I'm not sure what went wrong on your side, perhaps your cabal-install exe is too old - mine works with instructions provided in both Linux and Windows.

@Bodigrim To facilitate demonstraction, I've recorded screencasts showing how my test works in vanilla Emacs without user config:

Short version with output:
output2

Longer version showing versions of the tools used if anyone is curious to reproduce:
output1

@andreasabel
Copy link
Collaborator

Apologies, @sergv, I misread the instructions, I thought that tasty-progress-test is just your fork of THIS repo.
So, I can build the tests fine.
On my mac, I cannot observe any difference between present tasty and your modification. I suppose this is good.
In fact, if I run the tasty-progress-test in emacs, neither version displays any percentages.

Here the very boring movie where emacs censors the percentage for 15 seconds:

Screen.Recording.2023-12-08.at.05.58.18.mov

@sergv
Copy link
Contributor Author

sergv commented Dec 8, 2023

@andreasabel When TERM value is dumb the tasty won't print any percentages, which is old behavior. This PR is concerned with making Emacs show something reasonable when TERM value within it's M-x shell is something other than dumb.

@andreasabel
Copy link
Collaborator

Anyone else wants to test, @VictorCMiraldo ?

@Bodigrim
Copy link
Collaborator

Bodigrim commented Dec 8, 2023

I reproduced the instructions following the video and can confirm that it works as advertised. Thanks!

@Bodigrim Bodigrim merged commit b152a0b into UnkindPartition:master Dec 8, 2023
13 checks passed
@sergv
Copy link
Contributor Author

sergv commented Nov 3, 2024

NB b152a0b commit (Simplify further: \r is sufficient to clear whole line, no need for additional escape sequence) was reverted but it was not really crucial to make Emacs work with tasty. But it did introduce undesirable effects on most other terminals.

The problem with not issuing 'clear line' escape sequence is that regular terminals upon seeing \r will move the cursor to 0th column and further output will overwrite line's contents. But if new contents is shorter then old trailing garbage will remain, Emacs clears it up by \r but most other applications don't hence the need for the escape sequence.

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.

3 participants