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(repl): Add key binding to force a new line #14536

Merged
merged 2 commits into from
Jun 15, 2022
Merged

Conversation

sigmaSd
Copy link
Contributor

@sigmaSd sigmaSd commented May 8, 2022

Add Alt Enter key binding to force a new line, usually this is used when you write a single line and then you want to break it.

@CLAassistant
Copy link

CLAassistant commented May 8, 2022

CLA assistant check
All committers have signed the CLA.

@bartlomieju
Copy link
Member

@sigmaSd thanks for opening the PR. What's the reasoning for this functionality? If it is a popular binding, I'd expect that rustyline would implement it out-of-the-box.

@imjamesb
Copy link
Contributor

imjamesb commented May 9, 2022

Would this also work on a Mac whereas Mac has the option key, not alt?

@sigmaSd
Copy link
Contributor Author

sigmaSd commented May 9, 2022

I'm ok with any keybidings, if It doesn't work we can change it to ctrl but I don't have a mac to test.

Regarding rustyline, it follows emacs and vi editor keys but the difference is in an editor the enter key is unambiguous it always means a new line, in a repl it can either mean a new line or a eval request, that's why we rely on heuristics such as is the parentheses balenced or not, but there are cases when heuristics can fail and you simply want to add a new line for example when refactoring a single to line to a multiline.

I can add a demo of how this looks like if it's still unclear.

@sigmaSd
Copy link
Contributor Author

sigmaSd commented May 9, 2022

Here is a random example where I want to refactor the initial one-liner to a function, In this case the heuristic would fail because the input is valid and it will try to evaluate it, that's why I'm forcing new lines.

deno3.mov

@sigmaSd
Copy link
Contributor Author

sigmaSd commented May 10, 2022

For precedence, I just noticed that fish shell has this feature and its also bound to Alt+enter

@bartlomieju
Copy link
Member

@sigmaSd sounds good, we can add it. Please merge with main and open a PR to https://github.com/denoland/manual that updates REPL chapter with this binding

@sigmaSd
Copy link
Contributor Author

sigmaSd commented May 12, 2022

done

@bartlomieju bartlomieju added this to the 1.22 milestone May 13, 2022
editor.bind_sequence(
KeyEvent(KeyCode::Enter, Modifiers::ALT),
EventHandler::Simple(Cmd::Newline),
);
Copy link
Member

@dsherret dsherret May 17, 2022

Choose a reason for hiding this comment

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

This doesn't seem to work in Windows terminal or command prompt—the terminal goes full screen when I press alt+enter.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Guess its bound already, I can try ControlEnter or shiftEnter

Copy link
Contributor Author

Choose a reason for hiding this comment

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

shiftenter and ctrlenter doesn't seem to work on linux, do you have a suggestion for a keybinding?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can also make it platform dependent, alt enter on Linux Mac, and maybe shift or ctrl enter on windows if one of those works

Copy link
Member

Choose a reason for hiding this comment

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

I know @lucacasonato was interested in this issue. Luca WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed the keybinding to CTRL-S, at least that should work cross platform.

sigmaSd added a commit to sigmaSd/manual that referenced this pull request May 29, 2022
@bartlomieju bartlomieju modified the milestones: 1.22, 1.23 Jun 1, 2022
Copy link
Member

@bartlomieju bartlomieju left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @sigmaSd!

@bartlomieju bartlomieju merged commit 8bfa89a into denoland:main Jun 15, 2022
@jsejcksn
Copy link
Contributor

@bartlomieju @dsherret In the blog post release notes, this is listed with a capital S:

Using Ctrl + S combination you can now force a new line when editing code in the REPL

However, the sequence requires lowercase s. I think this is probably assumed by hardware keyboard users because of convention, but many software keyboards allow input of different letter forms without modifier keys, so the information in the release notes is incorrect.

Can it be changed to ctrl + s? It's not clear where to post issues about the blog release notes, so I'm posting here.

@bartlomieju
Copy link
Member

You mean you want to change the blog post to use ctrl + s instead of Ctrl + S?

@jsejcksn
Copy link
Contributor

You mean you want to change the blog post to use ctrl + s instead of Ctrl + S?

@bartlomieju Yes, exactly.

It's not clear where to post issues about the blog release notes, so I'm posting here.

Also, where is the official issue tracker for the blog?

@bartlomieju
Copy link
Member

You mean you want to change the blog post to use ctrl + s instead of Ctrl + S?

@bartlomieju Yes, exactly.

Sure I can change it.

It's not clear where to post issues about the blog release notes, so I'm posting here.

Also, where is the official issue tracker for the blog?

You can post issues in deno repository, the blog is in a private repo.

@bartlomieju
Copy link
Member

@jsejcksn this is fixed now in the blog post.

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.

None yet

6 participants