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

undo/redo extension #311

Closed
wants to merge 5 commits into from
Closed

undo/redo extension #311

wants to merge 5 commits into from

Conversation

Trinitou
Copy link
Contributor

already discussed in #161 and #223

Now I found time to do a first draft. I tried to (hopefully) integrate suggestions the came from different people in those threads.

Some questions I already have:

  • Should
    • perform_undo and perform_redo return bools and not call mark_dirty_for_undo/notify_undo afterwards
    • perform_undo and perform_redo return nothing and call mark_dirty_for_undo/notify_undo afterwards (maybe simpler but more redundant?)
  • Is there a better, shorter name than 'incremental change event object' (to all native speakers out there ;) )?
  • Maybe @baconpaul: In this current draft, I assume that 'incremental change event object's can be applied in both directions (for undo AND redo). Or should there rather be two types: 'incremental undo objects' and 'incremental redo objects'?

@abique
Copy link
Contributor

abique commented Mar 11, 2023

Maybe the change related to the stream notes could go in a separate PR?

Copy link
Collaborator

@baconpaul baconpaul left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A set of comments from reading this

include/clap/ext/draft/undo-redo.h Outdated Show resolved Hide resolved
include/clap/ext/draft/undo-redo.h Outdated Show resolved Hide resolved
include/clap/ext/draft/undo-redo.h Outdated Show resolved Hide resolved
} clap_plugin_undo_redo_t;

typedef struct clap_host_undo_redo {
// Tell the host that the plugin state has changed internally and
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So basically the semantic is "when i have undo/redo gestures internally I call these two along with doing them" but then I think order matters a lot. So like if I undo, have I undone before I call these and what is the object on the stack for the next stream? If I undo redo undo before the host calls me back what happens? There's some tricky state management here which I think we need to document and consider.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I hope I could resolve some of those concerns with the 'pending objects' concept. This means, the plugin has to buffer pending objects and the host must sync all pending objects first before it can perform an undo/redo on the plugin.

I think the buffering shouldn't be much of a problem for the plugin as it already has those objects for its internal undo/redo. So it just needs to store them a bit longer until the host has pulled them.

include/clap/ext/draft/undo-redo.h Outdated Show resolved Hide resolved
#include "../../string-sizes.h"
#include "../../stream.h"

/// @page Undo/Redo
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few things that are missing

  1. We show the undo/redo stack in our UI. Can we query the host undo/redo stack here?

  2. The host will put other things on the undo redo stack. Do we want to be able to trigger them from the ui and interleave those stacks? I'm thinking especially of things like a setting a parameter from outside the plugin - what does that do as far as undo/redo? We really need to document that case.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My approach here would be that the plugin only knows about its internal undo/redo stacks and not the host's stacks. So if the plugin does internal undo/redo, the host might remove those steps from between own undo/redo steps. I think this would simplify things for the plugin devs and the host would be responsible for placing the plugin undo/redo objects in the appropriate place. You think that's OK?

Concerning parameter changes from outside, I would add a note to the documentation that the plugin is always responsible for creating undo steps for parameter changes, even if they come from the host and the host must not create undo steps for those parameter changes (?)

@baconpaul
Copy link
Collaborator

left some thoughts

@Trinitou
Copy link
Contributor Author

Trinitou commented Mar 14, 2023

Another aspect might still be missing: The host can only apply the undo/redo objects one by one via apply_event_object ATM. But what if it want's to perform multiple undo/redo actions in a row (e.g. when clicking a node in an undo history or pressing undo/redo key command really quick). Even worse, if those undo/redo steps contain a lot of preset changes, the plugin jumps between different presets.

Therefore, the plugin would want to first get all objects at once and then can decide to skip some updates (if there is a complete state change in a later object)

@Trinitou
Copy link
Contributor Author

Added a begin and end method as an approach for the issue described in my comment above.

@Trinitou
Copy link
Contributor Author

I think this is now resolved with #419

@Trinitou Trinitou closed this Sep 23, 2024
@Trinitou Trinitou deleted the undo_redo branch November 1, 2024 04:09
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.

3 participants