free-audio / clap

Audio Plugin API
https://cleveraudio.org/
MIT License
1.78k stars 100 forks source link

Initial take on undo support #336

Closed abique closed 1 year ago

baconpaul commented 1 year ago

just so i understand the protocol here from a plugin perspective

a host calls get_current_undo_info and the plugin returns the top of its undo stack or false. If the plugin returns the top of the undo stack and true the host may at a later date call request_undo with the id from that info. the plugin then checks if the id requested is still the top of the stack (so can still be applied) and if it is, returns true. After returning true, it does the undo and once the undo is done calls host->after_undo_request with success or failure.

If that's right, a few questions

  1. For informational purposes do we want the plugin to show the entire undo stack? Surge used to show this to the user (but it seems it doesn't in latest release! I should fix that). Or just the top?
  2. Should a plugin push undo items for host initiated changes? That is if a host does a destructive automation is that unable by the plugin? Or the host?
  3. If you never call after undo just curious what happens. No more undos from the host?
  4. begin/end change, undo, and redo. The plugin calls these as gates around a change, an undo, and a redo? So for instance anything you push to your undo stack gets a begin/end undo around it? If so, wouldn't almost every redo imply an undo and is that ok?
Trinitou commented 1 year ago
  1. For informational purposes do we want the plugin to show the entire undo stack? Surge used to show this to the user (but it seems it doesn't in latest release! I should fix that). Or just the top?

Maybe that could be part of an additional/future 'undo_history' extension? So a plugin just can support this single-step undo/redo extension and complete history would be (even more 😉) optional.

How I could imagine an undo/redo history feature: Additionally to get_current_undo/redo_info there could be a bool get_previous_undo_info(clap_plugin_t plugin, clap_id id, clap_undo_info_t info); Of course the counterpart would be a get_next_redo_info method. This would allow the host to iterate over the undo/redo steps to show a history. (Still the host would need to call request_undo/request_redo one by one to reach a point further down/up in history I suppose.)

  1. Should a plugin push undo items for host initiated changes? That is if a host does a destructive automation is that unable by the plugin? Or the host?

Yeah, also in general there should be some documentation/clarification/examples for what kind of events should cause an undo step to be generated and which shouldn't, I think. Like e.g. changing the plugin GUI scaling or renaming a preset in the plugin-internal browser shouldn't (I suppose) and so on. Even if it seems obvious I might not be for everybody.

abique commented 1 year ago

just so i understand the protocol here from a plugin perspective

a host calls get_current_undo_info and the plugin returns the top of its undo stack or false. If the plugin returns the top of the undo stack and true the host may at a later date call request_undo with the id from that info. the plugin then checks if the id requested is still the top of the stack (so can still be applied) and if it is, returns true. After returning true, it does the undo and once the undo is done calls host->after_undo_request with success or failure.

Yes that's right. Checking if the change is the one on top of the stack can be done asynchronously, this would be required if the plugin's undo manager isn't on the main thread.

If that's right, a few questions

1. For informational purposes do we want the plugin to show the entire undo stack? Surge used to show this to the user (but it seems it doesn't in latest release! I should fix that). Or just the top?

We just need to know the top one. Anything else can be known and remembered incrementally by the host, as the host lifetime is greater or equal to the plugin's lifetime.

Having the entire stack would make the interface much more complicated for very little benefit, and may force a highe design constraint on the plugin. We just need to know the top, when a change happened within the plugin, the host just adds in its own stack: "plugin X + change_id" and when the undo/redo happens (from the host) delegate that to the plugin.

2. Should a plugin push undo items for host initiated changes? That is if a host does a destructive automation is that unable by the plugin? Or the host?

I don't think that the plugin should deal with the host's undo stack. As a user, when I open the plugin window and work within it, I expect the undo/redo to be scoped to the plugin window. While when working on the host, some users expect the host's undo to include the plugin.

3. If you never call after undo just curious what happens. No more undos from the host?

Then it means that the plugin is misbehaving. The host could after some time, show a notification to the user: "plugin X is taking more time than expected to perform undo. Do you want to wait further or consider the operation as terminated?". Then the host could remember that the plugin's undo implementation is not reliable and decide to possibly not use it at all or to simply ignore the operation status (and not wait for it).

The host can also periodically poll the next undo/redo and see if it changed.

4. begin/end change, undo, and redo. The plugin calls these as gates around a change, an undo, and a redo? So for instance anything you push to your undo stack gets a begin/end undo around it? If so, wouldn't almost every redo imply an undo and is that ok?

change is when creating new change, and undo/redo when performing one of those. performing an undo doesn't create a new changes, it just moves the cursor down in the stack. If you perform an undo a change X, once finished I'd then implicitly expect that change X to be the current redo.

abique commented 1 year ago
  1. For informational purposes do we want the plugin to show the entire undo stack? Surge used to show this to the user (but it seems it doesn't in latest release! I should fix that). Or just the top?

Maybe that could be part of an additional/future 'undo_history' extension? So a plugin just can support this single-step undo/redo extension and complete history would be (even more wink) optional.

How I could imagine an undo/redo history feature: Additionally to get_current_undo/redo_info there could be a bool get_previous_undo_info(clap_plugin_t plugin, clap_id id, clap_undo_info_t info); Of course the counterpart would be a get_next_redo_info method. This would allow the host to iterate over the undo/redo steps to show a history. (Still the host would need to call request_undo/request_redo one by one to reach a point further down/up in history I suppose.)

Producing changes is an incremental process and the host can already record all the changes and cache the plugin's change history.

  1. Should a plugin push undo items for host initiated changes? That is if a host does a destructive automation is that unable by the plugin? Or the host?

Yeah, also in general there should be some documentation/clarification/examples for what kind of events should cause an undo step to be generated and which shouldn't, I think. Like e.g. changing the plugin GUI scaling or renaming a preset in the plugin-internal browser shouldn't (I suppose) and so on. Even if it seems obvious I might not be for everybody.

I'm not sure I want to make an exhaustive list of what goes in the undo and what doesn't. I'd expect that the plugin's undo stack is mirrored into the host. If a plugin starts flooding the undo stack, then of course the users won't like it.

Trinitou commented 1 year ago

If that's right, a few questions

  1. For informational purposes do we want the plugin to show the entire undo stack? Surge used to show this to the user (but it seems it doesn't in latest release! I should fix that). Or just the top?

We just need to know the top one. Anything else can be known and remembered incrementally by the host, as the host lifetime is greater or equal to the plugin's lifetime.

Having the entire stack would make the interface much more complicated for very little benefit, and may force a highe design constraint on the plugin. We just need to know the top, when a change happened within the plugin, the host just adds in its own stack: "plugin X + change_id" and when the undo/redo happens (from the host) delegate that to the plugin.

I think it might be a possible scenario that the plugin only remembers a certain limited number of undo steps (e.g. only the last 10 changes). So the host should not rely on all undo steps to be still present. How to catch that? Possible ways I see currently:

A and B might be too late for such a detection as the host's undo history might contain undo steps which are already discarded by the plugin. If C) is not viable, maybe D) might be the best option:

abique commented 1 year ago

I think it might be a possible scenario that the plugin only remembers a certain limited number of undo steps (e.g. only the last 10 changes). So the host should not rely on all undo steps to be still present. How to catch that?

I don't see any problems with that.

Possible ways I see currently:

* A) the host could be required to check get_current_undo_info before request_undo to validate that the undo step is still available

No, because it the plugin's undo manager lives in a different threads this approach would be racy.

* B) request_undo could return false or after_undo_request with success set to false and an according error message.

Yes that's what the current interface would do, if the given change_id isn't the next undo/redo, then don't do anything. It is like a compare_and_swap approach.

* C) the plugin could be required to keep all of its undo steps since instanciation if it supports this extension

This isn't necessary.

A and B might be too late for such a detection as the host's undo history might contain undo steps which are already discarded by the plugin. If C) is not viable, maybe D) might be the best option:

* D) there could be a method where the host in the beginning would ask the plugin for that maximum number of undo steps it can handle so that the host can discard old plugin undo steps accordingly. Could return -1 if the number is unlimited. (or 0 as this never makes sense as well if the plugin supports this extension at all)

Not necessary again.

baconpaul commented 1 year ago

performing an undo doesn't create a new changes, it just moves the cursor down in the stack. If you perform an undo a change X, once finished I'd then implicitly expect that change X to be the current redo.

so when you undo you actually create a redo which is the inverse of the undo. We run with a pair of stacks. But it sounds like the fact that an undo or redo changes the undo and redo stack shouldn’t generate a begin/end change event. The host after applying an undo or redo would have to refrain the top of both stacks. That’s totally fine I just want to make sure I understand it and we document it

explicitly

grab undo id Grab redo id Do undo Apply redo id will fail since this is isn’t the top even though no event arose from the plug-in to invalidate the stack

Is the protocol (vs the do undo generating a new invalidation message on both stacks)

abique commented 1 year ago

performing an undo doesn't create a new changes, it just moves the cursor down in the stack. If you perform an undo a change X, once finished I'd then implicitly expect that change X to be the current redo.

so when you undo you actually create a redo which is the inverse of the undo. We run with a pair of stacks. But it sounds like the fact that an undo or redo changes the undo and redo stack shouldn’t generate a begin/end change event. The host after applying an undo or redo would have to refrain the top of both stacks. That’s totally fine I just want to make sure I understand it and we document it

explicitly

grab undo id Grab redo id Do undo Apply redo id will fail since this is isn’t the top even though no event arose from the plug-in to invalidate the stack

Is the protocol (vs the do undo generating a new invalidation message on both stacks)

Yes, performing undo/redo, just moves the item from one stack to the other.