[MRG] Fix "invalid cursor position" error #145
Conversation
|
To clarify: the error occurs because first, |
|
This sounds sensible. Heads up, though, that the Qt console is mostly community maintained these days, because the core team don't use it very often and aren't especially familiar with Qt. @ccordoba12 do you want to have a look at this? |
|
@takluyver Who do I talk to, to become a maintainer for this project? I use it every day. |
|
I was hoping you might say that. ;-) Make a few PRs like this; I'd like to give Carlos the opportunity to review them (he also has an interest in the Qt console, particularly for use in Spyder), and if all goes well we'll make you a maintainer. |
|
Will do this weekend, thanks for pinging me :-) |
wmvanvliet
changed the title
|
@ccordoba12 somehow the stuff from #141 has gotten mixed up in this PR as well and I can't seem to untangle it right now. But otherwise this PR is ready for a review. This should squash the annoying "invalid cursor position" errors once and for all. Furthermore, this should prevent the console from ever "locking up". |
wmvanvliet
changed the title
| """Find the position in the text right after the prompt""" | ||
| return min(self._prompt_cursor.position() + 1, self._get_end_pos()) | ||
|
|
||
| _prompt_pos = property(_get_prompt_pos) |
There was a problem hiding this comment.
Couldn't this be written as
@property
def _prompt_pos(self):
return self._get_prompt_pos()?
I think it'd be clearer :-)
| return min(self._append_before_prompt_cursor.position(), | ||
| self._get_end_pos()) | ||
|
|
||
| _append_before_prompt_pos = property(_get_append_before_prompt_pos) |
There was a problem hiding this comment.
And this as
@property
def _append_before_prompt_pos(self):
return self._get_append_before_prompt_pos()?
| elif status == 'aborted': | ||
| self._process_execute_abort(msg) | ||
| # If status == 'error', a message of type 'error' will be sent with | ||
| # the details |
There was a problem hiding this comment.
Is this comment necessary? I don't think so :-)
| @@ -661,7 +666,7 @@ def reset(self, clear=False): | |||
|
|
|||
| # update output marker for stdout/stderr, so that startup | |||
| # messages appear after banner: | |||
| self._append_before_prompt_pos = self._get_cursor().position() | |||
| #self._append_before_prompt_pos = self._get_cursor().position() | |||
There was a problem hiding this comment.
Could this comment be removed?
| if content['ename'] == 'KeyboardInterrupt': | ||
| # If the user interrupted execution by pressing CTRL-C, only | ||
| # display a simple message. | ||
| self._append_plain_text("Execution aborted\n") |
There was a problem hiding this comment.
I like this idea a lot. It'd prevent displaying long tracebacks that don't mean much to the user.
@takluyver, what do you think about it?
|
I did a quick review for now. I still have to take a look at the logic :-) |
|
@wmvanvliet, does this close a particular issue? If so, please mention it in the PR description as Fixes #foo |
wmvanvliet
force-pushed
the
truncating_fix
branch
2 times, most recently
from
c6be6da
to
d0f8c3c
Compare
|
Rebased this to be a clean PR (no more code from others PR's sneaking their way into this one). @ccordoba12 Comments addressed, thanks for the review! |
| @@ -1653,7 +1646,7 @@ def _get_last_lines(self, text, num_lines, return_count=False): | |||
| break | |||
| i += 1 | |||
| if return_count: | |||
| return text[pos:], i | |||
| return text[pos:], i + 1 | |||
|
I added one minor comment, just for my own enlightenment :-) Otherwise, this looks good to me. @wmvanvliet, thanks a lot for working on a fix for this nasty bug. It's really appreciated! |
|
Yeah, I forgot to say thanks! |
Since the QTextEdit object has a maximumBlockCount constraint, lines of text can get removed from the beginning of the document at any time we append to the document. It is important to keep proper track of the beginning and end position of the prompt, also when text is inserted or removed. Instead of manaully tracking this position by computing offsets when doing text operations, this commit uses QTextCursor objects to do this. This commit fixes a bug where self._append_before_prompt_pos is incorrectly computed, due to text being removed as the prompt is being appended to the document. fixes jupyter#94, jupyter#107
wmvanvliet
force-pushed
the
truncating_fix
branch
from
d0f8c3c
to
7bea0a3
Compare
|
There were some erratic changes in this PR due to the mess I'd made of my Git branch. I think I've got it sorted out now. Thanks for catching that @ccordoba12. |
|
Thanks @wmvanvliet! Merging now :-) |
Since the
QTextEditobject that we use as main text widget has themaximumBlockCountconstraint set, lines of text can get removed from the beginning of the document at any time text is append to the document.This PR fixes a particularly nasty bug where
self._append_before_prompt_posis incorrectly computed, leading to theQTextCursor::setPosition: Position '14083' out of rangeerror.It is important to keep proper track of the beginning and end position of the prompt, also when text is inserted or removed. Instead of manually tracking this position by computing offsets when doing text operations, this PR uses QTextCursor objects to do this.
Fixes #76, #94, #107