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

feature: copy character from line above/below#721

Merged
maxbrunsfeld merged 2 commits into
atom:masterfrom
jacekkopecky:copy-from-line-above
Jul 23, 2015
Merged

feature: copy character from line above/below#721
maxbrunsfeld merged 2 commits into
atom:masterfrom
jacekkopecky:copy-from-line-above

Conversation

@jacekkopecky
Copy link
Copy Markdown
Contributor

This PR implements VIM insert-mode commands ctrl-y and ctrl-e for copying from the line above/below the cursor.
A few questions in relation to this PR:

  • is lib/insert-mode.coffee an OK new file for this?
  • if so, should Insert mode put #720 live there, too?
  • should I worry that some might use ctrl-e for going to the end of the line? (default in Atom)

Oh, and congratulations on Atom 1.0.0! 🎆 🎈

@bronson bronson mentioned this pull request Jul 2, 2015
@bronson
Copy link
Copy Markdown
Contributor

bronson commented Jul 2, 2015

Very cool.

I agree, the ctrl-e conflict is worrisome. I really like that normal mode feels like Vim and insert mode feels like Atom.

Would it make sense to give vim-mode a configuration option: "Vim-Flavored Insert Mode" and "Atom-Flavored Insert Mode"? When there's a conflict, this setting would tell whose keybindings to prefer.

I'd set mine to Atom-flavored.

@bronson
Copy link
Copy Markdown
Contributor

bronson commented Jul 3, 2015

Oh heck, users can always bind ctrl-e back to editor:move-to-end-of-line if they care. At least this way there's a choice.

Merged it to vim-mode-next. This PR looks great.

@bronson
Copy link
Copy Markdown
Contributor

bronson commented Jul 4, 2015

Getting strange errors when running this patch in vim-mode-next: https://travis-ci.org/bronson/vim-mode-next/builds/69569825

Looks related to the activateResources/getEditorElement mocking... Maybe a timing issue?

To reproduce locally:

git clone https://github.com/bronson/vim-mode-next
git checkout 0ad53fb
cd vim-mode-next
apm install
apm test

Very curious what's going on.

Since your patch works fine in vim-mode, feel free to ignore. :)

@jacekkopecky
Copy link
Copy Markdown
Contributor Author

just guessing, but it seems like it needed a change of 'vim-mode' to 'vim-mode-next' in the spec file: https://github.com/bronson/vim-mode-next/blob/0ad53fbfa4b2c626e00d7e16c98e95fc6232e920/spec/insert-mode-spec.coffee#L7

@bronson
Copy link
Copy Markdown
Contributor

bronson commented Jul 5, 2015

DOH!! Like this? bronson/vim-mode-next@654a9c3

You're right, how did I miss that? Sorry. :)

@bronson
Copy link
Copy Markdown
Contributor

bronson commented Jul 5, 2015

Yep, that was it. vim-mode-next 0.54.11 has ctrl-e/ctrl-y support again.

@jacekkopecky jacekkopecky force-pushed the copy-from-line-above branch from 0bafbfc to 4c929af Compare July 22, 2015 20:18
@maxbrunsfeld
Copy link
Copy Markdown
Contributor

I think I'd like to not override the ctrl-e key-binding by default. I agree with @bronson that by default, insert mode should feel like Atom. It's great to have the command implemented though: anybody who wants this command can just add the keybinding to their keymap.cson.

@jacekkopecky
Copy link
Copy Markdown
Contributor Author

No worries, done. I tend to use ctrl-y a lot, but I'm not sure I've ever used ctrl-e anyway.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

😄

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

well we're not throwing away perfectly good specs, are we? 8-)

maxbrunsfeld pushed a commit that referenced this pull request Jul 23, 2015
feature: copy character from line above/below
@maxbrunsfeld maxbrunsfeld merged commit 7683dcd into atom:master Jul 23, 2015
@jacekkopecky jacekkopecky deleted the copy-from-line-above branch July 23, 2015 19:37
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants