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

tpodowd · 2022-06-29T12:30:43Z

NB: This PR includes PR202 (#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 · 2022-06-29T15:12:27Z

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 · 2022-06-30T00:06:56Z

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 · 2022-06-30T09:04:20Z

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 · 2022-07-01T03:26:00Z

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

tpodowd · 2022-07-01T03:27:37Z

@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.

Also some cleanup and fixup of commits chzyer/readline#207

wader · 2022-07-02T00:36:23Z

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 · 2022-07-02T01:28:45Z

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 · 2022-07-02T08:27:42Z

@tpodowd Great 👍 no stress

tpodowd · 2022-07-04T01:04:19Z

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.

Fixes: Tab Completion Candidate Pager for when candidates don't fit on a single page chzyer/readline#207

wader · 2022-07-04T09:22:35Z

@tpodowd Works great 👍

tpodowd · 2022-07-04T10:19:43Z

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

wader · 2022-07-04T10:22:28Z

Yeap will do!

wader · 2022-08-15T08:08:32Z

@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 · 2022-08-15T09:12:00Z

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 · 2022-08-22T03:35:40Z

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 · 2022-08-22T08:34:12Z

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 · 2022-08-24T06:15:00Z

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 · 2022-08-24T10:02:34Z

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 · 2022-08-25T09:02:15Z

Filed an issue #212

- Don't overwrite existing text on same line as the prompt - Don't refresh screen when simply appending characters to buffer - Don't refresh screen unnessarily when pressing enter key - Handle prompts longer than screen width. - Fix wide characters in prompt - Fix screen edge issue when next character is wide. - Fix screen edge issue for masked characters - Fix narrow masked characteter, masking wide input - Fix wide masked character, masking narrow input - Reworked backspacesequence for index to use same algorithm as used for lineedge and reduce the control sequences to 2. - Reworked cleanup to incorporate initial cursor column position and avoid overwriting existing text as well as simplifying the control sequences used. - Fixed double width character detection and updated unit tests - Handle emoji in text or prompts. - Implement windows ANSI absolute horizonal position ansi code. - Get windows cursor position directly and don't send ansi DSR code - Don't write out empty mask runes - Cleanup - removed unused hadCLean variable

tpodowd · 2023-02-28T03:28:49Z

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.

Introduce a new operation.isPrompting field which is true from when a prompt has been written until data is returned to the caller. When Write is called on the wrapWriter to write to stdout or stderr, check if we are currently prompting the user for input and if so clean up the prompt and write the data before redrawing the prompt at its new location after the written data. Previously terminal.IsReading() was used for this, but this had various race conditions and it was not correct to check this field to make prompt and buffer redrawing decisions. In turn, I removed all the isReading code also. The old isReading() check was actually checking if the terminal goroutine was actively waiting for more input.

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 completion mode. - Added aggregate completion when entering completeselectmode where the candiddates dwindle down sharing a larger common prefix. This makes typing a little faster than having to select. 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-b 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 exitselectmode which cleaned up all the data but left us in complete mode such that if CompleteRefresh was callled directly, the data was not initialized. - 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).

…ode. The completion candidates were not being cleaned up properly if the Enter key was pressed while in complete mode. This could lead to both candidates and any program output being mixed on the screen. Cleanly exit complete mode and redraw when the Enter is pressed while in complete mode.

wader · 2023-03-04T10:01:08Z

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 · 2023-03-04T10:09:31Z

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 · 2023-03-04T10:20:38Z

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 · 2023-03-04T10:25:05Z

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.

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 simpliest 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.

tpodowd · 2023-03-05T03:53:58Z

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 · 2023-03-06T14:03:12Z

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

tpodowd mentioned this pull request Jun 29, 2022

fix(autocomplete newline): autocomplete newline #206

Open

tpodowd mentioned this pull request Jul 1, 2022

Autocomplete CharBackspace behaviour question #204

Closed

wader added a commit to wader/fq that referenced this pull request Jul 1, 2022

readline: Rebase on top of tpodowd's redraw/completion fixes PR

44bab27

Also some cleanup and fixup of commits chzyer/readline#207

wader mentioned this pull request Jul 1, 2022

readline: Rebase on top of tpodowd's redraw/completion fixes PR wader/fq#305

Merged

wader added a commit to wader/fq that referenced this pull request Jul 4, 2022

readline: Rebase on top of tpodowd's update PR

a512269

Fixes: Tab Completion Candidate Pager for when candidates don't fit on a single page chzyer/readline#207

wader mentioned this pull request Jul 4, 2022

readline: Rebase on top of tpodowd's update PR wader/fq#308

Merged

tpodowd added 2 commits February 28, 2023 11:00

Fix simple race condition related to offset calculations introduced.

586d8ee

tpodowd force-pushed the completion branch from d356858 to f0ae515 Compare February 28, 2023 03:16

tpodowd added 2 commits March 4, 2023 18:59

tpodowd force-pushed the completion branch from f0ae515 to c30c5a0 Compare March 5, 2023 03:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

tpodowd commented Jun 29, 2022 •

edited

Loading

wader commented Jun 29, 2022

tpodowd commented Jun 30, 2022

wader commented Jun 30, 2022

tpodowd commented Jul 1, 2022

tpodowd commented Jul 1, 2022

wader commented Jul 2, 2022

tpodowd commented Jul 2, 2022

wader commented Jul 2, 2022

tpodowd commented Jul 4, 2022

wader commented Jul 4, 2022

tpodowd commented Jul 4, 2022

wader commented Jul 4, 2022

wader commented Aug 15, 2022

wader commented Aug 15, 2022

tpodowd commented Aug 22, 2022

wader commented Aug 22, 2022

tpodowd commented Aug 24, 2022

wader commented Aug 24, 2022

wader commented Aug 25, 2022

tpodowd commented Feb 28, 2023

wader commented Mar 4, 2023 •

edited

Loading

tpodowd commented Mar 4, 2023

wader commented Mar 4, 2023

wader commented Mar 4, 2023

tpodowd commented Mar 5, 2023

wader commented Mar 6, 2023

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

Are you sure you want to change the base?

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

Conversation

tpodowd commented Jun 29, 2022 • edited Loading

wader commented Jun 29, 2022

tpodowd commented Jun 30, 2022

wader commented Jun 30, 2022

tpodowd commented Jul 1, 2022

tpodowd commented Jul 1, 2022

wader commented Jul 2, 2022

tpodowd commented Jul 2, 2022

wader commented Jul 2, 2022

tpodowd commented Jul 4, 2022

wader commented Jul 4, 2022

tpodowd commented Jul 4, 2022

wader commented Jul 4, 2022

wader commented Aug 15, 2022

wader commented Aug 15, 2022

tpodowd commented Aug 22, 2022

wader commented Aug 22, 2022

tpodowd commented Aug 24, 2022

wader commented Aug 24, 2022

wader commented Aug 25, 2022

tpodowd commented Feb 28, 2023

wader commented Mar 4, 2023 • edited Loading

tpodowd commented Mar 4, 2023

wader commented Mar 4, 2023

wader commented Mar 4, 2023

tpodowd commented Mar 5, 2023

wader commented Mar 6, 2023

tpodowd commented Jun 29, 2022 •

edited

Loading

wader commented Mar 4, 2023 •

edited

Loading