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

More vim movements for filebrowser #195

Merged
merged 5 commits into from
Sep 6, 2015

Conversation

Sh4pe
Copy link

@Sh4pe Sh4pe commented Sep 3, 2015

This pull request enables new vim-like movements to the file browser:

  • It adds the vim movements gg and G that jump to the top and the bottom of the file browser
  • It add the vim movements Ctrl+e and Ctrl+y that scroll up and scroll down one line
  • It adds the vim movements Ctrl+f and Ctrl+b that scroll down and scroll up one page

* 'gg' goes to top
* 'G' goes to bottom
* '^e' moves up one line
* '^e' moves down one line
* '^f' scrolls down one screen
* '^f' scrolls up one screen
if (modifierFlags & NSControlKeyMask) {
[self.actionDelegate actionScrollUpOneScreen];
}
return YES;
Copy link
Collaborator

Choose a reason for hiding this comment

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

For 'e', 'y', 'f', and 'b' YES is returned even if the control key isn't pressed. I think return YES should be moved into the if statements and NO should be returned otherwise.

Copy link
Author

Choose a reason for hiding this comment

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

The meaning of the return value of processKeyModeNormal was not entirely clear to me. It returns YES when the key command is known, NO otherwise, right?

If that's the case then I absolutely agree with you.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, that's correct.

Copy link
Author

Choose a reason for hiding this comment

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

Done, fixed it in commit 4f65da6

@dnerdy
Copy link
Collaborator

dnerdy commented Sep 4, 2015

This is looking good. It could use some tests here, but other than that it works as advertised. I'll let @qvacua manage the final merge.

Thanks for the contribution @Sh4pe.

@Sh4pe
Copy link
Author

Sh4pe commented Sep 5, 2015

You are very welcome. I'll add the tests when I've time for it, probably later today or tomorrow.

@Sh4pe
Copy link
Author

Sh4pe commented Sep 5, 2015

I added unit tests for the new vim key movements. Okay like that?

qvacua pushed a commit that referenced this pull request Sep 6, 2015
@qvacua qvacua merged commit 49fe393 into qvacua:master Sep 6, 2015
@Sh4pe Sh4pe deleted the more-vim-movements-filebrowser branch September 9, 2015 13:26
@jordwalke
Copy link
Collaborator

Thanks!

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.

4 participants