search

AdamsLair / duality

a 2D Game Development Framework
https://adamslair.github.io/duality
MIT License
1.41k stars 288 forks source link

List dict add remove undo support #623

Closed deanljohnson closed 6 years ago

deanljohnson commented 6 years ago

Summary

Implemented Undo and Redo support for IList and IDictionary property editors as discussed in #426. Writeback is automatically handled through the macro system of the UndoRedoManager.

There exists a minor issue with the AddKeyToDictionary action. Undo'ing the action requires two ctrl+z's. It seems like typing in the Add Entry field is pushing an EditPropertyAction onto the undo/redo stack. I'm not sure how to remove that or how to append it to the AddKeyToDictionary action. CanAppend is not called on the AddKeyToDictionary action.

Another minor issue is the use of DualityEditorApp.NotifyObjPropChanged(this, new ObjectSelection(Scene.Current)); to allow the PropertyEditors to update after the action is done/undone. We don't have references to the target objects and so we can't send a more specific ObjPropChanged signal. This is used in quite a few other UndoRedoActions so I don't think it's that bad, but it'd be nice to be able to emit a more specific signal.

ilexp commented 6 years ago

cc @AdamsLair/duality-contributors for first-level code review.

ilexp commented 6 years ago

There exists a minor issue with the AddKeyToDictionary action. Undo'ing the action requires two ctrl+z's. It seems like typing in the Add Entry field is pushing an EditPropertyAction onto the undo/redo stack.

That's weird. Not strictly speaking part of the issue or PR, but since you're on similar code, can you check out why that happens / where it is triggered? This probably shouldn't trigger an UndoRedo in the first place. If there's an easy fix, great. Otherwise, the double Ctrl+Z could just remain for now, to be solved later.

Another minor issue is the use of DualityEditorApp.NotifyObjPropChanged(this, new ObjectSelection(Scene.Current)); to allow the PropertyEditors to update after the action is done/undone. We don't have references to the target objects and so we can't send a more specific ObjPropChanged signal. This is used in quite a few other UndoRedoActions so I don't think it's that bad, but it'd be nice to be able to emit a more specific signal.

This should be fine. The editor wide NotifyObjPropChanged is intended for objects that are edited directly, i.e. ones that you can make a selection of an edit in the Object Inspector or a specialized editor. More fine-grained notifications (like for specific list or dictionary objects within properties) should be mapped to their next biggest parent, because in the end someone else needs to determine whether a change event affects their UI, and they don't know or want to deal with any specific objects current implementation details.

Edit: Ah, just re-read your message - you are talking about this being a Scene notification rather than a Component or GameObject specific one. I agree completely, this would be neat, but probably something for a new issue. As you noted, this is common among some UndoRedo actions and could be addressed for them as a group at some point.

ilexp commented 6 years ago

Merged. Thanks! 🙂

Edit: Just noticed after the merge that I previously asked whether you could look into the rogue "Add Key" UndoRedo action - would have been easier to wait with the merge until later. My bad - was already in review mode. If you find a good fix, let's do this in another PR.