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

Add logic to handle ANSI escape sequences to move cursor #616

Open
wants to merge 33 commits into
base: main
Choose a base branch
from

Conversation

jsbautista
Copy link
Contributor

@jsbautista jsbautista commented Jul 30, 2024

I made changes to the way the ANSI code \n is handled. Previously, whenever the \n code appeared, a new line was inserted unconditionally. After the change, a new line is inserted unless there is already a written line at the current position. In that case, instead of inserting a new line, the cursor moves one line down.

Preview with changes made:
image

Comparison of the result obtained with the changes made and in Ipython with QTDM

Preview with changes made:

PreviewTQDMQtConsole

Preview with IPython:

PreviewTQDMIpython

Fixes: #350
Related with: spyder-ide/spyder-kernels#494 spyder-ide/spyder#6172

@dalthviz dalthviz added this to the 5.6.0 milestone Jul 31, 2024
@dalthviz
Copy link
Collaborator

Thanks for your work here @jsbautista ! As quick comment, adding a test for this new move cursor action logic could be worthy

@dalthviz dalthviz changed the title [WIP] PR: Fix Tqdm qt console [WIP] PR: Add logic to handle ANSI escape sequence to move cursor up Jul 31, 2024
@dalthviz dalthviz changed the title [WIP] PR: Add logic to handle ANSI escape sequence to move cursor up [WIP] PR: Add logic to handle ANSI escape sequences to move cursor up Jul 31, 2024
@dalthviz dalthviz changed the title [WIP] PR: Add logic to handle ANSI escape sequences to move cursor up [WIP] Add logic to handle ANSI escape sequences to move cursor up Aug 8, 2024
@dalthviz dalthviz changed the title [WIP] Add logic to handle ANSI escape sequences to move cursor up [WIP] Add logic to handle ANSI escape sequences to move cursor Aug 15, 2024
@dalthviz dalthviz removed this from the 5.6.0 milestone Aug 15, 2024
@TheMatt2
Copy link
Contributor

TheMatt2 commented Sep 5, 2024

Preview with changes made in #607:

image

I appreciate the goal to add these ANSI codes to allow the cursor to move between lines. This limitation seems to be the main thing standing between qtconsole being a "true terminal emulator", that is compliant'ish with other terminals.

Though, I see some issues with the changes in this MR.

Your GIF seems to show output that is different that what the intended behavior should be. Here is what your prueba() prints in VS Code and MacOS's terminal:

image

image

I suggest the goal should be to get QtConsole to give output that matches these screenshots. "Segunda prueba" should be printed on the second line, as this string comes before the "\x1b[A" control character. Not "Segunda prueba" on the last line, as shown in your GIF.

It also looks like this PR breaks qtconsole/tests/test_00_console_widget.py::TestConsoleWidget::test_print_carriage_return

@dalthviz
Copy link
Collaborator

dalthviz commented Sep 6, 2024

Hi @TheMatt2 thank you for the feedback and giving this work a check! I think the current GIF being shown is a little bit out of date. The main goal of this PR is to give a handling similar to ANSI codes related with cursor movement as you would observe over IPython (but we could indeed give a check to the behavior of other terminals/platforms to contrast possible differences in behavior). So running the prueba code with the latest PR commit on Windows you would get the same output as if you run it with IPython:

  • Code:
def prueba():
    print("\rPrimera prueba", end="")
    print("\n", end="")
    print("\rSegunda prueba", end="")
    print("\x1b[A", end="")
    print("\n\n", end="")
    print("\rReemplazo", end="")
    print()
  • This PR:
    image

  • IPython:
    image

Still, we have some edge cases that need handling and also, as you mention, the changes here are causing some tests as the carriage return one to fail but we are giving all that a check.

Hopefully this makes current state of this PR/work clearer but let us know if you have further suggestions/comments/ideas!

@TheMatt2
Copy link
Contributor

TheMatt2 commented Sep 7, 2024

I think the current GIF being shown is a little bit out of date. The main goal of this PR is to give a handling similar to ANSI codes related with cursor movement as you would observe over IPython (but we could indeed give a check to the behavior of other terminals/platforms to contrast possible differences in behavior). ...

Thank you for your response and clarification. Now I'm with you.

jsbautista and others added 3 commits September 8, 2024 23:01
Co-authored-by: Daniel Althviz Moré <16781833+dalthviz@users.noreply.github.com>
@dalthviz dalthviz changed the title [WIP] Add logic to handle ANSI escape sequences to move cursor Add logic to handle ANSI escape sequences to move cursor Sep 16, 2024
@dalthviz dalthviz added this to the 5.6.1 milestone Sep 16, 2024
@dalthviz
Copy link
Collaborator

Checking aroung some issue related with Rich, using the changes here alone are not enough to see the improvements done. For more details you can check spyder-ide/spyder-kernels#494 (comment) (where using the Rich Console API is needed - force_jupyter=False)

Copy link
Collaborator

@dalthviz dalthviz left a comment

Choose a reason for hiding this comment

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

Thanks @jsbautista for your work here! Left a question and a suggestion. Other than that seems like there is not much else to do here, although maybe could be worthy to do some extra manually testing with things like Rich and TQDM (I already did some while checking spyder-ide/spyder-kernels#494)

qtconsole/tests/test_ansi_code_processor.py Outdated Show resolved Hide resolved
qtconsole/ansi_code_processor.py Show resolved Hide resolved
jsbautista and others added 2 commits September 24, 2024 14:03
Co-authored-by: Daniel Althviz Moré <16781833+dalthviz@users.noreply.github.com>
Copy link
Collaborator

@ccordoba12 ccordoba12 left a comment

Choose a reason for hiding this comment

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

Thanks @jsbautista for your work on this! Just some small style suggestions and a comment for you.

qtconsole/ansi_code_processor.py Outdated Show resolved Hide resolved
qtconsole/ansi_code_processor.py Outdated Show resolved Hide resolved
qtconsole/ansi_code_processor.py Outdated Show resolved Hide resolved
qtconsole/console_widget.py Outdated Show resolved Hide resolved
qtconsole/console_widget.py Outdated Show resolved Hide resolved
qtconsole/console_widget.py Outdated Show resolved Hide resolved
@@ -182,6 +182,42 @@ def test_combined(self):
self.assertEqual(splits, ['abc', None, 'def', None])
self.assertEqual(actions, [[], ['carriage-return'], [], ['backspace']])

def test_move_cursor_up(self):
string = '\x1b[A\x1b[5A\x1b[F\x1b[5F'
Copy link
Collaborator

@ccordoba12 ccordoba12 Oct 9, 2024

Choose a reason for hiding this comment

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

Please add a comment above this line to explain what it does.

@dalthviz dalthviz self-requested a review October 17, 2024 19:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ANSI escape sequences other than color
4 participants