Skip to content

Make browse history to work for lines longer than the console's width #173

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 5 commits into from
Dec 13, 2016
Merged

Make browse history to work for lines longer than the console's width #173

merged 5 commits into from
Dec 13, 2016

Conversation

@mariacamilarg
Copy link
Contributor

@minrk minrk changed the title PR: Browse history in IPython console works for lines longer than the console's width Browse history in IPython console works for lines longer than the console's width Dec 9, 2016
minrk
minrk reviewed Dec 9, 2016
View reviewed changes
Copy link
Contributor

@minrk minrk left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! I think we can get away without having to check the column width by just changing the couple of EndOfLines to EndOfBlock.

qtconsole/history_console_widget.py Outdated
c.movePosition(QtGui.QTextCursor.EndOfLine)
at_eol = (c.position() == current_pos)

if self.console_width > pos:
Copy link
Contributor

Choose a reason for hiding this comment

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

self.console_width isn't a trustworthy metric. This is an input value for setting the initial width of the window, not the current window size.

qtconsole/history_console_widget.py Outdated
else:
c.movePosition(QtGui.QTextCursor.EndOfBlock)
col = c.columnNumber()
at_eol = (c.position() == current_pos + col)
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this block is quite correct. When I am at the end of a wrapped line, I get that eol should be c.position() == current_pos, not current_pos + col. I think you can do without the else: block here, and just make the change to use EndOfBlock instead of EndOfLine.

qtconsole/history_console_widget.py Outdated
at_eol = (c.position() == current_pos)

if self.console_width > pos:
c.movePosition(QtGui.QTextCursor.EndOfBlock)
Copy link
Contributor

Choose a reason for hiding this comment

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

Since you've changed this to EndOfBlock, I think you need to change the cursor.movePosition(QtGui.QTextCursor.EndOfLine) below to EndOfBlock as well.

@mariacamilarg mariacamilarg mentioned this pull request Dec 9, 2016
Closed
@mariacamilarg
Copy link
Contributor Author

mariacamilarg commented Dec 10, 2016 •
edited
Loading

@minrk thank you very much for your revision, I completely agree with you. Last night I was getting some weird errors and that was why I made so many tricks, but it is working now. I think this new commit goes according to your comments :)

@wmvanvliet
Copy link
Contributor

For me on OSX, I can reproduce the history problem mentioned in spyder-ide/spyder#3215, but this PR in it's current form breaks it even further. Now whenever there is a line that spans multiple lines, the up and down keys stop working.

@mariacamilarg
Copy link
Contributor Author

mariacamilarg commented Dec 10, 2016 •
edited
Loading

@wmvanvliet You're right. And this bugs me a lot because I addresed the problem first the way it is now and it didn't work. But then it did (I'm sure it was my mistake, but it still kinda weird).

I just tested it with my initial commit (https://github.com/mariacamilaremolinagutierrez/qtconsole/blob/9d74bca1bb10f28b9f16b2c88ece340a31c1f4f8/qtconsole/history_console_widget.py) and it is working. I know this still has the problem that self.console_width isn't a trustworthy metric, but that can be fixed asking the current width not the initial one.

Does this commit work for you? (you only need to use the file I'm linking)

Also thanks for reviewing this :)

@wmvanvliet
Copy link
Contributor

if I have done my git commands correctly... no, also the first commit does not work for me on OSX.

@wmvanvliet wmvanvliet self-assigned this Dec 12, 2016
@wmvanvliet wmvanvliet added this to the 4.3 milestone Dec 12, 2016
@wmvanvliet wmvanvliet added the bug label Dec 12, 2016
wmvanvliet
wmvanvliet suggested changes Dec 12, 2016
View reviewed changes
Copy link
Contributor

@wmvanvliet wmvanvliet left a comment

Choose a reason for hiding this comment

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

I think I managed to debug this PR. Give my proposed changes a try and see if it works for you.

qtconsole/console_widget.py Outdated
else:
cursor = self._control.textCursor()
return cursor.positionInBlock()

Copy link
Contributor

Choose a reason for hiding this comment

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

You don't need this function.

qtconsole/history_console_widget.py
c = self._get_cursor()
current_pos = c.position()
c.movePosition(QtGui.QTextCursor.EndOfLine)
c.movePosition(QtGui.QTextCursor.EndOfBlock)
Copy link
Contributor

Choose a reason for hiding this comment

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

also change line 116/117 to: cursor.movePosition(QtGui.QTextCuresor.EndOfBlock)

qtconsole/history_console_widget.py Outdated

# Set a search prefix based on the cursor position.
col = self._get_input_buffer_cursor_column()
pos = self._get_input_buffer_cursor_position_in_block()
Copy link
Contributor

Choose a reason for hiding this comment

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

use: pos = self._get_input_buffer_cursor_pos()

@mariacamilarg
Copy link
Contributor Author

@wmvanvliet thank you very much for your help in this, it is working for me as well 😄 I just pushed the changes.

wmvanvliet
wmvanvliet approved these changes Dec 13, 2016
View reviewed changes
Copy link
Contributor

@wmvanvliet wmvanvliet left a comment

Choose a reason for hiding this comment

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

Works perfectly for me.

@ccordoba12
Copy link
Member

Thanks @minrk and @wmvanvliet for your reviews, and of course @mariacamilaremolinagutierrez for your work :-)

@ccordoba12 ccordoba12 changed the title Browse history in IPython console works for lines longer than the console's width Make browse history to work for lines longer than the console's width Dec 13, 2016
@ccordoba12 ccordoba12 merged commit cf7eb87 into spyder-ide:master Dec 13, 2016
@minrk
Copy link
Contributor

minrk commented Dec 14, 2016

Awesome, thanks everyone!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers

@minrk minrk minrk left review comments

@wmvanvliet wmvanvliet wmvanvliet approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@wmvanvliet wmvanvliet

Labels

bug

Projects

None yet

Milestone

4.3

Development

Successfully merging this pull request may close these issues.

5 participants

@mariacamilarg @wmvanvliet @ccordoba12 @minrk @mariacamilaremolinagutierrez