[4.1] undo/redo in media manager

[vlaucht]

Pull Request for Issue # .

Summary of Changes

Added Undo/Redo functionality for MediaManager Plugins.

All changes will be saved in a history, and can then be undone or redone with the buttons in the menu.

This could also be used to preserve the state of the plugin. As of now, if a change has been made and then the plugin is switched, the changes will be applied in a destructive way, and returning back to the plugin does not allow to return to the original state (For example: crop an image, switch to resize, go back to cropping and you will find, that it is not possible to extend the cropped area back to the images original size). The history stores the plugin state as well, and this could be used in a later PR to restore the state of the plugin.

Testing Instructions

Edit an image in the MediaManager and click undo or redo in the menu to revert or redo the changes.

[laoneo]

I have tested this item :white_check_mark: successfully on 4b2b742cc3552e8cccfb8b6b9ed65053af7479bb

Good work!!

---

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/30511.}

[laoneo]

Didn't do a code review, @dgrammatiko any feedback?

[dgrammatiko]

The code seems fine. Although when I added the @todos in the script I had a different implementation on my mind:

• diff the data so the memory doesn't explode exponentially
• ~~find a way to record the actions (eg form fields state) so the redo actually replays the last action~~
• Introduce a numerical field in the media manager options for the allowed history snapshots (ala Photoshop's history length)

EDIT The current implementation actually stores the plugin state so that's ok (the second point)

[vlaucht]

I have added a length limit option.

[vlaucht]

@dgrammatiko As for the first point, I believe I have already solved this by changing the events on which the history points are stored. They are only sent once a change has been made and not multiple times during the change how it was implemented before. For the last point I have added an option in the settings to limit the history length.

To restore the plugins state, I'm still trying to figure out a solution. I do store the plugins state, but since the changed image is rendered and stored every time the plugin changes, I would need to somehow go over each plugin and apply the changes to the original image again. So for example, if my first change is cropping the image, and then I switch to resize and change the size, I'm applying this to the already cropped image. If I go back to the crop plugin, I would need to apply the crop to the original image to allow the user to extend the crop area, but then I would also need to apply resize (and all other changes made in other plugins) to the original image as well. Any thougths?

[dgrammatiko]

@vlaucht I think there is a fundamental piece missing here, you need to register the plugin execution function with their payload in order to replay them. A thought experiment, that will probably work in this context is like:

// Create an API point that all plugins run in order to execute a command
// Execute Plugin Command
Joomla.MediaManager.execCommand = (command, payload) => {
  new Promise(command({ ...payload }))
    .then(resolve => storeToHistory)
    .catch(rollbackLastHistoryEntry);
};

// The history store also needs to change a little bit like
// I think what you have right now is an array of {file: 'string', data: 'string'}
which should become
{
file: 'string',
data: 'string',
command: 'string',
payload: {}
}

// Maybe also you need to store the plugin name and the tab index
// So you could safely switch to the right context (execCommand will need to switch plugins iirc)

The combination of these changes should allow you to replay safely any plugin command. Of course, quite some checks need to be implemented but I think this approach is viable

[vlaucht]

Right now it stores the following: { file: 'string', data: { pluginName (e.g. 'crop'): { pluginData (e.g. width, heigth }, } }

So do you mean I need to implement an exec function and then run it for each plugin in order to restore the images state?

[dgrammatiko]

So do you mean I need to implement an exec function and then run it for each plugin in order to restore the images state?

I think it's safe to assume that there might be some intermediate functions in the chain of execution. Eg an input has an oninput event that executes a function and manipulates the value before handing it to the execution function, so by storing only the form data is not safe for replaying the command. I think the idea to have an API that is called per plugin command is the only safe way here. By the way the implementation of the history will also be simpler (the history points will derive from the executed commands, sequentially, so theoretically it should need less pampering)

[vlaucht]

Ah, I see. But this still only works per plugin. It doesnt solve the issue how to restore all other plugins simultaneously.

So if I apply crop to an image (see step2) and then rotate it (step3) and then go back to crop and want to increase the cropped area, I cant (step4), because it is already rendered. So I need to apply the crop effect stored in the history again to the original image, to let me increase the cropped area. But then I would also need to apply rotate to this image again. Same goes for all other plugins applied in between.

[dgrammatiko]

But this still only works per plugin. It doesnt solve the issue how to restore all other plugins simultaneously.

That's the reason for a promise based solution. I had a comment Maybe also you need to store the plugin name and the tab index which means that if you want to undo 2 steps the program will queue 2 commands (history -1 and then history - 2) and execute them sequentially. Also the tab select iirc correctly has already some initialization code for each plugin, thus you also have to replay that part (afaik right now this is synchronous so it might have to be done in an async way (promise or await) to make sure you don't end up in race conditions

and then go back to crop and want to increase the cropped area, I cant (step4)

Not all commands will be always executable and that is expected (also in photoshop you don't get to redo everything), thus the catch part, you can throw am alert, something like this command cannot be re-applied, due to changed dimensions or something along that line

[richard67]

I've allowed myself to fix the conflicts here because they were caused by the recent merge of my PR #30373 .

[laoneo]

@vlaucht there are now some styling issues, can you have a look on them? Then I would do another round of tests. If all is ok, would be nice to get that in. If there are then still some tasks open, we can do them in a followup pr or open an issue.

[particthistle]

When you make a change without the patch applied, clicking the reset button returns you back to the first history point (original loading of the image you're editing).

When you make a change with the patch applied, clicking the reset button does nothing now.

• The reset button should remain, and should reload the original loaded image.
• Additionally, there's no notification that the file as been successfully saved (like you would have on any other content in the system), so perhaps this could be added at the same time (or does another issue need to be raised?).

Aside from that, very nice addition to the Media Manager.

[particthistle]

I have tested this item :red_circle: unsuccessfully on 7034307b56cede8797dd32e855a2aabb11a55b88

Addition of Undo/Redo breaks Reset functionality per the above (Seems I missed hitting submit on this last night when testing.)

---

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/30511.}

[dgrammatiko]

Addition of Undo/Redo breaks Reset functionality per the above

Save is a destructing actions or plain English: reset only works from the last loaded image (when you hit save the saved image is your new starting point). That was the initial implementation AFAIR so there is nothing broken here

[Tejashrimajage]

I have tested this item :red_circle: unsuccessfully on 7034307b56cede8797dd32e855a2aabb11a55b88

---

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/30511.}

[PhilETaylor]

Please can you fix the merge issues with the Conflicting files so that this can be retested.

[HLeithner]

This pull request has automatically rebased to 4.2-dev.

[rdeutz]

Good idea, thanks for proposing the feature.

Closing this PR because of inactivity for a longer time.

[brianteeman]

@rdeutz

#PROD2020/024 - There shall be no indiscriminate closing of Issues or Pull Requests Closing of Issues and Pull requests PR's shall NOT be done without underlying rationale and individual reasoning in the Issue or PR. Elapsing of a time period, inactivity of the submitter are examples that do NOT constitute valid reasons in and by themselves.