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: auto-completion for input component #324

Merged
merged 19 commits into from
Nov 3, 2023
Merged

Conversation

XOR-op
Copy link
Contributor

@XOR-op XOR-op commented Oct 27, 2023

3.mp4

This PR implements auto-completion for input component. Specifically, a 2D table is used for the completion as what shells (e.g. zsh, fish) do. Currently, the only implemented completion functionality is interactive-cd, but other completion can benefit with this new component.

@XOR-op XOR-op changed the title [Feature] Auto-completion for input component feat: auto-completion for input component Oct 27, 2023
@sxyazi
Copy link
Owner

sxyazi commented Oct 27, 2023

Thank you very much for your PR; this is a feature I've been wanting to implement for a while. 🙏🏻

Can we implement it asynchronously? This would allow us to move away from std::fs and lay the foundation for future asynchronous task extensions.

@XOR-op
Copy link
Contributor Author

XOR-op commented Oct 27, 2023

That's a good point. But I think we need to discuss how we can achieve asynchronization. Currently in my implementation, I define two callback functions:

pub init_completion:   Option<Box<dyn Fn(&str) -> Vec<String> + Send>>,
pub finish_completion: Option<Box<dyn Fn(&str, &str) -> String + Send>>,

The init_completion is used to generate a list of completion items, and the finish_completion combine the current value as well as the selection to get the final completion. Both of the callbacks are invoked in Input::type_(), which is a non-async function and called by an also non-async function Executor::handle. To my current understanding, we need to refactor a large part of codebase to achieve this goal, and that is also far away from this PR's scope.

As for this interface, if we finally refactor the whole codebase with async, it would be easy to change the two callbacks to the async version, as long as this hasn't been exposed as an extension API. But I'd be glad to hear if you have different opinions.

@sxyazi
Copy link
Owner

sxyazi commented Oct 27, 2023

I'm wondering why we need to treat completion as a separate component, are there other cases besides the input where completion needs to be implemented independently? I think treating it as a separate component would make things complex.

For instance, if we want to implement it asynchronously, we'd need communication between the two components because they don't share state. Achieving the overlaying completion on top of the input is also not straightforward; we'd have to calculate the relative position between the two components:

image

When editing a file very far down the list, to prevent the input from overflowing the terminal, I might display the input above the file instead of below it. In this case, having an independent completion would require us to recalculate its position relative to the input and determine its direction (above or below):

image

If it were part of the input, we could include the completion's width and height within the input and only decide how to display the whole input.

@XOR-op
Copy link
Contributor Author

XOR-op commented Oct 27, 2023

I don't think there are any other cases where we need completion. In fact, I don't add the completion component directly into Ctx, instead it's a member of input. In terms of core logic, it's majorly directly operated by Input and its methods. If the state you mean is logic state instead of UI state, I think an input can directly access that through struct member access.

The reason why I want a dedicated component is I want to customize it's size more flexibly during the rendering (e.g. initially the input has a width of 50, but the completion part takes too many space that it requires a width of 100, or even needs to be changed for the next completion. In this case I don't think it's good to adjust the width of the input).

I hope to hear if you have some advice on this. The completion looks like a semi-component to me, which is deeply coupling with the input but needs special rendering considerations.

@sxyazi
Copy link
Owner

sxyazi commented Oct 30, 2023

Sorry for the late response; these few days have been unexpectedly busy lol

I revisited the code, and it appears that time-consuming tasks only occur in init_completion, while finish_completion simply applies the candidate options to the Input. Therefore, it's only necessary to make init_completion asynchronous. That is related only to completion, so we can update the new candidate options with emit!(Call(Exec::call("updateCompletions"))) when the asynchronous task is completed, like this:

emit!(Call(
Exec::call("find", vec![s])
.with_bool("previous", prev)
.with_bool("smart", case == FinderCase::Smart)
.with_bool("insensitive", case == FinderCase::Insensitive)
.vec(),
KeymapLayer::Manager
));

@XOR-op
Copy link
Contributor Author

XOR-op commented Oct 31, 2023

@sxyazi I have made a new commit with support for async init_completion.

@sxyazi
Copy link
Owner

sxyazi commented Nov 1, 2023

Thank you, I'll make the remaining done

@sxyazi
Copy link
Owner

sxyazi commented Nov 3, 2023

@XOR-op I think I have completed the implementation. Would you like to give it a review?

@XOR-op
Copy link
Contributor Author

XOR-op commented Nov 3, 2023

It looks like the quit logic has some bugs. For example, when there has been a completion, push ESC twice will close the input component but leave the completion component open.

@sxyazi
Copy link
Owner

sxyazi commented Nov 3, 2023

ah sorry, I forgot to push the final piece of code... I just committed it 🤣

@XOR-op
Copy link
Contributor Author

XOR-op commented Nov 3, 2023

LGTM, the improved version is great!

@sxyazi sxyazi merged commit 5b66f6f into sxyazi:main Nov 3, 2023
3 checks passed
@sxyazi sxyazi mentioned this pull request Nov 4, 2023
56 tasks
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 10, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants