TAStudio modifies global "On Movie End" setting

[RetroEdit]

(Split from #3735 ; originally written 2023-09-22)
I recently realized Play Movie will automatically override your On Movie End setting to be Record. EDIT 2024-08-18: See below: it's probably not directly Play Movie related.

This is extremely undesirable behavior for me... it's definitely possible to change the toggle inside Play Movie to pause on a specific frame, but the current behavior of silently altering user-selected settings seems extremely bad practice to me. It's sort of a UI choice more than a bug per se, though.
At the very least, it should remember if you wanted to end on a specific frame (unfortunately, this alone may have downsides because pausing on a non-movie end frame via play movie is sort of specific to the movie). Personally, I would like the a direct equivalent to the On Movie End options selection visible within Play Movie if it's going to permanently alter those settings.

[Morilli]

Can you explain when this happens? Just playing a movie using the play movie dialog does respect the "On Movie End" settings for me.

[RetroEdit]

Okay, I can reproduce this, but my steps are a little weird and artificial. Starting with no config:

1. Open game, record movie, stop movie
2. Use play movie to load the movie
   2a. Upon further testing, opening the movie in other ways (Recent list, drag+drop) also seems to work under certain circumstances?
3. Open TAStudio
4. Manually save the config using Config > Save Config
5. With TAStudio still open, force EmuHawk to crash to avoid overwriting the config. My method of choice: open Lua console, enter while true do end into it, when it says the EmuHawk is not responding, close it.

There seem to only be two places in the entire codebase where the OnMovieEnd setting is changed implicitly: TAStudio and Macros.

BizHawk/src/BizHawk.Client.EmuHawk/tools/TAStudio/TAStudio.cs

Lines 290 to 292 in d9331c5

	_originalEndAction = Config.Movies.MovieEndAction;
	MainForm.DisableRewind();
	Config.Movies.MovieEndAction = MovieEndAction.Record;

BizHawk/src/BizHawk.Client.EmuHawk/tools/TAStudio/TAStudio.cs

Line 741 in d9331c5

Config.Movies.MovieEndAction = _originalEndAction;

BizHawk/src/BizHawk.Client.EmuHawk/tools/Macros/MovieZone.cs

Lines 200 to 204 in 963b925

	if (movie.InputLogLength >= _emulator.Frame)
	{
	movie.SwitchToPlay();
	config.Movies.MovieEndAction = MovieEndAction.Record; // TODO: this is a bad place to do this, and introduces a config dependency
	}

To me, implicit config changes to OnMovieEnd seem kind of hacky, especially when it's changing it to "Record", which can append junk to the end of your movie. As demonstrated above, if you open TAStudio and it doesn't clean up properly (e.g., maybe it crashed for some reason), it will indeed permanently and silently save what is intended as a temporary setting change to your config. This TAStudio detail seems the most likely culprit for the issue I encountered 11 months ago -- although it was probably more along the lines of TAStudio exiting unusually and then me closing the emulator and having the config save upon BizHawk closing.

TAStudio should keep that info within the TAStudio session if it needs it, not touch the config at all. No need to preserve the original 10-year old code of b53cc90 .

I can't speak to macros because I haven't used them enough, but silent implicit modification also seems questionable there.

[Morilli]

This doesn't seem trivial to do. Well, it is kinda, but it's a bit hacky. The main issue here is that the following code needs to know whether tastudio is active:

BizHawk/src/BizHawk.Client.Common/movie/MovieSession.cs

Lines 97 to 107 in ccb69b1

	public void HandleFrameAfter()
	{
	if (Movie is ITasMovie tasMovie)
	{
	tasMovie.GreenzoneCurrentFrame();
	if (tasMovie.IsPlayingOrFinished() && Settings.MovieEndAction == MovieEndAction.Record && Movie.Emulator.Frame >= tasMovie.InputLogLength)
	{
	HandleFrameLoopForRecordMode();
	return;
	}
	}

Just knowing that the current movie is a TasMovie is not enough when we want to keep support for the movie end action when playing tasproj movies outside of TAStudio.
But checking TAStudio's load state in MovieSession is... not possible without either passing it in or doing other questionable things.
Here's a conceptual diff that would work if ^ was solved:

diff --git a/src/BizHawk.Client.Common/movie/MovieSession.cs b/src/BizHawk.Client.Common/movie/MovieSession.cs
index d1a748fe1..11ef21269 100644
--- a/src/BizHawk.Client.Common/movie/MovieSession.cs
+++ b/src/BizHawk.Client.Common/movie/MovieSession.cs
@@ -99,7 +99,8 @@ namespace BizHawk.Client.Common
 			if (Movie is ITasMovie tasMovie)
 			{
 				tasMovie.GreenzoneCurrentFrame();
-				if (tasMovie.IsPlayingOrFinished() && Settings.MovieEndAction == MovieEndAction.Record && Movie.Emulator.Frame >= tasMovie.InputLogLength)
+				bool tastudioLoaded = false;
+				if (tasMovie.IsPlayingOrFinished() && (tastudioLoaded || Settings.MovieEndAction == MovieEndAction.Record) && Movie.Emulator.Frame >= tasMovie.InputLogLength)
 				{
 					HandleFrameLoopForRecordMode();
 					return;
diff --git a/src/BizHawk.Client.EmuHawk/MainForm.Events.cs b/src/BizHawk.Client.EmuHawk/MainForm.Events.cs
index 5fb9121a1..f7ebd3ecb 100644
--- a/src/BizHawk.Client.EmuHawk/MainForm.Events.cs
+++ b/src/BizHawk.Client.EmuHawk/MainForm.Events.cs
@@ -189,13 +189,6 @@ namespace BizHawk.Client.EmuHawk
 			MovieEndRecordMenuItem.Checked = Config.Movies.MovieEndAction == MovieEndAction.Record;
 			MovieEndStopMenuItem.Checked = Config.Movies.MovieEndAction == MovieEndAction.Stop;
 			MovieEndPauseMenuItem.Checked = Config.Movies.MovieEndAction == MovieEndAction.Pause;
-
-			// Arguably an IControlMainForm property should be set here, but in reality only Tastudio is ever going to interfere with this logic
-			MovieEndFinishMenuItem.Enabled =
-			MovieEndRecordMenuItem.Enabled =
-			MovieEndStopMenuItem.Enabled =
-			MovieEndPauseMenuItem.Enabled =
-				!Tools.Has<TAStudio>();
 		}
 
 		private void AVSubMenu_DropDownOpened(object sender, EventArgs e)
diff --git a/src/BizHawk.Client.EmuHawk/tools/TAStudio/TAStudio.cs b/src/BizHawk.Client.EmuHawk/tools/TAStudio/TAStudio.cs
index 04c2917fd..2e353177d 100644
--- a/src/BizHawk.Client.EmuHawk/tools/TAStudio/TAStudio.cs
+++ b/src/BizHawk.Client.EmuHawk/tools/TAStudio/TAStudio.cs
@@ -34,7 +34,6 @@ namespace BizHawk.Client.EmuHawk
 		private readonly List<TasClipboardEntry> _tasClipboard = new List<TasClipboardEntry>();
 		private const string CursorColumnName = "CursorColumn";
 		private const string FrameColumnName = "FrameColumn";
-		private MovieEndAction _originalEndAction; // The movie end behavior selected by the user (that is overridden by TAStudio)
 		private UndoHistoryForm _undoForm;
 		private Timer _autosaveTimer;
 
@@ -287,9 +286,7 @@ namespace BizHawk.Client.EmuHawk
 			MainForm.AddOnScreenMessage("TAStudio engaged");
 			SetTasMovieCallbacks(CurrentTasMovie);
 			UpdateWindowTitle();
-			_originalEndAction = Config.Movies.MovieEndAction;
 			MainForm.DisableRewind();
-			Config.Movies.MovieEndAction = MovieEndAction.Record;
 			MainForm.SetMainformMovieInfo();
 			MovieSession.ReadOnly = true;
 			SetSplicer();
@@ -716,7 +713,6 @@ namespace BizHawk.Client.EmuHawk
 			_engaged = false;
 			MainForm.PauseOnFrame = null;
 			MainForm.AddOnScreenMessage("TAStudio disengaged");
-			Config.Movies.MovieEndAction = _originalEndAction;
 			WantsToControlRewind = false;
 			MainForm.EnableRewind(true);
 			MainForm.SetMainformMovieInfo();