Tab Completion Candidate Pager for when candidates don't fit on a single page

[tpodowd]

NB: This PR includes PR202 (https://github.com/chzyer/readline/pull/202) which is not yet merged. PR202 addressed various redraw issues with wide characters, edge of screen, multiple lines, masking etc. The main feature of the pager builds on these changes and redraw code so, I had to include them in this PR.

Main Features:

• If there are too many candidates returned by the completer, completeMode and completeSelectMode did not work properly. Similar to the bash completion pager, list candidates and offer "--More--" on the end of each page. User can select " ", "y" or "Y" to keep listing or "q", "Q", "n", "N" to stop listing. When paging completes, we also exit out of completionMode.
• Added aggregate completion when entering completeSelectMode where the candidates dwindle down sharing a larger common prefix. This makes typing a little faster than having to select. It's a more bash-like behaviour.

Other Fixes:

• Fix various crashes where candidates are too wide for the width of the screen and causes division by zero.
• Fix crash with wide (Asian characters) in completion.
• Streamline redrawing as CompleteRefresh was called too often.
• Fix crashes around ctrl-a and ctrl-e in select mode when candidates don't fit on a line
• Fix prev/next candidates in select mode when candidates don't fit on a line
• Fix crash when ctrl-k was pressed in select mode. This caused us to exit completeSelectMode which cleaned up all the data but left us in completeMode such that if CompleteRefresh was called directly, the data was not initialised.
• Fix complete and select mode redraw issues when candidates did not fit on one line.
• Fix cursor position issues after CompleteRefresh especially if the prompt and buffer also went over 1 line.
• Fix redraw issue where exiting completion mode using certain key presses leaves candidates on the screen.

Fixes for Windows:

• Use window size for visible height/width instead of buffer size
• Adjust for Window's EOL behaviour.

Notes:

• Added Height info to different structures as the decision to page or not required height information.
• Added OnSizeChange(). Didn't know if I could get rid of the OnWidthChange()? Would be nice to remove the Width stuff and just have Size (width + height info).

I tested this patch on Linux and Window and it seems to work well.

[wader]

Did some testing with fq master on macOS and did not notice anything strange

If there are too many candidates returned by the completer, completeMode and completeSelectMode did not work properly. Similar to the bash completion pager, list candidates and offer "--More--" on the end of each page. User can select " ", "y" or "Y" to keep listing or "q", "Q", "n", "N" to stop listing. When paging completes, we also exit out of completionMode.

Seems to work fine

Added aggregate completion when entering completeSelectMode where the candidates dwindle down sharing a larger common prefix. This makes typing a little faster than having to select. It's a more bash-like behaviour.

Not sure i understand how this works. Can you give an example somehow?

[tpodowd]

Thanks for having a look at this @wader .

Added aggregate completion when entering completeSelectMode where the candidates dwindle down sharing a larger common prefix. This makes typing a little faster than having to select. It's a more bash-like behaviour.

Not sure i understand how this works. Can you give an example somehow?

Sure. Let's say you are completing file names and you press tab, the completer returns:

afile anotherfile filename.1234 filename.1456

Old Behaviour:

• We are in complete mode and 4 files/candidates above are displayed.
• press f
  • Screen displays: prompt> f
  • completer now returns filename.1234 and filename.1456 and they are displayed beneath.
• press TAB
  • Screen displays: prompt> f
  • And enters selectmode where you can select between the 2 files.
  • if there were more choices and you didn't want to select them, but rather just type to complete, you'd end up typing everything unless you exit complete mode and then tab into it again.

New Behaviour:

• We are in complete mode and 4 files/candidates above are displayed.
• press f
  • Screen displays: prompt> f
  • completer now returns filename.1234 and filename.1456 and they are displayed beneath.
• press TAB
  • Screen displays: prompt> filename.1 (the common prefix of both is completed) and bell is rung to indicate partial completion.
  • Then you either press tab again and/or keep typing to finish.

[wader]

Sure. Let's say you are completing file names and you press tab, the completer returns: ...

Aha thanks for detailed explanation. Was a bit hard to find a good test case with fq but now i see and it works and feel natural i think, nice improvement 👍

[tpodowd]

Cool. Thanks for checking @wader . Hope you enjoy the patch :-)

[tpodowd]

@chzyer - let me know if you have any questions on this one either. Hope you find some time to review. I think this will also fix off some of the other open issues/prs.

[wader]

Noticed a draw issue while testing. Seem to also happen with only #202 but does not happen with readline chzyer master.

Reproduction:

# run fq repl
$ go run fq.go -i
# press "r" then tab to see completions, now press enter to eval just "r"
# options are not cleared before retuning from readline, error message and prompt
# ends up mixed with completion options
null> r
error: expr: function not defined: r/0  remainder repeat    repl
null> e   rindex    rint      root      round     rpad      rtmp
rtrimstr

With readline master the options are cleared before returning.

[tpodowd]

Hi @wader - awesome. Thanks for the feedback. I have replicated it locally also. I will fix this either this weekend or Monday as time allows.

[wader]

@tpodowd Great 👍 no stress

[tpodowd]

Hi @wader - I have added a commit that fixes this issue. Please have a look and let me know if it works for you. I was able to replicate the issue locally by adding some dummy completion candidates to my code and this fixed it so I believe it will work for you also.

[wader]

@tpodowd Works great 👍

[tpodowd]

Thanks for the update @wader . Let me know if you notice anything else. Enjoy.

