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

feat(quickopen): Add binds to open in splits to Quick Open #3733

Merged
merged 7 commits into from
Jul 12, 2021

Conversation

CrossR
Copy link
Member

@CrossR CrossR commented Jul 3, 2021

Fix #564
Fix #2232

A rough first pass, but adds default bindings for selecting items from the quick open to open in current/vertical/horizontal/new tab.

Binds aren't finalised, so some input there would be nice (Ctrl-s and Ctrl-v are the defaults in Fzf in vim which is why I went with them).

Comment on lines 51 to 71
let selectHorizontal =
register(
"list.selectHorizontal",
ListSelect({
direction: Oni_Core.SplitDirection.Horizontal,
}),
);
let selectVertical =
register(
"list.selectVertical",
ListSelect({
direction: Oni_Core.SplitDirection.Vertical({shouldReuse: false}),
}),
);
let selectNewTab =
register(
"list.selectNewTab",
ListSelect({
direction: Oni_Core.SplitDirection.NewTab,
}),
);
Copy link
Member

Choose a reason for hiding this comment

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

Awesome! Thanks for adding these 🎉

Comment on lines 128 to 132
bind(
~key="<C-v>",
~command=Commands.List.selectVertical.id,
~condition="listFocus" |> WhenExpr.parse,
),
Copy link
Member

Choose a reason for hiding this comment

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

The only concern I have here is that the Control+V keybinding would conflict with paste on Windows/Linux (ie, regressing #2308)

A couple ideas:

  • We could use Control+Shift+V for opening in vertical tab
  • We could use the Shift+CR keybinding for vertical, and Control+T for new tab

Comment on lines 65 to 69
let selectNewTab =
register(
"list.selectNewTab",
ListSelect({
direction: Oni_Core.SplitDirection.NewTab,
Copy link
Member

Choose a reason for hiding this comment

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

Looks like this will address #2232 🎉

Copy link
Member

@bryphe bryphe left a comment

Choose a reason for hiding this comment

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

Thanks for taking this on, @CrossR ! Approach looks great to me. Just had a comment about the Control+V binding -

Comment on lines 133 to 137
bind(
~key="<C-s>",
~command=Commands.List.selectHorizontal.id,
~condition="listFocus" |> WhenExpr.parse,
),
Copy link
Member

Choose a reason for hiding this comment

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

This also conflicts with the save keybinding, but I think this one is OK - understandable that save wouldn't occur when the quickmenu is up.

@CrossR
Copy link
Member Author

CrossR commented Jul 8, 2021

Did a quick survey of some common binds for this sort of thing and updated them accordingly, but with Shift-enter as vertical open by default since we use c-v as copy.

That said, obviously if a user wanted to swap they could just do something like:

  { "key": "<C-s>", "command": "list.selectHorizontal", "when": "listFocus" }

The only other thing I wanted to check here is if the command names are okay. VSCode has no equivalent, so wasn't sure if they should be prefaced with oni or something.

@bryphe
Copy link
Member

bryphe commented Jul 8, 2021

Did a quick survey of some common binds for this sort of thing and updated them accordingly, but with Shift-enter as vertical open by default since we use c-v as copy.

Awesome! Thanks for looking into this @CrossR . Looks good to me.

The only other thing I wanted to check here is if the command names are okay. VSCode has no equivalent, so wasn't sure if they should be prefaced with oni or something.

Hmm, good point - VSCode does have a list.* set of commands, so prefixing it with oni. will make it clear these are Onivim-specific, and keep us safe if VSCode does introduce keybindings with the same name.

bryphe added a commit that referenced this pull request Jul 9, 2021
__Issue:__ Using `:tabnew` or `:tabedit` (or Control+T in #3733) would create a new vim-style layout tab, but there'd be an extra 'editor tab' in the initial window split.

__Defect:__ There was an extra copy/create of the editor in the layout code.

__Fix:__ Remove extraneous copy

__Todo:__
- [x] Add regression test
@CrossR
Copy link
Member Author

CrossR commented Jul 9, 2021

Swapped the names, added some basic docs, just waiting for the CI to be happy then I'll squash/merge.

@CrossR
Copy link
Member Author

CrossR commented Jul 10, 2021

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@bryphe
Copy link
Member

bryphe commented Jul 12, 2021

Sorry about the build issues; the CentOS build failure looks unrelated. I'll override and merge.

Thanks for the awesome contribution @CrossR !

@bryphe bryphe merged commit a6bd89a into onivim:master Jul 12, 2021
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.

QuickOpen: Open in new (layout) tab Commands: QuickOpen - open in vertical split / open in horizontal split
2 participants