Skip to content

feat: immediately execute command #882

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

Open
wants to merge 15 commits into
base: main
Choose a base branch
from

Conversation

mrdgo
Copy link

@mrdgo mrdgo commented Feb 11, 2025

Description

Pass a boolean value immediately_execute to each invocation of get_line.
This way, the nu command commandline edit can accept a flag --accept and not only edit the currently, but also immediately execute the result.

API

The API severely changes, as now this flag has to be passed to get_line and get_line_helper.
This could be changed to a channel, but then the initialization of the module would get more complicated.

Comment on lines -737 to +766
// If the `external_printer` feature is enabled, we need to
// periodically yield so that external printers get a chance to
// print. Otherwise, we can just block until we receive an event.
#[cfg(feature = "external_printer")]
if event::poll(EXTERNAL_PRINTER_WAIT)? {
if !immediately_execute {
// If the `external_printer` feature is enabled, we need to
// periodically yield so that external printers get a chance to
// print. Otherwise, we can just block until we receive an event.
#[cfg(feature = "external_printer")]
if event::poll(EXTERNAL_PRINTER_WAIT)? {
events.push(crossterm::event::read()?);
}
#[cfg(not(feature = "external_printer"))]
events.push(crossterm::event::read()?);
}
#[cfg(not(feature = "external_printer"))]
events.push(crossterm::event::read()?);

// Receive all events in the queue without blocking. Will stop when
// a line of input is completed.
while !completed(&events) && event::poll(Duration::from_millis(0))? {
events.push(crossterm::event::read()?);
}

// If we believe there's text pasting or resizing going on, batch
// more events at the cost of a slight delay.
if events.len() > EVENTS_THRESHOLD
|| events.iter().any(|e| matches!(e, Event::Resize(_, _)))
{
while !completed(&events) && event::poll(POLL_WAIT)? {
// a line of input is completed.
// Receive all events in the queue without blocking. Will stop when
while !completed(&events) && event::poll(Duration::from_millis(0))? {
events.push(crossterm::event::read()?);
}

// If we believe there's text pasting or resizing going on, batch
// more events at the cost of a slight delay.
if events.len() > EVENTS_THRESHOLD
|| events.iter().any(|e| matches!(e, Event::Resize(_, _)))
{
while !completed(&events) && event::poll(POLL_WAIT)? {
events.push(crossterm::event::read()?);
}
}
Copy link
Member

Choose a reason for hiding this comment

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

could you explain the purpose of these changes? it seems like only the if immediately_execute below all this should be necessary to accomplish the change, but not sure what you had in mind.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for the review! Now that I see how github displays the change, I am confused as well. All of this is indentation, except for the if !immediately_execute. I think that github confuses the two instances of events.push(crossterm::event::read()?);.

Copy link
Author

Choose a reason for hiding this comment

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

The point is that I don't want reedline to read from stdin when it is in immediately_accept mode. So I skip all these conditional/looped input reads and further down add the Submit to the queue.

@mrdgo
Copy link
Author

mrdgo commented Feb 19, 2025

I'm so sorry for missing some of the calls. Now, clippy should pass

@VuiMuich
Copy link

VuiMuich commented Apr 3, 2025

Is there anything that could be helped to get this merged?

@mrdgo
Copy link
Author

mrdgo commented Apr 15, 2025

Ping maintainers?

@VuiMuich
Copy link

Well, when I am new to a repo I try to be careful in my interaction as I don't want to annoy the maintainers.
If I am not mistaken you are a first contributor to this project yourself, so I understand why you might be hesitant to drop a ping as well.

Btw: part reason, why I would like to see this getting into the next release is that I recently learned about the brush-shell project, so there is two very interesting shells depending on this.

@132ikl
Copy link
Member

132ikl commented Apr 16, 2025

sorry, this one fell off my radar and i've been very busy recently. i will take another look when i get the chance

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.

3 participants