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

Implement 'quick access' via Alt-<n> #79

Merged
merged 5 commits into from
May 10, 2021
Merged

Conversation

yuvipanda
Copy link
Contributor

Puts numbers 0-9 next to commands above current selection.
Ctrl- should activate them - but since Ctrl- are
reserved by terminal, this does not currently work. Need to
find different sets of keyboard shortcuts.

Numbers are above current selection, since the user must use
the arrow keys to go over the commands below current selection
before reaching selection.

image

Puts numbers 0-9 next to commands *above* current selection.
Ctrl-<number> should activate them - but since Ctrl-<num> are
reserved by terminal, this does not currently work. Need to
find different sets of keyboard shortcuts.

Numbers are *above* current selection, since the user must use
the arrow keys to go over the commands below current selection
before reaching selection.
@yuvipanda
Copy link
Contributor Author

Can't think of a proper keybinding tho :(

@ellie
Copy link
Member

ellie commented May 9, 2021

Nice idea! How about using Alt? Alt+Enter could then also be used for #78

@yuvipanda
Copy link
Contributor Author

Hmm, Using Key::Alt doesn't seem to work either - and I'm not sure how to debug these either. Any println calls are cleared when the tui exits.

@ClashTheBunny
Copy link

Hmm, Using Key::Alt doesn't seem to work either - and I'm not sure how to debug these either. Any println calls are cleared when the tui exits.

can you print to stderr and then do redirection of 2>atuin.log when running atuin?

@conradludgate conradludgate mentioned this pull request May 9, 2021
@yuvipanda yuvipanda changed the title [WIP] Implement 'quick access' via numbers Implement 'quick access' via numbers May 9, 2021
@yuvipanda
Copy link
Contributor Author

Yay, that helped @ClashTheBunny. This works now \o/

Is https://github.com/ellie/atuin/blob/e43e5ce74a85d87a625295b9b089a1b5b8e26fab/src/command/search.rs#L354 how the selected command is passed back to the shell? My redirecting stderr seemed to cause weird things to not work, so am trying to understand the mechanism by which this works.

@yuvipanda
Copy link
Contributor Author

cargo run search -i doesn't seem to work anymore, but if I do a cargo build --release it works fine

@ellie
Copy link
Member

ellie commented May 9, 2021

how the selected command is passed back to the shell?

In an extremely gross + awful way that I could find no alternative for :/

https://github.com/ellie/atuin/blob/de2e34ac500c17e80fe4dcf2ed1c08add8998fa3/src/shell/atuin.zsh#L29

This is needed (afaik) because in order for the TUI to work, it needs to use stdout.

Are you getting any kind of error? cargo run -- search -i works ok for me! The arrow keys might not work properly because the terminal could be in the wrong mode, but otherwise it's fine

@yuvipanda
Copy link
Contributor Author

Hmm, cargo run -- search -i continues to not work for me - if I press enter on a command, it's not actually showing up. Not sure why. I ran cargo clean first as well.

Same as <Enter>
@yuvipanda
Copy link
Contributor Author

I've removed '0' since it's the same as Enter.

@yuvipanda yuvipanda changed the title Implement 'quick access' via numbers Implement 'quick access' via Alt-<n> May 9, 2021
src/command/search.rs Outdated Show resolved Hide resolved
src/command/search.rs Outdated Show resolved Hide resolved
- Use ? operator for getting selected item
- Use RangeInclusive to check if character pressed is a number
@yuvipanda
Copy link
Contributor Author

Very helpful @conradludgate! I've updated the PR

@conradludgate
Copy link
Collaborator

Nice one! Looks good to me, will review properly in the morning

@@ -169,6 +192,16 @@ async fn key_handler(
.map_or(app.input.clone(), |h| h.command.clone()),
);
}
Key::Alt(c) if ('1'..='9').contains(&c) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I've thought of an even better way of doing this :D

Suggested change
Key::Alt(c) if ('1'..='9').contains(&c) => {
Key::Alt('1'..='9') => {

Rust pattern matching is so powerful

Copy link
Collaborator

Choose a reason for hiding this comment

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

If you want to read up more about patterns, see https://doc.rust-lang.org/book/ch18-03-pattern-syntax.html

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@conradludgate if I use the range in the pattern, how do I get access to c?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm. Good point. I think maybe

Key::Alt(c @ '0'..='9')

Copy link
Member

Choose a reason for hiding this comment

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

While this is cool, I think for the sakes of clarity the current approach might be best. Contributors new to Rust (and many not new to Rust) might find it confusing or unclear

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

For now, ignore this. I think I'll go in for a round 2 cleaning up some code later. I also want to start working on a minimum supported rust version and getting that enforced

Copy link
Member

Choose a reason for hiding this comment

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

Just to close this off because I would really like this feature in tonight's release:

In terms of minimum supported version, Atuin will only ever support the latest stable. It might build + run fine with other versions, but I think it's best that we commit to supporting one and only one for the time being. Perhaps that will change in the future.

I'm planning on adding some contributing docs and other such things later

Copy link
Collaborator

@conradludgate conradludgate left a comment

Choose a reason for hiding this comment

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

It works for me. Nice one

@ellie ellie merged commit c02934d into atuinsh:main May 10, 2021
@bl-ue
Copy link
Contributor

bl-ue commented May 10, 2021

This feature doesn't appear to be optional, so it will always be there for users. The demo gif on the README needs to be updated... 🤔

@ellie
Copy link
Member

ellie commented May 10, 2021

@bl-ue Indeed, I can get one of those sorted once the release is out! Unless anyone else wants to ;P

If you'd rather not have this feature then we could add an opt-out? I'd prefer to have it be the default though 😄

@bl-ue
Copy link
Contributor

bl-ue commented May 10, 2021

I don't mind it a bit; it's very convenient. I knew it would sound like I wanted it configurable, but I was merely trying to make it clear that the screenshot is certainly out-of-date, because this feature is always there ;)

@yuvipanda
Copy link
Contributor Author

Feels good that a feature I contributed is helpful :D

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

5 participants