[wader]

Yeap will do!

[wader]

@tpodowd Hey again, hope everything is good. I started to experiment with multiline input in fq but notice that one issue is that having \n in a readline string works a bit weird, once you start typing the prompt will start to feed way. Any idea if it would be possible to support?

Another issue is that history files at moment uses \n are separator.. but that probably "just" need some history API changes to workaround.

[wader]

To clarify the use-case for me would be to support things like writing starting quote " and the past text that include "real" new lines (and there is code to understand that there is a unterminated string) and then write an ending ".

[tpodowd]

Hi @wader - sorry I've been traveling. Finally back. I don't actually use the multiline input in my use case. Not sure exactly what you mean by the prompt will start to feed away. Was this an issue before the changes also, or only after this PR? Probably possible to fix things I would imagine. Just need a small example and some keys to type to help me replicate and understand. Is it possible to replicate the issue with any of the examples, perhaps example/readline-multiline?

[wader]

Nice and hope the travels were good! No worries and maybe I should have been more clear this is probably more of a feature request than a bug, and I don't think your changes broke anything.

Think i might have a reproduction using the multiline example. The idea in fq is to have something similar to how shell readline can do multiline by opening a quote " or ' and press enter and do multiple lines and close it.

diff --git a/example/readline-multiline/readline-multiline.go b/example/readline-multiline/readline-multiline.go
index 2192cf6..090753f 100644
--- a/example/readline-multiline/readline-multiline.go
+++ b/example/readline-multiline/readline-multiline.go
@@ -32,7 +32,7 @@ func main() {
                        rl.SetPrompt(">>> ")
                        continue
                }
-               cmd := strings.Join(cmds, " ")
+               cmd := strings.Join(cmds, "\n")
                cmds = cmds[:0]
                rl.SetPrompt("> ")
                rl.SaveHistory(cmd)

And then run:

$ go run example/readline-multiline/readline-multiline.go
> line1
>>> line2
>>> line3;
line1
line2
line3;
# press arrow up and you get multiple lines as input
# press arrow left to try to go back into the multiline input
# the lines start to scroll up

[tpodowd]

Hi @wader - I see. I'm currently working on other projects not related to readline which will keep me busy for a while. Since this is unrelated to this PR, perhaps create an Issue for it instead. Maybe someone will get to work on it before I do. I wouldn't mind the ability to split input across multiple lines using " or ' similar to how bash readline works, but my project that uses readline is currently stable so have moved off it for a bit. If a bug pops up I'll have a look at that though. Feel free to reach out over mail if you want to run anything by me in the meantime.

[wader]

No worries! and I agree, let's create a new issue for it. Will probably do it later today or so, should probably also include some things about that it will affect how history works.

[wader]

Filed an issue https://github.com/chzyer/readline/issues/212

[tpodowd]

This PR has been rebased on top of PR202 again (PR202 was rebased on top of the latest master and one other commit was added to fix a race condition it introduced). So, as of now, d9af567 and 586d8ee are from PR202 and the 3 other commits are unique to this PR.

The first 2 commits e4b415f and 4fda9f0 are exactly the same as before. I added one new commit which fixes a race condition when the terminal is resized. Note that this race condition also exists in master. The commit message explains it. I'll paste it below also:

Fix terminal resize race conditions

The runebuf, search and complete data structures all maintain their own cache of the terminal width and height. When the terminal is resized, a signal is sent to a go routine which calls a function that sets the new width and height to each data structure instance. However, this races with the main thread reading the sizes.

Instead of introducing more locks, it makes sense that the terminal itself caches it's width and height and the other structures just get it as necessary. This removes all the racing.

As part of this change, search, complete and runebuf constructor changes to no longer require the initial sizes. Also each structure needs a reference to the Terminal so they can get the width/height. As the io.Writer parameter is actually the terminal anyway, the simplest option was just to change the type from the io.Writer to Terminal. I don't believe that anyone would be calling these functions directly so the signature changes should be ok. I also removed the no longer used OnWidthChange() and OnSizeChange() functions from these three structures.

[wader]

Hey, thanks for keeping this one rebased. Will probably do a new fq release soonish and plan on rebasing my changes on top on this one and do some testing. Plan on rebasing it on #202?

Really wish we could end up with some readline fork with common changes somehow 🤔 a bit of a mess to keep track 😄

[tpodowd]

Hi @wader - good to hear from you. I added another patch to #202 a few minutes ago, so yes, I will also rebase this one again. Before I rebase, I'll probably do a little more testing. I'm hoping I didn't break anything on windows :-) so I want to test that first. I'll ping you also when its done. Would be great to get you to test it again also.

[wader]

The same and hope all is good with you too. I'm keeping a close eye on this repo :) Great, will rebase and test once you ping me. And i should also get myself a windows VM somehow, i've previously used microsofts IE testing vm images but now i can't find them anymore.

[wader]

BTW I had this setup for a while https://github.com/wader/fq/blob/master/doc/dev.md#setup-docker-desktop-with-golang-windows-container when iterating on shared linux/mac/windows code, but i think the windows cli behaved weirdly via docker so maybe not ideal for readline development.

[tpodowd]

Hi @wader - I rebased this branch on top of #202 again. I have tested it on linux and windows. Seems to work ok. I'll be doing more testing. Let me know if you find any issues.

[wader]

Hey did some basic testing with fq on windows using the new "cmd.exe", seems to work fine 👍