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

CommandFold should be available in Normal mode #493

Merged
merged 2 commits into from
Jul 22, 2016
Merged

CommandFold should be available in Normal mode #493

merged 2 commits into from
Jul 22, 2016

Conversation

aminroosta
Copy link
Contributor

This PR changes CommandFold to be used in Normal mode, just like all other fold commands.

Note that i am explicitly setting vimState.currentMode = ModeName.Normal, the reason is that vscode sometimes changes to Visual mode when folding the block:

Fore example this works fine.
screen shot 2016-07-21 at 4 15 59 pm
screen shot 2016-07-21 at 4 16 13 pm

But when the cursor is on the beginning line it goes to Visual mode!

screen shot 2016-07-21 at 4 16 30 pm

screen shot 2016-07-21 at 4 16 42 pm

@@ -1094,12 +1094,12 @@ class CommandDot extends BaseCommand {

@RegisterAction
class CommandFold extends BaseCommand {
modes = [ModeName.Visual, ModeName.VisualLine];
modes = [ModeName.Normal];
Copy link
Member

Choose a reason for hiding this comment

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

I suppose zc also works in Visual/Visual Line mode, so we just need to add Normal mode to the list.

@rebornix
Copy link
Member

BTW, per Vim's document, zc is used for fold close while zf is used for folding. So maybe CommandFold is not correct?

@johnfn
Copy link
Member

johnfn commented Jul 21, 2016

But why did you remove Visual and VisualLine from the list?

@aminroosta
Copy link
Contributor Author

@johnfn You are right it seems vim also supports zc and zo in visual mode.
Also i see that vim switches to Normal mode after these commands (zc & zo).
I will update the PR accordingly.

@rebornix I think the current behavior of zc is correct, as i understand zf is used to create manual folds.

I'm also interested in implementing za to toggle folds.
But it seems vscode has no such API to access fold regions.
Do you have any suggestions on if or how za could possibly be implemented?

@rebornix i think this is another use case for having Apis for folded regions. related vscode discussion.

@johnfn
Copy link
Member

johnfn commented Jul 22, 2016

Awesome, LGTM.

re: za, it's probable that it's simply not possible. Check with @rebornix, and if you can't find it, file an issue on the vscode Github. We have an issue open already for missing Vim features: microsoft/vscode#8997

@johnfn johnfn merged commit ad5fe89 into VSCodeVim:master Jul 22, 2016
@rebornix
Copy link
Member

@aminroosta I'll take a deep look into Vim folding doc and see what kind of API it requires then I'd like to create a separate issue for it.

@aminroosta
Copy link
Contributor Author

aminroosta commented Jul 25, 2016

@rebornix Do you think if it's a bad idea to figure out folded origins by using #486 MoveDownByScreenLine and then recording cursor position and then going back?!

@sandy081
This is one example why i think microsoft/vscode#2771 Does not really address what vim emulators need. If we had information about folded regions we could easily implement all cursorMove commands.

@rebornix
I want to implement za and the only workaround seems using cursorMove: down api and then recording cursor position and going back. do you think it worth it?

@rebornix
Copy link
Member

@aminroosta Sandeep's solution is reasonable as we are always trying to avoid exposing View Model to users. The reason that we want to know if it's a folded area in the case of za, we need to determine whehter we should call editor.fold or editor.unfold, right? It looks like a shortcut somehow. Do you think it would make sense that Code support a new command shortcut, maybe called toggleFold?

It's an open question. If there are more commands relying Fold Area information, I think providing this info directly will make everything simpler.

@rebornix
Copy link
Member

rebornix commented Jul 25, 2016

@aminroosta lastly, I like the idea of moving up then down to detect whether it's inside a folded area. As this operation is real fast and it follows a heavy Fold/UnFold command, I think it's a good workaround. People wont' notice that you did the cursor movement back and forth.

@aminroosta
Copy link
Contributor Author

@rebornix I think having information on folding areas gives vim extension writers more power and control on how to move the cursor. I can assure you that there will be other cases where this will bee needed 😉

I like the idea of moving up then down to detect whether it's inside a folded area.

When do you think #486 will be ready, i will add support for za after it gets merged.

@aminroosta
Copy link
Contributor Author

we are always trying to avoid exposing View Model to users.

I think in this case vscode should expose this information, to me it's one of the basic informations you should be able to ask the TextEditor, just like cursor postion or selection stop and end.

@aminroosta
Copy link
Contributor Author

screen shot 2016-07-25 at 1 44 14 pm

screen shot 2016-07-25 at 1 44 06 pm

@rebornix For example this workaround will not work for the above case. But it's better than nothing :)

@aminroosta
Copy link
Contributor Author

@rebornix @sandy081 Here is another example where folding area information is needed:

zj:
Move downwards to the start of the next fold. A closed fold
is counted as one fold. When a count is used, repeats the command [count] times.
This command can be used after an operator.

zk:
Move upwards to the end of the previous fold. A closed fold
is counted as one fold. When a count is used, repeats the command [count] times.
This command can be used after an operator.

@rebornix
Copy link
Member

I see, folding info is a must in this case. Since we already held on feature implementation for July Iteration, I'll create a separate issue to track this and see if we can make it in August. Thanks again for your input, they help a lot for both Vim and Code!

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.

3 participants