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

Support more readline keys #70

Merged
merged 7 commits into from
Sep 15, 2023
Merged

Conversation

storyn26383
Copy link
Contributor

@storyn26383 storyn26383 commented Sep 14, 2023

With this PR, we can use:

  • CTRL_A, HOME: move to the beginning of line
  • CTRL_E, END: move to the end of line

see: #67 (review)

@jessarcher
Copy link
Member

Hey! I dig the CTRL_A/Home and CTRL_E/End stuff, especially with how little code it adds.

I'm not so sure about the usefulness of CTRL_U in prompts, or whether the complexity of CTRL_W is worth it. I'm also worried about going down a path where we reimplement everything from readline. I'm not sure if there are enough people who would use it to justify the added maintenance burden.

I'd rather see CTRL_A/Home and CTRL_E/End also scroll to the top and bottom of any lists when a list item is currently highlighted, but I think that is being worked on in #69.

@storyn26383
Copy link
Contributor Author

storyn26383 commented Sep 15, 2023

Yap, I agree with you, we should keep it clean and simple.

I'm also worried about going down a path where we reimplement everything from readline.

Use CTRL_A / Home and CTRL_E/ End to scroll to the top and bottom is cool!
Maybe we can also add CTRL_B and CTRL_F to scroll page as well? (But this is vim's key binding 😛)

Should we implement scroll to the top and bottom feature in this PR, or open a new PR after #69 merged?

@jessarcher
Copy link
Member

jessarcher commented Sep 15, 2023

Thanks!

I just gave this a test, and the only issue is that the options callback on the search prompt gets called unnecessarily. I think you'd need to update the key listener of the search prompt so that it only sets highlighted to null instead of falling through to the default condition, which performs the search.

If we implement the home/end behaviour when focused on a list, we'll probably need to use the new ignore feature from #58 to conditionally make home/end only apply to the typed value when highlighted is null.

Should we implement scroll to the top and bottom feature in this PR, or open a new PR after #69 merged?

I'd hold off on that one until #58 and #69 are sorted as there are some overlaps and interdependent features.

If you can just fix the search callback I'll check it again and then hand it over to Taylor for final review.

@jessarcher jessarcher marked this pull request as draft September 15, 2023 01:49
@storyn26383
Copy link
Contributor Author

I think the suggest prompt should have the same behavior, so i fixed it too.
Thanks for review :)

@jessarcher
Copy link
Member

I think the suggest prompt should have the same behavior, so i fixed it too. Thanks for review :)

Good catch!

@jessarcher
Copy link
Member

I've added docblocks to the CTRL_ constants so I can get my editor to remind me what they all are 😅

@jessarcher jessarcher marked this pull request as ready for review September 15, 2023 12:10
@jessarcher jessarcher mentioned this pull request Sep 15, 2023
@taylorotwell taylorotwell merged commit 61f61cd into laravel:main Sep 15, 2023
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants