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 command repeat (like Vim's :h count) #39

Merged
merged 3 commits into from
Aug 31, 2023
Merged

Conversation

jparoz
Copy link
Contributor

@jparoz jparoz commented Aug 17, 2023

This is close to a minimal functional implementation of command repeating, similar to Vim's concept of a count. It builds on the work by @KiLLeRRaT in #23, by rebasing and incorporating the feedback from that pull request. Any feedback is welcome.

I'm fairly happy with how things are working now, but I have two main gripes:

  1. Not all commands make sense with a count, e.g. i; for example, 5i for some reason navigates down 2 cells and then enters insert mode, while 4i navigates down 2 cells and doesn't enter insert mode. I'm guessing it's an "enter-exit-enter-exit" kind of thing, such that the parity of the count decides which state it ends up in. In any case, I think this is confusing behaviour. A possible remedy would be to store a boolean somewhere (maybe in commands.js?) which decides whether a command is allowed to be repeated; there may be many others which also produce confusing behaviour, and this would let us decide that case-by-case. I would be happy to implement this or something similar, but thought I'd leave that for another pull request.
  2. It takes a long time to execute many repeats. This was mentioned in Added repeatCount functionality #23, and so here I've implemented a cap of 99 repeats (the value is just clamped); but this feels a bit unsatisfactory to me, especially for movement commands. The main time that I reach for counts is when I want to e.g. scroll down 50 rows, so I do 50j. My Vim-brain expects this to be instant, but of course in sheetkeys this causes a loop which triggers 50 consecutive j key events, so it takes over a second. I don't really know how to solve this though; it might just be impossible with the current model in sheetkeys (or impossible in general). Oh well.

Closes #1
Supersedes #23

@jparoz jparoz mentioned this pull request Aug 17, 2023
1 task
@KiLLeRRaT
Copy link
Contributor

Cool, I'm super glad you're working on this, thanks!

I just have gotten around to this again lately, and I'm unlikely to, but I will try to follow this PR and look forward to seeing it merged soon :)

Just a question, by removing the repeat undo. If you did 50 deletes, will you have to then undo 50 times manually using the Sheets functionality?

Cheers,
Albert

@jparoz
Copy link
Contributor Author

jparoz commented Aug 19, 2023

It seems so, yes; I just tested with "5dd" and it deleted 5 rows, then I had to press undo 5 times to recover the rows. I see why this might pose a problem, especially with 50 or 99 repeats of a command like this; but @philc's comments on #23 indicated that his preference was to not do anything too fancy to handle this for now.

One way to recover from an accidental nuclear repeated delete would be to use Sheets' "Version history" to roll back the changes; this interface seems to clump these kinds of actions together pretty well, and keeps a record of changes over time.

I think to properly implement undoing repeated commands, it would take a lot more machinery to track what actions are both executed and undone, and it may be ultimately impossible to implement predictably. For example, if we store a list of all the actions which are issued via sheetkeys, can we also capture actions issued with the toolbar or menus? Can we detect when somebody uses the menu to undo? How do we handle getting out of sync, if it's possible? This could possibly all be done, but I think landing this PR with the simple version of undo is a good place to start.

@philc philc merged commit 2ce0c75 into philc:master Aug 31, 2023
@philc
Copy link
Owner

philc commented Aug 31, 2023

Looks good. Thank you @jparoz and @KiLLeRRaT .

@jparoz, can you introduce another PR which allows for an optional key in commands.js called "nonRepeatable", and mark the commands which are non-repeateable?

Briefly scanning through the list of commands, there are many which have binary outcomes and shouldn't be repeatable, like "alignLeft", "clip", "scrollToBottom", "enterVisualMode", "clear", "copy", "showHelp", "reloadPage", and many others. I've opened #40 to track this.

@KiLLeRRaT
Copy link
Contributor

Hi @philc , do have any rough idea when you think you'd update the Chrome Store version of SheetKeys? HEh I find that quite convenient as it will then auto update on all my machines, instead of manually pulling on each one.

Thanks!

@philc
Copy link
Owner

philc commented Sep 17, 2023

@KiLLeRRaT I've just submitted v0.3 to the chrome store.

@KiLLeRRaT
Copy link
Contributor

@KiLLeRRaT I've just submitted v0.3 to the chrome store.

Great, thanks for that. Looking forward to it :)

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.

Repeat command n times
3 participants