-
-
Notifications
You must be signed in to change notification settings - Fork 734
feat:Bind Ctrl-v to visually editing the selected command, exactly as per fc #2851
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
Open
wolf
wants to merge
2
commits into
atuinsh:main
Choose a base branch
from
wolf:wolf/edit-and-accept-like-fc
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a maintainer here, just procrastinating on other work I should be doing, so take this suggestion with a grain of salt.
You might consider a different fallback here. Some minimal Fedora derivatives come without
EDITORset and withvim-minimalinstalled which doesn't provide/usr/bin/vim, just/usr/bin/vi. Debian looks like it relies on the alternatives system so callingeditorwill get you the default.You could probe to see if there's a
/usr/bin/editorand then fall back tovi, or you could turn it into an error instructing the user to set the correct environment variables. That'd look something like this (which I've not tested at all):There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using
viinstead ofvimsounds right. In systems withvim,viis usually just another name forvim. A thing I was thinking, related to this, is that perhapsconfig.tomlcould optionally specify an editor, and that would be something to check for first. Reporting an error seems like user-friendly behavior. Is that the general way atuin handles situations like this? With respect to the exact editors used, I feel like I would want to probe forviand issue the error only after that. That way I maintain the same sequence used byfc(modulo any change inconfig.toml). But maybe my desire to emulatefcis not appropriate.When you say
/usr/bin/editor, you're just usingeditoras a stand-in, right? Just double-checking. You're asking that I make sure no matter what they have specified, even in one of those variables, that their choice actually exists?Finally, it occurs to me that this is a feature you might want to turn on/off in
config.toml. It seems unlikely, at least to me, but it's a thought. I feel like "off" means "just don't touch Ctrl-v". Hmm. And also I should look to see any place else that mentions keys and consequences, e.g., help text, documentation, etc. A PR that adds a key should update such things as well.It wasn't immediately obvious to me how to write a test (or a couple of tests) in the test section of
interactive.rssince running a visual editor requires interactive input. This is on my mind.Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I see,
editoris not just a place-holder. On some OSs, that's an actual thing. Actually probing (as you suggest) for the existence of/usr/bin/editorsounds like good behavior. I wonder if we also need to check the actual OS, so that we knoweditoris appropriate. My initial thought is no.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I know how to do this as your are suggesting. Making sure specific editors exist feels like the code will be bulky compared to what is currently in this PR. Looking specifically for
/usr/bin/editor; and maybe adding a key toconfig.toml. But I also think these are good ideas, regardless of whether you are a maintainer or not.I will explore implementing this, and update this PR for further review.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, I wouldn't enumerate
nvim,emacs, etc. As long as the probe for/usr/bin/editorhappens after checking theEDITORenvironment variable I don't think you need to worry about checking the OS, either, although that might be necessary if some distribution ships something else as/usr/bin/editorand also don't set anEDITORenvironment variable.Personally, I feel like we can get away without a
config.tomlsetting for the editor since theEDITORenvironment variable is so widely used and I have a hard time imagining someone who wants to usenvimfor git commit messages, butemacsfor editing commands in Atuin.For whatever it's worth, I lean slightly towards presenting the user with an error over defaulting to
viif nothing is set. I expect nearly every Atuin user knows how to usevi, but for any that don't it would be a shocking experience to accidentally end up there. I don't have a great sense of whether or not that matches the general Atuin style though.There's a table of shortcuts, but I'm not sure where the source for that document is.
Finally, for testing maybe you could set
EDITORto a shell script that writes a string to the given tempfile so it's not interactive? I've not thought hard about that, though.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh! A script instead of an actual editor is a great idea! I don't know why I didn't think of that. That's exactly how I test all my dotfile shell functions.