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

Fixed behavior of h, l and I, gg commands added #15

Merged
merged 4 commits into from Dec 6, 2016

Conversation

pwoosam
Copy link
Collaborator

@pwoosam pwoosam commented Dec 1, 2016

commands I and gg added
command G simplified
issue #8 fixed

commands I and gg added
command G simplified
issue 8 fixed
Copy link
Contributor

@Nodd Nodd 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 your PR. I have a few comments to improve it, please have a look.

@@ -52,10 +52,18 @@ def _move_cursor(self, movement, repeat=1):
editor.setTextCursor(cursor)
self._widget.update_vim_cursor()

def _editor_cursor(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

Useful helper, thanks. Could you use it in _move_cursor() above too ?

cursor = self._editor_cursor()
if not cursor.atBlockStart():
self._move_cursor(QTextCursor.Left)
if repeat > 1: self.h(repeat-1)
Copy link
Contributor

Choose a reason for hiding this comment

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

Please put the code block in a new line, even if it's small it will be more clear. (same below)

self._widget.update_vim_cursor()

def G(self, repeat=1):
self._move_cursor(QTextCursor.Start)
Copy link
Contributor

Choose a reason for hiding this comment

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

number+G is supposed to send the cursor to a specific line number, your version doesn't do that anymore ?
G puts the cursor to the top inly if no number was entered, i.e. repeat == 1

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I actually wasn't aware that you could jump lines that way! I thought G and gg were different because I only used their default uses: G goes to last line, gg goes to first line. And actually I mixed their default uses too! Whoops, I'll get on it

self._move_cursor(QTextCursor.Start)

def gg(self, repeat=1):
self._move_cursor(QTextCursor.End)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is is the same as G, where entering a number before gg does something different ?

used _editor_cursor() in _move_cursor()
fixed indentation for h() and l()
fixed behavior of gg
restored behavior of G
Allows G to have 0 as a default repeat value. This way 1G will go to the
first line and G will go to the last line.
@pwoosam
Copy link
Collaborator Author

pwoosam commented Dec 6, 2016

In commit ca748fe I added some lines to allow G to have a different default behavior than if G was preceded by a number (1 or greater). I'm not sure if this was the best way to do it, but it seemed to be the most simple.

Without the change, the default value for repeat would always be 1, regardless of the default value provided at the definition of the function.

I also tried changing VimWidget.on_text_changed() and VimKeys.__call__() to call VimKeys.method() when no number is preceded by the command and to call VimKeys.method(repeat) when a number is provided. However, that required every key function use keyword arguments.

Copy link
Contributor

@Nodd Nodd left a comment

Choose a reason for hiding this comment

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

Thank you for the update, a last detail and it's good to go !

@@ -294,6 +315,8 @@ def on_text_changed(self, text):
if text.startswith("0"):
# Special case to simplify regexp
repeat, key, leftover = 1, "0", text[1:]
elif text.startswith("G"):
repeat, key, leftover = 0, "G", text[1:]
Copy link
Contributor

Choose a reason for hiding this comment

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

Use -1 rather than 0, it will be clearer (-1 as an index in python means the last one).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

All done! Thanks for the help :)

repeat should be -1 to represent last line, rather than 0
@Nodd Nodd merged commit 75590f6 into spyder-ide:master Dec 6, 2016
@Nodd
Copy link
Contributor

Nodd commented Dec 6, 2016

Thanks !

@ccordoba12 ccordoba12 added this to the v0.1 milestone Dec 9, 2016
@ccordoba12 ccordoba12 changed the title issue 8 fixed and commands added Fixed behavior of h and l and commands added Dec 9, 2016
@ccordoba12 ccordoba12 changed the title Fixed behavior of h and l and commands added Fixed behavior of h, l and I, gg commands added Dec 9, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants