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

Add Home and End key for list prompt #79

Merged
merged 11 commits into from
Oct 11, 2023

Conversation

crazywhalecc
Copy link
Contributor

@crazywhalecc crazywhalecc commented Sep 21, 2023

This PR (Plan A for additional keys) allows list-related prompts to support using Home and End key, which allows us to go to top and bottom. This PR is related to #69 .

The main functions of this PR are:

  1. Refactoring all highlight methods related to lists so that they can freely pass in new highlight or offset values while adapting to changes in display. This can also make it easier to implement the Page key (if needed), which may only require two lines of code
  2. Add the response of the Home and End keys to the list, which can quickly switch to the top or bottom when highlighted within the list.
  3. Add related tests.

And I also have two questions:

First, I tested on macOS Terminal and Windows Terminal. They both uses \e[H (xterm style) character. I don't know how to be compatible with responses from different terminals.

# For Home key, examples:
"\e[1~"    # linux console
"\e[H~"    # xterm (Windows Terminal, macOS Terminal, etc)
"\eOH"     # gnome terminal

Second, I have written almost identical code many times, but I don't know how to better merge them because different Prompts use different list filters, such as $this->options and $this->matches.

In addition, I have plan B, which is simply: write a trait HighlightList, including member variables such as public ?int $highlighted, and convert methods such as highlightNext() into updateHighlight($options , $scroll, $highlighted), handles multiple page highlighting options within a common method. But this only separates the dependent variables, and passing in an array increases the complexity of the updateHighlight method.

@TWithers
Copy link
Contributor

I tested on macOS Terminal and Windows Terminal. They both uses \e[H (xterm style) character. I don't know how to be compatible with responses from different terminals.

I just created a PR to add support for multiple versions of home and end: #87
If that gets accepted, then you could solve the issue you mentioned.

@crazywhalecc
Copy link
Contributor Author

I merged the current branch into my branch, and it seems that the added changes to Key::oneOf and Key::HOME will affect some previously written test scripts.

The current Home and End keys work fine, if there's anything missing I'd be happy to help.

@TWithers
Copy link
Contributor

TWithers commented Oct 4, 2023

@crazywhalecc I think your recent changes cover the issues you described. It is just using the Key::oneOf() in your match statement and using arrays in your tests. There were no other significant changes.

@crazywhalecc
Copy link
Contributor Author

crazywhalecc commented Oct 4, 2023

@TWithers Oh yeah... My mistake, I forgot about it after long vacation 😵‍💫

@jessarcher jessarcher force-pushed the feat/home-end-list-key branch from a799387 to 63d5ace Compare October 11, 2023 07:14
@jessarcher
Copy link
Member

Hey @crazywhalecc, thanks heaps for this! 🙌

I've removed the offset code for now because it's not needed for this specific feature and it was getting pretty complex. I've also centralised the logic into an existing trait related to the scroll behaviour.

I've also fixed a small issue where pressing home/end while navigating a list would reset the cursor position of the text input.

@jessarcher jessarcher marked this pull request as ready for review October 11, 2023 07:23
@crazywhalecc
Copy link
Contributor Author

crazywhalecc commented Oct 11, 2023

Thank you so much! I'm looking forward to this feature being added. 👍

The original consideration of using highlightOffset() was because I also added PageUp and PageDn codes locally. Using offset can easily control this behavior, and it can replace the repeated next and previous logic, but , of course they have not been added yet.

@taylorotwell taylorotwell merged commit 984d911 into laravel:main Oct 11, 2023
4 checks passed
@crazywhalecc crazywhalecc deleted the feat/home-end-list-key branch October 12, 2023 02:21
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.

4 participants