Add very basic skin editor undo / redo support

[peppy]

• Closes Add ability to undo in skin editor #12966

I made this work and it seems like it might be okay to get it in in this state? I'm hesitant to write tests or spend more time on this due to the fact that #22388 will likely require deep modifications in how change handling is done (assuming we move every target container to a single json structure or similar).

Put another way, I wasn't sure if I should work on undo/redo support before considering the aforementioned restructure, but getting it working to a MVP level was quick enough that I propose we get this out to users so they can benefit from it while the larger restructuring tasks happen in the background.

Of note: if any new elements are added or removed, all elements will be recycled. This is the simplest way of handling this, especially since we don't have any ID for tracking movement of elements around. There may be room to use SkinnableTargetContainer's bindable Components list to better handle this in the future.

osu.2023-02-03.at.10.02.43.mp4

PRing this to get some initial feedback on direction.

Issues of note:

• Changes to SettingsSourceAttributes do not trigger state saves.

[bdach]

Generally code seems inoffensive on a read-through, some structure issues from me though

[bdach]

With the way this is being abstracted away further, I'm not sure that the namespace or the xmldoc here make much sense.

Namespace comment also applies to IEditorChangeHandler and TransactionalCommitComponent.

[peppy]

Honestly wasn't sure if we should still keep the underlying TransactionalCommitComponent, but left it there to reduce changes for the time being. I'd be okay with moving this to another namespace, but I'm honestly not sure where they should exist. Open to suggestion.

[bdach]

Let's leave this where it is until the dust settles and we see the final shape of the skin editor better for now I guess.

[bdach]

I can break this pretty hard using the "scene library" toolbar.

1. Start at main menu
2. Click the "Gameplay" scene from the library
3. Wait for it to load
4. Make any change
5. Notice that you can undo twice, and the second undo erases every component from the skin

2023-02-06.19-08-59.mp4

It appears that the initial save in the skin editor change handler fails when doing this (resulting json is {}), because the firstTarget is null.

Careful not to trip over #22263 and #17391 when reproducing.

I also got this to crash once on a mismatched EndChange() without StartChange() call, but I'm not completely sure what I did to make it happen, so I'm not sure I'll be able to make it happen again.

[bdach]

Never mind, I repro'd the second thing immediately.

1. Go to gameplay
2. Seek track to be close to end
3. Open skin editor
4. Select a component and start dragging it with left mouse
5. Wait for map to end, progressing to results screen
6. Release mouse to 💥

2023-02-06.19-17-28.mp4

Crash trace is, unsurprisingly:

Unhandled exception. System.InvalidOperationException: Cannot call EndChange without a previous call to BeginChange.
   at osu.Game.Screens.Edit.TransactionalCommitComponent.EndChange() in /home/dachb/Documents/opensource/osu/osu.Game/Screens/Edit/TransactionalCommitComponent.cs:line 49
   at osu.Game.Overlays.SkinEditor.SkinEditor.EndChange() in /home/dachb/Documents/opensource/osu/osu.Game/Overlays/SkinEditor/SkinEditor.cs:line 485
   at osu.Game.Screens.Edit.Compose.Components.BlueprintContainer`1.OnDragEnd(DragEndEvent e) in /home/dachb/Documents/opensource/osu/osu.Game/Screens/Edit/Compose/Components/BlueprintContainer.cs:line 212
   at osu.Framework.Graphics.Drawable.TriggerEvent(UIEvent e)
   at System.Linq.Enumerable.TryGetFirst[TSource](IEnumerable`1 source, Func`2 predicate, Boolean& found)

[bdach]

see above

[peppy]

It appears that the initial save in the skin editor change handler fails when doing this (resulting json is {}), because the firstTarget is null.

This actually goes a bit deeper. firstTarget is non-null but the components haven't finished loading at the point the state is saved. SkinBlueprintContainer gets around this by watching for changes in components, but it has an easier task because it doesn't need to know which change is the initial one.. so if we used that method it would be best-effort..

osu/osu.Game/Overlays/SkinEditor/SkinBlueprintContainer.cs

Lines 56 to 62 in 26efb8e

	foreach (var targetContainer in targetContainers)
	{
	var bindableList = new BindableList<ISkinnableDrawable> { BindTarget = targetContainer.Components };
	bindableList.BindCollectionChanged(componentsChanged, true);
	targetComponents.Add(bindableList);
	}

Edit: And of course, the Components list is populated after async load of the components.

[peppy]

I've fixed both the issues you found. See what you think of the solutions.

[bdach]

Changes look okay (the ensureStateSaved() thing feels a little wonky but looks correct), issues look fixed, but e162fd5 broke tests and 0320ba7 broke code quality.

On a quick look the first one may not be very easy to fix, may just need to update assertions? Second issue should be trivial to resolve (not sure where that conditional access in the ctor there even comes from).

[bdach]

Seems solid enough for an initial implementation as far as I'm concerned.

I did find one issue with the "Revert to default" option and how it interacts with undo, but I think it's relatively minor and don't really want to be blocking this PR too much longer, so I'll open a follow-up issue for it.

[bdach]

Tracking aforementioned issue as #22562