Add logic to handle ANSI escape sequences to move cursor

[jsbautista]

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:

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

Preview with changes made:

Preview with IPython:

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

[dalthviz]

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

[TheMatt2]

Preview with changes made in #607:

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:

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]

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:
  

• IPython:
  

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]

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.

[dalthviz]

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)

[dalthviz]

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)

[ccordoba12]

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

[dalthviz]

Thanks @jsbautista for all your work here! This LGTM 👍

Just in case, do you have any further comments @ccordoba12 ? Also, in case this is ready to be merged, should the millestone be set to be 5.7.0 maybe (since this adds new functionality)?

[ccordoba12]

Looks good to me now, thanks @jsbautista and @dalthviz for this great improvement!

[ccordoba12]

Also, in case this is ready to be merged, should the millestone be set to be 5.7.0 maybe (since this adds new functionality)?

I think this is closer to be a bug fix than new functionality because it doesn't add new config options, features or shortcuts. As you can see in our changelog, we release a new minor version when there are additions like those to the package, or when dropping Python versions or changing dependencies.

[ccordoba12]

Also, since this basically fixes spyder-ide/spyder#6172, a PR in Spyder should be created to sync our Qtconsole subrepo there with these changes.