Make reedline handling cursor shapes more configurable

[cschierig]

This pr is a follow-up to #494. There were various complaints about my implementation being to invasive, so for now I made it toggleable, but I would like not only be able to toggle this, but to actually make it configurable.

If merged, this should resolve #514.

[fdncred]

@CozyPenguin Any updates on this? Do you still plan to finish it or did you run out of time?

[cschierig]

The current implementation already leaves unconfigured cursors untouched, so I don't think there is anything else to do.

If nothing else is missing, I think this is ready to merge.

[fdncred]

ok, thanks for the update @CozyPenguin. we'll need to land this and then update nushell. @sholderbach, thoughts on this pr?

[sholderbach]

Sorry for not paying attention here. Thanks for your efforts @CozyPenguin.
Shoud be a simple way to get this more userfriendly on the nushell side.

Only thing that strikes me as overkill is using a hashmap for like 2 or 3 entries.(but we can fix that later if necessary)

[cschierig]

Yeah the HashMap probably is overkill, I just didn't have a lot of time and that was the simplest solution. I can make this a simple disabled-by-default toggle for now and work on a better way to configure this later if that's ok for you. I think the most important thing is to correct my mistake (enabling it by default) as quickly as possible.

I personally would prefer a proper config implementation right away instead of taking something half-baked that you're not comfortable with long term. The first commit of the two implements the toggle, if you want a temporary solution to disable the cursors.

[cschierig]

I should have some time to work on it this week. Are you fine with a struct containing a field for each mode? That would probably also represent nushells config structure more closely.

[dandavison]

I'm not blocked by this -- I've reverted #494 locally. I just want to ask a question about cursor blinking:

I initially tried changing cursor::CursorShape::Block to cursor::CursorShape::Line as follows:

diff --git a/src/painting/painter.rs b/src/painting/painter.rs
index bace91d..1028916 100644
--- a/src/painting/painter.rs
+++ b/src/painting/painter.rs
@@ -179,7 +179,7 @@ impl Painter {
             .queue(RestorePosition)?
             .queue(cursor::SetCursorShape(match prompt_mode {
                 PromptEditMode::Vi(crate::PromptViMode::Insert) => cursor::CursorShape::Line,
-                _ => cursor::CursorShape::Block,
+                _ => cursor::CursorShape::Line,
             }))?
             .queue(cursor::Show)?;

However when I did that, I noticed that there was a blinking effect where the cursor would pulse for a few seconds after displaying the prompt.

Is that right, do you know the blinking/pulsing effect I'm referring to? Hopefully I didn't imagine it! Anyway, I just wanted to ask - could we have the option to not have the blinking effect, in addition to having the option to not change the cursor type?

[fdncred]

I had to revert this too. It's so aggravating to me. I'm currently using WezTerm on mac and don't see any unexpected blinking pulsing effect.

Actually, I didn't revert it, I just changed it to CursorShape::Line like the diff.

[cschierig]

Anyway, I just wanted to ask - could we have the option to not have the blinking effect, in addition to having the option to not change the cursor type?

That should be possible, but I would like to leave that to another pr and finally get this merged.

[cschierig]

I think this is ready now, but I'm happy to change anything if something's wrong/missing. Now, the behaviour is disabled by default, so it shouldn't bother anyone unless it is enabled in nushell. (and I've changed the config away from a hashmap to a struct). @fdncred @sholderbach

[fdncred]

Thanks @CozyPenguin. We have a nushell release coming up Dec 20th. I'd vote to land this soon after that release unless @sholderbach has objections/changes.

[sholderbach]

Thank you very much for iterating on that. Looks good and tidy with the struct!