Skip to content
This repository has been archived by the owner on Apr 19, 2024. It is now read-only.

Correct cursor location when terminal wraps lines #353

Closed
wants to merge 1 commit into from

Conversation

ecordell
Copy link
Contributor

@ecordell ecordell commented May 8, 2021

Before this change, the cursor location would be wrong if a line entry was wrapped by the terminal (no newlines). It made editing a wrapped line impossible (cursor could only move to the beginning of the lowest line), and caused future writes to go to the wrong location, over top of the input.

Here's an example before the change:

Screen Shot 2021-05-07 at 10 23 05 AM

And after:

Screen Shot 2021-05-07 at 9 24 52 PM

(the input shows wrapped when entering, and collapses down to one line once it's entered)

I didn't see a test suite that seemed appropriate to update with these changes, but I tested the basic editing functionality manually (on macos, with iterm2) and that the special keys (home, end, delete, fwd delete) work as expected.

@AlecAivazis
Copy link
Owner

Thanks for taking the time to put this together @ecordell. I'll find some time in the coming days to test this out. As usual with PRs that modify the printing logic of cursors, I would like to get some extra eyes looking at it aswell.

cc @coryb @mislav

@AlecAivazis
Copy link
Owner

Also, it's pretty exciting to see that RedHat is using survey for their needs.

I'm curious if you would mind being tagged in PRs that affect cursors? One of the biggest challenges for me in maintaining this library is balancing fixes and new features against the needs of the large companies that use it. If you wouldn't mind lending a pair of eyes, it would help everyone in the community.

@AlecAivazis
Copy link
Owner

Finally found some time to pull this change down and test it out on my windows and mac machines - it seems to work as intended. I'm going to give the others some more time to test this out but it should be merged soon.

Thanks for being patient!

@AlecAivazis
Copy link
Owner

I'm not sure why the tests aren't running - do you mind closing and resubmitting this @ecordell ?

@AlecAivazis
Copy link
Owner

Closing this in favor of #360 so I can push it through myself

@AlecAivazis
Copy link
Owner

@ecordell - this is now released under v2.2.13

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants