Let prompts choose to repaint on enter

[ysthakur]

This PR goes towards addressing #348. It adds fn repaint_on_enter(&self) -> bool to the Prompt trait so that a prompt can choose to be repainted on enter. I figured an option could be added to $env.config on the Nushell side to enable a transient prompt. After that, the hooks described in this issue will hopefully start working.

[fdncred]

If reedline is going to support a transient prompt, which would be totally rad, I'd love to see an example in this repo that shows folks how to make a basic one.

[ysthakur]

So it looks like repaint only repaints the prompt, not the input (at least in this particular case, maybe it's just that the input's already been cleared by the time repaint is called here). I tried making the custom_prompt example use ! as a transient prompt and only the prompt shows up, not previously typed text:

[ysthakur]

Okay, looks like the line buffer was being cleared by the time the if prompt.repaint_on_enter() line was hit. I changed it to add the last line from history to the line buffer, repaint, and then remove the last line again. I'm not sure how the history is implemented but hopefully there isn't a lot of overhead getting the last line?

Looks like syntax highlighting works too. The above screenshot is just the custom_prompt example with the highlighter from the highlighter example pasted in. Let me know if it'd be better to make a completely separate transient_prompt example (and restore the custom_prompt example).

[fdncred]

Let me know if it'd be better to make a completely separate transient_prompt example (and restore the custom_prompt example).

Yes, another example would be best. It also has to be confirmed that your change doesn't break anything else or the other examples. Also, since you're pulling the history, you need to make sure it works with text-based history and sqlite-based history, both supported by reedline.

[ysthakur]

So I added some more stuff to the transient prompt example (completer, highlighter, hinter, validator) to confirm that they're working. I assume that if this PR's accepted, all that extra stuff will have to be removed right before this PR's merged?

[fdncred]

I ran this PR example. I'm not sure what it's supposed to do or how to make it work. I just get this

    Finished dev [unoptimized + debuginfo] target(s) in 3.55s
     Running `target/debug/examples/transient_prompt`
Transient prompt demo:
Abort with Ctrl-C or Ctrl-D
(transient_prompt example)>ls
::: a
::: b
::: d
:::
:::
:::

[ysthakur]

My bad, I added a custom validator (line 80) so it only goes to the next line if you end your input with a ? Let me document that later

[sholderbach]

Thanks for giving this a shot and working up an example and doing due diligence on the testing.

From a long term maintenance perspective I think this API is a bit too bug prone and can cause confusion.

[ysthakur]

I ended up going with a fn get_transient_prompt since it didn't require changing any method signatures and only required adding a method to Prompt. It does, however, not make the most sense to have a whole Prompt object to represent the post-submit prompt, so let me know if the render_post_submit_* or some other approach would be better

[ysthakur]

Here's a recording of how it currently looks:

[fdncred]

I was getting ready to object to the unwrap() in the engine.rs, but it appears that we have many unwraps in this code base. Seems like we should fix those eventually. (not in this pr, of course).

[fdncred]

Wait a second, now that I look at these unwraps, most of them are in tests, which is fine. I found 10 that are not in tests that we need to fix. @ysthakur, would you mind adding error handling to the one you have in engine.rs? I really don't think we want to continue adding unwraps in our code.

[ysthakur]

Perhaps if the result of lock() is an Err, it could go back to using the normal prompt and simply log an error elsewhere so that the user won't have to see the error?

The whole Arc<Mutex<Box<dyn Prompt>>> thing is a bit unfortunate, though. Any idea how to get around that so even the unwrap isn't needed?

[ysthakur]

By the way, in case you're planning on getting rid of the unwraps, there's also a bunch of .expect("todo: error handling")s, mainly in engine.rs, that may need to be looked at

[fdncred]

there's also a bunch of .expect("todo: error handling")

good point. sounds like something for a followup pr, if anyone is interested. hint hint. 😆

[ysthakur]

I made another branch to try using a Cell<Option<Box<dyn Prompt>>> to avoid the mutex, but the only way I could think to make it work is a bit hacky: before calling repaint, it removes the Some from the Cell, calls repaint (during which self.transient_prompt contains a None), and then resets self.transient_prompt to a Some containing the original Box<dyn Prompt>. It's unlikely repaint would need the transient prompt, since the only place it seems like it'd be used is submit_buffer, but it might cause bugs down the line anyway.

Edit: I managed to make it not use Cell, but moving the transient_prompt field out while repaint is being called still feels like a hack.

[sholderbach]

If you can get by with this fn (mut self, Box<>) -> Self signature, then I don't think the whole Arc<Mutex<Box<>>> is needed, right?

[ysthakur]

It's not, but self.repaint(self.transient_prompt.as_ref()) requires mutably and immutably borrowing self at the same time, and the only way to get around it that I can think of is by moving the transient_prompt field out of self, replacing it with None, calling repaint, and then moving transient_prompt back into self. That feels like a hack, though. Here's a diff (from a separate branch) showing it.

[ysthakur]

@sholderbach Just wanted to nudge you in case you didn't see my previous comment (also, I just pushed a commit to use Option.take() instead of std::mem::replace)

[sholderbach]

@ysthakur sorry for not getting back to you!

Yes I like the variant with .take(). To me that is pretty idiomatic.

[ysthakur]

@sholderbach Great, just merged that other branch into this PR's branch

[sholderbach]

Thanks for sticking with us!

Let's land this, so the road is clear to figure out the best way to configure this in nushell.

(with https://github.com/nushell/nushell/blob/ba6d8ad2617c2100021d6e3c949c0b9088f2667b/Cargo.toml#L167 you can just cargo update -p reedline to get the latest unreleased version for testing)

[ysthakur]

Thanks for merging this PR, and all the feedback!

[fdncred]

Can't wait to see what this looks like in nushell. It should be interesting. Good work @ysthakur!