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

feat: Support selecting candidates when completion is paged #66

Merged
merged 5 commits into from
Sep 3, 2024

Conversation

YangchenYe323
Copy link
Contributor

This resolves #65

Upon reflection, I removed PagerMode in which the user could do nothing but view the pages and make paging a transparent feature under CompleteMode and CompleteSelectionMode. The benefits are:

  1. The user could select a candidate from the completions to enter on the terminal, which is currently impossible.
  2. the user can continue typing and have the auto completor update completion even if the number of candidates requires pagination.

Below is a demo from the example/readline-paged-completion I added:

Recording.2024-08-07.215104.mp4

@YangchenYe323
Copy link
Contributor Author

@slingamn @wader Would you like to take a look? Thanks

@slingamn
Copy link
Member

slingamn commented Aug 8, 2024

cc @tpodowd

@slingamn
Copy link
Member

slingamn commented Aug 8, 2024

I'll review this over the next week or so but from a first look, the functionality is beautiful :-)

@tpodowd
Copy link
Collaborator

tpodowd commented Aug 8, 2024

Hi there. This does look like a nice feature. I very lightly reviewed the code and it looks good, but I haven't played with it yet. It would be nice to have a few more comments. Also, maybe we should change choise to choice as this PR has spread the odd (old) spelling to a function name now also. I'm very snowed under for the rest of this week. I can try it out and review in more detail next week.

@wader
Copy link
Collaborator

wader commented Aug 8, 2024

Nice! would it make sense that moving the selection when paged mode is needed does not make it wrap-around but instead moves row by row up or down? and only warps-around if moving past the "real" last or first row? and j/k could work as now i guess?

@YangchenYe323
Copy link
Contributor Author

would it make sense that moving the selection when paged mode is needed does not make it wrap-around but instead moves row by row up or down?

We could distinguish the arrow keys and the tab key. Left/right arrow keys will move selection within the row and wrap to the beginning/end of that row, whereas the tab would proceed through pages. Does it sound more ergonomic to you?

@wader
Copy link
Collaborator

wader commented Aug 10, 2024

We could distinguish the arrow keys and the tab key. Left/right arrow keys will move selection within the row and wrap to the beginning/end of that row, whereas the tab would proceed through pages. Does it sound more ergonomic to you?

Sounds good! what about arrow keys up/down, could that move selection one row up or down and when reach start or end row of a page it scrolls one row at a time if possible and wraps if not?

@YangchenYe323
Copy link
Contributor Author

Sounds good! what about arrow keys up/down, could that move selection one row up or down and when reach start or end row of a page it scrolls one row at a time if possible and wraps if not?

Hmm, this would require more effort because my current implementation assumes the paging structure and always starts displaying completion from the start of a page.

@wader
Copy link
Collaborator

wader commented Aug 10, 2024

Aha I see, no worries. Maybe could be something to iterate on in the future.

@wader
Copy link
Collaborator

wader commented Aug 11, 2024

Played around a bit, works fine 👍 but i wonder is the lower right entry skipped on purpose on non-last pages?

@YangchenYe323
Copy link
Contributor Author

@wader Good catch. It is a bug and I believe it's fixed now.

@YangchenYe323
Copy link
Contributor Author

@slingamn @tpodowd I believe this is ready for review. Would you like to take a look? Thanks!

@slingamn
Copy link
Member

Thanks, I'll plan to take a detailed look this weekend.

@slingamn
Copy link
Member

Sorry about the delay. This looks good to me. Does anyone else have thoughts?

@YangchenYe323
Copy link
Contributor Author

@tpodowd @wader Sorry for pinging again. Would you like to have another look? We have a production application where this feature could be really useful to us.

@slingamn slingamn merged commit 7b16f76 into ergochat:master Sep 3, 2024
1 check passed
@slingamn
Copy link
Member

slingamn commented Sep 3, 2024

@YangchenYe323 I merged this and tagged it as v0.1.3-rc1, which should unblock your use case (you should be able to pull it into your project with go get github.com/ergochat/[email protected]).

The preliminary changelog since v0.1.2 is your change here and also #68, that's all. I am holding v0.1.3 for a bit to see if I can solve #69, but I should release it soon either way.

@wader could you try testing v0.1.3-rc1 with fq?

@wader
Copy link
Collaborator

wader commented Sep 3, 2024

@slingamn seems to work fine 👍 tried with:

$ go get ...
$ go run . -ni
null> [range(10000) | {key: "a"+tostring, value: "a"+ tostring}] | from_entries | .a<tab>

@YangchenYe323 nice work

@YangchenYe323
Copy link
Contributor Author

Confirm that this works for me, too. Thanks so much!

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.

Could not select candidate in pager mode
4 participants