From afcc3f7d3f5583a384125d2ec2d4338f79b07f28 Mon Sep 17 00:00:00 2001 From: SuuperW Date: Sat, 12 Jul 2025 21:57:43 -0500 Subject: [PATCH 01/11] Handle exceptions while writing a movie file, and use a temporary file until writing has completed. Avoids crashing and prevents loss of data in the event of an exception mid-write. --- .global.editorconfig.ini | 9 ++ .../Api/Classes/MovieApi.cs | 4 +- src/BizHawk.Client.Common/FileWriteResult.cs | 96 ++++++++++++ src/BizHawk.Client.Common/FileWriter.cs | 131 +++++++++++++++++ .../FrameworkZipWriter.cs | 108 +++++++++++--- src/BizHawk.Client.Common/IZipWriter.cs | 12 ++ .../movie/MovieConversionExtensions.cs | 12 +- .../movie/MovieSession.cs | 18 ++- .../movie/bk2/Bk2Movie.IO.cs | 38 +++-- .../movie/import/IMovieImport.cs | 6 +- .../movie/interfaces/IMovie.cs | 4 +- .../movie/interfaces/IMovieSession.cs | 2 +- .../movie/tasproj/TasMovie.cs | 4 +- .../savestates/SavestateFile.cs | 12 +- .../savestates/ZipStateSaver.cs | 34 ++++- src/BizHawk.Client.EmuHawk/MainForm.Movie.cs | 9 +- src/BizHawk.Client.EmuHawk/MainForm.cs | 39 +++-- .../movie/EditCommentsForm.cs | 3 +- .../tools/TAStudio/TAStudio.IToolForm.cs | 17 ++- .../tools/TAStudio/TAStudio.MenuItems.cs | 48 ++++-- .../tools/TAStudio/TAStudio.cs | 138 +++++++++++++----- .../Movie/FakeMovieSession.cs | 6 +- 22 files changed, 625 insertions(+), 125 deletions(-) create mode 100644 src/BizHawk.Client.Common/FileWriteResult.cs create mode 100644 src/BizHawk.Client.Common/FileWriter.cs diff --git a/.global.editorconfig.ini b/.global.editorconfig.ini index cfb2583f906..c8ecdcc3f08 100644 --- a/.global.editorconfig.ini +++ b/.global.editorconfig.ini @@ -141,6 +141,15 @@ dotnet_diagnostic.CA2229.severity = silent # Opt in to preview features before using them dotnet_diagnostic.CA2252.severity = silent # CSharpDetectPreviewFeatureAnalyzer very slow +## Nullable rules; generics are a bit wonky and I have no idea why we're allowed to configure these to be not errors or why they aren't errors by default. + +# Nullability of reference types in value doesn't match target type. +dotnet_diagnostic.CS8619.severity = error +# Make Foo an error for class Foo where T : class. Use `where T : class?` if Foo should be allowed. +dotnet_diagnostic.CS8634.severity = error +# Nullability of type argument doesn't match 'notnull' constraint. +dotnet_diagnostic.CS8714.severity = error + ## .NET DocumentationAnalyzers style rules # Place text in paragraphs diff --git a/src/BizHawk.Client.Common/Api/Classes/MovieApi.cs b/src/BizHawk.Client.Common/Api/Classes/MovieApi.cs index 6f6bbe5a4fb..8042fadd0c6 100644 --- a/src/BizHawk.Client.Common/Api/Classes/MovieApi.cs +++ b/src/BizHawk.Client.Common/Api/Classes/MovieApi.cs @@ -53,6 +53,7 @@ public string GetInputAsMnemonic(int frame) return Bk2LogEntryGenerator.GenerateLogEntry(_movieSession.Movie.GetInputState(frame)); } + // TODO: Change return type to FileWriteResult public void Save(string filename) { if (_movieSession.Movie.NotActive()) @@ -70,7 +71,8 @@ public void Save(string filename) } _movieSession.Movie.Filename = filename; } - _movieSession.Movie.Save(); + FileWriteResult result = _movieSession.Movie.Save(); + if (result.Exception != null) throw result.Exception; } public IReadOnlyDictionary GetHeader() diff --git a/src/BizHawk.Client.Common/FileWriteResult.cs b/src/BizHawk.Client.Common/FileWriteResult.cs new file mode 100644 index 00000000000..1c5913326e8 --- /dev/null +++ b/src/BizHawk.Client.Common/FileWriteResult.cs @@ -0,0 +1,96 @@ +#nullable enable + +using System.Diagnostics; +using System.IO; + +namespace BizHawk.Client.Common +{ + public enum FileWriteEnum + { + Success, + FailedToOpen, + FailedDuringWrite, + FailedToDeleteOldFile, + FailedToRename, + } + + /// + /// Provides information about the success or failure of an attempt to write to a file. + /// + public class FileWriteResult + { + public readonly FileWriteEnum Error = FileWriteEnum.Success; + public readonly Exception? Exception; + public readonly string WritePath; + + public bool IsError => Error != FileWriteEnum.Success; + + public FileWriteResult(FileWriteEnum error, string path, Exception? exception) + { + Error = error; + Exception = exception; + WritePath = path; + } + + /// + /// Converts this instance to a different generic type. + /// The new instance will take the value given only if this instance has no error. + /// + /// The value of the new instance. Ignored if this instance has an error. + public FileWriteResult Convert(T value) where T : class + { + if (Error == FileWriteEnum.Success) return new(value, WritePath); + else return new(this); + } + + public FileWriteResult(FileWriteResult other) : this(other.Error, other.WritePath, other.Exception) { } + + public string UserFriendlyErrorMessage() + { + switch (Error) + { + case FileWriteEnum.Success: + return $"The file {WritePath} was written successfully."; + case FileWriteEnum.FailedToOpen: + if (WritePath.Contains(".saving")) + { + return $"The temporary file {WritePath} already exists and could not be deleted."; + } + return $"The file {WritePath} could not be created."; + case FileWriteEnum.FailedDuringWrite: + return $"An error occurred while writing the file."; // No file name here; it should be deleted. + case FileWriteEnum.FailedToDeleteOldFile: + string fileWithoutPath = Path.GetFileName(WritePath); + return $"The file {WritePath} was created successfully, but the old file could not be deleted. You may manually rename the temporary file {fileWithoutPath}."; + case FileWriteEnum.FailedToRename: + fileWithoutPath = Path.GetFileName(WritePath); + return $"The file {WritePath} was created successfully, but could not be renamed. You may manually rename the temporary file {fileWithoutPath}."; + default: + return "unreachable"; + } + } + } + + /// + /// Provides information about the success or failure of an attempt to write to a file. + /// If successful, also provides a related object instance. + /// + public class FileWriteResult : FileWriteResult where T : class // Note: "class" also means "notnull". + { + /// + /// Value will be null if is true. + /// Otherwise, Value will not be null. + /// + public readonly T? Value = default; + + public FileWriteResult(FileWriteEnum error, string path, Exception? exception) : base(error, path, exception) { } + + public FileWriteResult(T value, string path) : base(FileWriteEnum.Success, path, null) + { + Debug.Assert(value != null, "Should not give a null value on success. Use the non-generic type if there is no value."); + Value = value; + } + + public FileWriteResult(FileWriteResult other) : base(other.Error, other.WritePath, other.Exception) { } + } +} diff --git a/src/BizHawk.Client.Common/FileWriter.cs b/src/BizHawk.Client.Common/FileWriter.cs new file mode 100644 index 00000000000..4486d5bc098 --- /dev/null +++ b/src/BizHawk.Client.Common/FileWriter.cs @@ -0,0 +1,131 @@ +#nullable enable + +using System.Diagnostics; +using System.IO; + +using BizHawk.Common.StringExtensions; + +namespace BizHawk.Client.Common +{ + public class FileWriter : IDisposable + { + private FileStream? _stream; // is never null until this.Dispose() + public FileStream Stream + { + get => _stream ?? throw new ObjectDisposedException("Cannot access a disposed FileStream."); + } + public string FinalPath; + public string TempPath; + + public bool UsingTempFile => TempPath != FinalPath; + + private bool _finished = false; + + private FileWriter(string final, string temp, FileStream stream) + { + FinalPath = final; + TempPath = temp; + _stream = stream; + } + + // There is no public constructor. This is the only way to create an instance. + public static FileWriteResult Create(string path) + { + string writePath = path; + try + { + // If the file already exists, we will write to a temporary location first and preserve the old one until we're done. + if (File.Exists(path)) + { + writePath = path.InsertBeforeLast('.', ".saving", out bool inserted); + if (!inserted) writePath = $"{path}.saving"; + + if (File.Exists(writePath)) + { + // The user should probably have dealt with this on the previously failed save. + // But maybe we should support plain old "try again", so let's delete it. + File.Delete(writePath); + } + } + FileStream fs = new(writePath, FileMode.Create, FileAccess.Write); + return new(new FileWriter(path, writePath, fs), writePath); + } + catch (Exception ex) // There are many exception types that file operations might raise. + { + return new(FileWriteEnum.FailedToOpen, writePath, ex); + } + } + + /// + /// This method must be called after writing has finished and must not be called twice. + /// Dispose will be called regardless of the result. + /// + /// If called twice. + public FileWriteResult CloseAndDispose() + { + // In theory it might make sense to allow the user to try again if we fail inside this method. + // If we implement that, it is probably best to make a static method that takes a FileWriteResult. + // So even then, this method should not ever be called twice. + if (_finished) throw new InvalidOperationException("Cannot close twice."); + + _finished = true; + Dispose(); + + if (!UsingTempFile) return new(FileWriteEnum.Success, FinalPath, null); + + if (File.Exists(FinalPath)) + { + try + { + File.Delete(FinalPath); + } + catch (Exception ex) + { + return new(FileWriteEnum.FailedToDeleteOldFile, TempPath, ex); + } + } + try + { + File.Move(TempPath, FinalPath); + } + catch (Exception ex) + { + return new(FileWriteEnum.FailedToRename, TempPath, ex); + } + + return new(FileWriteEnum.Success, FinalPath, null); + } + + /// + /// Closes and deletes the file. Use if there was an error while writing. + /// Do not call after this. + /// + public void Abort() + { + if (_dispoed) throw new ObjectDisposedException("Cannot use a disposed file stream."); + _finished = true; + Dispose(); + + try + { + // Delete because the file is almost certainly useless and just clutter. + File.Delete(TempPath); + } + catch { /* eat? this is probably not very important */ } + } + + private bool _dispoed; + public void Dispose() + { + if (_dispoed) return; + _dispoed = true; + + _stream!.Flush(flushToDisk: true); + _stream.Dispose(); + _stream = null; + + // The caller should call CloseAndDispose and handle potential failure. + Debug.Assert(_finished, $"{nameof(FileWriteResult)} should not be disposed before calling {nameof(CloseAndDispose)}"); + } + } +} diff --git a/src/BizHawk.Client.Common/FrameworkZipWriter.cs b/src/BizHawk.Client.Common/FrameworkZipWriter.cs index e57bcfef63b..a0e18891b83 100644 --- a/src/BizHawk.Client.Common/FrameworkZipWriter.cs +++ b/src/BizHawk.Client.Common/FrameworkZipWriter.cs @@ -1,3 +1,5 @@ +#nullable enable + using System.IO; using System.IO.Compression; @@ -7,18 +9,19 @@ namespace BizHawk.Client.Common { public class FrameworkZipWriter : IZipWriter { - private ZipArchive _archive; + private ZipArchive? _archive; - private FileStream _fs; + private FileWriter? _fs; - private Zstd _zstd; + private Zstd? _zstd; private readonly CompressionLevel _level; private readonly int _zstdCompressionLevel; - public FrameworkZipWriter(string path, int compressionLevel) + private Exception? _writeException = null; + private bool _disposed; + + private FrameworkZipWriter(int compressionLevel) { - _fs = new(path, FileMode.Create, FileAccess.Write); - _archive = new(_fs, ZipArchiveMode.Create, leaveOpen: true); if (compressionLevel == 0) _level = CompressionLevel.NoCompression; else if (compressionLevel < 5) @@ -32,38 +35,95 @@ public FrameworkZipWriter(string path, int compressionLevel) _zstdCompressionLevel = compressionLevel * 2 + 1; } - public void WriteItem(string name, Action callback, bool zstdCompress) + public static FileWriteResult Create(string path, int compressionLevel) + { + FileWriteResult fs = FileWriter.Create(path); + if (fs.IsError) return new(fs); + + FrameworkZipWriter ret = new(compressionLevel); + ret._fs = fs.Value!; + ret._archive = new(ret._fs.Stream, ZipArchiveMode.Create, leaveOpen: true); + + return fs.Convert(ret); + } + + public FileWriteResult CloseAndDispose() { - // don't compress with deflate if we're already compressing with zstd - // this won't produce meaningful compression, and would just be a timesink - using var stream = _archive.CreateEntry(name, zstdCompress ? CompressionLevel.NoCompression : _level).Open(); + if (_archive == null || _fs == null) throw new ObjectDisposedException("Cannot use disposed ZipWriter."); + + // We actually have to do this here since it has to be done before the file stream is closed. + _archive.Dispose(); + _archive = null; - if (zstdCompress) + FileWriteResult ret; + if (_writeException == null) { - using var z = _zstd.CreateZstdCompressionStream(stream, _zstdCompressionLevel); - callback(z); + ret = _fs.CloseAndDispose(); } else { - callback(stream); + ret = new(FileWriteEnum.FailedDuringWrite, _fs.TempPath, _writeException); + _fs.Abort(); } + + // And since we have to close stuff, there's really no point in not disposing here. + Dispose(); + return ret; } - public void Dispose() + public void Abort() + { + if (_archive == null || _fs == null) throw new ObjectDisposedException("Cannot use disposed ZipWriter."); + + _archive.Dispose(); + _archive = null; + + _fs.Abort(); + + Dispose(); + } + + public void WriteItem(string name, Action callback, bool zstdCompress) { - if (_archive != null) + if (_archive == null || _zstd == null) throw new ObjectDisposedException("Cannot use disposed ZipWriter."); + if (_writeException != null) return; + + try { - _archive.Dispose(); - _archive = null; + // don't compress with deflate if we're already compressing with zstd + // this won't produce meaningful compression, and would just be a timesink + using var stream = _archive.CreateEntry(name, zstdCompress ? CompressionLevel.NoCompression : _level).Open(); + + if (zstdCompress) + { + using var z = _zstd.CreateZstdCompressionStream(stream, _zstdCompressionLevel); + callback(z); + } + else + { + callback(stream); + } } - _fs?.Flush(flushToDisk: true); - _fs?.Dispose(); - _fs = null; - if (_zstd != null) + catch (Exception ex) { - _zstd.Dispose(); - _zstd = null; + _writeException = ex; + // We aren't returning the failure until closing. Should we? I don't want to refactor that much calling code without a good reason. } } + + public void Dispose() + { + if (_disposed) return; + _disposed = true; + + // _archive should already be disposed by CloseAndDispose, but just in case + _archive?.Dispose(); + _archive = null; + _zstd!.Dispose(); + _zstd = null; + + _fs!.Dispose(); + _fs = null; + } } } diff --git a/src/BizHawk.Client.Common/IZipWriter.cs b/src/BizHawk.Client.Common/IZipWriter.cs index fa1de4e6513..f061b7ed74a 100644 --- a/src/BizHawk.Client.Common/IZipWriter.cs +++ b/src/BizHawk.Client.Common/IZipWriter.cs @@ -5,5 +5,17 @@ namespace BizHawk.Client.Common public interface IZipWriter : IDisposable { void WriteItem(string name, Action callback, bool zstdCompress); + + /// + /// This method must be called after writing has finished and must not be called twice. + /// Dispose will be called regardless of the result. + /// + FileWriteResult CloseAndDispose(); + + /// + /// Closes and deletes the file. Use if there was an error while writing. + /// Do not call after this. + /// + void Abort(); } } diff --git a/src/BizHawk.Client.Common/movie/MovieConversionExtensions.cs b/src/BizHawk.Client.Common/movie/MovieConversionExtensions.cs index 945a216c7a8..aa4c913c243 100644 --- a/src/BizHawk.Client.Common/movie/MovieConversionExtensions.cs +++ b/src/BizHawk.Client.Common/movie/MovieConversionExtensions.cs @@ -71,7 +71,7 @@ public static IMovie ToBk2(this IMovie old) return bk2; } - public static ITasMovie ConvertToSavestateAnchoredMovie(this ITasMovie old, int frame, byte[] savestate) + public static FileWriteResult ConvertToSavestateAnchoredMovie(this ITasMovie old, int frame, byte[] savestate) { string newFilename = ConvertFileNameToTasMovie(old.Filename); @@ -115,11 +115,11 @@ public static ITasMovie ConvertToSavestateAnchoredMovie(this ITasMovie old, int } } - tas.Save(); - return tas; + FileWriteResult saveResult = tas.Save(); + return saveResult.Convert(tas); } - public static ITasMovie ConvertToSaveRamAnchoredMovie(this ITasMovie old, byte[] saveRam) + public static FileWriteResult ConvertToSaveRamAnchoredMovie(this ITasMovie old, byte[] saveRam) { string newFilename = ConvertFileNameToTasMovie(old.Filename); @@ -146,8 +146,8 @@ public static ITasMovie ConvertToSaveRamAnchoredMovie(this ITasMovie old, byte[] tas.Subtitles.Add(sub); } - tas.Save(); - return tas; + FileWriteResult saveResult = tas.Save(); + return saveResult.Convert(tas); } #pragma warning disable RCS1224 // private but for unit test diff --git a/src/BizHawk.Client.Common/movie/MovieSession.cs b/src/BizHawk.Client.Common/movie/MovieSession.cs index eaaeef1fdc5..f676826720b 100644 --- a/src/BizHawk.Client.Common/movie/MovieSession.cs +++ b/src/BizHawk.Client.Common/movie/MovieSession.cs @@ -244,8 +244,9 @@ public void RunQueuedMovie(bool recordMode, IEmulator emulator) public void AbortQueuedMovie() => _queuedMovie = null; - public void StopMovie(bool saveChanges = true) + public FileWriteResult StopMovie(bool saveChanges = true) { + FileWriteResult/*?*/ result = null; if (Movie.IsActive()) { var message = "Movie "; @@ -262,8 +263,17 @@ public void StopMovie(bool saveChanges = true) if (saveChanges && Movie.Changes) { - Movie.Save(); - Output($"{Path.GetFileName(Movie.Filename)} written to disk."); + result = Movie.Save(); + if (result.IsError) + { + Output($"Failed to write {Path.GetFileName(Movie.Filename)} to disk."); + Output(result.UserFriendlyErrorMessage()); + return result; + } + else + { + Output($"{Path.GetFileName(Movie.Filename)} written to disk."); + } } Movie.Stop(); @@ -279,6 +289,8 @@ public void StopMovie(bool saveChanges = true) } Movie = null; + + return result ?? new(FileWriteEnum.Success, "", null); } public IMovie Get(string path, bool loadMovie) diff --git a/src/BizHawk.Client.Common/movie/bk2/Bk2Movie.IO.cs b/src/BizHawk.Client.Common/movie/bk2/Bk2Movie.IO.cs index d5735e452f1..5ba581d3f1c 100644 --- a/src/BizHawk.Client.Common/movie/bk2/Bk2Movie.IO.cs +++ b/src/BizHawk.Client.Common/movie/bk2/Bk2Movie.IO.cs @@ -1,3 +1,5 @@ +#nullable enable + using System.Globalization; using System.IO; @@ -11,25 +13,25 @@ namespace BizHawk.Client.Common { public partial class Bk2Movie { - public void Save() + public FileWriteResult Save() { - Write(Filename); + return Write(Filename); } - public void SaveBackup() + public FileWriteResult SaveBackup() { if (string.IsNullOrWhiteSpace(Filename)) { - return; + return new(FileWriteEnum.Success, "", null); } - var backupName = Filename.InsertBeforeLast('.', insert: $".{DateTime.Now:yyyy-MM-dd HH.mm.ss}", out _); + string backupName = Filename.InsertBeforeLast('.', insert: $".{DateTime.Now:yyyy-MM-dd HH.mm.ss}", out _); backupName = Path.Combine(Session.BackupDirectory, Path.GetFileName(backupName)); - Write(backupName, isBackup: true); + return Write(backupName, isBackup: true); } - protected virtual void Write(string fn, bool isBackup = false) + protected virtual FileWriteResult Write(string fn, bool isBackup = false) { SetCycleValues(); // EmulatorVersion used to store the unchanging original emulator version. @@ -40,13 +42,27 @@ protected virtual void Write(string fn, bool isBackup = false) Header[HeaderKeys.EmulatorVersion] = VersionInfo.GetEmuVersion(); Directory.CreateDirectory(Path.GetDirectoryName(fn)!); - using var bs = new ZipStateSaver(fn, Session.Settings.MovieCompressionLevel); - AddLumps(bs, isBackup); + var createResult = ZipStateSaver.Create(fn, Session.Settings.MovieCompressionLevel); + if (createResult.IsError) return createResult; - if (!isBackup) + ZipStateSaver saver = createResult.Value!; + try + { + AddLumps(saver, isBackup); + } + catch (Exception ex) + { + saver.Abort(); + return new(FileWriteEnum.FailedDuringWrite, createResult.WritePath, ex); + } + + FileWriteResult result = saver.CloseAndDispose(); + if (!isBackup && !result.IsError) { Changes = false; } + + return result; } public void SetCycleValues() //TODO IEmulator should not be an instance prop of movies, it should be passed in to every call (i.e. from MovieService) --yoshi @@ -133,7 +149,7 @@ private void LoadBk2Fields(ZipStateLoader bl) bl.GetLump(BinaryStateLump.SyncSettings, abort: false, tr => { - string line; + string? line; while ((line = tr.ReadLine()) != null) { if (!string.IsNullOrWhiteSpace(line)) diff --git a/src/BizHawk.Client.Common/movie/import/IMovieImport.cs b/src/BizHawk.Client.Common/movie/import/IMovieImport.cs index 91a30f734fa..c183d980ec9 100644 --- a/src/BizHawk.Client.Common/movie/import/IMovieImport.cs +++ b/src/BizHawk.Client.Common/movie/import/IMovieImport.cs @@ -67,7 +67,11 @@ public ImportResult Import( Result.Movie.Hash = hash; } - Result.Movie.Save(); + if (Result.Movie.Save().IsError) + { + Result.Errors.Add($"Could not write the file {newFileName}"); + return Result; + } } return Result; diff --git a/src/BizHawk.Client.Common/movie/interfaces/IMovie.cs b/src/BizHawk.Client.Common/movie/interfaces/IMovie.cs index c6b9d68c80a..ac3d276eea5 100644 --- a/src/BizHawk.Client.Common/movie/interfaces/IMovie.cs +++ b/src/BizHawk.Client.Common/movie/interfaces/IMovie.cs @@ -74,12 +74,12 @@ public interface IMovie : IBasicMovieInfo /// /// Forces the creation of a backup file of the current movie state /// - void SaveBackup(); + FileWriteResult SaveBackup(); /// /// Instructs the movie to save the current contents to Filename /// - void Save(); + FileWriteResult Save(); /// updates the and headers from the currently loaded core void SetCycleValues(); diff --git a/src/BizHawk.Client.Common/movie/interfaces/IMovieSession.cs b/src/BizHawk.Client.Common/movie/interfaces/IMovieSession.cs index 908f3b75ed9..3ab9a23158e 100644 --- a/src/BizHawk.Client.Common/movie/interfaces/IMovieSession.cs +++ b/src/BizHawk.Client.Common/movie/interfaces/IMovieSession.cs @@ -83,7 +83,7 @@ void QueueNewMovie( /// clears the queued movie void AbortQueuedMovie(); - void StopMovie(bool saveChanges = true); + FileWriteResult StopMovie(bool saveChanges = true); /// /// Create a new (Tas)Movie with the given path as filename. If is true, diff --git a/src/BizHawk.Client.Common/movie/tasproj/TasMovie.cs b/src/BizHawk.Client.Common/movie/tasproj/TasMovie.cs index 28d585cdd22..94e2b478eb9 100644 --- a/src/BizHawk.Client.Common/movie/tasproj/TasMovie.cs +++ b/src/BizHawk.Client.Common/movie/tasproj/TasMovie.cs @@ -210,7 +210,9 @@ public override bool ExtractInputLog(TextReader reader, out string errorMessage) // We are in record mode so replace the movie log with the one from the savestate if (Session.Settings.EnableBackupMovies && MakeBackup && Log.Count != 0) { - SaveBackup(); + // TODO: This isn't ideal, but making it ideal would mean a big refactor. + FileWriteResult saveResult = SaveBackup(); + if (saveResult.Exception != null) throw saveResult.Exception; MakeBackup = false; } diff --git a/src/BizHawk.Client.Common/savestates/SavestateFile.cs b/src/BizHawk.Client.Common/savestates/SavestateFile.cs index d2558a9b044..393f8ac76ca 100644 --- a/src/BizHawk.Client.Common/savestates/SavestateFile.cs +++ b/src/BizHawk.Client.Common/savestates/SavestateFile.cs @@ -58,14 +58,16 @@ public SavestateFile( _userBag = userBag; } - public void Create(string filename, SaveStateConfig config) + public FileWriteResult Create(string filename, SaveStateConfig config) { - // the old method of text savestate save is now gone. - // a text savestate is just like a binary savestate, but with a different core lump - using var bs = new ZipStateSaver(filename, config.CompressionLevelNormal); + FileWriteResult createResult = ZipStateSaver.Create(filename, config.CompressionLevelNormal); + if (createResult.IsError) return createResult; + var bs = createResult.Value!; using (new SimpleTime("Save Core")) { + // the old method of text savestate save is now gone. + // a text savestate is just like a binary savestate, but with a different core lump if (config.Type == SaveStateType.Text) { bs.PutLump(BinaryStateLump.CorestateText, tw => _statable.SaveStateText(tw)); @@ -129,6 +131,8 @@ public void Create(string filename, SaveStateConfig config) { bs.PutLump(BinaryStateLump.LagLog, tw => tasMovie.LagLog.Save(tw), zstdCompress: true); } + + return bs.CloseAndDispose(); } public bool Load(string path, IDialogParent dialogParent) diff --git a/src/BizHawk.Client.Common/savestates/ZipStateSaver.cs b/src/BizHawk.Client.Common/savestates/ZipStateSaver.cs index b2a9bbf42ec..84e36482fbb 100644 --- a/src/BizHawk.Client.Common/savestates/ZipStateSaver.cs +++ b/src/BizHawk.Client.Common/savestates/ZipStateSaver.cs @@ -1,3 +1,5 @@ +#nullable enable + using System.IO; using BizHawk.Common; @@ -25,9 +27,9 @@ private static void WriteEmuVersion(Stream s) sw.WriteLine(VersionInfo.GetEmuVersion()); } - public ZipStateSaver(string path, int compressionLevel) + private ZipStateSaver(FrameworkZipWriter zip) { - _zip = new FrameworkZipWriter(path, compressionLevel); + _zip = zip; // we put these in every zip, so we know where they came from // a bit redundant for movie files given their headers, but w/e @@ -35,6 +37,34 @@ public ZipStateSaver(string path, int compressionLevel) PutLump(BinaryStateLump.BizVersion, WriteEmuVersion, false); } + public static FileWriteResult Create(string path, int compressionLevel) + { + FileWriteResult result = FrameworkZipWriter.Create(path, compressionLevel); + if (result.IsError) return new(result); + else return result.Convert(new ZipStateSaver(result.Value!)); + } + + /// + /// This method must be called after writing has finished and must not be called twice. + /// Dispose will be called regardless of the result. + /// + public FileWriteResult CloseAndDispose() + { + FileWriteResult result = _zip.CloseAndDispose(); + Dispose(); + return result; + } + + /// + /// Closes and deletes the file. Use if there was an error while writing. + /// Do not call after this. + /// + public void Abort() + { + _zip.Abort(); + Dispose(); + } + public void PutLump(BinaryStateLump lump, Action callback, bool zstdCompress = true) { var filePath = AsTarbomb ? lump.FileName : $"{TOP_LEVEL_DIR_NAME}/{lump.FileName}"; diff --git a/src/BizHawk.Client.EmuHawk/MainForm.Movie.cs b/src/BizHawk.Client.EmuHawk/MainForm.Movie.cs index f97519c3651..b9e7f04f946 100644 --- a/src/BizHawk.Client.EmuHawk/MainForm.Movie.cs +++ b/src/BizHawk.Client.EmuHawk/MainForm.Movie.cs @@ -125,7 +125,14 @@ public void StopMovie(bool saveChanges = true) } else { - MovieSession.StopMovie(saveChanges); + FileWriteResult saveResult = MovieSession.StopMovie(saveChanges); + if (saveResult.IsError) + { + this.ShowMessageBox( + $"Failed to save movie.\n{saveResult.UserFriendlyErrorMessage()}\n{saveResult.Exception.Message}", + "Error", + EMsgBoxIcon.Error); + } SetMainformMovieInfo(); } } diff --git a/src/BizHawk.Client.EmuHawk/MainForm.cs b/src/BizHawk.Client.EmuHawk/MainForm.cs index 13adb58e103..6b7b93adfde 100644 --- a/src/BizHawk.Client.EmuHawk/MainForm.cs +++ b/src/BizHawk.Client.EmuHawk/MainForm.cs @@ -881,7 +881,18 @@ private void CheckMayCloseAndCleanup(object/*?*/ closingSender, CancelEventArgs } Tools.Close(); - MovieSession.StopMovie(); + FileWriteResult saveResult = MovieSession.StopMovie(); + if (saveResult.IsError) + { + if (!this.ModalMessageBox2( + caption: "Quit anyway?", + icon: EMsgBoxIcon.Question, + text: "The currently playing movie could not be saved. Continue and quit anyway? All unsaved changes will be lost.")) + { + closingArgs.Cancel = true; + return; + } + } // zero 03-nov-2015 - close game after other steps. tools might need to unhook themselves from a core. CloseGame(); SaveConfig(); @@ -2705,8 +2716,16 @@ private void SaveMovie() { if (MovieSession.Movie.IsActive()) { - MovieSession.Movie.Save(); - AddOnScreenMessage($"{MovieSession.Movie.Filename} saved."); + FileWriteResult result = MovieSession.Movie.Save(); + if (result.IsError) + { + AddOnScreenMessage($"Failed to save {MovieSession.Movie.Filename}."); + AddOnScreenMessage(result.UserFriendlyErrorMessage()); + } + else + { + AddOnScreenMessage($"{MovieSession.Movie.Filename} saved."); + } } } @@ -4191,10 +4210,14 @@ public void SaveState(string path, string userFriendlyStateName, bool fromLua = return; } - try + FileWriteResult result = new SavestateFile(Emulator, MovieSession, MovieSession.UserBag) + .Create(path, Config.Savestates); + if (result.IsError) + { + AddOnScreenMessage($"Unable to save state {path}"); + } + else { - new SavestateFile(Emulator, MovieSession, MovieSession.UserBag).Create(path, Config.Savestates); - if (SavestateSaved is not null) { StateSavedEventArgs args = new(userFriendlyStateName); @@ -4207,10 +4230,6 @@ public void SaveState(string path, string userFriendlyStateName, bool fromLua = AddOnScreenMessage($"Saved state: {userFriendlyStateName}"); } } - catch (IOException) - { - AddOnScreenMessage($"Unable to save state {path}"); - } if (!fromLua) { diff --git a/src/BizHawk.Client.EmuHawk/movie/EditCommentsForm.cs b/src/BizHawk.Client.EmuHawk/movie/EditCommentsForm.cs index 0ddb2e07fa3..c19de4fa71e 100644 --- a/src/BizHawk.Client.EmuHawk/movie/EditCommentsForm.cs +++ b/src/BizHawk.Client.EmuHawk/movie/EditCommentsForm.cs @@ -55,7 +55,8 @@ private void Save() _movie.Comments.Add(c.Value.ToString()); } - _movie.Save(); + FileWriteResult result = _movie.Save(); + if (result.IsError) throw result.Exception!; } private void Cancel_Click(object sender, EventArgs e) diff --git a/src/BizHawk.Client.EmuHawk/tools/TAStudio/TAStudio.IToolForm.cs b/src/BizHawk.Client.EmuHawk/tools/TAStudio/TAStudio.IToolForm.cs index a5e13e94086..2c5a9ae2bfc 100644 --- a/src/BizHawk.Client.EmuHawk/tools/TAStudio/TAStudio.IToolForm.cs +++ b/src/BizHawk.Client.EmuHawk/tools/TAStudio/TAStudio.IToolForm.cs @@ -170,12 +170,23 @@ public override bool AskSaveChanges() StopSeeking(); if (CurrentTasMovie?.Changes is not true) return true; - var result = DialogController.DoWithTempMute(() => this.ModalMessageBox3( + var shouldSaveResult = DialogController.DoWithTempMute(() => this.ModalMessageBox3( caption: "Closing with Unsaved Changes", icon: EMsgBoxIcon.Question, text: $"Save {WindowTitleStatic} project?")); - if (result is null) return false; - if (result.Value) SaveTas(); + if (shouldSaveResult == true) + { + FileWriteResult saveResult = SaveTas(); + while (saveResult.IsError && shouldSaveResult != true) + { + shouldSaveResult = this.ModalMessageBox3( + $"Failed to save movie. {saveResult.UserFriendlyErrorMessage()}\n{saveResult.Exception.Message}\n\nTry again?", + "Error", + EMsgBoxIcon.Error); + if (shouldSaveResult == true) saveResult = SaveTas(); + } + } + if (shouldSaveResult is null) return false; else CurrentTasMovie.ClearChanges(); return true; } diff --git a/src/BizHawk.Client.EmuHawk/tools/TAStudio/TAStudio.MenuItems.cs b/src/BizHawk.Client.EmuHawk/tools/TAStudio/TAStudio.MenuItems.cs index 604324a6199..a3c281c0bce 100644 --- a/src/BizHawk.Client.EmuHawk/tools/TAStudio/TAStudio.MenuItems.cs +++ b/src/BizHawk.Client.EmuHawk/tools/TAStudio/TAStudio.MenuItems.cs @@ -43,11 +43,15 @@ private void StartNewProjectFromNowMenuItem_Click(object sender, EventArgs e) { if (AskSaveChanges()) { - var newProject = CurrentTasMovie.ConvertToSavestateAnchoredMovie( + var result = CurrentTasMovie.ConvertToSavestateAnchoredMovie( Emulator.Frame, StatableEmulator.CloneSavestate()); + DisplayMessageIfFailed(() => result, "Failed to create movie."); - MainForm.PauseEmulator(); - LoadMovie(newProject, true); + if (result.Value is ITasMovie newProject) + { + MainForm.PauseEmulator(); + LoadMovie(newProject, true); + } } } @@ -57,9 +61,14 @@ private void StartANewProjectFromSaveRamMenuItem_Click(object sender, EventArgs { var saveRam = SaveRamEmulator?.CloneSaveRam(clearDirty: false) ?? throw new Exception("No SaveRam"); GoToFrame(TasView.AnyRowsSelected ? TasView.FirstSelectedRowIndex : 0); - var newProject = CurrentTasMovie.ConvertToSaveRamAnchoredMovie(saveRam); - MainForm.PauseEmulator(); - LoadMovie(newProject, true); + var result = CurrentTasMovie.ConvertToSaveRamAnchoredMovie(saveRam); + DisplayMessageIfFailed(() => result, "Failed to create movie."); + + if (result.Value is ITasMovie newProject) + { + MainForm.PauseEmulator(); + LoadMovie(newProject, true); + } } } @@ -115,30 +124,30 @@ public bool LoadMovieFile(string filename, bool askToSave = true) private void SaveTasMenuItem_Click(object sender, EventArgs e) { - SaveTas(); + DisplayMessageIfFailed(() => SaveTas(), "Failed to save movie."); if (Settings.BackupPerFileSave) { - SaveTas(saveBackup: true); + DisplayMessageIfFailed(() => SaveTas(saveBackup: true), "Failed to save backup."); } } private void SaveAsTasMenuItem_Click(object sender, EventArgs e) { - SaveAsTas(); + DisplayMessageIfFailed(() => SaveAsTas(), "Failed to save movie."); if (Settings.BackupPerFileSave) { - SaveTas(saveBackup: true); + DisplayMessageIfFailed(() => SaveTas(saveBackup: true), "Failed to save backup."); } } private void SaveBackupMenuItem_Click(object sender, EventArgs e) { - SaveTas(saveBackup: true); + DisplayMessageIfFailed(() => SaveTas(saveBackup: true), "Failed to save backup."); } private void SaveBk2BackupMenuItem_Click(object sender, EventArgs e) { - SaveTas(saveAsBk2: true, saveBackup: true); + DisplayMessageIfFailed(() => SaveTas(saveAsBk2: true, saveBackup: true), "Failed to save backup."); } private void SaveSelectionToMacroMenuItem_Click(object sender, EventArgs e) @@ -220,13 +229,24 @@ private void ToBk2MenuItem_Click(object sender, EventArgs e) { MessageStatusLabel.Text = "Exporting to .bk2..."; MessageStatusLabel.Owner.Update(); + Cursor = Cursors.WaitCursor; var bk2 = CurrentTasMovie.ToBk2(); bk2.Filename = fileInfo.FullName; bk2.Attach(Emulator); // required to be able to save the cycle count for ICycleTiming emulators - bk2.Save(); - MessageStatusLabel.Text = $"{bk2.Name} exported."; + FileWriteResult saveResult = bk2.Save(); Cursor = Cursors.Default; + + while (saveResult.IsError) + { + DialogResult d = MessageBox.Show( + $"Failed to save .bk2. {saveResult.UserFriendlyErrorMessage()}\nTry again?", + "Error", + MessageBoxButtons.YesNo); + if (d == DialogResult.Yes) saveResult = bk2.Save(); + else break; + } + if (!saveResult.IsError) MessageStatusLabel.Text = $"{bk2.Name} exported."; } else { diff --git a/src/BizHawk.Client.EmuHawk/tools/TAStudio/TAStudio.cs b/src/BizHawk.Client.EmuHawk/tools/TAStudio/TAStudio.cs index 07712656610..255182f0670 100644 --- a/src/BizHawk.Client.EmuHawk/tools/TAStudio/TAStudio.cs +++ b/src/BizHawk.Client.EmuHawk/tools/TAStudio/TAStudio.cs @@ -36,6 +36,7 @@ public static Icon ToolIcon private const string FrameColumnName = "FrameColumn"; private UndoHistoryForm _undoForm; private Timer _autosaveTimer; + private bool _lastAutoSaveSuccess = true; private readonly int _defaultMainSplitDistance; private readonly int _defaultBranchMarkerSplitDistance; @@ -199,11 +200,7 @@ private void Tastudio_Load(object sender, EventArgs e) _autosaveTimer = new Timer(components); _autosaveTimer.Tick += AutosaveTimerEventProcessor; - if (Settings.AutosaveInterval > 0) - { - _autosaveTimer.Interval = (int)Settings.AutosaveInterval; - _autosaveTimer.Start(); - } + ScheduleAutoSave(Settings.AutosaveInterval); MainVertialSplit.SetDistanceOrDefault( Settings.MainVerticalSplitDistance, @@ -273,20 +270,27 @@ private bool Engage() { changesString = "The current movie has unsaved changes. Would you like to save before closing it?"; } - var result = DialogController.ShowMessageBox3( + var shouldSaveResult = DialogController.ShowMessageBox3( "TAStudio will create a new project file from the current movie.\n\n" + changesString, "Convert movie", EMsgBoxIcon.Question); - if (result == true) + if (shouldSaveResult == true) { - MovieSession.Movie.Save(); + FileWriteResult saveResult = MovieSession.Movie.Save(); + if (saveResult.IsError) + { + DisplayMessageIfFailed(() => saveResult, "Failed to save movie."); + return false; + } } - else if (result == null) + else if (shouldSaveResult == null) { return false; } - var tasMovie = ConvertCurrentMovieToTasproj(); + var tasMovie = MovieSession.Movie.ToTasMovie(); + // No need to save new movie, as there are no changes. + // User will save future changes if they want (potentially via auto-save). success = LoadMovie(tasMovie); } @@ -327,6 +331,16 @@ private bool Engage() return true; } + private void ScheduleAutoSave(uint secondsUntil) + { + _autosaveTimer.Stop(); + if (secondsUntil != 0) + { + _autosaveTimer.Interval = (int)secondsUntil; + _autosaveTimer.Start(); + } + } + private void AutosaveTimerEventProcessor(object sender, EventArgs e) { if (CurrentTasMovie == null) @@ -340,28 +354,55 @@ private void AutosaveTimerEventProcessor(object sender, EventArgs e) return; } + FileWriteResult saveResult; if (Settings.AutosaveAsBackupFile) { if (Settings.AutosaveAsBk2) { - SaveTas(saveAsBk2: true, saveBackup: true); + saveResult = SaveTas(saveAsBk2: true, saveBackup: true); } else { - SaveTas(saveBackup: true); + saveResult = SaveTas(saveBackup: true); } } else { if (Settings.AutosaveAsBk2) { - SaveTas(saveAsBk2: true); + saveResult = SaveTas(saveAsBk2: true); } else { - SaveTas(); + saveResult = SaveTas(); } } + + if (saveResult.IsError && _lastAutoSaveSuccess) + { + // Should we alert the user? + // Let's try again once after a bit, then alert if it fails again. + ScheduleAutoSave(60); + } + else if (saveResult.IsError) + { + _autosaveTimer.Stop(); + bool tryAgain = DialogController.ShowMessageBox2( + $"Failed to auto-save. {saveResult.UserFriendlyErrorMessage()}\n{saveResult.Exception.Message}\n\nTry again?", + "Error"); + if (tryAgain) + { + AutosaveTimerEventProcessor(null, null); + return; + } + ScheduleAutoSave(Settings.AutosaveInterval); + } + else + { + ScheduleAutoSave(Settings.AutosaveInterval); + } + + _lastAutoSaveSuccess = !saveResult.IsError; } private static readonly string[] N64CButtonSuffixes = { " C Up", " C Down", " C Left", " C Right" }; @@ -514,13 +555,6 @@ private int? FirstNonEmptySelectedFrame } } - private ITasMovie ConvertCurrentMovieToTasproj() - { - var tasMovie = MovieSession.Movie.ToTasMovie(); - tasMovie.Save(); // should this be done? - return tasMovie; - } - private bool LoadMovie(ITasMovie tasMovie, bool startsFromSavestate = false, int gotoFrame = 0) { _engaged = false; @@ -735,12 +769,23 @@ private string SuggestedTasProjName() $"{Game.FilesystemSafeName()}.{MovieService.TasMovieExtension}"); } - private void SaveTas(bool saveAsBk2 = false, bool saveBackup = false) + private void DisplayMessageIfFailed(Func action, string message) + { + FileWriteResult result = action(); + if (result.IsError) + { + DialogController.ShowMessageBox( + $"{message}\n{result.UserFriendlyErrorMessage()}\n{result.Exception.Message}", + "Error", + EMsgBoxIcon.Error); + } + } + + private FileWriteResult SaveTas(bool saveAsBk2 = false, bool saveBackup = false) { if (string.IsNullOrEmpty(CurrentTasMovie.Filename) || CurrentTasMovie.Filename == DefaultTasProjName()) { - SaveAsTas(); - return; + return SaveAsTas(); } _autosaveTimer.Stop(); @@ -757,22 +802,29 @@ private void SaveTas(bool saveAsBk2 = false, bool saveBackup = false) movieToSave.Attach(Emulator); } + FileWriteResult result; if (saveBackup) - movieToSave.SaveBackup(); + result = movieToSave.SaveBackup(); else - movieToSave.Save(); + result = movieToSave.Save(); - MessageStatusLabel.Text = saveBackup - ? $"Backup .{(saveAsBk2 ? MovieService.StandardMovieExtension : MovieService.TasMovieExtension)} saved to \"Movie backups\" path." - : "File saved."; - Cursor = Cursors.Default; - if (Settings.AutosaveInterval > 0) + if (!result.IsError) { - _autosaveTimer.Start(); + MessageStatusLabel.Text = saveBackup + ? $"Backup .{(saveAsBk2 ? MovieService.StandardMovieExtension : MovieService.TasMovieExtension)} saved to \"Movie backups\" path." + : "File saved."; + } + else + { + MessageStatusLabel.Text = "Failed to save movie."; } + + Cursor = Cursors.Default; + ScheduleAutoSave(Settings.AutosaveInterval); + return result; } - private void SaveAsTas() + private FileWriteResult SaveAsTas() { _autosaveTimer.Stop(); @@ -788,25 +840,33 @@ private void SaveAsTas() TAStudioProjectsFSFilterSet, this); + FileWriteResult saveResult = null; if (fileInfo != null) { MessageStatusLabel.Text = "Saving..."; MessageStatusLabel.Owner.Update(); Cursor = Cursors.WaitCursor; CurrentTasMovie.Filename = fileInfo.FullName; - CurrentTasMovie.Save(); + saveResult = CurrentTasMovie.Save(); Settings.RecentTas.Add(CurrentTasMovie.Filename); - MessageStatusLabel.Text = "File saved."; Cursor = Cursors.Default; - } - if (Settings.AutosaveInterval > 0) - { - _autosaveTimer.Start(); + if (!saveResult.IsError) + { + MessageStatusLabel.Text = "File saved."; + ScheduleAutoSave(Settings.AutosaveInterval); + } + else + { + MessageStatusLabel.Text = "Failed to save."; + } } UpdateWindowTitle(); // changing the movie's filename does not flag changes, so we need to ensure the window title is always updated MainForm.UpdateWindowTitle(); + + if (fileInfo != null) return saveResult; + else return new(FileWriteEnum.Success, "", null); // user cancelled, so we were successful in not saving } protected override string WindowTitle diff --git a/src/BizHawk.Tests.Client.Common/Movie/FakeMovieSession.cs b/src/BizHawk.Tests.Client.Common/Movie/FakeMovieSession.cs index 1eba1af9d96..4172ea1701c 100644 --- a/src/BizHawk.Tests.Client.Common/Movie/FakeMovieSession.cs +++ b/src/BizHawk.Tests.Client.Common/Movie/FakeMovieSession.cs @@ -67,6 +67,10 @@ public FakeMovieSession(IEmulator emulator) public void PopupMessage(string message) => throw new NotImplementedException(); public void QueueNewMovie(IMovie movie, string systemId, string loadedRomHash, PathEntryCollection pathEntries, IDictionary preferredCores) => throw new NotImplementedException(); public void RunQueuedMovie(bool recordMode, IEmulator emulator) => throw new NotImplementedException(); - public void StopMovie(bool saveChanges = true) => Movie?.Stop(); + public FileWriteResult StopMovie(bool saveChanges = true) + { + Movie?.Stop(); + return new(FileWriteEnum.Success, "", null); + } } } From 7dd2fafe5e6213eedd0be6b71e59bc30ca623df9 Mon Sep 17 00:00:00 2001 From: SuuperW Date: Sat, 12 Jul 2025 21:57:43 -0500 Subject: [PATCH 02/11] Add support for backup files in FileWriter and better error reporting. --- src/BizHawk.Client.Common/FileWriteResult.cs | 47 ++++-- src/BizHawk.Client.Common/FileWriter.cs | 155 +++++++++++++----- .../FrameworkZipWriter.cs | 6 +- src/BizHawk.Client.Common/IZipWriter.cs | 3 +- .../movie/MovieSession.cs | 2 +- .../movie/bk2/Bk2Movie.IO.cs | 4 +- .../savestates/ZipStateSaver.cs | 5 +- .../tools/TAStudio/TAStudio.cs | 2 +- .../Movie/FakeMovieSession.cs | 2 +- 9 files changed, 159 insertions(+), 67 deletions(-) diff --git a/src/BizHawk.Client.Common/FileWriteResult.cs b/src/BizHawk.Client.Common/FileWriteResult.cs index 1c5913326e8..633efbd7a3c 100644 --- a/src/BizHawk.Client.Common/FileWriteResult.cs +++ b/src/BizHawk.Client.Common/FileWriteResult.cs @@ -1,7 +1,6 @@ #nullable enable using System.Diagnostics; -using System.IO; namespace BizHawk.Client.Common { @@ -10,6 +9,8 @@ public enum FileWriteEnum Success, FailedToOpen, FailedDuringWrite, + FailedToDeleteOldBackup, + FailedToMakeBackup, FailedToDeleteOldFile, FailedToRename, } @@ -21,17 +22,19 @@ public class FileWriteResult { public readonly FileWriteEnum Error = FileWriteEnum.Success; public readonly Exception? Exception; - public readonly string WritePath; + internal readonly FileWritePaths Paths; public bool IsError => Error != FileWriteEnum.Success; - public FileWriteResult(FileWriteEnum error, string path, Exception? exception) + internal FileWriteResult(FileWriteEnum error, FileWritePaths writer, Exception? exception) { Error = error; Exception = exception; - WritePath = path; + Paths = writer; } + public FileWriteResult() : this(FileWriteEnum.Success, new("", ""), null) { } + /// /// Converts this instance to a different generic type. /// The new instance will take the value given only if this instance has no error. @@ -39,32 +42,42 @@ public FileWriteResult(FileWriteEnum error, string path, Exception? exception) /// The value of the new instance. Ignored if this instance has an error. public FileWriteResult Convert(T value) where T : class { - if (Error == FileWriteEnum.Success) return new(value, WritePath); + if (Error == FileWriteEnum.Success) return new(value, Paths); else return new(this); } - public FileWriteResult(FileWriteResult other) : this(other.Error, other.WritePath, other.Exception) { } + public FileWriteResult(FileWriteResult other) : this(other.Error, other.Paths, other.Exception) { } public string UserFriendlyErrorMessage() { + Debug.Assert(!IsError || (Exception != null), "FileWriteResult with an error should have an exception."); + switch (Error) { + // We include the full path since the user may not have explicitly given a directory and may not know what it is. case FileWriteEnum.Success: - return $"The file {WritePath} was written successfully."; + return $"The file \"{Paths.Final}\" was written successfully."; case FileWriteEnum.FailedToOpen: - if (WritePath.Contains(".saving")) + if (Paths.Final != Paths.Temp) { - return $"The temporary file {WritePath} already exists and could not be deleted."; + return $"The temporary file \"{Paths.Temp}\" could not be opened."; } - return $"The file {WritePath} could not be created."; + return $"The file \"{Paths.Final}\" could not be created."; case FileWriteEnum.FailedDuringWrite: return $"An error occurred while writing the file."; // No file name here; it should be deleted. + } + + string success = $"The file was created successfully at \"{Paths.Temp}\" but could not be moved"; + switch (Error) + { + case FileWriteEnum.FailedToDeleteOldBackup: + return $"{success}. Unable to remove old backup file \"{Paths.Backup}\"."; + case FileWriteEnum.FailedToMakeBackup: + return $"{success}. Unable to create backup. Failed to move \"{Paths.Final}\" to \"{Paths.Backup}\"."; case FileWriteEnum.FailedToDeleteOldFile: - string fileWithoutPath = Path.GetFileName(WritePath); - return $"The file {WritePath} was created successfully, but the old file could not be deleted. You may manually rename the temporary file {fileWithoutPath}."; + return $"{success}. Unable to remove the old file \"{Paths.Final}\"."; case FileWriteEnum.FailedToRename: - fileWithoutPath = Path.GetFileName(WritePath); - return $"The file {WritePath} was created successfully, but could not be renamed. You may manually rename the temporary file {fileWithoutPath}."; + return $"{success} to \"{Paths.Final}\"."; default: return "unreachable"; } @@ -83,14 +96,14 @@ public class FileWriteResult : FileWriteResult where T : class // Note: "clas /// public readonly T? Value = default; - public FileWriteResult(FileWriteEnum error, string path, Exception? exception) : base(error, path, exception) { } + internal FileWriteResult(FileWriteEnum error, FileWritePaths paths, Exception? exception) : base(error, paths, exception) { } - public FileWriteResult(T value, string path) : base(FileWriteEnum.Success, path, null) + internal FileWriteResult(T value, FileWritePaths paths) : base(FileWriteEnum.Success, paths, null) { Debug.Assert(value != null, "Should not give a null value on success. Use the non-generic type if there is no value."); Value = value; } - public FileWriteResult(FileWriteResult other) : base(other.Error, other.WritePath, other.Exception) { } + public FileWriteResult(FileWriteResult other) : base(other.Error, other.Paths, other.Exception) { } } } diff --git a/src/BizHawk.Client.Common/FileWriter.cs b/src/BizHawk.Client.Common/FileWriter.cs index 4486d5bc098..039c8f47693 100644 --- a/src/BizHawk.Client.Common/FileWriter.cs +++ b/src/BizHawk.Client.Common/FileWriter.cs @@ -2,57 +2,69 @@ using System.Diagnostics; using System.IO; +using System.Threading; using BizHawk.Common.StringExtensions; namespace BizHawk.Client.Common { + public class FileWritePaths(string final, string temp) + { + public readonly string Final = final; + public readonly string Temp = temp; + public string? Backup; + } + + /// + /// Provides a mechanism for safely overwriting files, by using a temporary file that only replaces the original after writing has been completed. + /// Optionally makes a backup of the original file. + /// public class FileWriter : IDisposable { + private FileStream? _stream; // is never null until this.Dispose() public FileStream Stream { get => _stream ?? throw new ObjectDisposedException("Cannot access a disposed FileStream."); } - public string FinalPath; - public string TempPath; + public FileWritePaths Paths; - public bool UsingTempFile => TempPath != FinalPath; + public bool UsingTempFile => Paths.Temp != Paths.Final; private bool _finished = false; - private FileWriter(string final, string temp, FileStream stream) + private FileWriter(FileWritePaths paths, FileStream stream) { - FinalPath = final; - TempPath = temp; + Paths = paths; _stream = stream; } - // There is no public constructor. This is the only way to create an instance. + + /// + /// Create a FileWriter instance, or return an error if unable to access the file. + /// public static FileWriteResult Create(string path) { string writePath = path; + // If the file already exists, we will write to a temporary location first and preserve the old one until we're done. + if (File.Exists(path)) + { + writePath = path.InsertBeforeLast('.', ".saving", out bool inserted); + if (!inserted) writePath = $"{path}.saving"; + + // The file might already exist, if a prior file write failed. + // Maybe the user should have dealt with this on the previously failed save. + // But we want to support plain old "try again", so let's ignore that. + } + FileWritePaths paths = new(path, writePath); try { - // If the file already exists, we will write to a temporary location first and preserve the old one until we're done. - if (File.Exists(path)) - { - writePath = path.InsertBeforeLast('.', ".saving", out bool inserted); - if (!inserted) writePath = $"{path}.saving"; - - if (File.Exists(writePath)) - { - // The user should probably have dealt with this on the previously failed save. - // But maybe we should support plain old "try again", so let's delete it. - File.Delete(writePath); - } - } FileStream fs = new(writePath, FileMode.Create, FileAccess.Write); - return new(new FileWriter(path, writePath, fs), writePath); + return new(new FileWriter(paths, fs), paths); } catch (Exception ex) // There are many exception types that file operations might raise. { - return new(FileWriteEnum.FailedToOpen, writePath, ex); + return new(FileWriteEnum.FailedToOpen, paths, ex); } } @@ -60,8 +72,9 @@ public static FileWriteResult Create(string path) /// This method must be called after writing has finished and must not be called twice. /// Dispose will be called regardless of the result. /// + /// If not null, renames the original file to this path. /// If called twice. - public FileWriteResult CloseAndDispose() + public FileWriteResult CloseAndDispose(string? backupPath = null) { // In theory it might make sense to allow the user to try again if we fail inside this method. // If we implement that, it is probably best to make a static method that takes a FileWriteResult. @@ -71,29 +84,63 @@ public FileWriteResult CloseAndDispose() _finished = true; Dispose(); - if (!UsingTempFile) return new(FileWriteEnum.Success, FinalPath, null); - - if (File.Exists(FinalPath)) + Paths.Backup = backupPath; + if (!UsingTempFile) { - try - { - File.Delete(FinalPath); - } - catch (Exception ex) - { - return new(FileWriteEnum.FailedToDeleteOldFile, TempPath, ex); - } + // The chosen file did not already exist, so there is nothing to back up and nothing to rename. + return new(FileWriteEnum.Success, Paths, null); } + try { - File.Move(TempPath, FinalPath); + // When everything goes right, this is all we need. + File.Replace(Paths.Temp, Paths.Final, backupPath); + return new(FileWriteEnum.Success, Paths, null); } - catch (Exception ex) + catch { - return new(FileWriteEnum.FailedToRename, TempPath, ex); + // When things go wrong, we have to do a lot of work in order to + // figure out what went wrong and tell the user. + return FindTheError(); + } + } + + private FileWriteResult FindTheError() + { + // It is an unfortunate reality that .NET provides horrible exception messages + // when using File.Replace(source, destination, backup). They are not only + // unhelpful by not telling which file operation failed, but can also be a lie. + // File.Move isn't great either. + // So, we will split this into multiple parts and subparts. + + // 1) Handle backup file, if necessary + // a) Delete the old backup, if it exists. We check existence here to avoid DirectoryNotFound errors. + // If this fails, return that failure. + // If it succeeded but the file somehow still exists, report that error. + // b) Ensure the target directory exists. + // Rename the original file, and similarly report any errors. + // 2) Handle renaming of temp file, the same way renaming of original for backup was done. + + if (Paths.Backup != null) + { + try { DeleteIfExists(Paths.Backup); } + catch (Exception ex) { return new(FileWriteEnum.FailedToDeleteOldBackup, Paths, ex); } + if (!TryWaitForFileToVanish(Paths.Backup)) return new(FileWriteEnum.FailedToDeleteOldBackup, Paths, new Exception("The file was supposedly deleted but is still there.")); + + try { MoveFile(Paths.Final, Paths.Backup); } + catch (Exception ex) { return new(FileWriteEnum.FailedToMakeBackup, Paths, ex); } + if (!TryWaitForFileToVanish(Paths.Final)) return new(FileWriteEnum.FailedToMakeBackup, Paths, new Exception("The file was supposedly moved but is still in the orignal location.")); } - return new(FileWriteEnum.Success, FinalPath, null); + try { DeleteIfExists(Paths.Final); } + catch (Exception ex) { return new(FileWriteEnum.FailedToDeleteOldFile, Paths, ex); } + if (!TryWaitForFileToVanish(Paths.Final)) return new(FileWriteEnum.FailedToDeleteOldFile, Paths, new Exception("The file was supposedly deleted but is still there.")); + + try { MoveFile(Paths.Temp, Paths.Final); } + catch (Exception ex) { return new(FileWriteEnum.FailedToRename, Paths, ex); } + if (!TryWaitForFileToVanish(Paths.Temp)) return new(FileWriteEnum.FailedToRename, Paths, new Exception("The file was supposedly moved but is still in the orignal location.")); + + return new(FileWriteEnum.Success, Paths, null); } /// @@ -109,7 +156,7 @@ public void Abort() try { // Delete because the file is almost certainly useless and just clutter. - File.Delete(TempPath); + File.Delete(Paths.Temp); } catch { /* eat? this is probably not very important */ } } @@ -127,5 +174,35 @@ public void Dispose() // The caller should call CloseAndDispose and handle potential failure. Debug.Assert(_finished, $"{nameof(FileWriteResult)} should not be disposed before calling {nameof(CloseAndDispose)}"); } + + + private static void DeleteIfExists(string path) + { + if (File.Exists(path)) + { + File.Delete(path); + } + } + + private static void MoveFile(string source, string destination) + { + FileInfo file = new(destination); + file.Directory.Create(); + File.Move(source, destination); + } + + /// + /// Supposedly it is possible for File.Delete to return before the file has actually been deleted. + /// And File.Move too, I guess. + /// + private static bool TryWaitForFileToVanish(string path) + { + for (var i = 25; i != 0; i--) + { + if (!File.Exists(path)) return true; + Thread.Sleep(10); + } + return false; + } } } diff --git a/src/BizHawk.Client.Common/FrameworkZipWriter.cs b/src/BizHawk.Client.Common/FrameworkZipWriter.cs index a0e18891b83..a8c9fcd6521 100644 --- a/src/BizHawk.Client.Common/FrameworkZipWriter.cs +++ b/src/BizHawk.Client.Common/FrameworkZipWriter.cs @@ -47,7 +47,7 @@ public static FileWriteResult Create(string path, int compre return fs.Convert(ret); } - public FileWriteResult CloseAndDispose() + public FileWriteResult CloseAndDispose(string? backupPath = null) { if (_archive == null || _fs == null) throw new ObjectDisposedException("Cannot use disposed ZipWriter."); @@ -58,11 +58,11 @@ public FileWriteResult CloseAndDispose() FileWriteResult ret; if (_writeException == null) { - ret = _fs.CloseAndDispose(); + ret = _fs.CloseAndDispose(backupPath); } else { - ret = new(FileWriteEnum.FailedDuringWrite, _fs.TempPath, _writeException); + ret = new(FileWriteEnum.FailedDuringWrite, _fs.Paths, _writeException); _fs.Abort(); } diff --git a/src/BizHawk.Client.Common/IZipWriter.cs b/src/BizHawk.Client.Common/IZipWriter.cs index f061b7ed74a..dc67fde2f5a 100644 --- a/src/BizHawk.Client.Common/IZipWriter.cs +++ b/src/BizHawk.Client.Common/IZipWriter.cs @@ -10,7 +10,8 @@ public interface IZipWriter : IDisposable /// This method must be called after writing has finished and must not be called twice. /// Dispose will be called regardless of the result. /// - FileWriteResult CloseAndDispose(); + /// If not null, renames the original file to this path. + FileWriteResult CloseAndDispose(string backupPath = null); /// /// Closes and deletes the file. Use if there was an error while writing. diff --git a/src/BizHawk.Client.Common/movie/MovieSession.cs b/src/BizHawk.Client.Common/movie/MovieSession.cs index f676826720b..d63501bf4a9 100644 --- a/src/BizHawk.Client.Common/movie/MovieSession.cs +++ b/src/BizHawk.Client.Common/movie/MovieSession.cs @@ -290,7 +290,7 @@ public FileWriteResult StopMovie(bool saveChanges = true) Movie = null; - return result ?? new(FileWriteEnum.Success, "", null); + return result ?? new(); } public IMovie Get(string path, bool loadMovie) diff --git a/src/BizHawk.Client.Common/movie/bk2/Bk2Movie.IO.cs b/src/BizHawk.Client.Common/movie/bk2/Bk2Movie.IO.cs index 5ba581d3f1c..04248f75a96 100644 --- a/src/BizHawk.Client.Common/movie/bk2/Bk2Movie.IO.cs +++ b/src/BizHawk.Client.Common/movie/bk2/Bk2Movie.IO.cs @@ -22,7 +22,7 @@ public FileWriteResult SaveBackup() { if (string.IsNullOrWhiteSpace(Filename)) { - return new(FileWriteEnum.Success, "", null); + return new(); } string backupName = Filename.InsertBeforeLast('.', insert: $".{DateTime.Now:yyyy-MM-dd HH.mm.ss}", out _); @@ -53,7 +53,7 @@ protected virtual FileWriteResult Write(string fn, bool isBackup = false) catch (Exception ex) { saver.Abort(); - return new(FileWriteEnum.FailedDuringWrite, createResult.WritePath, ex); + return new(FileWriteEnum.FailedDuringWrite, createResult.Paths, ex); } FileWriteResult result = saver.CloseAndDispose(); diff --git a/src/BizHawk.Client.Common/savestates/ZipStateSaver.cs b/src/BizHawk.Client.Common/savestates/ZipStateSaver.cs index 84e36482fbb..68dcd337a69 100644 --- a/src/BizHawk.Client.Common/savestates/ZipStateSaver.cs +++ b/src/BizHawk.Client.Common/savestates/ZipStateSaver.cs @@ -48,9 +48,10 @@ public static FileWriteResult Create(string path, int compression /// This method must be called after writing has finished and must not be called twice. /// Dispose will be called regardless of the result. /// - public FileWriteResult CloseAndDispose() + /// If not null, renames the original file to this path. + public FileWriteResult CloseAndDispose(string? backupPath = null) { - FileWriteResult result = _zip.CloseAndDispose(); + FileWriteResult result = _zip.CloseAndDispose(backupPath); Dispose(); return result; } diff --git a/src/BizHawk.Client.EmuHawk/tools/TAStudio/TAStudio.cs b/src/BizHawk.Client.EmuHawk/tools/TAStudio/TAStudio.cs index 255182f0670..cac2ab05279 100644 --- a/src/BizHawk.Client.EmuHawk/tools/TAStudio/TAStudio.cs +++ b/src/BizHawk.Client.EmuHawk/tools/TAStudio/TAStudio.cs @@ -866,7 +866,7 @@ private FileWriteResult SaveAsTas() MainForm.UpdateWindowTitle(); if (fileInfo != null) return saveResult; - else return new(FileWriteEnum.Success, "", null); // user cancelled, so we were successful in not saving + else return new(); // user cancelled, so we were successful in not saving } protected override string WindowTitle diff --git a/src/BizHawk.Tests.Client.Common/Movie/FakeMovieSession.cs b/src/BizHawk.Tests.Client.Common/Movie/FakeMovieSession.cs index 4172ea1701c..af65722f755 100644 --- a/src/BizHawk.Tests.Client.Common/Movie/FakeMovieSession.cs +++ b/src/BizHawk.Tests.Client.Common/Movie/FakeMovieSession.cs @@ -70,7 +70,7 @@ public FakeMovieSession(IEmulator emulator) public FileWriteResult StopMovie(bool saveChanges = true) { Movie?.Stop(); - return new(FileWriteEnum.Success, "", null); + return new(); } } } From 19acd9a0f0bc7946f2dedd01c123e8fabde52567 Mon Sep 17 00:00:00 2001 From: SuuperW Date: Tue, 15 Jul 2025 16:02:47 -0500 Subject: [PATCH 03/11] Handle most errors related to savestate files. Autosave last slot is still broken on close and on TAStudio open. --- .../Api/Classes/EmuClientApi.cs | 11 +- .../Api/Classes/SaveStateApi.cs | 17 ++- .../DialogControllerExtensions.cs | 15 +++ src/BizHawk.Client.Common/FileWriteResult.cs | 24 ++++- src/BizHawk.Client.Common/IMainFormForApi.cs | 12 ++- src/BizHawk.Client.Common/SaveSlotManager.cs | 60 ++++++++--- .../savestates/SavestateFile.cs | 5 +- src/BizHawk.Client.EmuHawk/MainForm.Events.cs | 17 ++- src/BizHawk.Client.EmuHawk/MainForm.Hotkey.cs | 2 +- src/BizHawk.Client.EmuHawk/MainForm.cs | 100 ++++++++++++------ 10 files changed, 204 insertions(+), 59 deletions(-) diff --git a/src/BizHawk.Client.Common/Api/Classes/EmuClientApi.cs b/src/BizHawk.Client.Common/Api/Classes/EmuClientApi.cs index fbc5c2636f2..1e00e4ba453 100644 --- a/src/BizHawk.Client.Common/Api/Classes/EmuClientApi.cs +++ b/src/BizHawk.Client.Common/Api/Classes/EmuClientApi.cs @@ -160,7 +160,16 @@ public bool OpenRom(string path) public void SaveRam() => _mainForm.FlushSaveRAM(); - public void SaveState(string name) => _mainForm.SaveState(Path.Combine(_config.PathEntries.SaveStateAbsolutePath(Game.System), $"{name}.State"), name, fromLua: false); + // TODO: Change return type to FileWriteResult. + // We may wish to change more than that, since we have a mostly-dupicate ISaveStateApi.Save, neither has documentation indicating what the differences are. + public void SaveState(string name) + { + FileWriteResult result = _mainForm.SaveState(Path.Combine(_config.PathEntries.SaveStateAbsolutePath(Game.System), $"{name}.State"), name); + if (result.Exception != null && result.Exception is not UnlessUsingApiException) + { + throw result.Exception; + } + } public int ScreenHeight() => _displayManager.GetPanelNativeSize().Height; diff --git a/src/BizHawk.Client.Common/Api/Classes/SaveStateApi.cs b/src/BizHawk.Client.Common/Api/Classes/SaveStateApi.cs index 9bbe860e9c5..271f036ed20 100644 --- a/src/BizHawk.Client.Common/Api/Classes/SaveStateApi.cs +++ b/src/BizHawk.Client.Common/Api/Classes/SaveStateApi.cs @@ -39,8 +39,17 @@ public bool LoadSlot(int slotNum, bool suppressOSD) return _mainForm.LoadQuickSave(slotNum, suppressOSD: suppressOSD); } - public void Save(string path, bool suppressOSD) => _mainForm.SaveState(path, path, true, suppressOSD); + // TODO: Change return type FileWriteResult. + public void Save(string path, bool suppressOSD) + { + FileWriteResult result = _mainForm.SaveState(path, path, suppressOSD); + if (result.Exception != null && result.Exception is not UnlessUsingApiException) + { + throw result.Exception; + } + } + // TODO: Change return type to FileWriteResult. public void SaveSlot(int slotNum, bool suppressOSD) { if (slotNum is < 0 or > 10) throw new ArgumentOutOfRangeException(paramName: nameof(slotNum), message: ERR_MSG_NOT_A_SLOT); @@ -49,7 +58,11 @@ public void SaveSlot(int slotNum, bool suppressOSD) LogCallback(ERR_MSG_USE_SLOT_10); slotNum = 10; } - _mainForm.SaveQuickSave(slotNum, suppressOSD: suppressOSD, fromLua: true); + FileWriteResult result = _mainForm.SaveQuickSave(slotNum, suppressOSD: suppressOSD); + if (result.Exception != null && result.Exception is not UnlessUsingApiException) + { + throw result.Exception; + } } } } diff --git a/src/BizHawk.Client.Common/DialogControllerExtensions.cs b/src/BizHawk.Client.Common/DialogControllerExtensions.cs index 768dd0a6e26..8e861b13582 100644 --- a/src/BizHawk.Client.Common/DialogControllerExtensions.cs +++ b/src/BizHawk.Client.Common/DialogControllerExtensions.cs @@ -1,6 +1,7 @@ #nullable enable using System.Collections.Generic; +using System.Diagnostics; namespace BizHawk.Client.Common { @@ -60,6 +61,20 @@ public static bool ModalMessageBox2( EMsgBoxIcon? icon = null) => dialogParent.DialogController.ShowMessageBox3(owner: dialogParent, text: text, caption: caption, icon: icon); + public static void ErrorMessageBox( + this IDialogParent dialogParent, + FileWriteResult fileResult, + string? prefixMessage = null) + { + Debug.Assert(fileResult.IsError && fileResult.Exception != null, "Error box must have an error."); + + string prefix = prefixMessage == null ? "" : prefixMessage + "\n"; + dialogParent.ModalMessageBox( + text: $"{prefix}{fileResult.UserFriendlyErrorMessage()}\n{fileResult.Exception!.Message}", + caption: "Error", + icon: EMsgBoxIcon.Error); + } + /// Creates and shows a System.Windows.Forms.OpenFileDialog or equivalent with the receiver () as its parent /// OpenFileDialog.RestoreDirectory (isn't this useless when specifying ? keeping it for backcompat) /// OpenFileDialog.Filter diff --git a/src/BizHawk.Client.Common/FileWriteResult.cs b/src/BizHawk.Client.Common/FileWriteResult.cs index 633efbd7a3c..c11193e347c 100644 --- a/src/BizHawk.Client.Common/FileWriteResult.cs +++ b/src/BizHawk.Client.Common/FileWriteResult.cs @@ -7,12 +7,17 @@ namespace BizHawk.Client.Common public enum FileWriteEnum { Success, + // Failures during a FileWriter write. FailedToOpen, FailedDuringWrite, FailedToDeleteOldBackup, FailedToMakeBackup, FailedToDeleteOldFile, FailedToRename, + Aborted, + // Failures from other sources + FailedToDeleteGeneric, + FailedToMoveForSwap, } /// @@ -26,7 +31,7 @@ public class FileWriteResult public bool IsError => Error != FileWriteEnum.Success; - internal FileWriteResult(FileWriteEnum error, FileWritePaths writer, Exception? exception) + public FileWriteResult(FileWriteEnum error, FileWritePaths writer, Exception? exception) { Error = error; Exception = exception; @@ -65,6 +70,15 @@ public string UserFriendlyErrorMessage() return $"The file \"{Paths.Final}\" could not be created."; case FileWriteEnum.FailedDuringWrite: return $"An error occurred while writing the file."; // No file name here; it should be deleted. + case FileWriteEnum.Aborted: + return "The operation was aborted."; + + case FileWriteEnum.FailedToDeleteGeneric: + return $"The file \"{Paths.Final}\" could not be deleted."; + //case FileWriteEnum.FailedToDeleteForSwap: + // return $"Failed to swap files. Unable to write to \"{Paths.Final}\""; + case FileWriteEnum.FailedToMoveForSwap: + return $"Failed to swap files. Unable to rename \"{Paths.Temp}\" to \"{Paths.Final}\""; } string success = $"The file was created successfully at \"{Paths.Temp}\" but could not be moved"; @@ -106,4 +120,12 @@ internal FileWriteResult(T value, FileWritePaths paths) : base(FileWriteEnum.Suc public FileWriteResult(FileWriteResult other) : base(other.Error, other.Paths, other.Exception) { } } + + /// + /// This only exists as a way to avoid changing the API behavior. + /// + public class UnlessUsingApiException : Exception + { + public UnlessUsingApiException(string message) : base(message) { } + } } diff --git a/src/BizHawk.Client.Common/IMainFormForApi.cs b/src/BizHawk.Client.Common/IMainFormForApi.cs index 5577fe3a014..765aea62863 100644 --- a/src/BizHawk.Client.Common/IMainFormForApi.cs +++ b/src/BizHawk.Client.Common/IMainFormForApi.cs @@ -93,10 +93,16 @@ public interface IMainFormForApi /// only referenced from bool RestartMovie(); - /// only referenced from - void SaveQuickSave(int slot, bool suppressOSD = false, bool fromLua = false); + FileWriteResult SaveQuickSave(int slot, bool suppressOSD = false); - void SaveState(string path, string userFriendlyStateName, bool fromLua = false, bool suppressOSD = false); + /// + /// Creates a savestate and writes it to a file. + /// + /// The path of the file to write. + /// The name to use for the state on the client's HUD. + /// If true, the client HUD will not show a success message. + /// Returns a value indicating if there was an error and (if there was) why. + FileWriteResult SaveState(string path, string userFriendlyStateName, bool suppressOSD = false); void SeekFrameAdvance(); diff --git a/src/BizHawk.Client.Common/SaveSlotManager.cs b/src/BizHawk.Client.Common/SaveSlotManager.cs index ea95e684f86..774e9821034 100644 --- a/src/BizHawk.Client.Common/SaveSlotManager.cs +++ b/src/BizHawk.Client.Common/SaveSlotManager.cs @@ -69,31 +69,65 @@ public void ToggleRedo(IMovie movie, int slot) public bool IsRedo(IMovie movie, int slot) => slot is >= 1 and <= 10 && movie is not ITasMovie && _redo[slot - 1]; - public void SwapBackupSavestate(IMovie movie, string path, int currentSlot) + /// + /// Takes the .state and .bak files and swaps them + /// + public FileWriteResult SwapBackupSavestate(IMovie movie, string path, int currentSlot) { - // Takes the .state and .bak files and swaps them + string backupPath = $"{path}.bak"; + string tempPath = $"{path}.bak.tmp"; + var state = new FileInfo(path); - var backup = new FileInfo($"{path}.bak"); - var temp = new FileInfo($"{path}.bak.tmp"); + var backup = new FileInfo(backupPath); if (!state.Exists || !backup.Exists) { - return; + return new(); } - if (temp.Exists) + // Delete old temp file if it exists. + try + { + if (File.Exists(tempPath)) File.Delete(tempPath); + } + catch (Exception ex) { - temp.Delete(); + return new(FileWriteEnum.FailedToDeleteGeneric, new(tempPath, ""), ex); } - backup.CopyTo($"{path}.bak.tmp"); - backup.Delete(); - state.CopyTo($"{path}.bak"); - state.Delete(); - temp.CopyTo(path); - temp.Delete(); + // Move backup to temp. + try + { + backup.MoveTo(tempPath); + } + catch (Exception ex) + { + return new(FileWriteEnum.FailedToMoveForSwap, new(tempPath, backupPath), ex); + } + // Move current to backup. + try + { + state.MoveTo(backupPath); + } + catch (Exception ex) + { + // Attempt to restore the backup + try { backup.MoveTo(backupPath); } catch { /* eat? unlikely to fail here */ } + return new(FileWriteEnum.FailedToMoveForSwap, new(backupPath, path), ex); + } + // Move backup to current. + try + { + backup.MoveTo(path); + } + catch (Exception ex) + { + // Should we attempt to restore? Unlikely to fail here since we've already touched all files. + return new(FileWriteEnum.FailedToMoveForSwap, new(path, tempPath), ex); + } ToggleRedo(movie, currentSlot); + return new(); } } } diff --git a/src/BizHawk.Client.Common/savestates/SavestateFile.cs b/src/BizHawk.Client.Common/savestates/SavestateFile.cs index 393f8ac76ca..966ca04c234 100644 --- a/src/BizHawk.Client.Common/savestates/SavestateFile.cs +++ b/src/BizHawk.Client.Common/savestates/SavestateFile.cs @@ -58,7 +58,7 @@ public SavestateFile( _userBag = userBag; } - public FileWriteResult Create(string filename, SaveStateConfig config) + public FileWriteResult Create(string filename, SaveStateConfig config, bool makeBackup) { FileWriteResult createResult = ZipStateSaver.Create(filename, config.CompressionLevelNormal); if (createResult.IsError) return createResult; @@ -132,7 +132,8 @@ public FileWriteResult Create(string filename, SaveStateConfig config) bs.PutLump(BinaryStateLump.LagLog, tw => tasMovie.LagLog.Save(tw), zstdCompress: true); } - return bs.CloseAndDispose(); + makeBackup = makeBackup && config.MakeBackups; + return bs.CloseAndDispose(makeBackup ? $"{filename}.bak" : null); } public bool Load(string path, IDialogParent dialogParent) diff --git a/src/BizHawk.Client.EmuHawk/MainForm.Events.cs b/src/BizHawk.Client.EmuHawk/MainForm.Events.cs index 1de3ec03ded..03e795d7e67 100644 --- a/src/BizHawk.Client.EmuHawk/MainForm.Events.cs +++ b/src/BizHawk.Client.EmuHawk/MainForm.Events.cs @@ -269,7 +269,7 @@ private void CloseRomMenuItem_Click(object sender, EventArgs e) } private void QuickSavestateMenuItem_Click(object sender, EventArgs e) - => SaveQuickSave(int.Parse(((ToolStripMenuItem) sender).Text)); + => SaveQuickSaveAndShowError(int.Parse(((ToolStripMenuItem) sender).Text)); private void SaveNamedStateMenuItem_Click(object sender, EventArgs e) => SaveStateAs(); @@ -305,7 +305,7 @@ private void SaveToCurrentSlotMenuItem_Click(object sender, EventArgs e) => SavestateCurrentSlot(); private void SavestateCurrentSlot() - => SaveQuickSave(Config.SaveSlot); + => SaveQuickSaveAndShowError(Config.SaveSlot); private void LoadCurrentSlotMenuItem_Click(object sender, EventArgs e) => LoadstateCurrentSlot(); @@ -1378,8 +1378,15 @@ private void ViewCommentsContextMenuItem_Click(object sender, EventArgs e) private void UndoSavestateContextMenuItem_Click(object sender, EventArgs e) { var slot = Config.SaveSlot; - _stateSlots.SwapBackupSavestate(MovieSession.Movie, $"{SaveStatePrefix()}.QuickSave{slot % 10}.State", slot); - AddOnScreenMessage($"Save slot {slot} restored."); + FileWriteResult swapResult = _stateSlots.SwapBackupSavestate(MovieSession.Movie, $"{SaveStatePrefix()}.QuickSave{slot % 10}.State", slot); + if (swapResult.IsError) + { + this.ErrorMessageBox(swapResult, "Failed to swap state files."); + } + else + { + AddOnScreenMessage($"Save slot {slot} restored."); + } } private void ClearSramContextMenuItem_Click(object sender, EventArgs e) @@ -1449,7 +1456,7 @@ private void SlotStatusButtons_MouseUp(object sender, MouseEventArgs e) if (sender == Slot9StatusButton) slot = 9; if (sender == Slot0StatusButton) slot = 10; - if (e.Button is MouseButtons.Right) SaveQuickSave(slot); + if (e.Button is MouseButtons.Right) SaveQuickSaveAndShowError(slot); else if (e.Button is MouseButtons.Left && HasSlot(slot)) _ = LoadQuickSave(slot); } diff --git a/src/BizHawk.Client.EmuHawk/MainForm.Hotkey.cs b/src/BizHawk.Client.EmuHawk/MainForm.Hotkey.cs index b131a08ace6..a8b96ab686b 100644 --- a/src/BizHawk.Client.EmuHawk/MainForm.Hotkey.cs +++ b/src/BizHawk.Client.EmuHawk/MainForm.Hotkey.cs @@ -10,7 +10,7 @@ private bool CheckHotkey(string trigger) { void SelectAndSaveToSlot(int slot) { - SaveQuickSave(slot); + SaveQuickSaveAndShowError(slot); Config.SaveSlot = slot; UpdateStatusSlots(); } diff --git a/src/BizHawk.Client.EmuHawk/MainForm.cs b/src/BizHawk.Client.EmuHawk/MainForm.cs index 6b7b93adfde..f2be7c9aa28 100644 --- a/src/BizHawk.Client.EmuHawk/MainForm.cs +++ b/src/BizHawk.Client.EmuHawk/MainForm.cs @@ -4006,8 +4006,21 @@ private void CloseGame(bool clearSram = false) } } + bool? tryAgain = null; + do + { + FileWriteResult stateSaveResult = AutoSaveStateIfConfigured(); + if (stateSaveResult.IsError) + { + tryAgain = this.ShowMessageBox3( + $"Failed to auto-save state. Do you want to try again?\n\nError details:\n{stateSaveResult.UserFriendlyErrorMessage()}\n{stateSaveResult.Exception.Message}", + "IOError while writing savestate", + EMsgBoxIcon.Error); + if (tryAgain == null) return; + } + } while (tryAgain == true); + StopAv(); - AutoSaveStateIfConfigured(); CommitCoreSettingsToConfig(); DisableRewind(); @@ -4029,9 +4042,14 @@ private void CloseGame(bool clearSram = false) GameIsClosing = false; } - private void AutoSaveStateIfConfigured() + private FileWriteResult AutoSaveStateIfConfigured() { - if (Config.AutoSaveLastSaveSlot && Emulator.HasSavestates()) SavestateCurrentSlot(); + if (Config.AutoSaveLastSaveSlot && Emulator.HasSavestates()) + { + return SaveQuickSave(Config.SaveSlot); + } + + return new(); } public bool GameIsClosing { get; private set; } // Lets tools make better decisions when being called by CloseGame @@ -4191,27 +4209,30 @@ public bool LoadQuickSave(int slot, bool suppressOSD = false) return LoadState(path: path, userFriendlyStateName: quickSlotName, suppressOSD: suppressOSD); } - public void SaveState(string path, string userFriendlyStateName, bool fromLua = false, bool suppressOSD = false) + private FileWriteResult SaveStateInternal(string path, string userFriendlyStateName, bool suppressOSD, bool makeBackup) { if (!Emulator.HasSavestates()) { - return; + return new(FileWriteEnum.Aborted, new("", ""), new UnlessUsingApiException("The current emulator does not support savestates.")); } if (ToolControllingSavestates is { } tool) { tool.SaveState(); - return; + // assume success by the tool: state was created, but not as a file. So no path. + return new(); } if (MovieSession.Movie.IsActive() && Emulator.Frame > MovieSession.Movie.FrameCount) { - AddOnScreenMessage("Cannot savestate after movie end!"); - return; + const string errmsg = "Cannot savestate after movie end!"; + AddOnScreenMessage(errmsg); + // Failed to create state due to limitations of our movie handling code. + return new(FileWriteEnum.Aborted, new("", ""), new UnlessUsingApiException(errmsg)); } FileWriteResult result = new SavestateFile(Emulator, MovieSession, MovieSession.UserBag) - .Create(path, Config.Savestates); + .Create(path, Config.Savestates, makeBackup); if (result.IsError) { AddOnScreenMessage($"Unable to save state {path}"); @@ -4225,25 +4246,32 @@ public void SaveState(string path, string userFriendlyStateName, bool fromLua = } RA?.OnSaveState(path); + if (Tools.Has()) + { + Tools.LuaConsole.LuaImp.CallSaveStateEvent(userFriendlyStateName); + } + if (!suppressOSD) { AddOnScreenMessage($"Saved state: {userFriendlyStateName}"); } } - if (!fromLua) - { - UpdateStatusSlots(); - } + return result; + } + + public FileWriteResult SaveState(string path, string userFriendlyStateName, bool suppressOSD = false) + { + return SaveStateInternal(path, userFriendlyStateName, suppressOSD, false); } - // TODO: should backup logic be stuffed in into Client.Common.SaveStateManager? - public void SaveQuickSave(int slot, bool suppressOSD = false, bool fromLua = false) + public FileWriteResult SaveQuickSave(int slot, bool suppressOSD = false) { if (!Emulator.HasSavestates()) { - return; + return new(FileWriteEnum.Aborted, new("", ""), new UnlessUsingApiException("The current emulator does not support savestates.")); } + var quickSlotName = $"QuickSave{slot % 10}"; var handled = false; if (QuicksaveSave is not null) @@ -4254,29 +4282,32 @@ public void SaveQuickSave(int slot, bool suppressOSD = false, bool fromLua = fal } if (handled) { - return; + // I suppose this is a success? But we have no path. + return new(); } if (ToolControllingSavestates is { } tool) { tool.SaveQuickSave(slot); - return; + // assume success by the tool: state was created, but not as a file. So no path. + return new(); } var path = $"{SaveStatePrefix()}.{quickSlotName}.State"; - new FileInfo(path).Directory?.Create(); - - // Make backup first - if (Config.Savestates.MakeBackups) - { - Util.TryMoveBackupFile(path, $"{path}.bak"); - } - - SaveState(path, quickSlotName, fromLua, suppressOSD); + var ret = SaveStateInternal(path, quickSlotName, suppressOSD, true); + UpdateStatusSlots(); + return ret; + } - if (Tools.Has()) + /// + /// Runs and displays a pop up message if there was an error. + /// + private void SaveQuickSaveAndShowError(int slot) + { + FileWriteResult result = SaveQuickSave(slot); + if (result.IsError) { - Tools.LuaConsole.LuaImp.CallSaveStateEvent(quickSlotName); + this.ErrorMessageBox(result, "Quick save failed."); } } @@ -4340,12 +4371,19 @@ private void SaveStateAs() var path = Config.PathEntries.SaveStateAbsolutePath(Game.System); new FileInfo(path).Directory?.Create(); - var result = this.ShowFileSaveDialog( + var shouldSaveResult = this.ShowFileSaveDialog( fileExt: "State", filter: EmuHawkSaveStatesFSFilterSet, initDir: path, initFileName: $"{SaveStatePrefix()}.QuickSave0.State"); - if (result is not null) SaveState(path: result, userFriendlyStateName: result); + if (shouldSaveResult is not null) + { + FileWriteResult saveResult = SaveState(path: shouldSaveResult, userFriendlyStateName: shouldSaveResult); + if (saveResult.IsError) + { + this.ErrorMessageBox(saveResult, "Unable to save."); + } + } if (Tools.IsLoaded()) { From 792e5dbadd3f213a5accdb0a6f9dea136d125a3d Mon Sep 17 00:00:00 2001 From: SuuperW Date: Wed, 16 Jul 2025 22:13:45 -0500 Subject: [PATCH 04/11] Update the ISaveRam interface for clarity. We no longer constrain the sram data length, because it already wasn't true for all cores. Cores that require a specific size should throw if they get the wrong size. --- .../movie/MovieConversionExtensions.cs | 1 - .../movie/bk2/Bk2Movie.HeaderApi.cs | 21 +-------- .../movie/interfaces/IMovie.cs | 2 +- src/BizHawk.Client.EmuHawk/MainForm.cs | 46 ++++++------------- .../movie/RecordMovie.cs | 3 +- .../tools/TAStudio/TAStudio.MenuItems.cs | 6 +-- .../Base Implementations/LinkedSaveRam.cs | 10 ++-- src/BizHawk.Emulation.Common/Extensions.cs | 2 +- .../Interfaces/Services/ISaveRam.cs | 13 ++++-- .../Arcades/MAME/MAME.ISaveRam.cs | 2 + .../Computers/AppleII/AppleII.ISaveRam.cs | 1 + .../Commodore64/Cartridge/Mapper0020.cs | 4 ++ .../Commodore64/Serial/Drive1541.SaveRam.cs | 2 + .../Computers/MSX/MSX.ISaveRam.cs | 3 ++ .../Atari/A7800Hawk/A7800Hawk.ISaveRam.cs | 3 ++ .../Atari/jaguar/VirtualJaguar.ISaveRam.cs | 2 + .../Consoles/Atari/lynx/Lynx.ISaveRam.cs | 4 +- .../GCE/Vectrex/VectrexHawk.ISaveRam.cs | 2 + .../Magnavox/Odyssey2/O2Hawk.ISaveRam.cs | 5 +- .../Nintendo/BSNES/BsnesCore.ISaveRam.cs | 2 + .../Nintendo/GBA/MGBAHawk.ISaveRam.cs | 13 ++++++ .../Nintendo/GBHawk/GBHawk.ISaveRam.cs | 5 +- .../GBHawkLink/GBHawkLink.ISaveRam.cs | 5 ++ .../GBHawkLink3x/GBHawkLink3x.ISaveRam.cs | 5 ++ .../GBHawkLink4x/GBHawkLink4x.ISaveRam.cs | 5 ++ .../Nintendo/Gameboy/Gambatte.ISaveRam.cs | 3 ++ .../Consoles/Nintendo/N64/N64.ISaveRam.cs | 2 + .../Consoles/Nintendo/NDS/MelonDS.ISaveRam.cs | 7 +++ .../Consoles/Nintendo/NES/NES.ISaveRam.cs | 12 +++++ .../Nintendo/QuickNES/QuickNES.ISaveRam.cs | 2 + .../Nintendo/SNES/LibsnesCore.ISaveRam.cs | 6 ++- .../Nintendo/SameBoy/SameBoy.ISaveRam.cs | 8 ++-- .../Consoles/PC Engine/PCEngine.ISaveRam.cs | 3 ++ .../Consoles/SNK/NeoGeoPort.cs | 2 + .../Sega/GGHawkLink/GGHawkLink.ISaveRam.cs | 5 ++ .../Consoles/Sega/SMS/SMS.ISaveRam.cs | 3 ++ .../Consoles/Sega/Saturn/Saturnus.cs | 6 ++- .../Consoles/Sega/gpgx64/GPGX.ISaveRam.cs | 15 ++++++ .../Consoles/Sony/PSX/Octoshock.cs | 2 + .../WonderSwan/WonderSwan.ISaveRam.cs | 2 + .../Libretro/Libretro.ISaveRam.cs | 4 ++ .../Waterbox/WaterboxCore.cs | 4 ++ 42 files changed, 177 insertions(+), 76 deletions(-) diff --git a/src/BizHawk.Client.Common/movie/MovieConversionExtensions.cs b/src/BizHawk.Client.Common/movie/MovieConversionExtensions.cs index aa4c913c243..c04a1c57c00 100644 --- a/src/BizHawk.Client.Common/movie/MovieConversionExtensions.cs +++ b/src/BizHawk.Client.Common/movie/MovieConversionExtensions.cs @@ -133,7 +133,6 @@ public static FileWriteResult ConvertToSaveRamAnchoredMovie(this ITas foreach (var (k, v) in old.HeaderEntries) tas.HeaderEntries[k] = v; - tas.StartsFromSaveRam = true; tas.SyncSettingsJson = old.SyncSettingsJson; foreach (string comment in old.Comments) diff --git a/src/BizHawk.Client.Common/movie/bk2/Bk2Movie.HeaderApi.cs b/src/BizHawk.Client.Common/movie/bk2/Bk2Movie.HeaderApi.cs index a05ff5a67b0..0e532b1064f 100644 --- a/src/BizHawk.Client.Common/movie/bk2/Bk2Movie.HeaderApi.cs +++ b/src/BizHawk.Client.Common/movie/bk2/Bk2Movie.HeaderApi.cs @@ -51,26 +51,7 @@ public virtual bool StartsFromSavestate } } - public bool StartsFromSaveRam - { - // ReSharper disable SimplifyConditionalTernaryExpression - get => Header.TryGetValue(HeaderKeys.StartsFromSaveram, out var s) ? bool.Parse(s) : false; - // ReSharper restore SimplifyConditionalTernaryExpression - set - { - if (value) - { - if (!Header.ContainsKey(HeaderKeys.StartsFromSaveram)) - { - Header.Add(HeaderKeys.StartsFromSaveram, "True"); - } - } - else - { - Header.Remove(HeaderKeys.StartsFromSaveram); - } - } - } + public bool StartsFromSaveRam => SaveRam != null; public override string GameName { diff --git a/src/BizHawk.Client.Common/movie/interfaces/IMovie.cs b/src/BizHawk.Client.Common/movie/interfaces/IMovie.cs index ac3d276eea5..79e53249908 100644 --- a/src/BizHawk.Client.Common/movie/interfaces/IMovie.cs +++ b/src/BizHawk.Client.Common/movie/interfaces/IMovie.cs @@ -67,7 +67,7 @@ public interface IMovie : IBasicMovieInfo byte[] SaveRam { get; set; } bool StartsFromSavestate { get; set; } - bool StartsFromSaveRam { get; set; } + bool StartsFromSaveRam { get; } string LogKey { get; set; } diff --git a/src/BizHawk.Client.EmuHawk/MainForm.cs b/src/BizHawk.Client.EmuHawk/MainForm.cs index f2be7c9aa28..5f2e51e9376 100644 --- a/src/BizHawk.Client.EmuHawk/MainForm.cs +++ b/src/BizHawk.Client.EmuHawk/MainForm.cs @@ -27,12 +27,8 @@ using BizHawk.Emulation.Common; using BizHawk.Emulation.Cores; -using BizHawk.Emulation.Cores.Computers.AppleII; -using BizHawk.Emulation.Cores.Computers.Commodore64; using BizHawk.Emulation.Cores.Computers.DOS; using BizHawk.Emulation.Cores.Consoles.Nintendo.QuickNES; -using BizHawk.Emulation.Cores.Consoles.SNK; -using BizHawk.Emulation.Cores.Nintendo.GBA; using BizHawk.Emulation.Cores.Nintendo.NES; using BizHawk.Emulation.Cores.Nintendo.SNES; @@ -1932,36 +1928,24 @@ private void LoadSaveRam() return; } + byte[] sram = null; try { - byte[] sram; - - // some cores might not know how big the saveram ought to be, so just send it the whole file - if (Emulator is AppleII or C64 or DOSBox or MGBAHawk or NeoGeoPort or NES { BoardName: "FDS" }) - { - sram = File.ReadAllBytes(saveramToLoad.FullName); - } - else - { - var oldRam = Emulator.AsSaveRam().CloneSaveRam(); - if (oldRam is null) - { - // we have a SaveRAM file, but the current core does not have save ram. - // just skip loading the saveram file in that case - return; - } - - // why do we silently truncate\pad here instead of warning\erroring? - sram = new byte[oldRam.Length]; - using var fs = saveramToLoad.OpenRead(); - _ = fs.Read(sram, 0, sram.Length); - } + sram = File.ReadAllBytes(saveramToLoad.FullName); + } + catch (Exception e) + { + AddOnScreenMessage("An IO error occurred while loading Sram"); + Console.Error.WriteLine(e); + } - Emulator.AsSaveRam().StoreSaveRam(sram); + try + { + if (sram != null) Emulator.AsSaveRam().StoreSaveRam(sram); } - catch (IOException e) + catch (Exception e) { - AddOnScreenMessage("An error occurred while loading Sram"); + AddOnScreenMessage("The core threw an error while loading Sram"); Console.Error.WriteLine(e); } } @@ -1987,9 +1971,7 @@ public bool FlushSaveRAM(bool autosave = false) var backupPath = $"{path}.bak"; var backupFile = new FileInfo(backupPath); - var saveram = Emulator.AsSaveRam().CloneSaveRam(); - if (saveram == null) - return true; + var saveram = Emulator.AsSaveRam().CloneSaveRam()!; try { diff --git a/src/BizHawk.Client.EmuHawk/movie/RecordMovie.cs b/src/BizHawk.Client.EmuHawk/movie/RecordMovie.cs index 716d39e679d..5e6fd433722 100644 --- a/src/BizHawk.Client.EmuHawk/movie/RecordMovie.cs +++ b/src/BizHawk.Client.EmuHawk/movie/RecordMovie.cs @@ -103,7 +103,7 @@ public RecordMovie( MaxDropDownItems = 32, Size = new(152, 21), }; - if (_emulator.HasSaveRam() && _emulator.AsSaveRam().CloneSaveRam(clearDirty: false) is not null) StartFromCombo.Items.Add(START_FROM_SAVERAM); + if (_emulator.HasSaveRam()) StartFromCombo.Items.Add(START_FROM_SAVERAM); if (_emulator.HasSavestates()) StartFromCombo.Items.Add(START_FROM_SAVESTATE); DefaultAuthorCheckBox = new() @@ -242,7 +242,6 @@ private void Ok_Click(object sender, EventArgs e) else if (selectedStartFromValue is START_FROM_SAVERAM && _emulator.HasSaveRam()) { var core = _emulator.AsSaveRam(); - movieToRecord.StartsFromSaveRam = true; movieToRecord.SaveRam = core.CloneSaveRam(clearDirty: false); } diff --git a/src/BizHawk.Client.EmuHawk/tools/TAStudio/TAStudio.MenuItems.cs b/src/BizHawk.Client.EmuHawk/tools/TAStudio/TAStudio.MenuItems.cs index a3c281c0bce..a07acced78f 100644 --- a/src/BizHawk.Client.EmuHawk/tools/TAStudio/TAStudio.MenuItems.cs +++ b/src/BizHawk.Client.EmuHawk/tools/TAStudio/TAStudio.MenuItems.cs @@ -36,7 +36,7 @@ private void NewFromSubMenu_DropDownOpened(object sender, EventArgs e) NewFromCurrentSaveRamMenuItem.Enabled = CurrentTasMovie.InputLogLength > 0 - && SaveRamEmulator != null; + && SaveRamEmulator?.SupportsSaveRam == true; } private void StartNewProjectFromNowMenuItem_Click(object sender, EventArgs e) @@ -59,7 +59,7 @@ private void StartANewProjectFromSaveRamMenuItem_Click(object sender, EventArgs { if (AskSaveChanges()) { - var saveRam = SaveRamEmulator?.CloneSaveRam(clearDirty: false) ?? throw new Exception("No SaveRam"); + var saveRam = SaveRamEmulator?.CloneSaveRam(clearDirty: false) ?? throw new Exception("No SaveRam; this button should have been disabled."); GoToFrame(TasView.AnyRowsSelected ? TasView.FirstSelectedRowIndex : 0); var result = CurrentTasMovie.ConvertToSaveRamAnchoredMovie(saveRam); DisplayMessageIfFailed(() => result, "Failed to create movie."); @@ -1315,7 +1315,7 @@ private void RightClickMenu_Opened(object sender, EventArgs e) StartANewProjectFromSaveRamMenuItem.Visible = selectionIsSingleRow - && SaveRamEmulator != null + && SaveRamEmulator?.SupportsSaveRam == true && !CurrentTasMovie.StartsFromSavestate; StartFromNowSeparator.Visible = StartNewProjectFromNowMenuItem.Visible || StartANewProjectFromSaveRamMenuItem.Visible; diff --git a/src/BizHawk.Emulation.Common/Base Implementations/LinkedSaveRam.cs b/src/BizHawk.Emulation.Common/Base Implementations/LinkedSaveRam.cs index cd2ea92225e..ccc86f5dbab 100644 --- a/src/BizHawk.Emulation.Common/Base Implementations/LinkedSaveRam.cs +++ b/src/BizHawk.Emulation.Common/Base Implementations/LinkedSaveRam.cs @@ -55,13 +55,17 @@ public void StoreSaveRam(byte[] data) int pos = 0; for (int i = 0; i < _numCores; i++) { - var toCopy = _linkedCores[i].AsSaveRam().CloneSaveRam(); // wait CloneSaveRam is already a copy, why are we copying it again - if (toCopy is null) continue; - var b = new byte[toCopy.Length]; + var numberBytesToCopy = _linkedCores[i].AsSaveRam().CloneSaveRam()?.Length; + if (numberBytesToCopy is null) continue; + var b = new byte[numberBytesToCopy.Value]; Buffer.BlockCopy(data, pos, b, 0, b.Length); pos += b.Length; _linkedCores[i].AsSaveRam().StoreSaveRam(b); } + + if (data.Length != pos) throw new InvalidOperationException("Incorrect sram size."); } + + public bool SupportsSaveRam => true; } } diff --git a/src/BizHawk.Emulation.Common/Extensions.cs b/src/BizHawk.Emulation.Common/Extensions.cs index c277ff125ec..3c935d8af09 100644 --- a/src/BizHawk.Emulation.Common/Extensions.cs +++ b/src/BizHawk.Emulation.Common/Extensions.cs @@ -134,7 +134,7 @@ public static IMemoryDomains AsMemoryDomains(this IEmulator core) public static bool HasSaveRam(this IEmulator core) { - return core != null && core.ServiceProvider.HasService(); + return core != null && core.ServiceProvider.HasService() && core.AsSaveRam()!.SupportsSaveRam; } public static ISaveRam AsSaveRam(this IEmulator core) diff --git a/src/BizHawk.Emulation.Common/Interfaces/Services/ISaveRam.cs b/src/BizHawk.Emulation.Common/Interfaces/Services/ISaveRam.cs index d0fa8d67e8f..e7c73e6153b 100644 --- a/src/BizHawk.Emulation.Common/Interfaces/Services/ISaveRam.cs +++ b/src/BizHawk.Emulation.Common/Interfaces/Services/ISaveRam.cs @@ -10,16 +10,16 @@ public interface ISaveRam : IEmulatorService { /// /// Returns a copy of the SaveRAM. Editing it won't do you any good unless you later call StoreSaveRam() - /// This IS allowed to return null. - /// Unfortunately, the core may think differently of a nonexisting (null) saveram vs a 0 size saveram. - /// Frontend users of the ISaveRam should treat null as nonexisting (and thus not even write the file, so that the "does not exist" condition can be roundtripped and not confused with an empty file) + /// This method must return null if and only if is false. /// /// Whether the saveram should be considered in a clean state after this call for purposes of byte[]? CloneSaveRam(bool clearDirty = true); /// - /// store new SaveRAM to the emu core. the data should be the same size as the return from ReadSaveRam() + /// Store new SaveRAM to the emu core. + /// The core must ignore calls to this method if is false. /// + /// The core may throw an exception if the given data is invalid. void StoreSaveRam(byte[] data); /// @@ -28,5 +28,10 @@ public interface ISaveRam : IEmulatorService /// This value should be considered a hint more than an absolute truth. /// bool SaveRamModified { get; } + + /// + /// Certain cores may support SaveRam only in certain situations, for example only for certain games. + /// + bool SupportsSaveRam { get; } } } diff --git a/src/BizHawk.Emulation.Cores/Arcades/MAME/MAME.ISaveRam.cs b/src/BizHawk.Emulation.Cores/Arcades/MAME/MAME.ISaveRam.cs index 7bd53d6ca4d..e87e031a273 100644 --- a/src/BizHawk.Emulation.Cores/Arcades/MAME/MAME.ISaveRam.cs +++ b/src/BizHawk.Emulation.Cores/Arcades/MAME/MAME.ISaveRam.cs @@ -18,6 +18,8 @@ private void GetNVRAMFilenames() public bool SaveRamModified => _nvramFilenames.Count > 0; + public bool SupportsSaveRam => _nvramFilenames.Count != 0; + public byte[] CloneSaveRam(bool clearDirty) { if (_nvramFilenames.Count == 0) diff --git a/src/BizHawk.Emulation.Cores/Computers/AppleII/AppleII.ISaveRam.cs b/src/BizHawk.Emulation.Cores/Computers/AppleII/AppleII.ISaveRam.cs index b686ccf80e9..0df774f7985 100644 --- a/src/BizHawk.Emulation.Cores/Computers/AppleII/AppleII.ISaveRam.cs +++ b/src/BizHawk.Emulation.Cores/Computers/AppleII/AppleII.ISaveRam.cs @@ -15,6 +15,7 @@ private void InitSaveRam() } public bool SaveRamModified => true; + public bool SupportsSaveRam => true; public byte[] CloneSaveRam(bool clearDirty) { diff --git a/src/BizHawk.Emulation.Cores/Computers/Commodore64/Cartridge/Mapper0020.cs b/src/BizHawk.Emulation.Cores/Computers/Commodore64/Cartridge/Mapper0020.cs index 469f1ddfc4c..b241fb41ed8 100644 --- a/src/BizHawk.Emulation.Cores/Computers/Commodore64/Cartridge/Mapper0020.cs +++ b/src/BizHawk.Emulation.Cores/Computers/Commodore64/Cartridge/Mapper0020.cs @@ -302,11 +302,15 @@ public void StoreSaveRam(byte[] data) var deltaBSize = reader.ReadInt32(); _deltaB = reader.ReadBytes(deltaBSize); + if (reader.BaseStream.Position != deltaASize + deltaBSize + 8) throw new InvalidOperationException("Incorrect sram size."); + DeltaSerializer.ApplyDelta(_originalMediaA, _chipA.Data, _deltaA); DeltaSerializer.ApplyDelta(_originalMediaB, _chipB.Data, _deltaB); _saveRamDirty = false; } public bool SaveRamModified => _saveRamDirty; + + public bool SupportsSaveRam => true; } } diff --git a/src/BizHawk.Emulation.Cores/Computers/Commodore64/Serial/Drive1541.SaveRam.cs b/src/BizHawk.Emulation.Cores/Computers/Commodore64/Serial/Drive1541.SaveRam.cs index a34b18e2f4c..b3a66063878 100644 --- a/src/BizHawk.Emulation.Cores/Computers/Commodore64/Serial/Drive1541.SaveRam.cs +++ b/src/BizHawk.Emulation.Cores/Computers/Commodore64/Serial/Drive1541.SaveRam.cs @@ -39,6 +39,8 @@ public void InitSaveRam(int diskCount) public bool SaveRamModified { get; private set; } = false; + public bool SupportsSaveRam => true; + public byte[] CloneSaveRam(bool clearDirty) { SaveDeltas(); diff --git a/src/BizHawk.Emulation.Cores/Computers/MSX/MSX.ISaveRam.cs b/src/BizHawk.Emulation.Cores/Computers/MSX/MSX.ISaveRam.cs index 7b226140c15..c4869319fa3 100644 --- a/src/BizHawk.Emulation.Cores/Computers/MSX/MSX.ISaveRam.cs +++ b/src/BizHawk.Emulation.Cores/Computers/MSX/MSX.ISaveRam.cs @@ -13,12 +13,15 @@ public void StoreSaveRam(byte[] data) { if (SaveRAM != null) { + if (data.Length != SaveRAM.Length) throw new InvalidOperationException("Incorrect sram size."); Array.Copy(data, SaveRAM, data.Length); } } public bool SaveRamModified { get; private set; } + public bool SupportsSaveRam => SaveRAM != null; + public byte[] SaveRAM; private byte SaveRamBank; } diff --git a/src/BizHawk.Emulation.Cores/Consoles/Atari/A7800Hawk/A7800Hawk.ISaveRam.cs b/src/BizHawk.Emulation.Cores/Consoles/Atari/A7800Hawk/A7800Hawk.ISaveRam.cs index 9e8d938d526..a71c0c11fbc 100644 --- a/src/BizHawk.Emulation.Cores/Consoles/Atari/A7800Hawk/A7800Hawk.ISaveRam.cs +++ b/src/BizHawk.Emulation.Cores/Consoles/Atari/A7800Hawk/A7800Hawk.ISaveRam.cs @@ -11,9 +11,12 @@ public byte[] CloneSaveRam(bool clearDirty) public void StoreSaveRam(byte[] data) { + if (data.Length != _hsram.Length) throw new InvalidOperationException("Incorrect sram size."); Buffer.BlockCopy(data, 0, _hsram, 0, data.Length); } public bool SaveRamModified => (_hsbios != null); + + public bool SupportsSaveRam => true; } } diff --git a/src/BizHawk.Emulation.Cores/Consoles/Atari/jaguar/VirtualJaguar.ISaveRam.cs b/src/BizHawk.Emulation.Cores/Consoles/Atari/jaguar/VirtualJaguar.ISaveRam.cs index 3992a453c1f..a1dbd5a5785 100644 --- a/src/BizHawk.Emulation.Cores/Consoles/Atari/jaguar/VirtualJaguar.ISaveRam.cs +++ b/src/BizHawk.Emulation.Cores/Consoles/Atari/jaguar/VirtualJaguar.ISaveRam.cs @@ -8,6 +8,8 @@ public partial class VirtualJaguar : ISaveRam public new bool SaveRamModified => _saveRamSize > 0 && _core.SaveRamIsDirty(); + public new bool SupportsSaveRam => _saveRamSize != 0; + public new byte[] CloneSaveRam(bool clearDirty) { if (_saveRamSize == 0) diff --git a/src/BizHawk.Emulation.Cores/Consoles/Atari/lynx/Lynx.ISaveRam.cs b/src/BizHawk.Emulation.Cores/Consoles/Atari/lynx/Lynx.ISaveRam.cs index 086e37f98df..a81cfeb4a86 100644 --- a/src/BizHawk.Emulation.Cores/Consoles/Atari/lynx/Lynx.ISaveRam.cs +++ b/src/BizHawk.Emulation.Cores/Consoles/Atari/lynx/Lynx.ISaveRam.cs @@ -22,7 +22,7 @@ public void StoreSaveRam(byte[] srcData) { if (!LibLynx.GetSaveRamPtr(Core, out var size, out var data)) { - throw new InvalidOperationException(); + return; } if (srcData.Length != size) throw new ArgumentException(message: "buffer too small", paramName: nameof(srcData)); @@ -31,5 +31,7 @@ public void StoreSaveRam(byte[] srcData) } public bool SaveRamModified => LibLynx.GetSaveRamPtr(Core, out int unused, out IntPtr unused2); + + public bool SupportsSaveRam => LibLynx.GetSaveRamPtr(Core, out int _, out IntPtr _); } } diff --git a/src/BizHawk.Emulation.Cores/Consoles/GCE/Vectrex/VectrexHawk.ISaveRam.cs b/src/BizHawk.Emulation.Cores/Consoles/GCE/Vectrex/VectrexHawk.ISaveRam.cs index d93b0732874..ebc749d05e5 100644 --- a/src/BizHawk.Emulation.Cores/Consoles/GCE/Vectrex/VectrexHawk.ISaveRam.cs +++ b/src/BizHawk.Emulation.Cores/Consoles/GCE/Vectrex/VectrexHawk.ISaveRam.cs @@ -12,5 +12,7 @@ public byte[] CloneSaveRam(bool clearDirty) public void StoreSaveRam(byte[] data) {} public bool SaveRamModified => false; + + public bool SupportsSaveRam => false; } } diff --git a/src/BizHawk.Emulation.Cores/Consoles/Magnavox/Odyssey2/O2Hawk.ISaveRam.cs b/src/BizHawk.Emulation.Cores/Consoles/Magnavox/Odyssey2/O2Hawk.ISaveRam.cs index 4950fc66785..929aac06ffb 100644 --- a/src/BizHawk.Emulation.Cores/Consoles/Magnavox/Odyssey2/O2Hawk.ISaveRam.cs +++ b/src/BizHawk.Emulation.Cores/Consoles/Magnavox/Odyssey2/O2Hawk.ISaveRam.cs @@ -11,13 +11,16 @@ public byte[] CloneSaveRam(bool clearDirty) public void StoreSaveRam(byte[] data) { - if (_syncSettings.Use_SRAM) + if (cart_RAM != null && _syncSettings.Use_SRAM) { + if (data.Length != cart_RAM.Length) throw new InvalidOperationException("Incorrect sram size."); Buffer.BlockCopy(data, 0, cart_RAM, 0, data.Length); Console.WriteLine("loading SRAM here"); } } public bool SaveRamModified => has_bat & _syncSettings.Use_SRAM; + + public bool SupportsSaveRam => cart_RAM != null; // The Use_SRAM setting implements behavior not officially supported by BizHawk. } } diff --git a/src/BizHawk.Emulation.Cores/Consoles/Nintendo/BSNES/BsnesCore.ISaveRam.cs b/src/BizHawk.Emulation.Cores/Consoles/Nintendo/BSNES/BsnesCore.ISaveRam.cs index 54fc08a59fa..c0a2fcdd241 100644 --- a/src/BizHawk.Emulation.Cores/Consoles/Nintendo/BSNES/BsnesCore.ISaveRam.cs +++ b/src/BizHawk.Emulation.Cores/Consoles/Nintendo/BSNES/BsnesCore.ISaveRam.cs @@ -11,6 +11,8 @@ public partial class BsnesCore : ISaveRam public bool SaveRamModified => _saveRamSize != 0; + public bool SupportsSaveRam => _saveRamSize != 0; + public byte[] CloneSaveRam(bool clearDirty) { if (_saveRamSize == 0) return null; diff --git a/src/BizHawk.Emulation.Cores/Consoles/Nintendo/GBA/MGBAHawk.ISaveRam.cs b/src/BizHawk.Emulation.Cores/Consoles/Nintendo/GBA/MGBAHawk.ISaveRam.cs index 76afb36a476..b445a3a9d99 100644 --- a/src/BizHawk.Emulation.Cores/Consoles/Nintendo/GBA/MGBAHawk.ISaveRam.cs +++ b/src/BizHawk.Emulation.Cores/Consoles/Nintendo/GBA/MGBAHawk.ISaveRam.cs @@ -33,6 +33,8 @@ public byte[] CloneSaveRam(bool clearDirty) public void StoreSaveRam(byte[] data) { + if (!SupportsSaveRam) return; + if (data.AsSpan().Slice(0, 8).SequenceEqual(_legacyHeader)) { data = LegacyFix(data); @@ -43,6 +45,17 @@ public void StoreSaveRam(byte[] data) public bool SaveRamModified => LibmGBA.BizGetSaveRam(Core, _saveScratch, _saveScratch.Length) > 0; + public bool SupportsSaveRam + { + get + { + // Is all of this necessary? Someone who knows how the core works might know. + int len = LibmGBA.BizGetSaveRam(Core, _saveScratch, _saveScratch.Length); + len = TruncateRTCIfUsingDeterministicTime(len); + return len != 0; + } + } + private static byte[] LegacyFix(byte[] saveram) { // at one point vbanext-hawk had a special saveram format which we want to load. diff --git a/src/BizHawk.Emulation.Cores/Consoles/Nintendo/GBHawk/GBHawk.ISaveRam.cs b/src/BizHawk.Emulation.Cores/Consoles/Nintendo/GBHawk/GBHawk.ISaveRam.cs index 9843e303d13..518f190a81a 100644 --- a/src/BizHawk.Emulation.Cores/Consoles/Nintendo/GBHawk/GBHawk.ISaveRam.cs +++ b/src/BizHawk.Emulation.Cores/Consoles/Nintendo/GBHawk/GBHawk.ISaveRam.cs @@ -11,13 +11,16 @@ public byte[] CloneSaveRam(bool clearDirty) public void StoreSaveRam(byte[] data) { - if (_syncSettings.Use_SRAM) + if (cart_RAM != null && _syncSettings.Use_SRAM) { + if (data.Length != cart_RAM.Length) throw new InvalidOperationException("Incorrect sram size."); Buffer.BlockCopy(data, 0, cart_RAM, 0, data.Length); Console.WriteLine("loading SRAM here"); } } public bool SaveRamModified => has_bat & _syncSettings.Use_SRAM; + + public bool SupportsSaveRam => cart_RAM != null; // The Use_SRAM setting implements behavior not officially supported by BizHawk. } } diff --git a/src/BizHawk.Emulation.Cores/Consoles/Nintendo/GBHawkLink/GBHawkLink.ISaveRam.cs b/src/BizHawk.Emulation.Cores/Consoles/Nintendo/GBHawkLink/GBHawkLink.ISaveRam.cs index f5e69eb7085..cf25dc0e830 100644 --- a/src/BizHawk.Emulation.Cores/Consoles/Nintendo/GBHawkLink/GBHawkLink.ISaveRam.cs +++ b/src/BizHawk.Emulation.Cores/Consoles/Nintendo/GBHawkLink/GBHawkLink.ISaveRam.cs @@ -54,14 +54,17 @@ public void StoreSaveRam(byte[] data) { if (L.cart_RAM != null && R.cart_RAM == null) { + if (data.Length != L.cart_RAM.Length) throw new InvalidOperationException("Incorrect sram size."); Buffer.BlockCopy(data, 0, L.cart_RAM, 0, L.cart_RAM.Length); } else if (R.cart_RAM != null && L.cart_RAM == null) { + if (data.Length != R.cart_RAM.Length) throw new InvalidOperationException("Incorrect sram size."); Buffer.BlockCopy(data, 0, R.cart_RAM, 0, R.cart_RAM.Length); } else if (R.cart_RAM != null && L.cart_RAM != null) { + if (data.Length != L.cart_RAM.Length + R.cart_RAM.Length) throw new InvalidOperationException("Incorrect sram size."); Buffer.BlockCopy(data, 0, L.cart_RAM, 0, L.cart_RAM.Length); Buffer.BlockCopy(data, L.cart_RAM.Length, R.cart_RAM, 0, R.cart_RAM.Length); } @@ -71,5 +74,7 @@ public void StoreSaveRam(byte[] data) } public bool SaveRamModified => (L.has_bat || R.has_bat) & linkSyncSettings.Use_SRAM; + + public bool SupportsSaveRam => L.cart_RAM != null || R.cart_RAM != null; // The Use_SRAM setting implements behavior not officially supported by BizHawk. } } diff --git a/src/BizHawk.Emulation.Cores/Consoles/Nintendo/GBHawkLink3x/GBHawkLink3x.ISaveRam.cs b/src/BizHawk.Emulation.Cores/Consoles/Nintendo/GBHawkLink3x/GBHawkLink3x.ISaveRam.cs index b4241b47301..6b9bd590da8 100644 --- a/src/BizHawk.Emulation.Cores/Consoles/Nintendo/GBHawkLink3x/GBHawkLink3x.ISaveRam.cs +++ b/src/BizHawk.Emulation.Cores/Consoles/Nintendo/GBHawkLink3x/GBHawkLink3x.ISaveRam.cs @@ -84,12 +84,17 @@ public void StoreSaveRam(byte[] data) if (R.cart_RAM != null) { Buffer.BlockCopy(data, temp, R.cart_RAM, 0, R.cart_RAM.Length); + temp += R.cart_RAM.Length; } + if (data.Length != temp) throw new InvalidOperationException("Incorrect sram size."); + Console.WriteLine("loading SRAM here"); } } public bool SaveRamModified => (L.has_bat || C.has_bat || R.has_bat) & Link3xSyncSettings.Use_SRAM; + + public bool SupportsSaveRam => L.cart_RAM != null || C.cart_RAM != null || R.cart_RAM != null; // The Use_SRAM setting implements behavior not officially supported by BizHawk. } } diff --git a/src/BizHawk.Emulation.Cores/Consoles/Nintendo/GBHawkLink4x/GBHawkLink4x.ISaveRam.cs b/src/BizHawk.Emulation.Cores/Consoles/Nintendo/GBHawkLink4x/GBHawkLink4x.ISaveRam.cs index e6fbe4a945f..aac9d7009b6 100644 --- a/src/BizHawk.Emulation.Cores/Consoles/Nintendo/GBHawkLink4x/GBHawkLink4x.ISaveRam.cs +++ b/src/BizHawk.Emulation.Cores/Consoles/Nintendo/GBHawkLink4x/GBHawkLink4x.ISaveRam.cs @@ -105,12 +105,17 @@ public void StoreSaveRam(byte[] data) if (D.cart_RAM != null) { Buffer.BlockCopy(data, temp, D.cart_RAM, 0, D.cart_RAM.Length); + temp += D.cart_RAM.Length; } + if (data.Length != temp) throw new InvalidOperationException("Incorrect sram size."); + Console.WriteLine("loading SRAM here"); } } public bool SaveRamModified => (A.has_bat || B.has_bat || C.has_bat || D.has_bat) & Link4xSyncSettings.Use_SRAM; + + public bool SupportsSaveRam => A.cart_RAM != null || B.cart_RAM != null || C.cart_RAM != null || D.cart_RAM != null; // The Use_SRAM setting implements behavior not officially supported by BizHawk. } } diff --git a/src/BizHawk.Emulation.Cores/Consoles/Nintendo/Gameboy/Gambatte.ISaveRam.cs b/src/BizHawk.Emulation.Cores/Consoles/Nintendo/Gameboy/Gambatte.ISaveRam.cs index 213ccd0d9d1..5df90660919 100644 --- a/src/BizHawk.Emulation.Cores/Consoles/Nintendo/Gameboy/Gambatte.ISaveRam.cs +++ b/src/BizHawk.Emulation.Cores/Consoles/Nintendo/Gameboy/Gambatte.ISaveRam.cs @@ -7,6 +7,8 @@ public partial class Gameboy : ISaveRam // need to wire more stuff into the core to actually know this public bool SaveRamModified => LibGambatte.gambatte_getsavedatalength(GambatteState) != 0; + public bool SupportsSaveRam => LibGambatte.gambatte_getsavedatalength(GambatteState) != 0; + public byte[] CloneSaveRam(bool clearDirty) { var length = LibGambatte.gambatte_getsavedatalength(GambatteState); @@ -24,6 +26,7 @@ public byte[] CloneSaveRam(bool clearDirty) public void StoreSaveRam(byte[] data) { var expected = LibGambatte.gambatte_getsavedatalength(GambatteState); + if (expected == 0) return; if (data.Length != expected) throw new ArgumentException(message: "Size of saveram data does not match expected!", paramName: nameof(data)); LibGambatte.gambatte_loadsavedata(GambatteState, data); diff --git a/src/BizHawk.Emulation.Cores/Consoles/Nintendo/N64/N64.ISaveRam.cs b/src/BizHawk.Emulation.Cores/Consoles/Nintendo/N64/N64.ISaveRam.cs index 974f913376f..02649f83b17 100644 --- a/src/BizHawk.Emulation.Cores/Consoles/Nintendo/N64/N64.ISaveRam.cs +++ b/src/BizHawk.Emulation.Cores/Consoles/Nintendo/N64/N64.ISaveRam.cs @@ -15,5 +15,7 @@ public void StoreSaveRam(byte[] data) } public bool SaveRamModified => true; + + public bool SupportsSaveRam => true; } } diff --git a/src/BizHawk.Emulation.Cores/Consoles/Nintendo/NDS/MelonDS.ISaveRam.cs b/src/BizHawk.Emulation.Cores/Consoles/Nintendo/NDS/MelonDS.ISaveRam.cs index 478b52fa846..eb4f647e628 100644 --- a/src/BizHawk.Emulation.Cores/Consoles/Nintendo/NDS/MelonDS.ISaveRam.cs +++ b/src/BizHawk.Emulation.Cores/Consoles/Nintendo/NDS/MelonDS.ISaveRam.cs @@ -9,6 +9,8 @@ public partial class NDS : ISaveRam public new bool SaveRamModified => IsDSiWare ? DSiWareSaveLength != 0 : _core.SaveRamIsDirty(_console); + public new bool SupportsSaveRam => IsDSiWare ? DSiWareSaveLength != 0 : _core.GetSaveRamLength(_console) != 0; + public new byte[] CloneSaveRam(bool clearDirty) { if (IsDSiWare) @@ -54,6 +56,7 @@ public partial class NDS : ISaveRam { if (IsDSiWare) { + if (DSiWareSaveLength == 0) return; if (data.Length == DSiWareSaveLength) { if (PublicSavSize > 0) _exe.AddReadonlyFile(data.AsSpan().Slice(0, PublicSavSize).ToArray(), "public.sav"); @@ -66,6 +69,10 @@ public partial class NDS : ISaveRam if (PrivateSavSize > 0) _exe.RemoveReadonlyFile("private.sav"); if (BannerSavSize > 0) _exe.RemoveReadonlyFile("banner.sav"); } + else + { + throw new InvalidOperationException("Incorrect sram size."); + } } else if (data.Length > 0) { diff --git a/src/BizHawk.Emulation.Cores/Consoles/Nintendo/NES/NES.ISaveRam.cs b/src/BizHawk.Emulation.Cores/Consoles/Nintendo/NES/NES.ISaveRam.cs index dccf2c15a6c..69dc640642c 100644 --- a/src/BizHawk.Emulation.Cores/Consoles/Nintendo/NES/NES.ISaveRam.cs +++ b/src/BizHawk.Emulation.Cores/Consoles/Nintendo/NES/NES.ISaveRam.cs @@ -15,6 +15,17 @@ public bool SaveRamModified } } + public bool SupportsSaveRam + { + get + { + if (Board == null) return false; + if (Board is FDS) return true; + if (Board.SaveRam == null) return false; + return true; + } + } + public byte[] CloneSaveRam(bool clearDirty) { if (Board is FDS fds) @@ -38,6 +49,7 @@ public void StoreSaveRam(byte[] data) return; } + if (data.Length != Board.SaveRam.Length) throw new InvalidOperationException("Incorrect sram size."); Array.Copy(data, Board.SaveRam, data.Length); } } diff --git a/src/BizHawk.Emulation.Cores/Consoles/Nintendo/QuickNES/QuickNES.ISaveRam.cs b/src/BizHawk.Emulation.Cores/Consoles/Nintendo/QuickNES/QuickNES.ISaveRam.cs index cde23166ed1..b0016bc8c41 100644 --- a/src/BizHawk.Emulation.Cores/Consoles/Nintendo/QuickNES/QuickNES.ISaveRam.cs +++ b/src/BizHawk.Emulation.Cores/Consoles/Nintendo/QuickNES/QuickNES.ISaveRam.cs @@ -21,6 +21,8 @@ public void StoreSaveRam(byte[] data) public bool SaveRamModified => QN.qn_has_battery_ram(Context); + public bool SupportsSaveRam => QN.qn_has_battery_ram(Context); + private byte[] _saveRamBuff; private void InitSaveRamBuff() diff --git a/src/BizHawk.Emulation.Cores/Consoles/Nintendo/SNES/LibsnesCore.ISaveRam.cs b/src/BizHawk.Emulation.Cores/Consoles/Nintendo/SNES/LibsnesCore.ISaveRam.cs index bae5d8c2a66..5629c00247b 100644 --- a/src/BizHawk.Emulation.Cores/Consoles/Nintendo/SNES/LibsnesCore.ISaveRam.cs +++ b/src/BizHawk.Emulation.Cores/Consoles/Nintendo/SNES/LibsnesCore.ISaveRam.cs @@ -11,6 +11,10 @@ public unsafe partial class LibsnesCore : ISaveRam Api.QUERY_get_memory_size(LibsnesApi.SNES_MEMORY.CARTRIDGE_RAM) != 0 || Api.QUERY_get_memory_size(LibsnesApi.SNES_MEMORY.SGB_CARTRAM) != 0; + public bool SupportsSaveRam => + Api.QUERY_get_memory_data(LibsnesApi.SNES_MEMORY.CARTRIDGE_RAM) != null + || Api.QUERY_get_memory_data(LibsnesApi.SNES_MEMORY.SGB_CARTRAM) != null; + public byte[] CloneSaveRam(bool clearDirty) { using (Api.EnterExit()) @@ -46,7 +50,7 @@ public void StoreSaveRam(byte[] data) size = Api.QUERY_get_memory_size(LibsnesApi.SNES_MEMORY.SGB_CARTRAM); } - if (size == 0) + if (size == 0 || buf == null) { return; } diff --git a/src/BizHawk.Emulation.Cores/Consoles/Nintendo/SameBoy/SameBoy.ISaveRam.cs b/src/BizHawk.Emulation.Cores/Consoles/Nintendo/SameBoy/SameBoy.ISaveRam.cs index c1d2c369544..d8eba45f1ca 100644 --- a/src/BizHawk.Emulation.Cores/Consoles/Nintendo/SameBoy/SameBoy.ISaveRam.cs +++ b/src/BizHawk.Emulation.Cores/Consoles/Nintendo/SameBoy/SameBoy.ISaveRam.cs @@ -6,6 +6,8 @@ public partial class Sameboy : ISaveRam { public bool SaveRamModified => LibSameboy.sameboy_sramlen(SameboyState) != 0; + public bool SupportsSaveRam => LibSameboy.sameboy_sramlen(SameboyState) != 0; + public byte[] CloneSaveRam(bool clearDirty) { int length = LibSameboy.sameboy_sramlen(SameboyState); @@ -23,12 +25,10 @@ public byte[] CloneSaveRam(bool clearDirty) public void StoreSaveRam(byte[] data) { int expected = LibSameboy.sameboy_sramlen(SameboyState); + if (expected == 0) return; if (data.Length != expected) throw new ArgumentException(message: "Size of saveram data does not match expected!", paramName: nameof(data)); - if (expected > 0) - { - LibSameboy.sameboy_loadsram(SameboyState, data, data.Length); - } + LibSameboy.sameboy_loadsram(SameboyState, data, data.Length); } } } diff --git a/src/BizHawk.Emulation.Cores/Consoles/PC Engine/PCEngine.ISaveRam.cs b/src/BizHawk.Emulation.Cores/Consoles/PC Engine/PCEngine.ISaveRam.cs index 0dd85e1a644..041ad68f819 100644 --- a/src/BizHawk.Emulation.Cores/Consoles/PC Engine/PCEngine.ISaveRam.cs +++ b/src/BizHawk.Emulation.Cores/Consoles/PC Engine/PCEngine.ISaveRam.cs @@ -6,6 +6,8 @@ public sealed partial class PCEngine : ISaveRam { public bool SaveRamModified { get; private set; } + public bool SupportsSaveRam => BRAM != null; + public byte[] CloneSaveRam(bool clearDirty) { if (clearDirty) SaveRamModified = false; @@ -16,6 +18,7 @@ public void StoreSaveRam(byte[] data) { if (BRAM != null) { + if (data.Length != BRAM.Length) throw new InvalidOperationException("Incorrect sram size."); Array.Copy(data, BRAM, data.Length); } diff --git a/src/BizHawk.Emulation.Cores/Consoles/SNK/NeoGeoPort.cs b/src/BizHawk.Emulation.Cores/Consoles/SNK/NeoGeoPort.cs index 51b48aa3196..7d7a4509755 100644 --- a/src/BizHawk.Emulation.Cores/Consoles/SNK/NeoGeoPort.cs +++ b/src/BizHawk.Emulation.Cores/Consoles/SNK/NeoGeoPort.cs @@ -51,6 +51,8 @@ public NeoGeoPort(CoreComm comm, byte[] rom, GameInfo game, } } + public new bool SupportsSaveRam => true; + public new byte[] CloneSaveRam(bool clearDirty) { _exe.AddTransientFile(Array.Empty(), "SAV:flash"); diff --git a/src/BizHawk.Emulation.Cores/Consoles/Sega/GGHawkLink/GGHawkLink.ISaveRam.cs b/src/BizHawk.Emulation.Cores/Consoles/Sega/GGHawkLink/GGHawkLink.ISaveRam.cs index 30967d77975..d7b93f3132f 100644 --- a/src/BizHawk.Emulation.Cores/Consoles/Sega/GGHawkLink/GGHawkLink.ISaveRam.cs +++ b/src/BizHawk.Emulation.Cores/Consoles/Sega/GGHawkLink/GGHawkLink.ISaveRam.cs @@ -52,14 +52,17 @@ public void StoreSaveRam(byte[] data) { if (L.SaveRAM != null && R.SaveRAM == null) { + if (data.Length != L.SaveRAM.Length) throw new InvalidOperationException("Incorrect sram size."); Buffer.BlockCopy(data, 0, L.SaveRAM, 0, L.SaveRAM.Length); } else if (R.SaveRAM != null && L.SaveRAM == null) { + if (data.Length != R.SaveRAM.Length) throw new InvalidOperationException("Incorrect sram size."); Buffer.BlockCopy(data, 0, R.SaveRAM, 0, R.SaveRAM.Length); } else if (R.SaveRAM != null && L.SaveRAM != null) { + if (data.Length != L.SaveRAM.Length + R.SaveRAM.Length) throw new InvalidOperationException("Incorrect sram size."); Buffer.BlockCopy(data, 0, L.SaveRAM, 0, L.SaveRAM.Length); Buffer.BlockCopy(data, L.SaveRAM.Length, R.SaveRAM, 0, R.SaveRAM.Length); } @@ -68,5 +71,7 @@ public void StoreSaveRam(byte[] data) } public bool SaveRamModified => linkSyncSettings.Use_SRAM; + + public bool SupportsSaveRam => L.SaveRAM != null || R.SaveRAM != null; // The Use_SRAM setting implements behavior not officially supported by BizHawk. } } diff --git a/src/BizHawk.Emulation.Cores/Consoles/Sega/SMS/SMS.ISaveRam.cs b/src/BizHawk.Emulation.Cores/Consoles/Sega/SMS/SMS.ISaveRam.cs index 936a037a54c..811c2ed2b37 100644 --- a/src/BizHawk.Emulation.Cores/Consoles/Sega/SMS/SMS.ISaveRam.cs +++ b/src/BizHawk.Emulation.Cores/Consoles/Sega/SMS/SMS.ISaveRam.cs @@ -14,6 +14,7 @@ public void StoreSaveRam(byte[] data) { if (SaveRAM != null) { + if (data.Length != SaveRAM.Length) throw new InvalidOperationException("Incorrect sram size."); Array.Copy(data, SaveRAM, data.Length); } @@ -22,6 +23,8 @@ public void StoreSaveRam(byte[] data) public bool SaveRamModified { get; private set; } + public bool SupportsSaveRam => SaveRAM != null; + public byte[] SaveRAM; private byte SaveRamBank; } diff --git a/src/BizHawk.Emulation.Cores/Consoles/Sega/Saturn/Saturnus.cs b/src/BizHawk.Emulation.Cores/Consoles/Sega/Saturn/Saturnus.cs index b8ddca7175d..3ed83d569ba 100644 --- a/src/BizHawk.Emulation.Cores/Consoles/Sega/Saturn/Saturnus.cs +++ b/src/BizHawk.Emulation.Cores/Consoles/Sega/Saturn/Saturnus.cs @@ -118,6 +118,8 @@ protected override HashSet ComputeHiddenPorts() public new bool SaveRamModified => true; + public new bool SupportsSaveRam => true; + public new byte[] CloneSaveRam(bool clearDirty) { var data = new byte[_saturnus.GetSaveRamLength()]; @@ -126,7 +128,9 @@ protected override HashSet ComputeHiddenPorts() } public new void StoreSaveRam(byte[] data) - => _saturnus.PutSaveRam(data, data.Length); + { + _saturnus.PutSaveRam(data, data.Length); + } public bool IsSTV => _isArcade; } diff --git a/src/BizHawk.Emulation.Cores/Consoles/Sega/gpgx64/GPGX.ISaveRam.cs b/src/BizHawk.Emulation.Cores/Consoles/Sega/gpgx64/GPGX.ISaveRam.cs index 37ad0602406..db5db5cf4f4 100644 --- a/src/BizHawk.Emulation.Cores/Consoles/Sega/gpgx64/GPGX.ISaveRam.cs +++ b/src/BizHawk.Emulation.Cores/Consoles/Sega/gpgx64/GPGX.ISaveRam.cs @@ -33,6 +33,11 @@ public void StoreSaveRam(byte[] data) return; } + if (!SupportsSaveRam) + { + return; + } + if (!Core.gpgx_put_sram(data, data.Length)) { throw new Exception("Core rejected saveram"); @@ -48,5 +53,15 @@ public bool SaveRamModified return size > 0 && area != IntPtr.Zero; } } + + public bool SupportsSaveRam + { + get + { + var size = 0; + var area = Core.gpgx_get_sram(ref size); + return size == 0 || area == IntPtr.Zero; + } + } } } diff --git a/src/BizHawk.Emulation.Cores/Consoles/Sony/PSX/Octoshock.cs b/src/BizHawk.Emulation.Cores/Consoles/Sony/PSX/Octoshock.cs index ae14e0e2b9c..680d9c4a854 100644 --- a/src/BizHawk.Emulation.Cores/Consoles/Sony/PSX/Octoshock.cs +++ b/src/BizHawk.Emulation.Cores/Consoles/Sony/PSX/Octoshock.cs @@ -984,6 +984,8 @@ public bool SaveRamModified } } + public bool SupportsSaveRam => true; + //THIS IS STILL AWFUL diff --git a/src/BizHawk.Emulation.Cores/Consoles/WonderSwan/WonderSwan.ISaveRam.cs b/src/BizHawk.Emulation.Cores/Consoles/WonderSwan/WonderSwan.ISaveRam.cs index 5ada0a45f37..ad47160552e 100644 --- a/src/BizHawk.Emulation.Cores/Consoles/WonderSwan/WonderSwan.ISaveRam.cs +++ b/src/BizHawk.Emulation.Cores/Consoles/WonderSwan/WonderSwan.ISaveRam.cs @@ -30,5 +30,7 @@ public void StoreSaveRam(byte[] data) } public bool SaveRamModified => BizSwan.bizswan_saveramsize(Core) > 0; + + public bool SupportsSaveRam => true; } } diff --git a/src/BizHawk.Emulation.Cores/Libretro/Libretro.ISaveRam.cs b/src/BizHawk.Emulation.Cores/Libretro/Libretro.ISaveRam.cs index 06d03f776fe..95ac053ba0f 100644 --- a/src/BizHawk.Emulation.Cores/Libretro/Libretro.ISaveRam.cs +++ b/src/BizHawk.Emulation.Cores/Libretro/Libretro.ISaveRam.cs @@ -12,6 +12,8 @@ public partial class LibretroHost : ISaveRam public bool SaveRamModified => _saveramSize > 0; + public bool SupportsSaveRam => _saveramSize > 0; + public byte[] CloneSaveRam(bool clearDirty) { if (_saveramSize > 0) @@ -39,6 +41,8 @@ public void StoreSaveRam(byte[] data) Marshal.Copy(data, index, m.Data, (int)m.Size); index += (int)m.Size; } + + if (data.Length != index) throw new InvalidOperationException("Incorrect sram size."); } } } diff --git a/src/BizHawk.Emulation.Cores/Waterbox/WaterboxCore.cs b/src/BizHawk.Emulation.Cores/Waterbox/WaterboxCore.cs index 6bfdffede0c..6fb3a4dcd1d 100644 --- a/src/BizHawk.Emulation.Cores/Waterbox/WaterboxCore.cs +++ b/src/BizHawk.Emulation.Cores/Waterbox/WaterboxCore.cs @@ -158,6 +158,8 @@ public unsafe bool SaveRamModified } } + public bool SupportsSaveRam => _saveramSize != 0; + public byte[] CloneSaveRam(bool clearDirty) { if (_saveramSize == 0) @@ -176,6 +178,8 @@ public byte[] CloneSaveRam(bool clearDirty) public void StoreSaveRam(byte[] data) { + if (!SupportsSaveRam) return; + // Checking if the size of the SaveRAM provided coincides with that expected. This is important for cores whose SaveRAM size can vary depending on their configuration. if (data.Length != _saveramSize) { From 1ba5744dfe2485043e1b5ea99f206221c4fa2d0f Mon Sep 17 00:00:00 2001 From: SuuperW Date: Sat, 19 Jul 2025 19:22:50 -0500 Subject: [PATCH 05/11] Update comments, rename method, and remove obsolete things. --- .../Api/Classes/EmuClientApi.cs | 2 +- src/BizHawk.Client.Common/FileWriter.cs | 3 +- src/BizHawk.Client.Common/IMainFormForApi.cs | 2 +- .../IMainFormForTools.cs | 3 -- src/BizHawk.Client.EmuHawk/MainForm.Events.cs | 4 +-- src/BizHawk.Client.EmuHawk/MainForm.Hotkey.cs | 2 +- src/BizHawk.Client.EmuHawk/MainForm.cs | 32 +++++++++---------- .../TAStudio/TAStudio.IControlMainForm.cs | 11 +++---- 8 files changed, 25 insertions(+), 34 deletions(-) diff --git a/src/BizHawk.Client.Common/Api/Classes/EmuClientApi.cs b/src/BizHawk.Client.Common/Api/Classes/EmuClientApi.cs index 1e00e4ba453..083870c8e07 100644 --- a/src/BizHawk.Client.Common/Api/Classes/EmuClientApi.cs +++ b/src/BizHawk.Client.Common/Api/Classes/EmuClientApi.cs @@ -86,7 +86,7 @@ private void CallStateSaved(object sender, StateSavedEventArgs args) public void CloseEmulator(int? exitCode = null) => _mainForm.CloseEmulator(exitCode); - public void CloseRom() => _mainForm.CloseRom(); + public void CloseRom() => _mainForm.LoadNullRom(); public void DisplayMessages(bool value) => _config.DisplayMessages = value; diff --git a/src/BizHawk.Client.Common/FileWriter.cs b/src/BizHawk.Client.Common/FileWriter.cs index 039c8f47693..a6e019b9e0b 100644 --- a/src/BizHawk.Client.Common/FileWriter.cs +++ b/src/BizHawk.Client.Common/FileWriter.cs @@ -167,8 +167,7 @@ public void Dispose() if (_dispoed) return; _dispoed = true; - _stream!.Flush(flushToDisk: true); - _stream.Dispose(); + _stream!.Dispose(); _stream = null; // The caller should call CloseAndDispose and handle potential failure. diff --git a/src/BizHawk.Client.Common/IMainFormForApi.cs b/src/BizHawk.Client.Common/IMainFormForApi.cs index 765aea62863..5735d7e9f8f 100644 --- a/src/BizHawk.Client.Common/IMainFormForApi.cs +++ b/src/BizHawk.Client.Common/IMainFormForApi.cs @@ -43,7 +43,7 @@ public interface IMainFormForApi void CloseEmulator(int? exitCode = null); /// only referenced from EmuClientApi - void CloseRom(bool clearSram = false); + void LoadNullRom(bool clearSram = false); /// only referenced from ClientLuaLibrary IDecodeResult DecodeCheatForAPI(string code, out MemoryDomain/*?*/ domain); diff --git a/src/BizHawk.Client.EmuHawk/IMainFormForTools.cs b/src/BizHawk.Client.EmuHawk/IMainFormForTools.cs index 3a2649872b9..1e5268bf1dd 100644 --- a/src/BizHawk.Client.EmuHawk/IMainFormForTools.cs +++ b/src/BizHawk.Client.EmuHawk/IMainFormForTools.cs @@ -20,9 +20,6 @@ public interface IMainFormForTools : IDialogController // TODO: remove? or does anything ever need access to the FirmwareManager FirmwareManager FirmwareManager { get; } - /// only referenced from - bool GameIsClosing { get; } - /// only referenced from bool HoldFrameAdvance { get; set; } diff --git a/src/BizHawk.Client.EmuHawk/MainForm.Events.cs b/src/BizHawk.Client.EmuHawk/MainForm.Events.cs index 03e795d7e67..67166e81a99 100644 --- a/src/BizHawk.Client.EmuHawk/MainForm.Events.cs +++ b/src/BizHawk.Client.EmuHawk/MainForm.Events.cs @@ -264,7 +264,7 @@ private void OpenAdvancedMenuItem_Click(object sender, EventArgs e) private void CloseRomMenuItem_Click(object sender, EventArgs e) { Console.WriteLine($"Closing rom clicked Frame: {Emulator.Frame} Emulator: {Emulator.GetType().Name}"); - CloseRom(); + LoadNullRom(); Console.WriteLine($"Closing rom clicked DONE Frame: {Emulator.Frame} Emulator: {Emulator.GetType().Name}"); } @@ -1391,7 +1391,7 @@ private void UndoSavestateContextMenuItem_Click(object sender, EventArgs e) private void ClearSramContextMenuItem_Click(object sender, EventArgs e) { - CloseRom(clearSram: true); + LoadNullRom(clearSram: true); } private void ShowMenuContextMenuItem_Click(object sender, EventArgs e) diff --git a/src/BizHawk.Client.EmuHawk/MainForm.Hotkey.cs b/src/BizHawk.Client.EmuHawk/MainForm.Hotkey.cs index a8b96ab686b..efc704dd8bf 100644 --- a/src/BizHawk.Client.EmuHawk/MainForm.Hotkey.cs +++ b/src/BizHawk.Client.EmuHawk/MainForm.Hotkey.cs @@ -89,7 +89,7 @@ void SelectAndLoadFromSlot(int slot) OpenRom(); break; case "Close ROM": - CloseRom(); + LoadNullRom(); break; case "Load Last ROM": LoadMostRecentROM(); diff --git a/src/BizHawk.Client.EmuHawk/MainForm.cs b/src/BizHawk.Client.EmuHawk/MainForm.cs index 5f2e51e9376..524d7869e08 100644 --- a/src/BizHawk.Client.EmuHawk/MainForm.cs +++ b/src/BizHawk.Client.EmuHawk/MainForm.cs @@ -889,7 +889,7 @@ private void CheckMayCloseAndCleanup(object/*?*/ closingSender, CancelEventArgs return; } } - // zero 03-nov-2015 - close game after other steps. tools might need to unhook themselves from a core. + CloseGame(); SaveConfig(); } @@ -3671,10 +3671,6 @@ private bool LoadRomInternal(string path, LoadRomArgs args, out bool failureIsFr loader.OnLoadSettings += CoreSettings; loader.OnLoadSyncSettings += CoreSyncSettings; - // this also happens in CloseGame(). But it needs to happen here since if we're restarting with the same core, - // any settings changes that we made need to make it back to config before we try to instantiate that core with - // the new settings objects - CommitCoreSettingsToConfig(); // adelikat: I Think by reordering things, this isn't necessary anymore CloseGame(); var nextComm = CreateCoreComm(); @@ -3954,13 +3950,12 @@ private void CommitCoreSettingsToConfig() } } - // whats the difference between these two methods?? - // its very tricky. rename to be more clear or combine them. - // This gets called whenever a core related thing is changed. - // Like reboot core. + /// + /// This closes the game but does not set things up for using the client with the new null emulator. + /// This method should only be called (outside of ) if the caller is about to load a new game with no user interaction between close and load. + /// private void CloseGame(bool clearSram = false) { - GameIsClosing = true; if (clearSram) { var path = Config.PathEntries.SaveRamAbsolutePath(Game, MovieSession.Movie); @@ -4007,7 +4002,7 @@ private void CloseGame(bool clearSram = false) CommitCoreSettingsToConfig(); DisableRewind(); - if (MovieSession.Movie.IsActive()) // Note: this must be called after CommitCoreSettingsToConfig() + if (MovieSession.Movie.IsActive()) // Note: this must be called after CommitCoreSettingsToConfig() because it checks if we have an active movie. { StopMovie(); } @@ -4016,12 +4011,15 @@ private void CloseGame(bool clearSram = false) CheatList.SaveOnClose(); Emulator.Dispose(); + + // This stuff might belong in LoadNullRom. + // However, Emulator.IsNull is used all over and at least one use (in LoadRomInternal) appears to depend on this code being here. + // Some refactoring is needed if these things are to be actually moved to LoadNullRom. Emulator = new NullEmulator(); Game = GameInfo.NullInstance; InputManager.SyncControls(Emulator, MovieSession, Config); RewireSound(); RebootStatusBarIcon.Visible = false; - GameIsClosing = false; } private FileWriteResult AutoSaveStateIfConfigured() @@ -4034,12 +4032,12 @@ private FileWriteResult AutoSaveStateIfConfigured() return new(); } - public bool GameIsClosing { get; private set; } // Lets tools make better decisions when being called by CloseGame - - public void CloseRom(bool clearSram = false) + /// + /// This closes the current ROM, closes tools that require emulator services, and sets things up for the user to interact with the client having no loaded ROM. + /// + /// True if SRAM should be deleted instead of saved. + public void LoadNullRom(bool clearSram = false) { - // This gets called after Close Game gets called. - // Tested with NESHawk and SMB3 (U) if (Tools.AskSave()) { CloseGame(clearSram); diff --git a/src/BizHawk.Client.EmuHawk/tools/TAStudio/TAStudio.IControlMainForm.cs b/src/BizHawk.Client.EmuHawk/tools/TAStudio/TAStudio.IControlMainForm.cs index 272afddc2b7..d19e0bb1a76 100644 --- a/src/BizHawk.Client.EmuHawk/tools/TAStudio/TAStudio.IControlMainForm.cs +++ b/src/BizHawk.Client.EmuHawk/tools/TAStudio/TAStudio.IControlMainForm.cs @@ -59,13 +59,10 @@ public void ToggleReadOnly() public void StopMovie(bool suppressSave) { - if (!MainForm.GameIsClosing) - { - Activate(); - _suppressAskSave = suppressSave; - StartNewTasMovie(); - _suppressAskSave = false; - } + Activate(); + _suppressAskSave = suppressSave; + StartNewTasMovie(); + _suppressAskSave = false; } public bool WantsToControlRewind { get; private set; } = true; From 2e889d0dcd8ea81cd39c63e1590604cdc9b4010a Mon Sep 17 00:00:00 2001 From: SuuperW Date: Sun, 20 Jul 2025 01:21:01 -0500 Subject: [PATCH 06/11] Move stuff around to make more sense for ask-user-about-saving-or-canceling flow even though not all asking and error handling is implemented yet. Also, fix: respect the cancel button. --- src/BizHawk.Client.EmuHawk/MainForm.cs | 74 +++++++++++++------------- 1 file changed, 38 insertions(+), 36 deletions(-) diff --git a/src/BizHawk.Client.EmuHawk/MainForm.cs b/src/BizHawk.Client.EmuHawk/MainForm.cs index 524d7869e08..60109fa59cc 100644 --- a/src/BizHawk.Client.EmuHawk/MainForm.cs +++ b/src/BizHawk.Client.EmuHawk/MainForm.cs @@ -867,31 +867,18 @@ private void CheckMayCloseAndCleanup(object/*?*/ closingSender, CancelEventArgs closingArgs.Cancel = true; return; } + // StopAv would be handled in CloseGame, but since we've asked the user about it, best to handle it now. StopAv(); } - if (!Tools.AskSave()) + SaveConfig(); // TODO: Handle failure. + + if (!CloseGame()) { closingArgs.Cancel = true; return; } - Tools.Close(); - FileWriteResult saveResult = MovieSession.StopMovie(); - if (saveResult.IsError) - { - if (!this.ModalMessageBox2( - caption: "Quit anyway?", - icon: EMsgBoxIcon.Question, - text: "The currently playing movie could not be saved. Continue and quit anyway? All unsaved changes will be lost.")) - { - closingArgs.Cancel = true; - return; - } - } - - CloseGame(); - SaveConfig(); } private readonly bool _suppressSyncSettingsWarning; @@ -2123,7 +2110,7 @@ private void LoadRomFromRecent(string rom) if (!LoadRom(romPath, new LoadRomArgs(ioa), out var failureIsFromAskSave)) { - if (failureIsFromAskSave) AddOnScreenMessage("ROM loading cancelled; a tool had unsaved changes"); + if (failureIsFromAskSave) AddOnScreenMessage("ROM loading cancelled due to unsaved changes"); else if (ioa is OpenAdvanced_LibretroNoGame || File.Exists(romPath)) AddOnScreenMessage("ROM loading failed"); else Config.RecentRoms.HandleLoadError(this, romPath, rom); } @@ -3624,6 +3611,12 @@ public bool LoadRom(string path, LoadRomArgs args, out bool failureIsFromAskSave private bool LoadRomInternal(string path, LoadRomArgs args, out bool failureIsFromAskSave) { failureIsFromAskSave = false; + if (!CloseGame()) + { + failureIsFromAskSave = true; + return false; + } + if (path == null) throw new ArgumentNullException(nameof(path)); if (args == null) @@ -3651,12 +3644,6 @@ private bool LoadRomInternal(string path, LoadRomArgs args, out bool failureIsFr // it is then up to the core itself to override its own local DeterministicEmulation setting bool deterministic = args.Deterministic ?? MovieSession.NewMovieQueued; - if (!Tools.AskSave()) - { - failureIsFromAskSave = true; - return false; - } - var loader = new RomLoader(Config, this) { ChooseArchive = LoadArchiveChooser, @@ -3671,8 +3658,6 @@ private bool LoadRomInternal(string path, LoadRomArgs args, out bool failureIsFr loader.OnLoadSettings += CoreSettings; loader.OnLoadSyncSettings += CoreSyncSettings; - CloseGame(); - var nextComm = CreateCoreComm(); IOpenAdvanced ioa = args.OpenAdvanced; @@ -3954,8 +3939,30 @@ private void CommitCoreSettingsToConfig() /// This closes the game but does not set things up for using the client with the new null emulator. /// This method should only be called (outside of ) if the caller is about to load a new game with no user interaction between close and load. /// - private void CloseGame(bool clearSram = false) + /// True if the game was closed. False if the user cancelled due to unsaved changes. + private bool CloseGame(bool clearSram = false) { + CommitCoreSettingsToConfig(); // Must happen before stopping the movie, since it checks for active movie. + + if (!Tools.AskSave()) + { + return false; + } + // There is a cheats tool, but cheats can be active while the "cheats tool" is not. And have auto-save option. + CheatList.SaveOnClose(); + + FileWriteResult saveResult = MovieSession.StopMovie(); + if (saveResult.IsError) + { + if (!this.ModalMessageBox2( + caption: "Quit anyway?", + icon: EMsgBoxIcon.Question, + text: "The currently playing movie could not be saved. Continue and quit anyway? All unsaved changes will be lost.")) + { + return false; + } + } + if (clearSram) { var path = Config.PathEntries.SaveRamAbsolutePath(Game, MovieSession.Movie); @@ -3979,7 +3986,7 @@ private void CloseGame(bool clearSram = false) EMsgBoxIcon.Error); if (result is false) break; - if (result is null) return; + if (result is null) return false; } } @@ -3993,23 +4000,16 @@ private void CloseGame(bool clearSram = false) $"Failed to auto-save state. Do you want to try again?\n\nError details:\n{stateSaveResult.UserFriendlyErrorMessage()}\n{stateSaveResult.Exception.Message}", "IOError while writing savestate", EMsgBoxIcon.Error); - if (tryAgain == null) return; + if (tryAgain == null) return false; } } while (tryAgain == true); StopAv(); - CommitCoreSettingsToConfig(); DisableRewind(); - if (MovieSession.Movie.IsActive()) // Note: this must be called after CommitCoreSettingsToConfig() because it checks if we have an active movie. - { - StopMovie(); - } - RA?.Stop(); - CheatList.SaveOnClose(); Emulator.Dispose(); // This stuff might belong in LoadNullRom. @@ -4020,6 +4020,8 @@ private void CloseGame(bool clearSram = false) InputManager.SyncControls(Emulator, MovieSession, Config); RewireSound(); RebootStatusBarIcon.Visible = false; + + return true; } private FileWriteResult AutoSaveStateIfConfigured() From a0c3d25deb734cfe72bf6e30dc3bc3222296db3f Mon Sep 17 00:00:00 2001 From: SuuperW Date: Fri, 18 Jul 2025 02:05:08 -0500 Subject: [PATCH 07/11] Handle SRAM saving failure. --- .../Api/Classes/EmuClientApi.cs | 1 + .../DialogControllerExtensions.cs | 31 +++- src/BizHawk.Client.Common/FileWriter.cs | 17 +++ src/BizHawk.Client.Common/IMainFormForApi.cs | 2 +- src/BizHawk.Client.EmuHawk/MainForm.Events.cs | 2 +- src/BizHawk.Client.EmuHawk/MainForm.Hotkey.cs | 2 +- src/BizHawk.Client.EmuHawk/MainForm.cs | 139 +++++++----------- 7 files changed, 105 insertions(+), 89 deletions(-) diff --git a/src/BizHawk.Client.Common/Api/Classes/EmuClientApi.cs b/src/BizHawk.Client.Common/Api/Classes/EmuClientApi.cs index 083870c8e07..b5d8ba1f8b1 100644 --- a/src/BizHawk.Client.Common/Api/Classes/EmuClientApi.cs +++ b/src/BizHawk.Client.Common/Api/Classes/EmuClientApi.cs @@ -158,6 +158,7 @@ public bool OpenRom(string path) public void RebootCore() => _mainForm.RebootCore(); + // TODO: Change return type to FileWriteResult. public void SaveRam() => _mainForm.FlushSaveRAM(); // TODO: Change return type to FileWriteResult. diff --git a/src/BizHawk.Client.Common/DialogControllerExtensions.cs b/src/BizHawk.Client.Common/DialogControllerExtensions.cs index 8e861b13582..9c83be78dc6 100644 --- a/src/BizHawk.Client.Common/DialogControllerExtensions.cs +++ b/src/BizHawk.Client.Common/DialogControllerExtensions.cs @@ -68,13 +68,40 @@ public static void ErrorMessageBox( { Debug.Assert(fileResult.IsError && fileResult.Exception != null, "Error box must have an error."); - string prefix = prefixMessage == null ? "" : prefixMessage + "\n"; + string prefix = prefixMessage ?? ""; dialogParent.ModalMessageBox( - text: $"{prefix}{fileResult.UserFriendlyErrorMessage()}\n{fileResult.Exception!.Message}", + text: $"{prefix}\n{fileResult.UserFriendlyErrorMessage()}\n{fileResult.Exception!.Message}", caption: "Error", icon: EMsgBoxIcon.Error); } + /// + /// If the action fails, asks the user if they want to try again. + /// The user will be repeatedly asked if they want to try again until either success or the user says no. + /// + /// Returns true on success or if the user said no. Returns false if the user said cancel. + public static bool DoWithTryAgainBox( + this IDialogParent dialogParent, + Func action, + string message) + { + FileWriteResult fileResult = action(); + while (fileResult.IsError) + { + string prefix = message ?? ""; + bool? askResult = dialogParent.ModalMessageBox3( + text: $"{prefix} Do you want to try again?\n\nError details:" + + $"{fileResult.UserFriendlyErrorMessage()}\n{fileResult.Exception!.Message}", + caption: "Error", + icon: EMsgBoxIcon.Error); + if (askResult == null) return false; + if (askResult == false) return true; + if (askResult == true) fileResult = action(); + } + + return true; + } + /// Creates and shows a System.Windows.Forms.OpenFileDialog or equivalent with the receiver () as its parent /// OpenFileDialog.RestoreDirectory (isn't this useless when specifying ? keeping it for backcompat) /// OpenFileDialog.Filter diff --git a/src/BizHawk.Client.Common/FileWriter.cs b/src/BizHawk.Client.Common/FileWriter.cs index a6e019b9e0b..8a0a83c22d5 100644 --- a/src/BizHawk.Client.Common/FileWriter.cs +++ b/src/BizHawk.Client.Common/FileWriter.cs @@ -39,6 +39,23 @@ private FileWriter(FileWritePaths paths, FileStream stream) _stream = stream; } + public static FileWriteResult Write(string path, byte[] bytes, string? backupPath = null) + { + FileWriteResult createResult = Create(path); + if (createResult.IsError) return createResult; + + try + { + createResult.Value!.Stream.Write(bytes); + } + catch (Exception ex) + { + return new(FileWriteEnum.FailedDuringWrite, createResult.Value!.Paths, ex); + } + + return createResult.Value.CloseAndDispose(backupPath); + } + /// /// Create a FileWriter instance, or return an error if unable to access the file. diff --git a/src/BizHawk.Client.Common/IMainFormForApi.cs b/src/BizHawk.Client.Common/IMainFormForApi.cs index 5735d7e9f8f..3b409f0f9a8 100644 --- a/src/BizHawk.Client.Common/IMainFormForApi.cs +++ b/src/BizHawk.Client.Common/IMainFormForApi.cs @@ -52,7 +52,7 @@ public interface IMainFormForApi void EnableRewind(bool enabled); /// only referenced from EmuClientApi - bool FlushSaveRAM(bool autosave = false); + FileWriteResult FlushSaveRAM(bool autosave = false); /// only referenced from EmuClientApi void FrameAdvance(bool discardApiHawkSurfaces = true); diff --git a/src/BizHawk.Client.EmuHawk/MainForm.Events.cs b/src/BizHawk.Client.EmuHawk/MainForm.Events.cs index 67166e81a99..a611b23d40e 100644 --- a/src/BizHawk.Client.EmuHawk/MainForm.Events.cs +++ b/src/BizHawk.Client.EmuHawk/MainForm.Events.cs @@ -315,7 +315,7 @@ private bool LoadstateCurrentSlot() private void FlushSaveRAMMenuItem_Click(object sender, EventArgs e) { - FlushSaveRAM(); + ShowMessageIfError(() => FlushSaveRAM(), "Failed to flush saveram!"); } private void ReadonlyMenuItem_Click(object sender, EventArgs e) diff --git a/src/BizHawk.Client.EmuHawk/MainForm.Hotkey.cs b/src/BizHawk.Client.EmuHawk/MainForm.Hotkey.cs index efc704dd8bf..26bd817a4f8 100644 --- a/src/BizHawk.Client.EmuHawk/MainForm.Hotkey.cs +++ b/src/BizHawk.Client.EmuHawk/MainForm.Hotkey.cs @@ -95,7 +95,7 @@ void SelectAndLoadFromSlot(int slot) LoadMostRecentROM(); break; case "Flush SaveRAM": - FlushSaveRAM(); + FlushSaveRAMMenuItem_Click(null, null); break; case "Display FPS": ToggleFps(); diff --git a/src/BizHawk.Client.EmuHawk/MainForm.cs b/src/BizHawk.Client.EmuHawk/MainForm.cs index 60109fa59cc..414bdea3ba8 100644 --- a/src/BizHawk.Client.EmuHawk/MainForm.cs +++ b/src/BizHawk.Client.EmuHawk/MainForm.cs @@ -1715,6 +1715,7 @@ public bool RunLibretroCoreChooser() // countdown for saveram autoflushing public int AutoFlushSaveRamIn { get; set; } + private bool AutoFlushSaveRamFailed; private void SetStatusBar() { @@ -1938,7 +1939,7 @@ private void LoadSaveRam() } } - public bool FlushSaveRAM(bool autosave = false) + public FileWriteResult FlushSaveRAM(bool autosave = false) { if (Emulator.HasSaveRam()) { @@ -1952,51 +1953,11 @@ public bool FlushSaveRAM(bool autosave = false) path = Config.PathEntries.SaveRamAbsolutePath(Game, MovieSession.Movie); } - var file = new FileInfo(path); - var newPath = $"{path}.new"; - var newFile = new FileInfo(newPath); - var backupPath = $"{path}.bak"; - var backupFile = new FileInfo(backupPath); - var saveram = Emulator.AsSaveRam().CloneSaveRam()!; - - try - { - Directory.CreateDirectory(file.DirectoryName!); - using (var fs = File.Create(newPath)) - { - fs.Write(saveram, 0, saveram.Length); - fs.Flush(flushToDisk: true); - } - - if (file.Exists) - { - if (Config.BackupSaveram) - { - if (backupFile.Exists) - { - backupFile.Delete(); - } - - file.MoveTo(backupPath); - } - else - { - file.Delete(); - } - } - - newFile.MoveTo(path); - } - catch (IOException e) - { - AddOnScreenMessage("Failed to flush saveram!"); - Console.Error.WriteLine(e); - return false; - } + return FileWriter.Write(path, saveram, $"{path}.bak"); } - return true; + return new(); } private void RewireSound() @@ -3056,8 +3017,22 @@ private void StepRunLoop_Core(bool force = false) AutoFlushSaveRamIn--; if (AutoFlushSaveRamIn <= 0) { - FlushSaveRAM(true); - AutoFlushSaveRamIn = Config.FlushSaveRamFrames; + FileWriteResult result = FlushSaveRAM(true); + if (result.IsError) + { + // For autosave, allow one failure before bothering the user. + if (AutoFlushSaveRamFailed) + { + this.ErrorMessageBox(result, "Failed to flush saveram!"); + } + AutoFlushSaveRamFailed = true; + AutoFlushSaveRamIn = Math.Min(600, Config.FlushSaveRamFrames); + } + else + { + AutoFlushSaveRamFailed = false; + AutoFlushSaveRamIn = Config.FlushSaveRamFrames; + } } } // why not skip audio if the user doesn't want sound @@ -3968,41 +3943,34 @@ private bool CloseGame(bool clearSram = false) var path = Config.PathEntries.SaveRamAbsolutePath(Game, MovieSession.Movie); if (File.Exists(path)) { - File.Delete(path); - AddOnScreenMessage("SRAM cleared."); + bool clearResult = this.DoWithTryAgainBox(() => { + try + { + File.Delete(path); + AddOnScreenMessage("SRAM cleared."); + return new(); + } + catch (Exception ex) + { + return new(FileWriteEnum.FailedToDeleteGeneric, new(path, ""), ex); + } + }, "Failed to clear SRAM."); + if (!clearResult) + { + return false; + } } } else if (Emulator.HasSaveRam()) { - while (true) - { - if (FlushSaveRAM()) break; - - var result = ShowMessageBox3( - owner: this, - "Failed flushing the game's Save RAM to your disk.\n" + - "Do you want to try again?", - "IOError while writing SaveRAM", - EMsgBoxIcon.Error); - - if (result is false) break; - if (result is null) return false; - } + bool flushResult = this.DoWithTryAgainBox( + () => FlushSaveRAM(), + "Failed flushing the game's Save RAM to your disk."); + if (!flushResult) return false; } - bool? tryAgain = null; - do - { - FileWriteResult stateSaveResult = AutoSaveStateIfConfigured(); - if (stateSaveResult.IsError) - { - tryAgain = this.ShowMessageBox3( - $"Failed to auto-save state. Do you want to try again?\n\nError details:\n{stateSaveResult.UserFriendlyErrorMessage()}\n{stateSaveResult.Exception.Message}", - "IOError while writing savestate", - EMsgBoxIcon.Error); - if (tryAgain == null) return false; - } - } while (tryAgain == true); + bool stateSaveResult = this.DoWithTryAgainBox(AutoSaveStateIfConfigured, "Failed to auto-save state."); + if (!stateSaveResult) return false; StopAv(); @@ -4286,11 +4254,7 @@ public FileWriteResult SaveQuickSave(int slot, bool suppressOSD = false) /// private void SaveQuickSaveAndShowError(int slot) { - FileWriteResult result = SaveQuickSave(slot); - if (result.IsError) - { - this.ErrorMessageBox(result, "Quick save failed."); - } + ShowMessageIfError(() => SaveQuickSave(slot), "Quick save failed."); } public bool EnsureCoreIsAccurate() @@ -4360,11 +4324,9 @@ private void SaveStateAs() initFileName: $"{SaveStatePrefix()}.QuickSave0.State"); if (shouldSaveResult is not null) { - FileWriteResult saveResult = SaveState(path: shouldSaveResult, userFriendlyStateName: shouldSaveResult); - if (saveResult.IsError) - { - this.ErrorMessageBox(saveResult, "Unable to save."); - } + ShowMessageIfError( + () => SaveState(path: shouldSaveResult, userFriendlyStateName: shouldSaveResult), + "Unable to save state."); } if (Tools.IsLoaded()) @@ -4655,6 +4617,15 @@ public bool ShowMessageBox2( _ => null, }; + public void ShowMessageIfError(Func action, string message) + { + FileWriteResult result = action(); + if (result.IsError) + { + this.ErrorMessageBox(result, message); + } + } + public void StartSound() => Sound.StartSound(); public void StopSound() => Sound.StopSound(); From 6186da21358a3845aa8c257ed0182a6e9300ca12 Mon Sep 17 00:00:00 2001 From: SuuperW Date: Thu, 4 Sep 2025 13:56:20 -0500 Subject: [PATCH 08/11] Fix: CheatList was getting forcibly autosaved, after the method that properly checks the autosave config option. --- src/BizHawk.Client.Common/tools/CheatList.cs | 12 +----------- src/BizHawk.Client.EmuHawk/MainForm.cs | 8 ++++---- 2 files changed, 5 insertions(+), 15 deletions(-) diff --git a/src/BizHawk.Client.Common/tools/CheatList.cs b/src/BizHawk.Client.Common/tools/CheatList.cs index a9cca688a9a..3f1fcd8b438 100644 --- a/src/BizHawk.Client.Common/tools/CheatList.cs +++ b/src/BizHawk.Client.Common/tools/CheatList.cs @@ -88,20 +88,10 @@ public bool AttemptToLoadCheatFile(IMemoryDomains domains) return file.Exists && Load(domains, file.FullName, false); } - public void NewList(string defaultFileName, bool autosave = false) + public void NewList(string defaultFileName) { _defaultFileName = defaultFileName; - if (autosave && _changes && _cheatList.Count is not 0) - { - if (string.IsNullOrEmpty(CurrentFileName)) - { - CurrentFileName = _defaultFileName; - } - - Save(); - } - _cheatList.Clear(); CurrentFileName = ""; Changes = false; diff --git a/src/BizHawk.Client.EmuHawk/MainForm.cs b/src/BizHawk.Client.EmuHawk/MainForm.cs index 414bdea3ba8..24f3c01a3be 100644 --- a/src/BizHawk.Client.EmuHawk/MainForm.cs +++ b/src/BizHawk.Client.EmuHawk/MainForm.cs @@ -3803,7 +3803,7 @@ private bool LoadRomInternal(string path, LoadRomArgs args, out bool failureIsFr if (previousRom != CurrentlyOpenRom) { - CheatList.NewList(Tools.GenerateDefaultCheatFilename(), autosave: true); + CheatList.NewList(Tools.GenerateDefaultCheatFilename()); if (Config.Cheats.LoadFileByGame && Emulator.HasMemoryDomains()) { if (CheatList.AttemptToLoadCheatFile(Emulator.AsMemoryDomains())) @@ -3820,7 +3820,7 @@ private bool LoadRomInternal(string path, LoadRomArgs args, out bool failureIsFr } else { - CheatList.NewList(Tools.GenerateDefaultCheatFilename(), autosave: true); + CheatList.NewList(Tools.GenerateDefaultCheatFilename()); } } @@ -3857,7 +3857,7 @@ private bool LoadRomInternal(string path, LoadRomArgs args, out bool failureIsFr DisplayManager.UpdateGlobals(Config, Emulator); DisplayManager.Blank(); ExtToolManager.BuildToolStrip(); - CheatList.NewList("", autosave: true); + CheatList.NewList(""); OnRomChanged(); return false; } @@ -4017,7 +4017,7 @@ public void LoadNullRom(bool clearSram = false) PauseOnFrame = null; CurrentlyOpenRom = null; CurrentlyOpenRomArgs = null; - CheatList.NewList("", autosave: true); + CheatList.NewList(""); OnRomChanged(); } } From eb67b087faad0d6b9801ce078076f0166981f380 Mon Sep 17 00:00:00 2001 From: SuuperW Date: Mon, 21 Jul 2025 02:48:17 -0500 Subject: [PATCH 09/11] Handle file writing errors for movies and all tools. --- .../DialogControllerExtensions.cs | 15 ++- src/BizHawk.Client.Common/FileWriter.cs | 17 ++++ src/BizHawk.Client.Common/lua/LuaFileList.cs | 18 +++- .../movie/MovieSession.cs | 2 + src/BizHawk.Client.Common/tools/CheatList.cs | 95 ++++++++++--------- .../tools/Watch/WatchList/WatchList.cs | 39 ++++---- src/BizHawk.Client.EmuHawk/MainForm.cs | 31 +++--- src/BizHawk.Client.EmuHawk/tools/CDL.cs | 38 +++++--- .../tools/Cheats/Cheats.cs | 19 +++- .../tools/Lua/LuaConsole.cs | 48 +++++++--- .../tools/Macros/MacroInput.cs | 1 + .../tools/TAStudio/TAStudio.IToolForm.cs | 11 +-- .../tools/Watch/RamSearch.cs | 35 +++++-- .../tools/Watch/RamWatch.cs | 70 +++++++++----- 14 files changed, 283 insertions(+), 156 deletions(-) diff --git a/src/BizHawk.Client.Common/DialogControllerExtensions.cs b/src/BizHawk.Client.Common/DialogControllerExtensions.cs index 9c83be78dc6..aa607e9507d 100644 --- a/src/BizHawk.Client.Common/DialogControllerExtensions.cs +++ b/src/BizHawk.Client.Common/DialogControllerExtensions.cs @@ -5,6 +5,13 @@ namespace BizHawk.Client.Common { + public enum TryAgainResult + { + Saved, + IgnoredFailure, + Canceled, + } + public static class DialogControllerExtensions { public static void AddOnScreenMessage(this IDialogParent dialogParent, string message, int? duration = null) @@ -80,7 +87,7 @@ public static void ErrorMessageBox( /// The user will be repeatedly asked if they want to try again until either success or the user says no. /// /// Returns true on success or if the user said no. Returns false if the user said cancel. - public static bool DoWithTryAgainBox( + public static TryAgainResult DoWithTryAgainBox( this IDialogParent dialogParent, Func action, string message) @@ -94,12 +101,12 @@ public static bool DoWithTryAgainBox( $"{fileResult.UserFriendlyErrorMessage()}\n{fileResult.Exception!.Message}", caption: "Error", icon: EMsgBoxIcon.Error); - if (askResult == null) return false; - if (askResult == false) return true; + if (askResult == null) return TryAgainResult.Canceled; + if (askResult == false) return TryAgainResult.IgnoredFailure; if (askResult == true) fileResult = action(); } - return true; + return TryAgainResult.Saved; } /// Creates and shows a System.Windows.Forms.OpenFileDialog or equivalent with the receiver () as its parent diff --git a/src/BizHawk.Client.Common/FileWriter.cs b/src/BizHawk.Client.Common/FileWriter.cs index 8a0a83c22d5..227ebb01681 100644 --- a/src/BizHawk.Client.Common/FileWriter.cs +++ b/src/BizHawk.Client.Common/FileWriter.cs @@ -56,6 +56,22 @@ public static FileWriteResult Write(string path, byte[] bytes, string? backupPat return createResult.Value.CloseAndDispose(backupPath); } + public static FileWriteResult Write(string path, Action writeCallback, string? backupPath = null) + { + FileWriteResult createResult = Create(path); + if (createResult.IsError) return createResult; + + try + { + writeCallback(createResult.Value!.Stream); + } + catch (Exception ex) + { + return new(FileWriteEnum.FailedDuringWrite, createResult.Value!.Paths, ex); + } + + return createResult.Value.CloseAndDispose(backupPath); + } /// /// Create a FileWriter instance, or return an error if unable to access the file. @@ -76,6 +92,7 @@ public static FileWriteResult Create(string path) FileWritePaths paths = new(path, writePath); try { + Directory.CreateDirectory(Path.GetDirectoryName(path)); FileStream fs = new(writePath, FileMode.Create, FileAccess.Write); return new(new FileWriter(paths, fs), paths); } diff --git a/src/BizHawk.Client.Common/lua/LuaFileList.cs b/src/BizHawk.Client.Common/lua/LuaFileList.cs index 13784bf6f8e..15b2493c0a0 100644 --- a/src/BizHawk.Client.Common/lua/LuaFileList.cs +++ b/src/BizHawk.Client.Common/lua/LuaFileList.cs @@ -102,9 +102,8 @@ public bool Load(string path, bool disableOnLoad) return true; } - public void Save(string path) + public FileWriteResult Save(string path) { - using var sw = new StreamWriter(path); var sb = new StringBuilder(); var saveDirectory = Path.GetDirectoryName(Path.GetFullPath(path)); foreach (var file in this) @@ -123,10 +122,19 @@ public void Save(string path) } } - sw.Write(sb.ToString()); + FileWriteResult result = FileWriter.Write(path, (fs) => + { + using var sw = new StreamWriter(fs); + sw.Write(sb.ToString()); + }); - Filename = path; - Changes = false; + if (!result.IsError) + { + Filename = path; + Changes = false; + } + + return result; } } } diff --git a/src/BizHawk.Client.Common/movie/MovieSession.cs b/src/BizHawk.Client.Common/movie/MovieSession.cs index d63501bf4a9..52055e4ce9b 100644 --- a/src/BizHawk.Client.Common/movie/MovieSession.cs +++ b/src/BizHawk.Client.Common/movie/MovieSession.cs @@ -385,6 +385,8 @@ private void HandlePlaybackEnd() switch (Settings.MovieEndAction) { case MovieEndAction.Stop: + // Technically this can save the movie, but it'd be weird to be in that situation. + // Do we want that? StopMovie(); break; case MovieEndAction.Record: diff --git a/src/BizHawk.Client.Common/tools/CheatList.cs b/src/BizHawk.Client.Common/tools/CheatList.cs index 3f1fcd8b438..5de61df7a35 100644 --- a/src/BizHawk.Client.Common/tools/CheatList.cs +++ b/src/BizHawk.Client.Common/tools/CheatList.cs @@ -210,7 +210,7 @@ public void DisableAll() public bool IsActive(MemoryDomain domain, long address) => _cheatList.Exists(cheat => !cheat.IsSeparator && cheat.Enabled && cheat.Domain == domain && cheat.Contains(address)); - public void SaveOnClose() + public FileWriteResult SaveOnClose() { if (_config.AutoSaveOnClose) { @@ -221,17 +221,27 @@ public void SaveOnClose() CurrentFileName = _defaultFileName; } - SaveFile(CurrentFileName); + return SaveFile(CurrentFileName); } else if (_cheatList.Count is 0 && !string.IsNullOrWhiteSpace(CurrentFileName)) { - File.Delete(CurrentFileName); + try + { + File.Delete(CurrentFileName); + } + catch (Exception ex) + { + return new(FileWriteEnum.FailedToDeleteGeneric, new(CurrentFileName, ""), ex); + } _config.Recent.Remove(CurrentFileName); + return new(); } } + + return new(); } - public bool Save() + public FileWriteResult Save() { if (string.IsNullOrWhiteSpace(CurrentFileName)) { @@ -241,54 +251,51 @@ public bool Save() return SaveFile(CurrentFileName); } - public bool SaveFile(string path) + public FileWriteResult SaveFile(string path) { - try - { - new FileInfo(path).Directory?.Create(); - var sb = new StringBuilder(); + var sb = new StringBuilder(); - foreach (var cheat in _cheatList) + foreach (var cheat in _cheatList) + { + if (cheat.IsSeparator) { - if (cheat.IsSeparator) - { - sb.AppendLine("----"); - } - else - { - // Set to hex for saving - var tempCheatType = cheat.Type; - - cheat.SetType(WatchDisplayType.Hex); - - sb - .Append(cheat.AddressStr).Append('\t') - .Append(cheat.ValueStr).Append('\t') - .Append(cheat.Compare is null ? "N" : cheat.CompareStr).Append('\t') - .Append(cheat.Domain != null ? cheat.Domain.Name : "").Append('\t') - .Append(cheat.Enabled ? '1' : '0').Append('\t') - .Append(cheat.Name).Append('\t') - .Append(cheat.SizeAsChar).Append('\t') - .Append(cheat.TypeAsChar).Append('\t') - .Append(cheat.BigEndian is true ? '1' : '0').Append('\t') - .Append(cheat.ComparisonType).Append('\t') - .AppendLine(); - - cheat.SetType(tempCheatType); - } + sb.AppendLine("----"); } - - File.WriteAllText(path, sb.ToString()); - + else + { + // Set to hex for saving + var tempCheatType = cheat.Type; + + cheat.SetType(WatchDisplayType.Hex); + + sb + .Append(cheat.AddressStr).Append('\t') + .Append(cheat.ValueStr).Append('\t') + .Append(cheat.Compare is null ? "N" : cheat.CompareStr).Append('\t') + .Append(cheat.Domain != null ? cheat.Domain.Name : "").Append('\t') + .Append(cheat.Enabled ? '1' : '0').Append('\t') + .Append(cheat.Name).Append('\t') + .Append(cheat.SizeAsChar).Append('\t') + .Append(cheat.TypeAsChar).Append('\t') + .Append(cheat.BigEndian is true ? '1' : '0').Append('\t') + .Append(cheat.ComparisonType).Append('\t') + .AppendLine(); + + cheat.SetType(tempCheatType); + } + } + FileWriteResult result = FileWriter.Write(path, (fs) => + { + StreamWriter sw = new(fs); + sw.Write(sb.ToString()); + }); + if (!result.IsError) + { CurrentFileName = path; _config.Recent.Add(CurrentFileName); Changes = false; - return true; - } - catch - { - return false; } + return result; } public bool Load(IMemoryDomains domains, string path, bool append) diff --git a/src/BizHawk.Client.Common/tools/Watch/WatchList/WatchList.cs b/src/BizHawk.Client.Common/tools/Watch/WatchList/WatchList.cs index 0a3e12d913c..eb5afeb1ded 100644 --- a/src/BizHawk.Client.Common/tools/Watch/WatchList/WatchList.cs +++ b/src/BizHawk.Client.Common/tools/Watch/WatchList/WatchList.cs @@ -1,5 +1,6 @@ using System.Collections; using System.Collections.Generic; +using System.Diagnostics; using System.Globalization; using System.IO; using System.Linq; @@ -338,39 +339,37 @@ public void Reload() } } - public bool Save() + public FileWriteResult Save() { if (string.IsNullOrWhiteSpace(CurrentFileName)) { - return false; + return new(); } - using (var sw = new StreamWriter(CurrentFileName)) - { - var sb = new StringBuilder(); - sb.Append("SystemID ").AppendLine(_systemId); + var sb = new StringBuilder(); + sb.Append("SystemID ").AppendLine(_systemId); - foreach (var watch in _watchList) - { - sb.AppendLine(watch.ToString()); - } + foreach (var watch in _watchList) + { + sb.AppendLine(watch.ToString()); + } + FileWriteResult result = FileWriter.Write(CurrentFileName, (fs) => + { + using var sw = new StreamWriter(fs); sw.WriteLine(sb.ToString()); - } + }); - Changes = false; - return true; + if (!result.IsError) Changes = false; + return result; } - public bool SaveAs(FileInfo file) + public FileWriteResult SaveAs(FileInfo file) { - if (file != null) - { - CurrentFileName = file.FullName; - return Save(); - } + Debug.Assert(file != null, "Cannot save as without a file name."); - return false; + CurrentFileName = file.FullName; + return Save(); } private bool LoadFile(string path, bool append) diff --git a/src/BizHawk.Client.EmuHawk/MainForm.cs b/src/BizHawk.Client.EmuHawk/MainForm.cs index 24f3c01a3be..b78285f738c 100644 --- a/src/BizHawk.Client.EmuHawk/MainForm.cs +++ b/src/BizHawk.Client.EmuHawk/MainForm.cs @@ -3924,18 +3924,17 @@ private bool CloseGame(bool clearSram = false) return false; } // There is a cheats tool, but cheats can be active while the "cheats tool" is not. And have auto-save option. - CheatList.SaveOnClose(); + TryAgainResult cheatSaveResult = this.DoWithTryAgainBox(CheatList.SaveOnClose, "Failed to save cheats."); + if (cheatSaveResult == TryAgainResult.Canceled) + { + return false; + } - FileWriteResult saveResult = MovieSession.StopMovie(); - if (saveResult.IsError) + // If TAStudio is open, we already asked about saving the movie. + if (!Tools.IsLoaded()) { - if (!this.ModalMessageBox2( - caption: "Quit anyway?", - icon: EMsgBoxIcon.Question, - text: "The currently playing movie could not be saved. Continue and quit anyway? All unsaved changes will be lost.")) - { - return false; - } + TryAgainResult saveMovieResult = this.DoWithTryAgainBox(() => MovieSession.StopMovie(), "Failed to save movie."); + if (saveMovieResult == TryAgainResult.Canceled) return false; } if (clearSram) @@ -3943,7 +3942,7 @@ private bool CloseGame(bool clearSram = false) var path = Config.PathEntries.SaveRamAbsolutePath(Game, MovieSession.Movie); if (File.Exists(path)) { - bool clearResult = this.DoWithTryAgainBox(() => { + TryAgainResult clearResult = this.DoWithTryAgainBox(() => { try { File.Delete(path); @@ -3955,7 +3954,7 @@ private bool CloseGame(bool clearSram = false) return new(FileWriteEnum.FailedToDeleteGeneric, new(path, ""), ex); } }, "Failed to clear SRAM."); - if (!clearResult) + if (clearResult == TryAgainResult.Canceled) { return false; } @@ -3963,14 +3962,14 @@ private bool CloseGame(bool clearSram = false) } else if (Emulator.HasSaveRam()) { - bool flushResult = this.DoWithTryAgainBox( + TryAgainResult flushResult = this.DoWithTryAgainBox( () => FlushSaveRAM(), "Failed flushing the game's Save RAM to your disk."); - if (!flushResult) return false; + if (flushResult == TryAgainResult.Canceled) return false; } - bool stateSaveResult = this.DoWithTryAgainBox(AutoSaveStateIfConfigured, "Failed to auto-save state."); - if (!stateSaveResult) return false; + TryAgainResult stateSaveResult = this.DoWithTryAgainBox(AutoSaveStateIfConfigured, "Failed to auto-save state."); + if (stateSaveResult == TryAgainResult.Canceled) return false; StopAv(); diff --git a/src/BizHawk.Client.EmuHawk/tools/CDL.cs b/src/BizHawk.Client.EmuHawk/tools/CDL.cs index 5c12ce5572e..317a1041285 100644 --- a/src/BizHawk.Client.EmuHawk/tools/CDL.cs +++ b/src/BizHawk.Client.EmuHawk/tools/CDL.cs @@ -214,19 +214,23 @@ public override bool AskSaveChanges() { if (_currentFilename != null) { - RunSave(); + bool saveResult2 = RunSave(); ShutdownCDL(); - return true; + return saveResult2; } } - // TODO - I don't like this system. It's hard to figure out how to use it. It should be done in multiple passes. - var result = DialogController.ShowMessageBox2("Save changes to CDL session?", "CDL Auto Save", EMsgBoxIcon.Question); - if (!result) + var result = DialogController.ShowMessageBox3("Save changes to CDL session?", "CDL Save", EMsgBoxIcon.Question); + if (result == false) { ShutdownCDL(); return true; } + else if (result == null) + { + ShutdownCDL(); + return false; + } if (string.IsNullOrWhiteSpace(_currentFilename)) { @@ -240,9 +244,9 @@ public override bool AskSaveChanges() return false; } - RunSave(); + bool saveResult = RunSave(); ShutdownCDL(); - return true; + return saveResult; } private bool _autoloading; @@ -341,11 +345,20 @@ private void OpenMenuItem_Click(object sender, EventArgs e) LoadFile(file.FullName); } - private void RunSave() + /// + /// returns false if the operation was canceled + /// + private bool RunSave() { - _recent.Add(_currentFilename); - using var fs = new FileStream(_currentFilename, FileMode.Create, FileAccess.Write); - _cdl.Save(fs); + TryAgainResult result = this.DoWithTryAgainBox( + () => FileWriter.Write(_currentFilename, _cdl.Save), + "Failed to save CDL session."); + if (result == TryAgainResult.Saved) + { + _recent.Add(_currentFilename); + return true; + } + return result != TryAgainResult.Canceled; } private void SaveMenuItem_Click(object sender, EventArgs e) @@ -386,8 +399,7 @@ private bool RunSaveAs() return false; SetCurrentFilename(file.FullName); - RunSave(); - return true; + return RunSave(); } private void SaveAsMenuItem_Click(object sender, EventArgs e) diff --git a/src/BizHawk.Client.EmuHawk/tools/Cheats/Cheats.cs b/src/BizHawk.Client.EmuHawk/tools/Cheats/Cheats.cs index 3c8447ea799..0addc928b85 100644 --- a/src/BizHawk.Client.EmuHawk/tools/Cheats/Cheats.cs +++ b/src/BizHawk.Client.EmuHawk/tools/Cheats/Cheats.cs @@ -142,7 +142,7 @@ private void LoadFile(FileSystemInfo file, bool append) } } - private bool SaveAs() + private FileWriteResult SaveAs() { var fileName = MainForm.CheatList.CurrentFileName; if (string.IsNullOrWhiteSpace(fileName)) @@ -156,7 +156,8 @@ private bool SaveAs() CheatsFSFilterSet, this); - return file != null && MainForm.CheatList.SaveFile(file.FullName); + if (file == null) return new(); + else return MainForm.CheatList.SaveFile(file.FullName); } private void Cheats_Load(object sender, EventArgs e) @@ -361,7 +362,12 @@ private void SaveMenuItem_Click(object sender, EventArgs e) { if (MainForm.CheatList.Changes) { - if (MainForm.CheatList.Save()) + FileWriteResult result = MainForm.CheatList.Save(); + if (result.IsError) + { + this.ErrorMessageBox(result); + } + else { UpdateMessageLabel(saved: true); } @@ -374,7 +380,12 @@ private void SaveMenuItem_Click(object sender, EventArgs e) private void SaveAsMenuItem_Click(object sender, EventArgs e) { - if (SaveAs()) + FileWriteResult result = SaveAs(); + if (result.IsError) + { + this.ErrorMessageBox(result); + } + else { UpdateMessageLabel(saved: true); } diff --git a/src/BizHawk.Client.EmuHawk/tools/Lua/LuaConsole.cs b/src/BizHawk.Client.EmuHawk/tools/Lua/LuaConsole.cs index 25892fd6061..baf1e723e58 100644 --- a/src/BizHawk.Client.EmuHawk/tools/Lua/LuaConsole.cs +++ b/src/BizHawk.Client.EmuHawk/tools/Lua/LuaConsole.cs @@ -680,15 +680,25 @@ private FileInfo GetSaveFileFromUser() return result is not null ? new FileInfo(result) : null; } - private void SaveSessionAs() + private FileWriteResult SaveSessionAs() { var file = GetSaveFileFromUser(); if (file != null) { - LuaImp.ScriptList.Save(file.FullName); - Config.RecentLuaSession.Add(file.FullName); - OutputMessages.Text = $"{file.Name} saved."; + FileWriteResult saveResult = LuaImp.ScriptList.Save(file.FullName); + if (saveResult.IsError) + { + this.ErrorMessageBox(saveResult); + OutputMessages.Text = $"Lua session could not be saved to {file.Name}"; + } + else + { + Config.RecentLuaSession.Add(file.FullName); + OutputMessages.Text = $"{file.Name} saved."; + } + return saveResult; } + return new(); } private void LoadSessionFromRecent(string path) @@ -716,7 +726,11 @@ public override bool AskSaveChanges() icon: EMsgBoxIcon.Question, text: $"Save {WindowTitleStatic} session?")); if (result is null) return false; - if (result.Value) SaveOrSaveAs(); + if (result.Value) + { + TryAgainResult saveResult = this.DoWithTryAgainBox(SaveOrSaveAs, "Failed to save Lua session."); + return saveResult != TryAgainResult.Canceled; + } else LuaImp.ScriptList.Changes = false; return true; } @@ -731,16 +745,20 @@ private void UpdateRegisteredFunctionsDialog() } } - private void SaveOrSaveAs() + private FileWriteResult SaveOrSaveAs() { if (!string.IsNullOrWhiteSpace(LuaImp.ScriptList.Filename)) { - LuaImp.ScriptList.Save(LuaImp.ScriptList.Filename); - Config.RecentLuaSession.Add(LuaImp.ScriptList.Filename); + FileWriteResult result = LuaImp.ScriptList.Save(LuaImp.ScriptList.Filename); + if (!result.IsError) + { + Config.RecentLuaSession.Add(LuaImp.ScriptList.Filename); + } + return result; } else { - SaveSessionAs(); + return SaveSessionAs(); } } @@ -783,8 +801,16 @@ private void SaveSessionMenuItem_Click(object sender, EventArgs e) { if (LuaImp.ScriptList.Changes) { - SaveOrSaveAs(); - OutputMessages.Text = $"{Path.GetFileName(LuaImp.ScriptList.Filename)} saved."; + FileWriteResult result = SaveOrSaveAs(); + if (result.IsError) + { + this.ErrorMessageBox(result, "Failed to save Lua session."); + OutputMessages.Text = $"Failed to save {Path.GetFileName(LuaImp.ScriptList.Filename)}"; + } + else + { + OutputMessages.Text = $"{Path.GetFileName(LuaImp.ScriptList.Filename)} saved."; + } } } diff --git a/src/BizHawk.Client.EmuHawk/tools/Macros/MacroInput.cs b/src/BizHawk.Client.EmuHawk/tools/Macros/MacroInput.cs index dc865572e01..a3758c05cea 100644 --- a/src/BizHawk.Client.EmuHawk/tools/Macros/MacroInput.cs +++ b/src/BizHawk.Client.EmuHawk/tools/Macros/MacroInput.cs @@ -112,6 +112,7 @@ public override bool AskSaveChanges() return true; } + // Intentionally not updating this to use FileWriter because this tool is going to be removed later. foreach (var zone in _unsavedZones) { SaveMacroAs(_zones[zone]); diff --git a/src/BizHawk.Client.EmuHawk/tools/TAStudio/TAStudio.IToolForm.cs b/src/BizHawk.Client.EmuHawk/tools/TAStudio/TAStudio.IToolForm.cs index 2c5a9ae2bfc..e880f0bda7b 100644 --- a/src/BizHawk.Client.EmuHawk/tools/TAStudio/TAStudio.IToolForm.cs +++ b/src/BizHawk.Client.EmuHawk/tools/TAStudio/TAStudio.IToolForm.cs @@ -176,15 +176,8 @@ public override bool AskSaveChanges() text: $"Save {WindowTitleStatic} project?")); if (shouldSaveResult == true) { - FileWriteResult saveResult = SaveTas(); - while (saveResult.IsError && shouldSaveResult != true) - { - shouldSaveResult = this.ModalMessageBox3( - $"Failed to save movie. {saveResult.UserFriendlyErrorMessage()}\n{saveResult.Exception.Message}\n\nTry again?", - "Error", - EMsgBoxIcon.Error); - if (shouldSaveResult == true) saveResult = SaveTas(); - } + TryAgainResult saveResult = this.DoWithTryAgainBox(() => SaveTas(), "Failed to save movie."); + return saveResult != TryAgainResult.Canceled; } if (shouldSaveResult is null) return false; else CurrentTasMovie.ClearChanges(); diff --git a/src/BizHawk.Client.EmuHawk/tools/Watch/RamSearch.cs b/src/BizHawk.Client.EmuHawk/tools/Watch/RamSearch.cs index a7371f5716f..020ff8916ef 100644 --- a/src/BizHawk.Client.EmuHawk/tools/Watch/RamSearch.cs +++ b/src/BizHawk.Client.EmuHawk/tools/Watch/RamSearch.cs @@ -1007,7 +1007,13 @@ private void SaveMenuItem_Click(object sender, EventArgs e) if (!string.IsNullOrWhiteSpace(watches.CurrentFileName)) { - if (watches.Save()) + FileWriteResult saveResult = watches.Save(); + if (saveResult.IsError) + { + this.ErrorMessageBox(saveResult); + MessageLabel.Text = $"Failed to save {Path.GetFileName(_currentFileName)}"; + } + else { _currentFileName = watches.CurrentFileName; MessageLabel.Text = $"{Path.GetFileName(_currentFileName)} saved"; @@ -1016,11 +1022,20 @@ private void SaveMenuItem_Click(object sender, EventArgs e) } else { - var result = watches.SaveAs(GetWatchSaveFileFromUser(CurrentFileName())); - if (result) + FileInfo/*?*/ file = GetWatchSaveFileFromUser(CurrentFileName()); + if (file != null) { - MessageLabel.Text = $"{Path.GetFileName(_currentFileName)} saved"; - Settings.RecentSearches.Add(watches.CurrentFileName); + var result = watches.SaveAs(file); + if (result.IsError) + { + this.ErrorMessageBox(result); + MessageLabel.Text = $"Failed to save {Path.GetFileName(_currentFileName)}"; + } + else + { + MessageLabel.Text = $"{Path.GetFileName(_currentFileName)} saved"; + Settings.RecentSearches.Add(watches.CurrentFileName); + } } } } @@ -1034,7 +1049,15 @@ private void SaveAsMenuItem_Click(object sender, EventArgs e) watches.Add(_searches[i]); } - if (watches.SaveAs(GetWatchSaveFileFromUser(CurrentFileName()))) + FileInfo/*?*/ file = GetWatchSaveFileFromUser(CurrentFileName()); + if (file == null) return; + FileWriteResult result = watches.SaveAs(file); + if (result.IsError) + { + this.ErrorMessageBox(result); + MessageLabel.Text = $"Failed to save {Path.GetFileName(_currentFileName)}"; + } + else { _currentFileName = watches.CurrentFileName; MessageLabel.Text = $"{Path.GetFileName(_currentFileName)} saved"; diff --git a/src/BizHawk.Client.EmuHawk/tools/Watch/RamWatch.cs b/src/BizHawk.Client.EmuHawk/tools/Watch/RamWatch.cs index e54bf3a62ff..f74609e0ae6 100644 --- a/src/BizHawk.Client.EmuHawk/tools/Watch/RamWatch.cs +++ b/src/BizHawk.Client.EmuHawk/tools/Watch/RamWatch.cs @@ -213,15 +213,8 @@ public override bool AskSaveChanges() if (result is null) return false; if (result.Value) { - if (string.IsNullOrWhiteSpace(_watches.CurrentFileName)) - { - SaveAs(); - } - else - { - _watches.Save(); - Config.RecentWatches.Add(_watches.CurrentFileName); - } + TryAgainResult saveResult = this.DoWithTryAgainBox(Save, "Failed to save watch list."); + return saveResult != TryAgainResult.Canceled; } else { @@ -591,13 +584,45 @@ private string CurrentFileName() : Game.FilesystemSafeName(); } - private void SaveAs() + private FileWriteResult SaveAs() { - var result = _watches.SaveAs(GetWatchSaveFileFromUser(CurrentFileName())); - if (result) + FileInfo/*?*/ file = GetWatchSaveFileFromUser(CurrentFileName()); + if (file == null) return new(); + + FileWriteResult result = _watches.SaveAs(file); + if (result.IsError) { - UpdateStatusBar(saved: true); + MessageLabel.Text = $"Failed to save {Path.GetFileName(_watches.CurrentFileName)}"; + } + else + { + MessageLabel.Text = $"{Path.GetFileName(_watches.CurrentFileName)} saved"; Config.RecentWatches.Add(_watches.CurrentFileName); + UpdateStatusBar(saved: true); + } + return result; + } + + private FileWriteResult Save() + { + if (string.IsNullOrWhiteSpace(_watches.CurrentFileName)) + { + return SaveAs(); + } + else + { + FileWriteResult saveResult = _watches.Save(); + if (saveResult.IsError) + { + MessageLabel.Text = $"Failed to save {Path.GetFileName(_watches.CurrentFileName)}"; + } + else + { + MessageLabel.Text = $"{Path.GetFileName(_watches.CurrentFileName)} saved"; + Config.RecentWatches.Add(_watches.CurrentFileName); + UpdateStatusBar(saved: true); + } + return saveResult; } } @@ -727,23 +752,20 @@ private void OpenMenuItem_Click(object sender, EventArgs e) private void SaveMenuItem_Click(object sender, EventArgs e) { - if (!string.IsNullOrWhiteSpace(_watches.CurrentFileName)) + FileWriteResult saveResult = Save(); + if (saveResult.IsError) { - if (_watches.Save()) - { - Config.RecentWatches.Add(_watches.CurrentFileName); - UpdateStatusBar(saved: true); - } - } - else - { - SaveAs(); + this.ErrorMessageBox(saveResult, "Failed to save watch list."); } } private void SaveAsMenuItem_Click(object sender, EventArgs e) { - SaveAs(); + FileWriteResult saveResult = SaveAs(); + if (saveResult.IsError) + { + this.ErrorMessageBox(saveResult, "Failed to save watch list."); + } } private void RecentSubMenu_DropDownOpened(object sender, EventArgs e) From e383cc41a91e08cc0a47d1fcf69fc8c413d340f4 Mon Sep 17 00:00:00 2001 From: SuuperW Date: Tue, 22 Jul 2025 02:59:57 -0500 Subject: [PATCH 10/11] Handle errors when saving config file. --- .../config/ConfigService.cs | 13 ++++------- src/BizHawk.Client.EmuHawk/MainForm.Events.cs | 22 +++++++++++++++---- src/BizHawk.Client.EmuHawk/MainForm.cs | 16 ++++++++------ .../config/ControllerConfig.cs | 6 ++++- 4 files changed, 36 insertions(+), 21 deletions(-) diff --git a/src/BizHawk.Client.Common/config/ConfigService.cs b/src/BizHawk.Client.Common/config/ConfigService.cs index 70286ddedb4..357d02ecc49 100644 --- a/src/BizHawk.Client.Common/config/ConfigService.cs +++ b/src/BizHawk.Client.Common/config/ConfigService.cs @@ -104,19 +104,14 @@ public static bool IsFromSameVersion(string filepath, out string msg) return config ?? new T(); } - public static void Save(string filepath, object config) + public static FileWriteResult Save(string filepath, object config) { - var file = new FileInfo(filepath); - try + return FileWriter.Write(filepath, (fs) => { - using var writer = file.CreateText(); + using var writer = new StreamWriter(fs); var w = new JsonTextWriter(writer) { Formatting = Formatting.Indented }; Serializer.Serialize(w, config); - } - catch - { - /* Eat it */ - } + }); } // movie 1.0 header stuff diff --git a/src/BizHawk.Client.EmuHawk/MainForm.Events.cs b/src/BizHawk.Client.EmuHawk/MainForm.Events.cs index a611b23d40e..63804cc917a 100644 --- a/src/BizHawk.Client.EmuHawk/MainForm.Events.cs +++ b/src/BizHawk.Client.EmuHawk/MainForm.Events.cs @@ -979,8 +979,15 @@ private void HkOverInputMenuItem_Click(object sender, EventArgs e) private void SaveConfigMenuItem_Click(object sender, EventArgs e) { - SaveConfig(); - AddOnScreenMessage("Saved settings"); + FileWriteResult result = SaveConfig(); + if (result.IsError) + { + this.ErrorMessageBox(result); + } + else + { + AddOnScreenMessage("Saved settings"); + } } private void SaveConfigAsMenuItem_Click(object sender, EventArgs e) @@ -992,8 +999,15 @@ private void SaveConfigAsMenuItem_Click(object sender, EventArgs e) initFileName: file); if (result is not null) { - SaveConfig(result); - AddOnScreenMessage("Copied settings"); + FileWriteResult saveResult = SaveConfig(result); + if (saveResult.IsError) + { + this.ErrorMessageBox(saveResult); + } + else + { + AddOnScreenMessage("Copied settings"); + } } } diff --git a/src/BizHawk.Client.EmuHawk/MainForm.cs b/src/BizHawk.Client.EmuHawk/MainForm.cs index b78285f738c..d84ef0e13ea 100644 --- a/src/BizHawk.Client.EmuHawk/MainForm.cs +++ b/src/BizHawk.Client.EmuHawk/MainForm.cs @@ -871,7 +871,12 @@ private void CheckMayCloseAndCleanup(object/*?*/ closingSender, CancelEventArgs StopAv(); } - SaveConfig(); // TODO: Handle failure. + TryAgainResult configSaveResult = this.DoWithTryAgainBox(() => SaveConfig(), "Failed to save config file."); + if (configSaveResult == TryAgainResult.Canceled) + { + closingArgs.Cancel = true; + return; + } if (!CloseGame()) { @@ -2399,7 +2404,7 @@ public ISettingsAdapter GetSettingsAdapterForLoadedCore() public SettingsAdapter GetSettingsAdapterForLoadedCoreUntyped() => new(Emulator, static () => true, HandlePutCoreSettings, MayPutCoreSyncSettings, HandlePutCoreSyncSettings); - private void SaveConfig(string path = "") + private FileWriteResult SaveConfig(string path = "") { if (Config.SaveWindowPosition) { @@ -2425,7 +2430,7 @@ private void SaveConfig(string path = "") } CommitCoreSettingsToConfig(); - ConfigService.Save(path, Config); + return ConfigService.Save(path, Config); } private void ToggleFps() @@ -3925,10 +3930,7 @@ private bool CloseGame(bool clearSram = false) } // There is a cheats tool, but cheats can be active while the "cheats tool" is not. And have auto-save option. TryAgainResult cheatSaveResult = this.DoWithTryAgainBox(CheatList.SaveOnClose, "Failed to save cheats."); - if (cheatSaveResult == TryAgainResult.Canceled) - { - return false; - } + if (cheatSaveResult == TryAgainResult.Canceled) return false; // If TAStudio is open, we already asked about saving the movie. if (!Tools.IsLoaded()) diff --git a/src/BizHawk.Client.EmuHawk/config/ControllerConfig.cs b/src/BizHawk.Client.EmuHawk/config/ControllerConfig.cs index 7d59bb50994..2954d6530f4 100644 --- a/src/BizHawk.Client.EmuHawk/config/ControllerConfig.cs +++ b/src/BizHawk.Client.EmuHawk/config/ControllerConfig.cs @@ -470,7 +470,11 @@ private void ButtonSaveDefaults_Click(object sender, EventArgs e) SaveToDefaults(cd); - ConfigService.Save(Config.ControlDefaultPath, cd); + FileWriteResult saveResult = ConfigService.Save(Config.ControlDefaultPath, cd); + if (saveResult.IsError) + { + this.ErrorMessageBox(saveResult); + } } } From 377ebcb9105526f709e673b8c2dbe756572189a1 Mon Sep 17 00:00:00 2001 From: SuuperW Date: Thu, 4 Sep 2025 03:26:35 -0500 Subject: [PATCH 11/11] Handle file errors when saving a macro. --- .../tools/Macros/MacroInput.cs | 2 +- .../tools/Macros/MovieZone.cs | 24 ++++++++++++------- .../tools/TAStudio/TAStudio.MenuItems.cs | 18 ++++++++++---- 3 files changed, 30 insertions(+), 14 deletions(-) diff --git a/src/BizHawk.Client.EmuHawk/tools/Macros/MacroInput.cs b/src/BizHawk.Client.EmuHawk/tools/Macros/MacroInput.cs index a3758c05cea..46ffd8d15e9 100644 --- a/src/BizHawk.Client.EmuHawk/tools/Macros/MacroInput.cs +++ b/src/BizHawk.Client.EmuHawk/tools/Macros/MacroInput.cs @@ -289,7 +289,7 @@ private bool SaveMacroAs(MovieZone macro) return false; } - macro.Save(result); + macro.Save(result); // ignore errors: This tool is going to be removed. Config!.RecentMacros.Add(result); return true; diff --git a/src/BizHawk.Client.EmuHawk/tools/Macros/MovieZone.cs b/src/BizHawk.Client.EmuHawk/tools/Macros/MovieZone.cs index 208899b3c49..e6f32f24416 100644 --- a/src/BizHawk.Client.EmuHawk/tools/Macros/MovieZone.cs +++ b/src/BizHawk.Client.EmuHawk/tools/Macros/MovieZone.cs @@ -199,19 +199,25 @@ public void PlaceZone(IMovie movie, Config config) } } - public void Save(string fileName) + public FileWriteResult Save(string fileName) { // Save the controller definition/LogKey // Save the controller name and player count. (Only for the user.) // Save whether or not the macro should use overlay input, and/or replace - string[] header = new string[4]; - header[0] = InputKey; - header[1] = _emulator.ControllerDefinition.Name; - header[2] = _emulator.ControllerDefinition.PlayerCount.ToString(); - header[3] = $"{Overlay},{Replace}"; - - File.WriteAllLines(fileName, header); - File.AppendAllLines(fileName, _log); + + return FileWriter.Write(fileName, (fs) => + { + using var writer = new StreamWriter(fs); + writer.WriteLine(InputKey); + writer.WriteLine(_emulator.ControllerDefinition.Name); + writer.WriteLine(_emulator.ControllerDefinition.PlayerCount.ToString()); + writer.WriteLine($"{Overlay},{Replace}"); + + foreach (string line in _log) + { + writer.WriteLine(line); + } + }); } public MovieZone(string fileName, IDialogController dialogController, IEmulator emulator, IMovieSession movieSession, ToolManager tools) diff --git a/src/BizHawk.Client.EmuHawk/tools/TAStudio/TAStudio.MenuItems.cs b/src/BizHawk.Client.EmuHawk/tools/TAStudio/TAStudio.MenuItems.cs index a07acced78f..83c853ffe4c 100644 --- a/src/BizHawk.Client.EmuHawk/tools/TAStudio/TAStudio.MenuItems.cs +++ b/src/BizHawk.Client.EmuHawk/tools/TAStudio/TAStudio.MenuItems.cs @@ -172,15 +172,25 @@ private void SaveSelectionToMacroMenuItem_Click(object sender, EventArgs e) if (file != null) { var selectionStart = TasView.SelectionStartIndex!.Value; - new MovieZone( + MovieZone macro = new( Emulator, Tools, MovieSession, start: selectionStart, - length: TasView.SelectionEndIndex!.Value - selectionStart + 1) - .Save(file.FullName); + length: TasView.SelectionEndIndex!.Value - selectionStart + 1); + FileWriteResult saveResult = macro.Save(file.FullName); - Config.RecentMacros.Add(file.FullName); + if (saveResult.IsError) + { + DialogController.ShowMessageBox( + $"Failed to save macro.\n{saveResult.UserFriendlyErrorMessage()}\n{saveResult.Exception.Message}", + "Error", + EMsgBoxIcon.Error); + } + else + { + Config.RecentMacros.Add(file.FullName); + } } }