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

Factored out execute function for the main loop #455

Merged
merged 1 commit into from
Nov 11, 2020

Conversation

tailhook
Copy link
Contributor

Most commands are factored out to the function, except:

  1. Commands that need extra input
  2. Suspend, which needs access to the terminal

This should be enough to implement ContextValidator::invoke.

Also this commit brings CompleteHint, SelfInsert, Insert commands
to the match over Cmd instead of keeping them as separate if
statements as latter doesn't seem to change their behavior.

This is needed to implement #453, and #293 (although, latter might need
input-accepting commands too or another approach).


This is somewhat super-minimal refactoring to move closer to our goal of invoking commands from ContextValidator::invoke or any other user hooks.

I think input_mode.is_emacs_mode() and a reference to kill_ring can also be incorporated into State object. So the only thing we need to propagate through the call chain is State. What do you think? (this can be a follow-up PR anyway)

@gwenn
Copy link
Collaborator

gwenn commented Oct 16, 2020

For #293 (more precisely, for composite command), we need to cumulate impacts of each action and update output accordingly only once.
But your PR is still useful.

@tailhook
Copy link
Contributor Author

For #293 (more precisely, for composite command), we need to cumulate impacts of each action and update output accordingly only once.

Well, I think we can do the following:

  1. Drop most move_cursor and refresh_* in the edit.rs
  2. Refresh line by looking at growing changes
  3. If line is not refreshed move cursor by comparing to old position

(sans few special commands, like Undo)
Then ValidateContext::invoke also would only do the "impact" and and line is refreshed afterwards.

Thoughts?

But your PR is still useful.

Okay. Let's merge it and proceed quickly. Because this is a kind of change which will conflict with every addition or even change or other command (including #452) and keeping track of which commands are added in the master and which are already there in this branch is time-consuming.

Most commands are factored out to the function, except:

1. Commands that need extra input
2. Suspend, which needs access to the terminal

This should be enough to implement `ContextValidator::invoke`.

Also this commit brings `CompleteHint`, `SelfInsert`, `Insert` commands
to the `match` over `Cmd` instead of keeping them as separate `if`
statements as latter doesn't seem to change their behavior.

This is needed to implement kkawakam#453, and kkawakam#293 (although, latter might need
input-accepting commands too or another approach).
@tailhook
Copy link
Contributor Author

tailhook commented Nov 9, 2020

Rebased on the new master. So do you accept this as a first step? As I've said rebasing this PR every time a big piece of error-prone work so I don't want to keep it in a branch for too long.

@gwenn
Copy link
Collaborator

gwenn commented Nov 9, 2020

Sorry, the conflict was introduced by your PR #452...

@tailhook
Copy link
Contributor Author

Sorry, the conflict was introduced by your PR #452...

Sure, fixed that.

@gwenn gwenn merged commit 90742b3 into kkawakam:master Nov 11, 2020
@gwenn
Copy link
Collaborator

gwenn commented Nov 11, 2020

Thanks.

@tailhook tailhook deleted the factor_out_execute branch November 11, 2020 10:20
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.

2 participants