Undo redo

[SuuperW]

Fixes various issues with undo and redo, along with a few related things. Most notably, I updated invalidation logic. (Maybe this doesn't belong in this PR? But at least it's related because undo and redo make use of the updates there, that's how I got into it.)

TasMovie's greenzone invalidations are now batched. This takes over one of the jobs that TAStudio's edit batching system was doing (from another recent PR of mine) so that BeginBatchEdit and related fucntions are not needed anymore. (but still in use for axis editing, that's a TODO for when I fix axis editing)

An invalidation batch represents a set of edits that should be considered one single edit by any code that is only concerned with knowing when the greenzone is invalidated, such as TAStudio wanting to know when it should refresh.

This introduces a small change to Lua behavior with tastudio.ongreenzoneinvalidated, in that it is now called less often. (just once per batch) Since it was already not a way for Lua to see every individual edited frame (some edits even in master branch change multiple frames with only one invalidation), this is probably okay. If it isn't okay, we should define in the Lua documentation exactly when it should be called, or provide another way to see when frames are edited.

There are new tests. I put together a FakeEmulator and FakeMovieSession to make these tests possible. I just got the tests to run, this may not be the best way to set things up.

Questions:

1. What should happen if the user presses Ctrl+Z in the middle of painting inputs (while the mouse button is down)? Should we disallow undo during paint? Keep painting but start a new batch? This PR does the latter. (Similar for right-click edits, but the effect of alt-rightclick [move frame] is perhaps not ideal since continuing the edit basically turns it into a copy since the code handling it doesn't anticipate an undo mid-operation... but really who's going to ever actually do that anyway?)
2. It's been mentioned in TAStudio's undo/redo code probably needs a rewrite #2301 that using undo can move the green arrow. I am not sure what the exact behavior was when that was written, but current behavior will move the green arrow if (and only if) making that same edit manually (e.g. paint an input with the mouse) would move the green arrow. (That is, if the user has manually changed the current frame [frame advancing/playing/rewinding/seeking by doing anything other than editing inputs], is not currently seeking to the green arrow, and the modified frame is in the past.) This seems reasonable to me, but @RetroEdit might disagree.
3. From one of my commits: "Use the marker list collection changed event to drive TAStudio refreshing, instead of doing a manual refresh on undo and everywhere else." Is this actually any better? Another option for undo purposes could be to have Undo/Redo return true/false indicating if the only change was to markers. This avoids a redundant refresh when undoing a marker change, but adds redundant refreshes if using "undo to here" with multiple marker changes.

Also I am aware of one issue that persists: If the movie has no inputs and you record a frame (which will be frame 0), then undo that frame record, TAStudio will automatically re-record a blank frame on frame 0 which deletes the redo history. This seems like a minor edge case bug and I don't see an easy fix so it may not be worth fixing.

Check if completed:

• I have run any relevant test suites
• I, the commit author, have read the licensing terms for contributors (last updated 2024-06-22) and am compliant

[YoshiRulz]

Testing more systems is definitely welcome! Are you able to add the tests first, or would it not compile?

[SuuperW]

I could probably make the tests be the first commit. I only added them when I was trying to figure out why there was a check for frame 0 and why there was a minus 1, but I never did actually figure that out. Both were added with commits that say they fix things without details so I assume they were not super obscure bugs, and probably related to some other code that has since changed.

I also don't think the tests are really relevant to the commits that are before the testing commit. I could try to add some more, but most of the changes are in TAStudio logic which isn't easily testable.

[vadosnaprimer]

Keep painting but start a new batch?

This.

[SuuperW]

So it turns out that tying undo batching to TAStudio batching isn't a great idea, because they are two different things and handling the edge cases correctly is confusing. I've re-done this PR (see updated OP for new approach and implications), including more bug fixes and tests.

[vadosnaprimer]

Is this actually any better?

Probably, tho can't tell if it has a functional difference too.

[SuuperW]

Force push added a new commit batching recording mode edits. #2301 should be fully resolved with this.

[SuuperW]

Can we go ahead and merge this?

[Morilli]

I don't feel comfortable reviewing >1k LOC changes, especially when I'm not even familiar with the original logic.

If you think this is a clear improvement over the previous implementation then I'm personally fine with merging and dealing with the inevitable fallout later in fixup patches. Especially the test infrastructure is probably going to help, there's lots of things that should be verified via tests but building the required test structure around it is hard, so having something to work off of is great.

[YoshiRulz]

I'd like to start with the unit tests. I had a look at them and they're not structured in the usual way, with a [TestInitialize] and [DataRow] (or separate methods where they pass delegates).
edit: snipe'd

[SuuperW]

I don't feel comfortable reviewing >1k LOC changes, especially when I'm not even familiar with the original logic.

That's fair. Especially since the original logic was difficult to understand, not well-organized, and inconsistent. I do think this is a clear improvement.

I'd like to start with the unit tests. I had a look at them and they're not structured in the usual way, with a [TestInitialize] and [DataRow] (or separate methods where they pass delegates). edit: snipe'd

The tests don't all initialize the same way, so I'm not sure [TestInitialize] is relevant. I'm also not sure where you'd want to use [DataRow]. But the primary reason I didn't use them is really just that I don't like declarative/attribute-based coding here so I didn't try.

Since you said "snipe'd" I will take that to mean you agree that it's fine to merge.

[vadosnaprimer]

I will test it in a couple days. Have the branch checked out, just distracted by something else.

[SuuperW]

rebased onto master, will wait for vadosnaprimer's test before merge

[YoshiRulz]

The tests don't all initialize the same way, so I'm not sure [TestInitialize] is relevant. I'm also not sure where you'd want to use [DataRow]. But the primary reason I didn't use them is really just that I don't like declarative/attribute-based coding here so I didn't try.

I imagined something like this.
8a65454 adds 25 tests but only 11 of them pass, and I was hoping that such a refactor would isolate the failures. With 94 of the now 121 tests passing it sort of did.

Since you said "snipe'd" I will take that to mean you agree that it's fine to merge.

No? It simply means that Morilli coincidentally posted while I was typing.
I've barely looked at anything apart from the tests.

[SuuperW]

I imagined something like this.

That looks absolutely horrid. This is so much more difficult to understand than what I wrote and looks way more error prone and difficult to maintain, since they share code in non-obvious ways. You've also converted some similar tests to a single method with multiple DataRow but this just removes the helpful test name (e.g. "RecordFrameZero" and "RecordFrameAtEnd" are now not as obviously being intentionally tested).
And I don't know what you mean by isolate the failures. Are they not already mostly isolated? A few tests are initially affected by two bugs but trying to avoid that would make the tests more convoluted and difficult to understand than they already are.

[vadosnaprimer]

I'm pressing Ctrl+Z and nothing is happening. Only works if I open the Undo dialog and press its buttons or doubleclick its rows. Shouldn't have mixed up configs, it does work from clean one.

Actual bug:

• Create new project
• Advance one frame.
• Undo
• Turn on Recording mode
• Unpause by hitting >||

System.ArgumentOutOfRangeException: Индекс за пределами диапазона. Индекс должен быть положительным числом, а его размер не должен превышать размер коллекции.
Имя параметра: index
в System.ThrowHelper.ThrowArgumentOutOfRangeException(ExceptionArgument argument, ExceptionResource resource)
в BizHawk.Client.Common.TasMovieChangeLog.MergeLastActions() в /src/BizHawk.Client.Common/movie/tasproj/TasMovie.History.cs:строка 196
в BizHawk.Client.EmuHawk.TAStudio.FastUpdateAfter() в /src/BizHawk.Client.EmuHawk/tools/TAStudio/TAStudio.IToolForm.cs:строка 119
в BizHawk.Client.EmuHawk.TAStudio.UpdateAfter() в /src/BizHawk.Client.EmuHawk/tools/TAStudio/TAStudio.IToolForm.cs:строка 109
в BizHawk.Client.EmuHawk.ToolManager.UpdateToolsAfter() в /src/BizHawk.Client.EmuHawk/tools/ToolManager.cs:строка 769
в BizHawk.Client.EmuHawk.MainForm.StepRunLoop_Core(Boolean force) в /src/BizHawk.Client.EmuHawk/MainForm.cs:строка 3265
в BizHawk.Client.EmuHawk.MainForm.ProgramRunLoop() в /src/BizHawk.Client.EmuHawk/MainForm.cs:строка 960
в BizHawk.Client.EmuHawk.Program.SubMain(String[] args) в /src/BizHawk.Client.EmuHawk/Program.cs:строка 377

Otherwise things I tried work ok.

[SuuperW]

Thanks, good find. I have fixed that bug.

Otherwise things I tried work ok.

OK to merge?

[vadosnaprimer]

If nobody minds.

[SuuperW]

@YoshiRulz do you mind?

[YoshiRulz]

I think merging and putting out a release candidate for testing would be a good idea, certainly more useful than a code review from me.