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

Slipscripts improvements: onUndo and state #97

Merged
merged 3 commits into from
Mar 14, 2025
Merged

Conversation

panglesd
Copy link
Owner

This PR adds two things in the slip object passed to scripts:

  • A slip.onUndo value, that allows to register a callback to be run during undo (in reverse order).
  • A slip.state object (initially empty) that is persisted between scripts.

Documentation is updated.

@meithecatte I would be grateful if you can test those changes! The CI should built binaries, so you don't have to build locally.

@meithecatte
Copy link

Hi, I'd love to test this, but in the presentation I'm working on, everything is kinda intertwined with a hacked-together animation system based on async iterators that attempts to implement #94 in userspace. Without proper integration between the two, I don't have much opportunity to easily get hands-on experience with the feature (I might find some time for this once I'm no longer on a deadline, though).

One thing that came up while I was trying to find some place to nevertheless fit it in: what happens if onUndo gets called from within an undo callback?

The usecase here is as follows: I have a small part of the slide that I'm repeatedly re-randomizing to underline that it can be arbitrary. This looks as follows:

startAnim = () => {
    animInterval = setInterval(rerandomizeShit, 100);
}

startAnim();

return {
    undo() {
        clearInterval(animInterval);
        // other stuff
        x.undo(); y.undo();
    }
};

However, having this in the background, off-screen, and possibly in another tab, can be quite CPU-intensive. Because of this, I'm pausing this animation once it goes off-screen:

clearInterval(animInterval);
return {
    undo() {
        x.undo();
        startAnim();
    }
};

Now, with the onUndo feature in place, a natural implementation would be:

let animInterval;

startAnim = () => {
    animInterval = setInterval(rerandomizeShit, 100);
    slip.onUndo(stopAnim);
};

stopAnim = () => {
    clearInterval(animInterval);
    slip.onUndo(startAnim);
};

startAnim();

With a follow-up script calling

stopAnim();

The question to think about is — will this behave properly, or break things horribly?

With this motivating example, I believe that it would be best if onUndo was explicitly guaranteed to be a no-op if itself called from within an undo callback. Indeed, this makes abstractions (both user-written and ones like slip.set_style) more composable.


Actually, now that I think about it, you're mixing camelCase (onUndo) with snake_case (set_style). I think the JavaScript style is the former? (e.g. clearInterval, classList, querySelectorAll, etc)


Regarding slip.state – are you sure that you want to put this within an additional .state namespace? For me, this kinda takes this from "that's the proper way to do it and therefore i Shall do it That Way" to "that's technically the proper way to do it but I'd rather just keep using dirty global variables". I can imagine that having it within .state is simpler to implement, of course.

@panglesd
Copy link
Owner Author

Hello, thanks a lot for taking the time to describe your use case!
Some quick notes on the current implementation of onUndo (indeed I should convert the other function names to camelCase):

  • Currently each script execution gets its own slip object, with functions that register undo callback specific to this step. And currently, a new slip object is crafted for every step. (That's why it's easier for me to add the state namespace). For instance:
    Step 1: 
    slip.state.slip = slip
    
    Step 2:
    slip.state.slip.onUndo(...) // Adding a new undo action to step 1!
    
  • So, currently, calling onUndo during undo will register a callback to a list of callback we already started traversing, doing nothing in effect (if you are calling slip.onUndo from the step you are undoing ^^)
  • In this design, your example should take a slip as input and only call slip.onUndo if slip is not null (but, while I'm used to this kind of programming, I understand this is not the right approach here!) .
  • I start to be convinced that you are right and it's not the right implementation. There should be a single onUndo function stored in a single "global" slip object, so users can define function without thinking about which slip they are using.

I'll do the opam release and then think about this and #94 a bit more, but:

  • Very happy to have you as a user! Good luck with your presentation :) you can test this PR after it, if you are still motivated!

@panglesd panglesd force-pushed the scripts-improvements branch from be7d778 to e2d72e0 Compare March 14, 2025 12:36
@panglesd
Copy link
Owner Author

I kept the state in slip.state for now, as that requires significantly more changes to what this PR was about.

It should be possible to call slip.onUndo during an undo callback, although the proper behaviour will be implemented when I'll add support for "animation steps" (steps that takes some time to run).

I added a slip.setProp too.

Thanks again for the suggestions!

@panglesd panglesd merged commit 71fc117 into main Mar 14, 2025
5 checks passed
panglesd added a commit that referenced this pull request Mar 14, 2025
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