Comments (23)
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!
from up.
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 π
from up.
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)
?
from up.
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 π€
from up.
Okay, I'll keep commenting here as and when use up
and miss a binding.
For now, it's just ctrl+w
from up.
@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!
from up.
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! β€οΈ 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!
from up.
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@a760a13 πΊ
So, now I've just got to work out how to implement the slightly harder stuff π€
from up.
Well, I've implemented kill-to-end-of-line (C-k)
for anyone who wants to try it out: cmacrae/up@edeaabe
from up.
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
from up.
@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?
- 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. - (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?)
- (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.)
- Now, for some more concrete "review-ish" stuff:
- I'd like, for key combinations with [Control], to have a
key(...)
incase
, apart from thectrlKey(...)
. 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 whyctrlKey
is just enough? - I think in
KeyCtrlE
case we should uselen(e.value)
instead ofe.lastw
? - I believe
killspace
as used inkill()
is now dangerously reusing the underlying array ofe.value
. As a result, the contents inkillspace
will get overwritten in some cases bye.insert()
. I suppose a solution could be to just usemake
orappend
to manage a separate buffer. Maybee.killspace = append(e.killspace[:0], e.value[e.cursor:]...)
? Or alternatively, explicitly adjusting the capacity ofe.value
, i.e.e.value = e.value[:e.cursor:e.cursor]
? - I believe the
if
inyank
is unnecessary β thefor
loop seems to already work OK for emptykillspace
. - Because
yank
callse.insert
in a loop, it's effectively O(n^2) now. I believe a good idea would be to instead extendinsert
to support taking a variadic list of runes:func (e *Editor) insert(new ...rune)
. With smart use ofcopy
,append
, etc., I think the method could be tweaked to still stay O(n), and still avoid using an explicitfor
loop. As an unexpected but welcome bonus, I believe theyank
method could then be removed, and thecase
forKeyCtrlY
could be simplified to just run:e.insert(e.killspace...)
! :)
- I'd like, for key combinations with [Control], to have a
That's all for now from me. I do like what you're doing here very much!
from up.
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 ofe.lastw
Agreed! I actually wrote it this way initially π
I believe
killspace
as used inkill()
is now dangerously reusing the underlying array ofe.value
. As a result, the contents inkillspace
will get overwritten in some cases bye.insert()
. I suppose a solution could be to just usemake
orappend
to manage a separate buffer. Maybee.killspace = append(e.killspace[:0], e.value[e.cursor:]...)
? Or alternatively, explicitly adjusting the capacity ofe.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
inyank
is unnecessary β thefor
loop seems to already work OK for emptykillspace
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 explicitfor
loop. As an unexpected but welcome bonus, I believe theyank
method could then be removed, and thecase
forKeyCtrlY
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 π
from up.
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?
from up.
@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! β€οΈ 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.
from up.
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
!
from up.
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.)
from up.
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" (scroll down from the link), and compare this with details on how append() works. 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? 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), a.k.a. Shlemiel the Painter's Algorithm. 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.
from up.
Has there been any update on this? Would really love readline movements like Ctrl + W
to delete a word.
from up.
@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!
from up.
Wow, integrating with full fledged Readline lib would be great!
from up.
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!
from up.
Hilariously, this support is just good enough that I keep finding myself hitting
C-d
to delete the character at the point... whichup
interprets aseof
so it boots me out with a completed command printed to the terminal. Means I can't useup
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 ?
from up.
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!
from up.
Related Issues (20)
- add --help option describing usage HOT 2
- Unable to paste text longer than 11 characters HOT 13
- Add delay to unsafe-full-throttle mode HOT 6
- Add tab completion functionality HOT 3
- ZSH integration HOT 3
- terminfo and kitty term HOT 6
- Allow to change colors HOT 3
- Logo time HOT 7
- Use a persistent shell instance, preferably an interactive one HOT 2
- Allow running up with no input HOT 3
- Chinese support got an issue HOT 1
- Make Ctrl-z work
- Make Ctrl-W work to erase the word before the cursor.
- snap package HOT 2
- Safety: ptrace, seccomp, whitelists/blacklists HOT 3
- [idea] diff mode HOT 1
- Input line not scrolling or wrapping when longer than the screen width HOT 1
- Feature Request: Shell Widget
- Run the pipeline up to the cursor only
- Add history
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
π Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. πππ
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google β€οΈ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from up.