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 skippable typings #774

Merged
merged 6 commits into from
Apr 20, 2022
Merged

Add skippable typings #774

merged 6 commits into from
Apr 20, 2022

Conversation

Neyxo
Copy link
Contributor

@Neyxo Neyxo commented Apr 17, 2022

This makes typing animations skippable by hitting enter while they are running.

I selected enter because of convenience and keyup in order to not interfere with other bindings. However, if you see another, better fitting/cleaner binding it should be easy to switch it. (I thought of dblclick for minimal interference as well, but its a bit too inconvenient imho)

I initially worked on master, without the masking fix #770 . In the "skipping" case this might still be a problem.

@Neyxo Neyxo changed the base branch from master to devel April 17, 2022 20:19
@Neyxo
Copy link
Contributor Author

Neyxo commented Apr 17, 2022

After using it on different typing() invocations I noticed that if the enter/return key isn't released quickly it'll easily accidentally skip the animation, so I decided to switch it to a different key entirely.
While I massively prefer enter, it can only be changed back to enter if the binding is changed to one with keypress /keydown without interfering with the existing ones instead of the current keyup binding.

@jcubic
Copy link
Owner

jcubic commented Apr 18, 2022

I like the idea of skipping the animation, but I would prefer the API method for this.
Maybe terminal::skip() and terminal::stop() and the user will decide what key he wants the skip to happen or maybe he will want to do this programmatically (like a buttons play/stop/skip).

@Neyxo
Copy link
Contributor Author

Neyxo commented Apr 18, 2022

I have now added skip(n). It skips the number of specified animations or everything if nothing is specified.
If you activated skipping everything you either stop it manually with skip_stop() or by setting the reset flag on a new invocation e.g. skip(1, true).
I also set the default keydown enter event to skip the animation, but this only works if an animation is currently running so it doesn't interfere with other shortcuts.
It can still be changed by setting another keyCode on the new skipKey setting.

Works like a charm for me.

@jcubic
Copy link
Owner

jcubic commented Apr 18, 2022

I don't think that skip needs a counter. The developer can easily not call animation if he wants, the developer control when he invokes the animation so he can skip animation in the middle on user request and don't invoke future animation. So count is redundant.

I also don't like having skipKey in animation. Why it's needed if the developer can use:

const options = {
    keymap: {
        ENTER: function(e, original) {
            if (this.animating()) {
                this.skip();
            } else {
                original();
            }
        }
    }
};

there only need to be an API to check if terminal is animating.

@jcubic
Copy link
Owner

jcubic commented Apr 18, 2022

Another useful thing that can be added is animation events.

  • onAnimationStart - triggered before animation start, return false will prevent the animation
  • onAnimationChange - triggered on each char
  • onAnimationEnd - triggered when animation ends

this will also allow you to have what you want. onAnimationStart you will call skip when the counter is 0, but this will be more flexible because users will be able to do many more things, the things I don't even know right now.

let skip_count = -1;
const options = {
    keymap: {
        ENTER: function(e, original) {
            if (this.animating()) {
                this.skip();
                skip_count = 10;
            } else {
                original();
            }
        }
    },
    onAnimationStart() {
        if (--skip_count < 0) {
            return false;
        }
    }
};

@Neyxo
Copy link
Contributor Author

Neyxo commented Apr 18, 2022

In terms of the skipKey I think your variant would be great, but I think the keymap is being blocked at line 8819:

key_down(e) {
if (animating) {
    return false;
}

So the terminal doesn't respond to any key events. I just tested it and your code only works if you remove the animating blocker. (Or am I missing something?)

Regarding the multi-skip I think there are use cases where you'll want to skip multiple animations, especially when using different speeds chained after each other. E.g:
with multi-skip

terminal.typing("echo", 100, "Do something fast", () => {
  terminal.typing("echo, 2000, "Do something slow", {
    terminal.typing("prompt, 100, "Normal again");
  });
});
///////////
onEnter() {
  skip(3)
}

vs without multi-skip

terminal.typing("echo", 100, "Do something fast", () => {
  if(customSkip){
    terminal.echo("Do something slow");
    terminal.echo("Normal again");
  } else {
    terminal.typing("echo, 2000, "Do something slow", () => {
       if(customSkip){
         terminal.echo("Normal again");
       } else {
         terminal.typing("echo, 2000, "Normal again");
       }
    });
  }
});
///////////
onEnter() {
  skip();
  setCustomSkip(true);
}

But can remove it from the PR if you don't want it.

@Neyxo
Copy link
Contributor Author

Neyxo commented Apr 18, 2022

I agree that onAnimation functions would be the most flexible solution instead of the multi-skip functionality. I'll try to find time to add them.

@Neyxo
Copy link
Contributor Author

Neyxo commented Apr 18, 2022

I removed multi-skip.
I probably won't have time to implement proper onAnimation functions in the next weeks.
I'd suggest that I open a new feature-issue and track it separately, so this PR can be merged and doesn't accumulate merge issues.

@jcubic
Copy link
Owner

jcubic commented Apr 18, 2022

About your concern that animation disables all user-defined key events, I would just refactor how key down is implemented and allow calling user-defined functions but don't do default action, I'm yet not sure how. I need to look at the code.

About the PR, please remove skipKey, then I will merge and we will figure out later how to make this work in user-defined key events. It would be nice if you can add:

terminal::animating

that just returns animating variable. Then you will have everything to make skippable animation. You will just use global keydown outside of terminal:

$(document).keydown(e => {
    if (term.animating() && e.which == 13) {
        term.skip();
    }
});

Later I would figure out how to make the keymap work with the current API when the animation is running. But for now, it will work and there would be no odd options.

@Neyxo
Copy link
Contributor Author

Neyxo commented Apr 19, 2022

Sounds like a plan, I removed the skipKey accordingly and added animating.

Actually $(document).keydown also gets blocked, but I can work my way around that.
I'll just use $(document).keyup for now, but something to consider if there's gonna be a rework.
$(document.documentElement).keydown is the way to go. Not sure why, but $(document) only works if the Line doc.bind('keydown.cmd', keydown_event); is disabled.

Very strange. It's technically a bug, but I don't think it's really worth changing.

@jcubic jcubic merged commit f0df72d into jcubic:devel Apr 20, 2022
@jcubic
Copy link
Owner

jcubic commented Apr 20, 2022

I've already merged but I will change the API. skip can work as a toggle, no need for two methods.

@jcubic jcubic mentioned this pull request Apr 20, 2022
3 tasks
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.

Typing animation don't respect mask
2 participants