Fix Carriage Return Handling in QtConsole

[TheMatt2]

Fix the handling of carriage return so it is not ignored at the end of a print line.

This allows for progress bars that rely on adding a carriage return at the end of the line such as:

print("Progress 55%", end = "\r")

tl;dr
Preserve position of cursor so ANSI commands such as \r that modify position will preserve the position they specified over multiple calls. This is particularly relevant if time passes between print calls (~100 ms).

Explanation

First of all, when you print() or call sys.stdout.write() in QtConsole, this ultimately causes the text to be passed to the frontend widget.

qtconsole/qtconsole/frontend_widget.py

Line 729 in 56e5a5e

def append_stream(self, text):

This will lead to the text by passed to the console widget, which will call the ANSI Processor in:

qtconsole/qtconsole/console_widget.py

Line 2085 in 56e5a5e

def _insert_plain_text(self, cursor, text, flush=False):

The ANSI preprocessor already handles the carriage return, so code that that uses the carriage return in the middle of a print already worked.

print("Hello\rWorld")
World

This will even work for calls from multiple prints, but this only works because the text is internally buffered, and recombined so print("Hello", end = "\r"); print("World") is all combined into the text "Hello\rWorld" by the time _insert_plain_text()` is called.

However, _insert_plain_text() and _flush_pending_stream() both force the cursor position to the end of the line at the end of the call. This means that a '\r' at the end of a line is effectively ignored, if something stops the text from being combined back together, such as more than ~100 ms passing between prints.

This PR fixes this issue by updating the cursor position to the place it was put to by _insert_plain_text().

Before this gets merged, I do want to make sure there are no odd situations where the cursor now not being the end of text leads to issues.

Behavior Change

Before

After

Notes

Related Github Issues

• Strange print using \r  #272
• Carriage Return Inconsistency #553
• Backspace (\b) and carriage return (\r) characters are not printed correctly in the console spyder-ide/spyder#195
• \r doesn't overwrite print() spyder-ide/spyder#20388

This does not exactly address #350, not adding support for any new ANSI codes, just fixing the existing support for '\r'

[TheMatt2]

Added regression test. Needed to run it manually, as the test_scroll() keeps failing (it worked once though)

[ccordoba12]

Hey @TheMatt2, thanks for your contribution! Unfortunately, it doesn't seem the right approach if it breaks scrolling.

My suggestion is for you to rethink your changes so that test_scroll passes consistently.

[TheMatt2]

Hey @TheMatt2, thanks for your contribution! Unfortunately, it doesn't seem the right approach if it breaks scrolling.

My suggestion is for you to rethink your changes so that test_scroll passes consistently.

Let me rephrase. test_scroll() is breaking for me, without my changes. I don't think my changes are impacting scroll, that test is failing on its own.

Working on it, to see if I can figure it out, and go through the CI results.

[ccordoba12]

Ok, sorry for the misunderstanding. Since I haven't seen that test failing before, I assumed it was do to your changes.

[TheMatt2]

So it seems the following error the CI reports is introduced by the PR, I'm looking into what the implications are.

FAILED qtconsole/tests/test_00_console_widget.py::test_scroll[True] - assert 1930 == 1170

The error I was referring to previously, is in my testing test_00_console_widget.py::test_scroll() hangs forever on MacOS, unrelated to the changes in this PR.

And on Windows, test_00_console_widget.py::test_scroll() is failing for me with an unrelated exception.

And it appears these failures were masking that there is an issue in this PR that needs to be addressed.
Suppose I may need to open issues for there others, but one thing at a time.

[TheMatt2]

Updated approach to not break test_scroll(). While I am not sure exactly why this broke the test, the issue was printing was screwed up if the user clicked around during printing (evidently setTextCursor()` does not work how I thought it did.)

[TheMatt2]

Change to draft, as test_carriage_return fails if w._executing = False. Want to resolve this before merging.

[TheMatt2]

After much revision, found a way to deal with this that solves the issue.

The issue was text is added using _insert_mode in cursor_widget.py if we are executing . The problem is insert mode breaks the \r handling, as we never "swallow" characters, only insert.

Solution is to only use _insert mode if text is being inserted on the line that holds the current prompt, as that is the only time we truly need this behavior.

This has been added to the test cases.

The way you could trigger this corner case where you still need insert mode is something like this

[TheMatt2]

At this point, the issue is solved, and all test cases pass, but I have one last edge case I want to chase down before removing the draft tag.

[TheMatt2]

Added fix for an edge case where switching between printing before the prompt, and printing after the prompt caused the self._prompt_sep to be printed in the wrong position.

All tests past... but there's a remaining issue if the prompt is a blank character... i.e. input(''). Still a draft.

[TheMatt2]

So if you run the following code fragment, you can hang the GUI, and it prints the error QTextCursor::setPosition: Position '429' out of range

This occurs on the main branch right now, not actually caused by any change I've made here. Attempting a fix.

[TheMatt2]

So I've confirmed this is a separate issue that doesn't require any changes to this PR.
I tried a fix for this, but it that fix actually broke like it is actually going to be a little involved.

Before digging too much into that, I would actually like some feedback on the change in this PR, as this seems to now all being working (took a while though).

[TheMatt2]

@ccordoba12 Thoughts?

[dalthviz]

Hi @TheMatt2 thank you for all you work here and sorry for the late response! Could it be possible for you to merge with latest main so this can get green CI (some tests fixes have been done over there)? Also, I gave a quick check to this PR manually and seems like things are working as expected 👍 Will try to do a more in depth code review now but if there are any specific changes that you think need more attention let us know!

[ccordoba12]

@ccordoba12 Thoughts?

Sorry for dropping the ball here. @dalthviz will take it from here, he's also a maintainer of this repo.

[dalthviz]

Thank you again @TheMatt2 for your work here! Left a comment to update a method docstring and several suggestions for format nitpicks. Other than that this LGTM 👍

[TheMatt2]

Merged in main and made format changes. None of the changes in main look like they impact this code directly.

Appreciate you guys taking a look at this.

As for a note on the readiness of this code, I think this code is tested and improves on the current behavior.
The sharpest point that requires some thought is thinking carefully about situations where the "prompt" for the user is a blank string, such as with input(''). Since the code uses prompt_start == prompt_end to signify there is no prompt at the moment, the code has to be carefully crafted to make sure this isn't an issue. This kind of issue is what is causing #610. For this MR, I made of point of structuring the code in such a way there this should not pose an issue for any situation that the original code could already handle.

[dalthviz]

Once again thank you @TheMatt2 ! This LGTM 👍

Just in case, do you have any further feedback @ccordoba12 ?

[ccordoba12]

Just some small suggestions for you @TheMatt2, the rest looks good to me.

[dalthviz]

@ccordoba12 to move this forward, is okay if we merge this as it is and then over a follow up PR I work on the style suggestions you left here?

[ccordoba12]

Sure, go ahead and merge this one.