nushell
feat: support external event queue
This helps nushell to implement commandline edit --accept, the same thing that zle accept-line does for zsh.
See this issue.
This looks interesting. Thanks! Generally, we'd like to not have unwrap() as much as possible. I'll have fun trying this out later.
Is there another immediate use for this kind of event injection besides for https://github.com/nushell/nushell/issues/11065 / https://github.com/nushell/nushell/pull/13728 ?
If there is a simpler solution with clear semantics that is easy to document/test I would prefer that to trying to be too general and drilling more holes into the internals. (e.g. now the ordering in the event loop could become load bearing to some behavior a library user may come up with, bikeshedding of Arc<Mutex<Vec<E>>> vs channel)
If I understand the goal of running the output from Nushell commandline immediately this could be achieved in a variety of ways. The requirement could be boiled down to a binary choice/enum. When commandline is executed we are outside of Reedline::read_line modifying the buffer, so if I grok it right there is not even a need to synchronize across threads from reedlines perspective.
Is there another immediate use for this kind of event injection besides for nushell/nushell#11065 / nushell/nushell#13728 ?
Fascinating question. I also thought about this. And honestly, probably not.
If there is a simpler solution with clear semantics that is easy to document/test I would prefer that to trying to be too general and drilling more holes into the internals. (e.g. now the ordering in the event loop could become load bearing to some behavior a library user may come up with, bikeshedding of
Arc<Mutex<Vec<E>>>vs channel)
I feel like, I don't fully understand what you mean. Reedline is single-threaded, right? Then the lock will be trivial to acquire. Arc<Mutex<X>> is the only way to have something like shared memory. I tried using a channel, but I struggled to make it work. I you wish, I would give it another try.
Something else, I figured after implementing: maybe I don't need the two queues. Currently, there is the local queue and a shared queue. Whenever the local queue is read, I append all the events from the shared queue beforehand. Maybe having one single queue at the first place is also a cleaner implementation?
If I understand the goal of running the output from Nushell
commandlineimmediately this could be achieved in a variety of ways. The requirement could be boiled down to a binary choice/enum. Whencommandlineis executed we are outside ofReedline::read_linemodifying the buffer, so if I grok it right there is not even a need to synchronize across threads from reedlines perspective.
Yes, that is the goal. And yes, I also don't see a need for synchronization, except that Rust requires a Mutex to allow the lifetime and access from different locations, basically a borrow check-proof shared memory. Feel free to propose an alternative implementation and I will give it a shot - I am a bit impatient about the feature and eager to implement it.
I found an additional potential use case for this feature: sudo !! currently only expands the command to sudo <previous command>. With this feature, we could add a configuration option to also instantly execute the new command - currently we have to enter twice. It's related, maybe not even different enough.
Check out my PR over at nushell. Basically check out mrdgo/nushell, cd nushell; cargo run and then commandline edit --accept "print 'hello'"
it seems to work for "print 'hello'" but not for "!!" or any of the other ! commands.
Because I didn't implement it for the !-commands yet. I'd do that in a separate PR, because I'd prefer to have it as an opt-in and accordingly add a config option
I added the two lines of code that immediately accept the bashisms. What are the maintainers' opinions on this? Obviously, a breaking change. But is it intended that we have to enter twice to accept a bash-expansion? Should this be configurable? I feel like an immediate accept is very sane default behavior.
OK looking at the way commandline works currently (and also your interim proposal for the change to the ! expansions) I still think there is no current need for a whole separate event queue. I feel that an overly broad public interface like that invites spooky action at a distance or unforeseen states that are harder to reason about.
This could be a simple bool execute_immediately, with an associated method to set it on the Reedline instance or something entirely happening in the nu-cli/EngineState machinations.
Looking at the curent requirements:
- Only required event
Submit-> no need for a fullReedlineEventqueue, if it is just countingSubmits - It would only ever execute one event in the current use with
commandlineandSubmit(with potential for what happens if more events got enqueued) -> just an (atomic) bool - There is no need for
Send/Synclogic because of the pedestrian implementation ofcommandline(I would rather be concerned if implementers ofCompleterorMenuwrongly think they would through such a channel get access to an event queue to influenceReedlines internal execution, but its order is not determinate or not really sensible.)
I don't particularly like how commandline is implemented and interacting with reedline but I am still inclined to say that the case for this kind of event queue is not particularly strong even if we come up with a better API for commandline shenanigans.
Nushell commandline itself never communicates in parallel with Reedline execution, everything there is happening on the engine state.
Hooks are executed only by nushell and if a ExecuteHostCommand keybinding is reached the Reedline instance yields from read_line as if a buffer content were submitted to the normal REPL loop. -> commandline never executes its code while Reedline has exclusive access.
Buffer content and cursor position is read before the hook evaluation through two methods on Reedline and then moved into the engine state.
The writing of new buffer content coming from commandline executions is managed rather hackily by running a few edit commands in the REPL loop. (This latter part is still a poor design to deal with illegal cursor positions.)
All of this is currently accomplished without shared state with reedline but rather happening on EngineState.
I am open to a better API on reedline's side to make synchronization within the REPL less of a careful dance, but I think this should choose its public API to the clear requirements from commandline instead of providing overly general access before it is actually needed.
Thank you for your thorough introduction to the overall architecture. Now this is much clearer and I understand, why this is a bad implementation.
As soon as I am done with my contributions over at tree-sitter-nu, I'll come back to this and incorporate my new insights into this implementation :)
Just making this draft until we get back around to it. Thanks for working on this.