Implement cursor

[b3nj5m1n]

This PR implements a cursor, meaning you can move around in the input field, delete or add chars in the middle, etc.

Demo

Changes

I added a field called cursor_index to the State struct. Every function interacting with the user input now has to use this value, so it's not a trivial change.

I added the following keybinds:

• Control + w (delete back one word)
• Control + a (go to the beginning of the line)
• Control + e (go to the end of the line)

These are the default bash keybinds for these operations, adding more should be fairly simple.

I also modified what I believe to be all places in the search.rs file affected by this, namely the other keybinds and the draw functions.

Problems

Arrow keys not recognised properly

For some reason, the Key::Right and Key::Left keybinds don't do anything. I looked at the termion docs, and it seems to be the correct value for the right and left arrow keys, but the events don't trigger for some reason. The up & down keys seem to work just fine.

For now, I did all my testing using the alternative vim-style keybinds. (Control + h/l)

Text encoding

The input is a String, so it supports unicode. My current implementation only works properly with ASCII text.

I think unicode should be supported, which is why I'm creating this as a draft PR.

I don't think making it work with unicode would be too hard, but it might be a bit of a hassle, so I want to make sure this is something you're interested in merging first before working on it.

Fixes

This would close #410

[conradludgate]

What's even harder than just utf8 support is grapheme cluster support. Eg an emoji can be made up of multiple unicode scalar points, so when you press back and scan through to find the previous char boundary, that would be in the middle of an emoji

What's worse is that you can't tell how multiple emojis might interact since all terminals/OSs might be supporting different emojis 😅

[conradludgate]

This does definitely seem nice to have though, thanks for the effort. We do need to figure out the left/right keys.

Have you been testing it by running atuin search -i or by using your shell binding (ctrl-R or up arrow)

[b3nj5m1n]

Have you been testing it by running atuin search -i or by using your shell binding (ctrl-R or up arrow)

I've only been testing it using search -i so far, I just tried installing it and when invoking it with control + r or up, the arrow keys work perfectly fine.

The up & down keys seem to work just fine.

I'm not sure if I just misremembered yesterday, or if that broke in the meantime, but when invoking with search -i, none of the arrow keys work. Everything works as expected when invoking with control + r or up though.

[b3nj5m1n]

What's even harder than just utf8 support is grapheme cluster support. Eg an emoji can be made up of multiple unicode scalar points, so when you press back and scan through to find the previous char boundary, that would be in the middle of an emoji

Shouldn't the chars() iterator on strings take care of that? My idea was using that, I can probably try it out sometime in the evening today.

If that solution wouldn't work, let me know if there's any alternative.

[ellie]

Ohhh awesome thanks for all the work here!

when invoking with search -i, none of the arrow keys work. Everything works as expected when invoking with control + r or up though.

Yup this will be because of the terminal mode

If you look in our ZSH bindings, we change that: https://github.com/ellie/atuin/blob/main/src/shell/atuin.zsh#L34

Depending on what mode the terminal is in, programs are sent different escape sequences for different characters. Some applications can be a bit messy and change things without changing them back. It was a nightmare to debug when I was first writing this 😂

So long as it works when called from the bindings + when in the correct terminal mode, it's all good.

I can probably try it out sometime in the evening today.

Definitely interested in the results of this one 🙌

[b3nj5m1n]

I only managed to do a bit of testing, but it seems like 8a65f41 implements proper unicode support.

Demo

I added a couple of functions, hopefully that's okay with you.

Would be great if you could do a bit of testing yourself and get back to me!

[conradludgate]

Can you test with a larger width emoji? For instance '👩‍👩‍👦'

See https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=8c71737da77c80a427ae6af6959fb397 for what I mean.

This is an edge case though and I'll be happy to ignore those for the sake of this change

[b3nj5m1n]

That breaks it a little, but it doesn't seem to crash an least.

I can try to get it working properly, but no promises.

What would the desired behaviour be in this case? Deleting the whole emoji in one go, or deleting the individual emojis making it up, like in firefox, for example?

[conradludgate]

I would not be so worried about making it 'correct'. Whichever you choose I think could be argued as unintuitive. This also ignores that not all setups will even render these multiple emojis together (or not be updated in future)

[ellie]

Yup I think either way of doing it would be fine, most terminals are unlikely to render things correctly anyway

I'd be happy to revisit this in the future should it become a problem, but if that's the only edge case atm then it looks good to me 😁

[b3nj5m1n]

I tried out an alternative approach, and it didn't fix the problem. (It actually made it worse)

If you're okay with these edge cases not being handled properly for now, I think this is probably the best compromise.

Let me know if there's any other changes you want me to make!

[ellie]

I'm happy with this! I don't think the fancy emoji are used often enough for it to be a problem - I doubt many people have a terminal setup that renders them right anyway

Should it be a larger problem we can revisit in the future

Thank you so much for the work here, this is great 🚀

[ellie]

@b3nj5m1n also, do you mind if I use one of your GIFs in the next release?

[b3nj5m1n]

@b3nj5m1n also, do you mind if I use one of your GIFs in the next release?

Sure, that'd be great.

---

Cippy has been failing this entire time because the function for handling keys has too many lines, what would you like to do about that? I had tried to use more compact syntax before, but then fmt was complaining. The only thing I could think of would be to separate all the events into their own functions, but there's not really a point to doing that except for pleasing clippy.

Alternatively, we could also add #[allow(clippy::too_many_lines)] to the function.

Just let me know what you would prefer!

[b3nj5m1n]

@ellie Pinging in case you missed my last comment.

[ellie]

Sure, that'd be great.

Thanks!!

Alternatively, we could also add #[allow(clippy::too_many_lines)] to the function.

I think in this case I'd be happy for us to use this approach

[b3nj5m1n]

Should be good to go now!

[ellie]

Nice work, thank you! Great to get this in 🚀

[b3nj5m1n]

Awesome, thank you so much!