Skip to content
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

Fix issues about long commands #26

Closed
wants to merge 4 commits into from
Closed

Conversation

abitmore
Copy link
Contributor

@abitmore abitmore commented May 4, 2019

This PR fixes a few bugs related to long commands.

Steps to reproduce the bugs:

  1. type a long command, longer than screen/terminal width, so the line is wrapped, do not press enter yet;
  2. press backspace, the first line will show twice (which is unexpected);
  3. press left arrow several times, press backspace or delete key, unexpected behavior;
  4. press left arrow several times, type anything, unexpected behavior;
  5. press enter;
  6. type a short command, press enter;
  7. press up arrow, up arrow, down arrow, down arrow, unexpected behavior.

(can also try commands which are longer than 2 lines.)

The root cause of the bugs is the tty_put('\r') call in reposition(), which doesn't work well when the command is too long. Another issue in reposition() is the use of rl_point in the loop, I don't know whether I'll break anything if change it to rl_end, so in the end I decided to not run into reposition() (as well as clear_line()) at all. The new code is maybe less efficient.

This PR is now ready for review/merge.

@abitmore abitmore changed the title Do not re-position when deleting the last char Fix issues about long commands May 5, 2019
@abitmore
Copy link
Contributor Author

abitmore commented May 5, 2019

I think this PR is ready for merge, please review.

@abitmore
Copy link
Contributor Author

abitmore commented May 5, 2019

I found there is an old line-scroll branch which is related to long commands, the last commit happened 9 year ago, why it hasn't been merged?

@troglobit
Copy link
Owner

I'll have a look and test your PR later tonight, from a first glance it looks great!

Regarding the old line-scroll branch, I really have no idea why I never merged that. But I guess I never finalized testing of it, and then just forgot about it. I'll have a look at that too, later, unless you beat me to it :)

@abitmore
Copy link
Contributor Author

abitmore commented May 5, 2019

Depends on terminal type (I guess, but don't know how to check), I found some strange behaviors when the command is wrapping.

If run BitShares cli_wallet inside tmux, when typing a long command, when the cursor is on the last column, typing one more character doesn't move the cursor to next line although the new character will show; typing one more character again then the cursor will show on next line at the correct position; with this PR applied, if now pressing backspace twice, the cursor behaviors are normal.

If run cli_wallet in a ssh session (from a terminal running in an Ubuntu desktop), when the cursor is on the last column, typing one more character then the cursor disappears;

  • if press backspace now, the cursor will show, but the last char won't disappear (won't be covered with a space);
  • if type one more character again now, then the cursor will reappear on next line at the correct position; pressing backspace once, the cursor behavior is normal; if pressing backspace again (when the cursor is on column one),
    • if applied this PR, the cursor doesn't move to the previous line;
    • without this PR, the cursor disappears but the first line will show again (show twice), so able to edit normally

If run cli_wallet directly from console (virtualbox Ubuntu server), when the cursor is on the last column, typing one more character doesn't move the cursor; typing one more character again then the cursor will show on next line; pressing backspace once, the behavior is normal; pressing backspace again (with this PR applied) the cursor doesn't move to the previous line.

Any idea?

@troglobit
Copy link
Owner

Ah, I'm starting to remember the issues I had with the line-scroll branch. You're definitely onto something with differences in terminal types, that's what haunted me as well. However, I have no really good idea how to solve it at the moment.

Also, without your PR the behavior for me is ugly, but if I press backspace on a too long line I can at least see the cursor return to the previous line -- ignoring the fact that I have ten duplicate lines above mine. With your PR don't get the ugliness with duplicate lines, but on the other hand the cursor doesn't return so I cannot see where I am. Pressing Ctrl-L refreshes the screen and the line though ...

I think what I wanted to accomplish with my line-scroll branch was to truncate long lines, i.e. move the entire contents to fit within the terminal. Displaying $ at beginning and end, to indicate more to the left or right, similar to how Emacs does when in that line-wrap mode.

So, as things are right now, I don't think I can merge your PR.

@abitmore
Copy link
Contributor Author

abitmore commented May 6, 2019

So you encountered the "cursor doesn't return to the previous line" issue as well, probably because the program decided that it should not go back to previous line with the special terminal emulator, so the tty_puts(backspace) call used in the code (actually the tty_flush() call) doesn't move the cursor up. Since tty_flush() finally calls write(), does it mean it's kernel that decided not to move the cursor up? If it's the case, for BitShares project, I guess I can find a way to cheat the kernel. But for editline itself, I think it's good to have a way to set desired behavior, or detect whether write('\b') will work then use a working alternative.

By the way, although the "truncate" behavior would be useful in some cases, personally I don't like it, because it would be inconvenient to copy long commands (with mouse, to another program, which is useful for BitShares). Again, best if the behavior is configurable.

I'll see what I can do.

@troglobit
Copy link
Owner

Having it configurable would be awesome. Thank you for your understanding, and your effort this far, it's very appreciated!

@abitmore
Copy link
Contributor Author

Because behavior of write(stdout,'\b') (which is currently used in editline to move cursor backwards) is terminal-dependent, this PR doesn't always work. Without ncurses library, the only way I've found is to always move cursor by the program itself rather than expecting write() to move cursor to our desired position, however, it perhaps require a major refactory of the code of editline. By the way, (I guess) the ncurses way to fix the \b behavior is to enable bw capacity.

@abitmore abitmore closed this May 13, 2019
@troglobit
Copy link
Owner

Yeah, we don't want to add fully fledged ncurses support to editline. There are other projects for that ...
However, we could always use terminfo and ANSI escape sequences to track the cursor relative to the currently edited line. That was one of the things I was looking to explore with my line-scroll branch.

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.

None yet

2 participants