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

Add support for S #546

Merged
merged 3 commits into from
Aug 4, 2016
Merged

Add support for S #546

merged 3 commits into from
Aug 4, 2016

Conversation

glibsm
Copy link
Contributor

@glibsm glibsm commented Aug 2, 2016

Fixes #536

keys = ["S"];

public async exec(position: Position, vimState: VimState): Promise<VimState> {
const lineStart = position.getFirstLineNonBlankChar();
Copy link
Member

Choose a reason for hiding this comment

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

Why firstLineNonBlankChar? S eats up the whole line, start to end.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@johnfn That wasn't my understanding from #536, or from my usage of vim.

For example, consider a sample function:

 def func(one, two):
     print o|ne
     print two

By pressing S I would expect to replace the second line with the correct indent, which this accomplishes.

Just nuking the whole line seems less useful, and not what my terminal vim does.

I'm happy to change to first char, but is that really desired?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@johnfn did some more testing and digging through manuals. c-c aka S is defined to "replace the whole line".

However, my 7.3 VIM and Atom vim-plus replace to the first non-blank character as I have described.

VSCode c-c does the whole line replace. In order to be consistent within VSCode, we should either make S do whole line, or update c-c to point to first non-white.

Copy link
Member

@johnfn johnfn Aug 2, 2016

Choose a reason for hiding this comment

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

Ahh, @rebornix cleared this up beautifully.

Thanks, @rebornix, and thanks @glibsm for pointing out that the command had different behavior than what I expected!

@rebornix
Copy link
Member

rebornix commented Aug 2, 2016

My two cents here:

  1. About the implementation. Since S is just a Synonym for cc and we do support cc in Code (a c operator then a c movement), you may want to just leverage these two existing actions to implement S. This way we can avoid difference between synonyms.
  2. About the behavior. Both you and @johnfn are correct, it depends on autoindent option. If autoindent it turned on, it will always keep the indentation of first changed lines. If it's off, it will delete the whole line as @johnfn described.

I think right now you just keep S the same as current Code's cc (by leverage existing actions) and once #508 is merged, you can send another PR to get the indentation problem fixed? I'll say that's another feature :)

@rebornix
Copy link
Member

rebornix commented Aug 2, 2016

Lastly, please update the roadmap about S.

@johnfn
Copy link
Member

johnfn commented Aug 2, 2016

👍 on what @rebornix said.

Also, if you want you could do numeric prefixes on S as well. Not a requirement for getting this pulled in - we can always do it later, of course.

@glibsm
Copy link
Contributor Author

glibsm commented Aug 2, 2016

@rebornix Thank you for the explanation, that makes total sense. Completely forgot about the autoindent .

I'll change this PR for S to be an alias of cc and update the roadmap.

@johnfn
Copy link
Member

johnfn commented Aug 4, 2016

Ready to merge?

@glibsm
Copy link
Contributor Author

glibsm commented Aug 4, 2016

@johnfn ready if you are.

@johnfn
Copy link
Member

johnfn commented Aug 4, 2016

Looks great to me. Props on doing the numeric prefix as well! :-)

@johnfn
Copy link
Member

johnfn commented Aug 4, 2016

... Oh wait, there are merge conflicts. Could you fix those? Then we will really be good to go. 😛

@glibsm
Copy link
Contributor Author

glibsm commented Aug 4, 2016

Whoops. Yeah, let me fix those

@glibsm
Copy link
Contributor Author

glibsm commented Aug 4, 2016

@johnfn should be good to go now without conflicts.

@johnfn johnfn merged commit 7b3eff9 into VSCodeVim:master Aug 4, 2016
@johnfn
Copy link
Member

johnfn commented Aug 4, 2016

Awesome! Thanks, @glibsm!

@glibsm glibsm deleted the shift-s branch August 4, 2016 07:29
@mnsr
Copy link

mnsr commented Aug 19, 2016

Thanks for this. But there's still one more thing.

Not sure if i should put this here, but the implementation of shift+S doesn't match standard VIM.

Shift+S adds indentation, in vscodevim it does not.

So if if i'm inside a function like this:

function Test() {
// << Shift+S should put the correct indentation here and switch to insert mode
}

@johnfn
Copy link
Member

johnfn commented Aug 19, 2016

@mnsr could you please open up an issue for that bug? Thanks. :)

@mnsr
Copy link

mnsr commented Aug 19, 2016

@johnfn yeah np

@glibsm
Copy link
Contributor Author

glibsm commented Aug 19, 2016

As discussed earlier, it's a bug with auto indent. Has it been fully implemented already?

@rebornix
Copy link
Member

@glibsm sorry for the late response and yes, it's already covered by #629. Kudos to @sectioneight .

@ascandella
Copy link
Contributor

Ha, small world. @glibsm and I sit right next to each other at work :)

This pull request was closed.
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.

5 participants