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 handling for meow-keypad-self-insert-undefined #700

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

Haxxflaxx
Copy link
Contributor

This doesn't fully solve the problem as it doesn't get applied to beacons as expected.
May be due to setting last-command-event with a let so it doesn't persist?

It does make it possible to self-insert using the keypad like before.
As it sets last-command-event calling prefix arg from keypad also works as expected.

It is now also possible to set meow-keypad-self-insert-undefined to nil and bind self-insert-command in keypad.

@Haxxflaxx
Copy link
Contributor Author

I also noticed there is something weird going on with undo when doing keypad self insert.

By using meow--keypad-execute it allows for running
self-insert-command on beacons.

Due to self-insert-command not placing a undo-boundry before this
point, explicitly place a boundry before executing the command.
@Haxxflaxx
Copy link
Contributor Author

I've fixed the issue where the command wasn't run on beacons, and also added an undo boundary before running it.

@Haxxflaxx
Copy link
Contributor Author

At this point I believe the keypad works the same for self insertions as it did before the refactor, with the exception that you can now explicitly set self insertions in the keypad map instead of having the variable toggle it for all unbound keys.

@DogLooksGood
Copy link
Collaborator

I think we have to be caution about this change, it may break things.

@Haxxflaxx
Copy link
Contributor Author

I think we have to be caution about this change, it may break things.

Sounds reasonable. Is there anything in particular that you are worried about?

@Haxxflaxx
Copy link
Contributor Author

We can't set last-command-event with a let as it may be needed for functions run after, e.g. repeat-post-hook

@DogLooksGood
Copy link
Collaborator

I'm thinking maybe we can achieve the goal with another method. If users want to call self-insert-command in a specific use case.

@Haxxflaxx
Copy link
Contributor Author

Can you elaborate on what you mean? Do you mean instead of setting last-command-event, we update some other part of the keypad which result in the correct value being set so we don't have to?

@Haxxflaxx
Copy link
Contributor Author

Haxxflaxx commented Dec 20, 2024

Ok, so after thinking a bit more about this I am

  1. amazed that this works at all, and
  2. fairly certain that I should have done something like this instead:
(setq last-command-event (aref (kbd (meow--keypad-format-keys)) 0))

This also makes me a bit uncertain meow--keypad-format-keys is the best choice as I would need to know if I should add a 'C-' or not when calling it. So maybe there would be something better to do there.

This sets the last-command-event properly with a correctly formated
key.
@Haxxflaxx
Copy link
Contributor Author

I see I made some more incorrect assumptions in my first solution.
I've replaced it with explicitly getting the last key pressed in the keypad and processing it properly before setting last-command-event.
Also moved it out of the let so that I can set the global value.

This allows the self-insert to work like it did (probably mostly by accident) before, but also with repeat maps.

@DogLooksGood
Copy link
Collaborator

Maybe we should patch last-command-event and last-input-event together? How do you think?

@Haxxflaxx
Copy link
Contributor Author

Yes you're right, we should also make sure that the last-input-event is correct. I also think we should wait as long as we can until we set last-command-event, and not set it when we are not executing a command.

After some thinking about it I think we could set last-input-event in meow--keypad-handle-input-event. We can do it the same way as I did for last-command-event in my last commit.

Then I think we should move setting last-command-event into meow--keypad-execute. That way it would be set for any command that would require it, as well as not setting it when we are still navigating around in the keypad.

@DogLooksGood
Copy link
Collaborator

Why we need a undo boundary?

@Haxxflaxx
Copy link
Contributor Author

Without the boundary it won't add one when doing self-insert.
This would mean that if we do some insert, then select some text and surround it self-insert ( followed by an undo; this would result in also undoing the previous insert.

There was a boundary placed there automatically before the refactor, and this is also the behavior I observe when binding the self-insert explicitly in the keypad map.

@DogLooksGood
Copy link
Collaborator

In beacon, we collapse the boundary so that the whole kmacro application can be reverted in one undo.

@DogLooksGood
Copy link
Collaborator

I'm also thinking is there a way to achieve this, at the same time, remove the need on binding H-j/H-k stuff. We had to bind H-j/H-k because in keypad leader dispatch, because the original command on j/k will never be found. If we can find the original command j/k just as how we find self-insert-command, then we can get rid of this tricky binding.

@Haxxflaxx
Copy link
Contributor Author

Haxxflaxx commented Dec 24, 2024

I haven't looked much into how the key rebinding to H-* is done. Are you thinking that it will work the same way as any other key to key rebinding,
e.g.

(meow-leader-define-key
 '("L" . "H-L") ;Bind 'L' to gain access to overriden key in motion.
 '("p" . "C-x p") ;Bind 'p' for easy project menu.
 )

Are we doing some special magic now instead of just sending H-L?
Are we in that case also doing this special magic for the other key rebinds?

Edit:
After thinking about it a bit more, I know that we do some lookup to find whatever the key sequence is bound to, but maybe that is only for things in the global map.
Are you thinking that we could just send whatever the key sequence that we rebind to instead of doing any of that rebinding?

@DogLooksGood
Copy link
Collaborator

DogLooksGood commented Dec 24, 2024

Sorry for my slow progress on this, I have some thoughts about a better way to achieve the same goal.

The way this PR works is to call a self-insert-command when there's no bound command found. My idea is to call the original command when there's no bound command found, therefore in most case, it will be the self-insert-command. It could solve this issue perfectly, at the same time, we can remove the whole idea of H-* rebind completely.

@Haxxflaxx
Copy link
Contributor Author

Ah, I see. That sounds like a good way of doing it. So an unbound key in the keypad would bypass the current meow state and call the current key map instead.

The only issue I can see is that it would be less obvious how to handle key conflicts in motion state. Currently you handle a conflict by explicitly binding something in the keypad to H-*.
This will instead solve the conflict by not binding the conflicting key in keypad.

This is mostly a problem that can be solved with documentation. It would be nice if we also had some explicit way of binding it as well, something like: '("L" . "L") which should just do the same as if you didn't bind it or '("A" . "L") if you would like to move it to another key.
I don't know if this is a great option, but it might be good to think about.

@DogLooksGood
Copy link
Collaborator

Please try this #705 and see if it works for you.

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