Skip to content
This repository was archived by the owner on Apr 6, 2018. It is now read-only.

Conversation

jonasws
Copy link
Contributor

@jonasws jonasws commented Oct 9, 2015

This PR implements the traditional ) and ( keybindings originally found in vim. I would greatly appreciate any comments and feedback on how I made it work with regex, as I'm certain there may be a slightly slicker way to pull it off (though I've tried to do similar to how other motions are implemented).

@mattr-
Copy link
Contributor

mattr- commented Oct 9, 2015

Great work @jonasws. There's still some behavior differences that we might want to look at addressing.

For example, in this repo, open spec/motions-spec.coffee in vim and go to line 1340. Now hit ) to go to the next sentence. In vim, this moves you to line 1342. With this patch, Atom does nothing.

While still on line 1340 in the same file, hit ( to back a sentence and you're taken all the way back to line 1081 where there's a ? with a space after it in a string. Doing the same thing in Vim only takes you back to the beginning of line 1339.

The larger conclusion to draw from this is that paragraph and section boundaries don't seem to count as sentence boundaries as the Vim docs say they should. From vim's :help sentence:

A sentence is defined as ending at a ., ! or ? followed by either the
end of a line, or by a space or tab. Any number of closing ), ], "
and ' characters may appear after the ., ! or ? before the spaces,
tabs or end of line. A paragraph and section boundary is also a sentence
boundary.

Just my 2cents, as I'm not the actual maintainer of this plugin. 😃

@jonasws
Copy link
Contributor Author

jonasws commented Oct 9, 2015

@mattr That is great feedback! I am not an expert vim user myself, so I figured someone had something to comment on that I completely missed. I also implemented this with the purpose of it to work on files consisting of mainly text (such as Markdown and LaTex), as I didn't think it was defined by vim what would happen when pressing those keys in a code file. I'll look into what vim says to try to make it compliant.

@maxbrunsfeld
Copy link
Contributor

Ok well the code looks very nice. It seems like a paragraph breaks, or empty lines, should be considered sentence boundaries, but other than that, I'd love to 🚢 this.

@jonasws
Copy link
Contributor Author

jonasws commented Oct 9, 2015

@maxbrunsfeld I have implemented sentence boundaries as we discussed. I have another question that we might want to address. How do we want things to work in "code files", where sentences do not occur as the normally would? I haven't been able to think of anything clever on that account.

@maxbrunsfeld
Copy link
Contributor

How do we want things to work in "code files", where sentences do not occur as the normally would?

I don't think you need to change the behavior based on the filetype or context.

@maxbrunsfeld
Copy link
Contributor

Are you happy w/ this as is, for your use case? If so, I'm fine with merging this as long as the tests pass. We can always refine the behavior in a subsequent PR.

@jonasws
Copy link
Contributor Author

jonasws commented Oct 9, 2015

I am happy with it as it is. As you said, making it work exactly like vim, is quite complex, and should probably be done in another PR, to keep the review work simpler.

keydown '('
expect(editor.getCursorBufferPosition()).toEqual [1, 0]


Copy link
Contributor

Choose a reason for hiding this comment

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

Could you remove this second blank line?

maxbrunsfeld pushed a commit that referenced this pull request Oct 9, 2015
@maxbrunsfeld maxbrunsfeld merged commit bf5081a into atom:master Oct 9, 2015
@maxbrunsfeld
Copy link
Contributor

Thanks @jonasws!

@jonasws jonasws mentioned this pull request Oct 23, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants