Readline movement support

[jonhiggs]

It would be great if up would support my Readline movement commands.

https://www.gnu.org/software/bash/manual/html_node/Bindable-Readline-Commands.html

Thanks.

[akavel]

I agree; but I don't plan to do this myself as of now, as I don't have enough resources for that, unfortunately.

If someone wants to jump in and help, I'm super happy to provide mentoring, design discussion and guidance! :heart: The process will be easiest for us both if you start talking to me here even before you start working on this. Also it's ok to fail - I won't be regretful if you don't complete the task even after we've put a lot of our time into it. I am more than sure we both will learn a lot from this anyway!

[cmacrae]

I'd love to see this! Got started and managed to get the basics implemented:

beginning-of-line (C-a)
end-of-line (C-e)
forward-char (C-f)
backward-char (C-b)

cmacrae/up@a760a13a0eb9f72097a302be19772b5b2a6142af 🕺 So, now I've just got to work out how to implement the slightly harder stuff 🤔

[cmacrae]

Next I'd be looking to implement:

forward-word (M-f)
backward-word (M-b)

Curious if others interested in this feature would also like to see kill-to-end-of-line (C-k)?

[cmacrae]

Well, I've implemented kill-to-end-of-line (C-k) for anyone who wants to try it out: cmacrae/up@edeaabe

[cmacrae]

Aaaaaand with C-k comes C-y: cmacrae/up@e5cc931
This adds the ability to "yank" the contents you last killed back into the buffer at the cursor, as those used to "kill" and "yank" would expect.

I settled on killspace being the name for killed runes to live, as killring, as some may expect, doesn't really convey what it is: a single buffer space with only one potential value, not a ring/collection of previously killed runes.

One note (I don't have time to address this just at the moment, burgers and beer are calling 🍔 🍺 ): I need to implement protection against killing an empty buffer of runes overwriting your previous killspace, so you don't lose previous kills if you C-k again when there's nothing there

[akavel]

@cmacrae Oh, wow, awesome! and man, you're going fast! :D

I've taken the liberty of looking into the code you've linked; I've written some thoughts I had, in case you're interested — in no particular order... or, I think actually "in order of appearance" XD... Hope that's OK for you that I wrote them here?

0. I think killspace as a name is super cool, I like it very much! :) I find it to communicate the intent well and stay short enough.
1. (I have some vague feeling that this starts to feel like it should eventually get moved into some kind of a separate module; but not really sure how to do this yet, and certainly don't want to do this in a bad way. Just doing it in place, as you did, is actually the awesomest approach I can see as of now! I would totally prefer merging it that way than overeagerly doing some module too early. Also absolutely don't mean to push anything on you here; I totally prefer to have your code merged before even thinking about the split; and this thinking may actually not even happen ever. I'm just kinda thinking and pondering loudly here — hope that's OK with you?)
2. (Similar kind of thought for much further future: from the OP's report I kinda seem to vaguely understand that it may be about wanting the key bindings/shortcuts to be configurable, and maybe via .inputrc. This could be an interesting idea for some far future development, too. Though even if so, I'm not convinced if we'd want an actual .inputrc parser. I mean it would be cool, but might show up to be too little bang for the buck. Though worth at least taking a closer look, should that excursion ever happen.)
3. Now, for some more concrete "review-ish" stuff:
   1. I'd like, for key combinations with [Control], to have a key(...) in case, apart from the ctrlKey(...). That's because I'm not actually sure what's the difference, and I'd prefer to be safe than sorry here. Unless you know for sure that we don't need this, and could show me some good reference explaining why ctrlKey is just enough?
   2. I think in KeyCtrlE case we should use len(e.value) instead of e.lastw?
   3. I believe killspace as used in kill() is now dangerously reusing the underlying array of e.value. As a result, the contents in killspace will get overwritten in some cases by e.insert(). I suppose a solution could be to just use make or append to manage a separate buffer. Maybe e.killspace = append(e.killspace[:0], e.value[e.cursor:]...)? Or alternatively, explicitly adjusting the capacity of e.value, i.e. e.value = e.value[:e.cursor:e.cursor]?
   4. I believe the if in yank is unnecessary — the for loop seems to already work OK for empty killspace.
   5. Because yank calls e.insert in a loop, it's effectively O(n^2) now. I believe a good idea would be to instead extend insert to support taking a variadic list of runes: func (e *Editor) insert(new ...rune). With smart use of copy, append, etc., I think the method could be tweaked to still stay O(n), and still avoid using an explicit for loop. As an unexpected but welcome bonus, I believe the yank method could then be removed, and the case for KeyCtrlY could be simplified to just run: e.insert(e.killspace...)! :)

That's all for now from me. I do like what you're doing here very much!

[cmacrae]

Hey @akavel 👋

Thanks for your encouragement and general positivity 😁 And of course; thanks so much for up 🤗 I've been using it almost every day since seeing your post on lobste.rs

Hope that's OK for you that I wrote them here?

Absolutely, it's fine to chat about here, makes sense.

I totally prefer to have your code merged before even thinking about the split; and this thinking may actually not even happen ever. I'm just kinda thinking and pondering loudly here — hope that's OK with you?

Of course!

I'm not convinced if we'd want an actual .inputrc parser. I mean it would be cool, but might show up to be too little bang for the buck. Though worth at least taking a closer look, should that excursion ever happen

I agree, could definitely be cool. I think the way OP's request came about was just that it felt natural/expected to be able to navigate in up's buffer using readline movement - it's certainly what I came here to ask for and was delighted to see that someone else had already asked. So, with that said, in my opinion a good basis for approach: implement keybindings people generally/reasonably expect, then maybe look at an .inputrc implementation if it becomes clear people want to do "fancier" things ✨

Now, for some more concrete "review-ish" stuff

This is the stuff I'm here for 🙌 I'm a novice at all this, I'm keen to learn from your insights. So please; do feel free to speak as openly/candidly as you wish

Unless you know for sure that we don't need this, and could show me some good reference explaining why ctrlKey is just enough?

Only reason I settled on ctrlKey is because key didn't seem to work as I expected 🤔 I tried key(tcell.KeyCtrlA) - for example.

I think in KeyCtrlE case we should use len(e.value) instead of e.lastw

Agreed! I actually wrote it this way initially 😅

I believe killspace as used in kill() is now dangerously reusing the underlying array of e.value. As a result, the contents in killspace will get overwritten in some cases by e.insert(). I suppose a solution could be to just use make or append to manage a separate buffer. Maybe e.killspace = append(e.killspace[:0], e.value[e.cursor:]...)? Or alternatively, explicitly adjusting the capacity of e.value, i.e. e.value = e.value[:e.cursor:e.cursor]?

Ah, very good point. I hadn't considered this. I like and understand your first example solution (e.killspace = append(e.killspace[:0], e.value[e.cursor:]...)), but don't understand the second, to be honest (e.value, i.e. e.value = e.value[:e.cursor:e.cursor]). Perhaps you could explain?
Would that just be so we don't potentially clobber e.killspace when using e.instert? 🤔

I believe the if in yank is unnecessary — the for loop seems to already work OK for empty killspace

Oh cool, okay. I'll give that a try

Because yank calls e.insert in a loop, it's effectively O(n^2) now

Gotta be honest, I don't understand this... haha. Could you take a moment to explain?

A good idea would be to instead extend insert to support taking a variadic list of runes: func (e *Editor) insert(new ...rune)

Excellent point. I haven't had a chance to use variadic arguments yet, so hadn't considered it. Thanks!

With smart use of copy, append, etc., I think the method could be tweaked to still stay O(n), and still avoid using an explicit for loop. As an unexpected but welcome bonus, I believe the yank method could then be removed, and the case for KeyCtrlY could be simplified to just run: e.insert(e.killspace...)! :)

Great! I'll see if I can wrap my head around this and get something working 😁

[cmacrae]

Oh, and before I forget, I had another idea for a feature that I feel fits in nicely here: the ability to "navigate" to previously run pipelines. Much like ⬆️ and ⬇️ (or C-p & C-n) in a shell allow you to get to your previously run command, it'd be nice to have in up.

For instance: I have some JSON I'm forming a jq pipeline around. I'm happily chipping away through the data to get what I want, and in my next Return I make a mistake. Now, my output in the buffer (the JSON I'd been narrowing down) has been replaced by jq's error output.

Rather than having to then navigate back through my pipeline with the cursor and manually deleting the bad part of the pipeline to get back to where I was, I could just hit ⬆️ (or C-p) to get my previous pipeline, hit Return, and I'm right back where I want to be to re-think my next step.

Hope that makes sense?

[akavel]

The idea for "history navigation" sounds interesting, and I feel like it could maybe actually kinda accidentally solve #4?... (!) Maybe the results from a few old runs could also be stored gzipped in memory?... just a random thought, not necessarily a good idea.

(part of the comment was hidden)

**Re: ctrlKey() vs key()** I don't understand `key` vs `ctrlKey` either, esp. for all the KeyCtrlXYZ keys, and for me it was the same that only one of them works, but I prefer to just list them both to be "better safe than sorry". **Re: killspace overwritten by insert()** About `e.value[:e.cursor:e.cursor]`, you can see [Go specification, subsection "Full slice expressions"](https://golang.org/ref/spec#Slice_expressions) (scroll down from the link), and compare this with details on [how append() works](https://golang.org/ref/spec#Appending_and_copying_slices). Changing e.value's cap would ensure that any future append(e.value) would allocate new underlying buffer, avoiding overriding killspace. If you feel the first approach is more readable, then that would make it the preferred choice here. Actually now that I look at it, the first one seems indeed more explicit/straightforward about the intention. **Re: *O(n^2)* in yank** Do you know the ["big O" notation](https://rob-bell.net/2009/06/a-beginners-guide-to-big-o-notation/)? That's the first question I need to ask, to know how to try answering you, as your "don't understand" here is very broad, so not sure what exactly you do vs. do not understand, sorry :) Trying to explain quickly: - e.killspace contains many bytes — specifically: *many1 = len(e.killspace)*; - yank() has a `for` loop, where it calls insert() *many1* times; - then, in insert(), there's a `copy(e.value[...], e.value[...])`, which will copy *many2* bytes (where *many2 = ~len(e.value)* ) — in other words, a "copy byte in memory" CPU operation (let's call it "MOV" to simplify) will be called *many2* times. Given that yank() calls insert() *many1* times, then each insert() calls MOV *many2* times, it means that in grand scheme of things, yank() will call MOV _many1 * many2_ times, a.k.a. _many ^ 2_ times, a.k.a. [_O(n^2)_](https://rob-bell.net/2009/06/a-beginners-guide-to-big-o-notation/), a.k.a. [Shlemiel the Painter's Algorithm](https://www.joelonsoftware.com/2001/12/11/back-to-basics/). We can do better here, there's no need to use the `for` loop. I can show you how if you want, or try to give you some more hints, or you can still try to find it yourself. I don't want to spoil too much fun of discovery for you here too fast ;) And by the way, we could actually just make insert() take a slice for that, but varargs will be much prettier and more appropriate here.

[akavel]

@cmacrae I didn't want to risk losing your contribution to neglect, so I took the liberty to merge your branch now, plus the tweaks/fixes I suggested. Sorry for not being more patient :D and thank you so much! :heart: I've released a v0.3.2 with those improvements. Also, I will now hide some comments in the thread which I feel are not very important for the main discussion here.

[cmacrae]

Thanks so much @akavel! I've been very busy over the last couple weeks, and a few busy weeks ahead of me. So I may be a bit quiet around this, but I hope to get some time over the holidays to tackle some of the points we've discussed 🤗

[dufferzafar]

Has there been any update on this? Would really love readline movements like Ctrl + W to delete a word.

[akavel]

@dufferzafar Could you try and add a prioritized list of what specific readline commands are the ones you miss the most? There's a lot of them, some may be tricky, so I don't expect all of them will be ever added to up; if you could list the ones you'd love most, me or someone else would have a better idea of what to focus on when trying to help you!

[dufferzafar]

Okay, I'll keep commenting here as and when use up and miss a binding.

For now, it's just ctrl+w

[cmacrae]

Oh hey! That’s so strange, I was actually thinking about this just yesterday and recalling the other parts I wanted to implement!

I’d love to get the stuff I mentioned above in early comments implemented, so I can perhaps take a look at getting all this rolling again 😊

[akavel]

@cmacrae ❤️ Just one thing to note: I started testing Ctrl-W in bash (I never used this shortcut before), and from what I see, it seems somewhat tricky. It would be awesome if you guys could try and collect a corpus of test cases, with expected input & output, to verify it does what we want... I wouldn't see much sense in adding a Ctrl-W that would work very differently than in bash/readline... it's very much OK if you just collect them and not write actual unit tests, I will then try to add the actual tests when merging. For example, assuming | marks cursor, IIUC sample tricky cases could look like:

old: `hello  my   |world`
new: `hello  |world`

old: `hello  my|   world`
new: `hello  |   world`

Though if you wish to add actual tests code too, you're certainly more than welcome! :) you can take a look at the existing (single.... :/) test, for some inspiration, if you like!

[cmacrae]

Hey @akavel! 👋 Yep, definitely agree it's important we accurately recreate any expected behaviour. I'll see what I can do in ways of testing/ensuring consistency.

One thing that could work is using this lib: https://github.com/chzyer/readline
I'm going to try and plug it in this morning and have a play around. If that works, it seems a lot of this will be solved for us!

[dufferzafar]

Wow, integrating with full fledged Readline lib would be great!

[dwurf]

Wow, integrating with full fledged Readline lib would be great!

Agreed. I use readline vi mode so it's really jarring switching between vi keys in bash/python/sqlite/... and emacs keys in up!

[NightMachinery]

I also think that it should be possible to just integrate with the tested-and-true readline library. Another idea is to somehow integrate it with a running neovim (I think this too should be possible, but readline still seems the more promising approach to me.)

[saulrh]

Hilariously, this support is just good enough that I keep finding myself hitting C-d to delete the character at the point... which up interprets as eof so it boots me out with a completed command printed to the terminal. Means I can't use up in its intended role, since it keeps losing the stream I wanted to work on, and I have to go back to shoving it into something in /tmp anyway!

[akavel]

Hilariously, this support is just good enough that I keep finding myself hitting C-d to delete the character at the point... which up interprets as eof so it boots me out with a completed command printed to the terminal. Means I can't use up in its intended role, since it keeps losing the stream I wanted to work on, and I have to go back to shoving it into something in /tmp anyway!

@saulrh Hmmm; but where does the C-d as non-EOF come from? Is it an Emacs thing? From my experience, in bash the C-d is typically interpreted as EOF/logout, so I'm sincerely quite surprised it can be treated as something else. For example, personally I'm quite wary of accidentally hitting C-d on a Linux machine now. But I'm no expert at readline, so I wonder where and how it is also used such that C-d means delete?

As for UP, given the above, I think the only reasonable way around this would be to allow full customization of keys in the app; but that's not something I would expect to do especially soon, seeing how I have something of a writer's block with regards to UP now, unfortunately 😢

That said, for your personal purposes, you should be fine with just downloading the source (no need to even git clone at this point, it's still a single Go file as of today!), removing the ~2 lines related to C-d, and compiling it with go build up.go. Would that work for you @saulrh ?

[saulrh]

Hmmm; but where does the C-d as non-EOF come from?

It's a thing that's common to readline and emacs, yeah. bash and company interpret it as EOF when it's the only thing on a line, but when the line isn't empty it's interpreted as "delete character at point". Not sure what the first system to use it like that was.

And agree, this would probably require either a) a real customization system or b) replacing the homegrown line-editor with a "real editor", something like readline or nvim --embed.

Would that work for you @saulrh ?

It would. Thanks for the suggestion!