From ca9d41f527ec9e6bfe8adb3d743428e09b3d8d79 Mon Sep 17 00:00:00 2001 From: Georges-Antoine Assi Date: Mon, 21 Sep 2026 20:16:05 -0400 Subject: [PATCH 1/4] Resolve saves from the platform mapping, not only the play action Save sync read the emulator solely from the game's Playnite play action. That action is a snapshot of the emulator mapping written once at import and never refreshed: the importer skips games that already exist, so a mapping later repointed at another emulator left every game it had imported still launching -- and resolving saves against -- the old one. Sync then gave up with a single message that named neither the emulator it looked at nor what to do about it (#138). The play action still leads, because a user who repoints it should have their saves follow it, but when it names an emulator no handler covers, the platform's own mapping -- which the settings screen presents as the thing that decides this -- now gets its turn before sync gives up. The mapping comes from the ROM sidecar, which every import rewrites, so it tracks the user's edits where the action does not. A play action can also name an emulator without naming a profile, and for RetroArch the profile identifies the core, which is a folder in the save path under sort_savefiles_enable. The profile is now taken from another candidate for the same emulator rather than resolving a path with the core folder missing, where a download would land somewhere RetroArch never reads. Stale actions are repaired rather than only worked around: an import repoints an existing game's action at its mapping's current emulator, but only while the action is still the one the plugin wrote. The sidecar records what was last applied; actions from before that record are recognised by the generated "Play in " name. Where two enabled mappings share a platform, neither touches the actions, so the two cannot fight over every game. The one catch-all failure message becomes three -- no emulator set, an unsupported one named, or a supported one whose save path could not be worked out -- and the sidecar's path, read and write move into RomMGameData, replacing the four hand-rolled copies in RomM.cs. Co-Authored-By: Claude Opus 5 (1M context) --- Games/RomMGameData.cs | 57 ++++++++++ Games/RomMImport.cs | 115 ++++++++++++++----- Games/RomMPlayAction.cs | 105 +++++++++++++++++ IRomm.cs | 7 ++ Models/RomM/Rom/RomMRomLocal.cs | 7 ++ RomM.Tests/RomM.Tests.csproj | 2 + RomM.Tests/RomMPlayActionTests.cs | 129 +++++++++++++++++++++ RomM.Tests/SaveEmulatorResolverTests.cs | 145 ++++++++++++++++++++++++ RomM.cs | 77 +++++-------- Saves/SaveEmulatorResolver.cs | 104 +++++++++++++++++ Saves/SaveSyncService.cs | 100 +++++++++++----- 11 files changed, 740 insertions(+), 108 deletions(-) create mode 100644 Games/RomMGameData.cs create mode 100644 Games/RomMPlayAction.cs create mode 100644 RomM.Tests/RomMPlayActionTests.cs create mode 100644 RomM.Tests/SaveEmulatorResolverTests.cs create mode 100644 Saves/SaveEmulatorResolver.cs diff --git a/Games/RomMGameData.cs b/Games/RomMGameData.cs new file mode 100644 index 0000000..59a4e2f --- /dev/null +++ b/Games/RomMGameData.cs @@ -0,0 +1,57 @@ +using Newtonsoft.Json; +using Playnite.SDK; +using RomM.Models.RomM.Rom; +using System; +using System.IO; + +namespace RomM.Games +{ + /// + /// The per-ROM sidecar ("<sha1>.json" under the plugin's data folder): the download + /// descriptors for every revision, the emulator mapping the ROM was imported under, and what + /// the importer last wrote onto the game's play action. + /// + /// Install, uninstall, the version menu and save sync all need it, and each used to carry its + /// own copy of the path building, the id parsing and the try/catch around a corrupt file. + /// + internal static class RomMGameData + { + public static string PathFor(string romDataPath, string sha1) => + Path.Combine(romDataPath ?? string.Empty, $"{sha1}.json"); + + /// + /// The sidecar for a game id, or null when the id is malformed, the file is missing or its + /// contents cannot be read. What a missing sidecar means is the caller's business. + /// + public static RomMRomLocal Load(string romDataPath, string gameId, ILogger logger, string gameName = null) + { + if (!RomMGameId.TryParse(gameId, out int _, out string sha1)) + { + logger?.Error($"{gameName ?? gameId} GameID is malformed!"); + return null; + } + + return LoadBySha1(romDataPath, sha1, logger, gameName); + } + + public static RomMRomLocal LoadBySha1(string romDataPath, string sha1, ILogger logger, string gameName = null) + { + var path = PathFor(romDataPath, sha1); + if (!File.Exists(path)) + return null; + + try + { + return JsonConvert.DeserializeObject(File.ReadAllText(path)); + } + catch (Exception ex) + { + logger?.Error(ex, $"{gameName ?? sha1} ROM data file is corrupted!"); + return null; + } + } + + public static void Save(string romDataPath, string sha1, RomMRomLocal data) => + File.WriteAllText(PathFor(romDataPath, sha1), JsonConvert.SerializeObject(data)); + } +} diff --git a/Games/RomMImport.cs b/Games/RomMImport.cs index 9db0800..c42d282 100644 --- a/Games/RomMImport.cs +++ b/Games/RomMImport.cs @@ -25,6 +25,10 @@ internal class RomMImport Dictionary _legacyGames; // unmigrated "!0..." games, keyed by GameId Dictionary _completionStatusMap; List _favourites; + // Two enabled mappings on one platform both walk the same ROMs, and each pass would repoint + // the play actions the other just wrote. Neither is more right than the other, so when that + // is the setup the actions are left exactly as they are. + bool _platformHasRivalMapping; public RomMImport(RomM plugin, LibraryImportGamesArgs args, EmulatorMapping mapping, List roms, List favourites) { @@ -61,6 +65,9 @@ public RomMImport(RomM plugin, LibraryImportGamesArgs args, EmulatorMapping mapp _completionStatusMap = plugin.Playnite.Database.CompletionStatuses.ToDictionary(cs => cs.Name, cs => cs.Id); _favourites = favourites; + + _platformHasRivalMapping = plugin.Settings?.Mappings? + .Count(m => m.Enabled && m.RomMPlatformId == mapping.RomMPlatformId) > 1; } // Builds the per-ROM download descriptor via the shared factory (see RomMRevisionFactory). @@ -125,8 +132,10 @@ public List ProcessData() } } - // Save Game ROM data to file - SaveGameData(ROM); + // What the sidecar held before this import overwrites it: the record of the + // play action the plugin last wrote, which decides whether that action is + // still ours to repoint at the mapping's current emulator. + var previous = RomMGameData.LoadBySha1(_plugin.ROMDataPath, ROM.SHA1, _plugin.Logger, ROM.Name); // Skip full import if ROM has already been imported Guid statusID = Guid.Empty; @@ -145,6 +154,8 @@ public List ProcessData() _plugin.Playnite.Database.Games.Update(existingGame); } + // Save Game ROM data to file + SaveGameData(ROM, previous, RefreshPlayAction(existingGame, previous)); importedGameIds.Add(gameID); continue; } @@ -153,10 +164,17 @@ public List ProcessData() // server under a new romMId, update the existing playnite entry instead. if (_plugin.Settings.KeepDeletedGames && UpdatedDeletedGame(ROM)) { + // The adopted entry is an existing game re-keyed under the new id, so its + // action is refreshed on the same terms as any other existing game's. + _existingGames.TryGetValue(gameID, out var adoptedGame); + SaveGameData(ROM, previous, RefreshPlayAction(adoptedGame, previous)); importedGameIds.Add(gameID); continue; } + // A new game gets the mapping's emulator outright, below. + SaveGameData(ROM, previous, new AppliedPlayAction(_mapping.EmulatorId, _mapping.EmulatorProfileId)); + var importedGame = ImportGame(ROM, statusID); if (importedGame != null) { @@ -229,14 +247,7 @@ private Game ImportGame(RomMRom ROM, Guid StatusID) metadata.InstallSize = ROM.FileSizeBytes; metadata.GameActions = new List { - new GameAction - { - Name = $"Play in {_mapping.Emulator.Name}", - Type = GameActionType.Emulator, - EmulatorId = _mapping.EmulatorId, - EmulatorProfileId = _mapping.EmulatorProfileId, - IsPlayAction = true, - }, + RomMPlayAction.Build(_mapping.Emulator.Name, _mapping.EmulatorId, _mapping.EmulatorProfileId), new GameAction { Type = GameActionType.URL, @@ -367,13 +378,15 @@ private bool UpdatedDeletedGame(RomMRom ROM) private MainSibling CheckForMainSibling(RomMRom ROM) => RomMSiblings.ClassifyMain(ROM, _romById); - private void SaveGameData(RomMRom ROM) + private void SaveGameData(RomMRom ROM, RomMRomLocal previous, AppliedPlayAction applied) { RomMRomLocal toSave = new RomMRomLocal { Name = ROM.Name, SHA1 = ROM.SHA1, MappingID = _mapping.MappingId, + AppliedEmulatorID = applied.EmulatorId, + AppliedEmulatorProfileID = applied.ProfileId, ROMVersions = new List() }; @@ -407,30 +420,70 @@ private void SaveGameData(RomMRom ROM) } // Carry over the user's previously selected version and only rewrite when something changed. - string sidecarPath = $"{_plugin.ROMDataPath}{ROM.SHA1}.json"; - string existingJson = null; - if (File.Exists(sidecarPath)) + foreach (var revision in previous?.ROMVersions ?? new List()) { - try - { - existingJson = File.ReadAllText(sidecarPath); - var localROM = JsonConvert.DeserializeObject(existingJson); - foreach (var revision in localROM?.ROMVersions ?? new List()) - { - var matchedRevision = toSave.ROMVersions.FirstOrDefault(x => x.Id == revision.Id); - if (matchedRevision != null) - matchedRevision.IsSelected = revision.IsSelected; - } - } - catch (Exception) - { - _plugin.Logger.Error($"{ROM.Name} GameID is malformed or {ROM.SHA1} json file is corrupted!"); - } + var matchedRevision = toSave.ROMVersions.FirstOrDefault(x => x.Id == revision.Id); + if (matchedRevision != null) + matchedRevision.IsSelected = revision.IsSelected; } string json = JsonConvert.SerializeObject(toSave); - if (json != existingJson) - File.WriteAllText(sidecarPath, json); + if (previous == null || json != JsonConvert.SerializeObject(previous)) + RomMGameData.Save(_plugin.ROMDataPath, ROM.SHA1, toSave); + } + + /// + /// Brings an already-imported game's play action back in step with its mapping, and reports + /// the action the sidecar should now record. + /// + /// Existing games are otherwise skipped wholesale by the importer, so a mapping repointed + /// at another emulator left every game it had imported launching -- and resolving its saves + /// against -- the old one. Only an action the plugin still owns is rewritten; once the user + /// has repointed it themselves it is theirs, and the sidecar keeps remembering what we last + /// wrote so that stays true across imports. + /// + private AppliedPlayAction RefreshPlayAction(Game game, RomMRomLocal previous) + { + var mapped = new AppliedPlayAction(_mapping.EmulatorId, _mapping.EmulatorProfileId); + var applied = previous != null + ? new AppliedPlayAction(previous.AppliedEmulatorID, previous.AppliedEmulatorProfileID) + : AppliedPlayAction.Unknown; + + // No action to keep in step -- and no game at all, on the adoption path where the + // re-keyed entry could not be found again. + var action = RomMPlayAction.Find(game?.GameActions); + if (action == null || _platformHasRivalMapping) + return applied; + + if (RomMPlayAction.Matches(action, mapped.EmulatorId, mapped.ProfileId)) + return mapped; + + var actionEmulatorName = _plugin.Playnite.Database.Emulators? + .FirstOrDefault(e => e.Id == action.EmulatorId)?.Name; + + if (!RomMPlayAction.IsUnedited(action, applied.EmulatorId, applied.ProfileId, actionEmulatorName)) + { + _plugin.Logger.Info($"[Importer] Leaving {game.Name}'s play action pointed at " + + $"{actionEmulatorName ?? ""}: it is no longer the one the plugin wrote."); + return applied; + } + + if (_mapping.Emulator == null) + return applied; + + action.Name = RomMPlayAction.NameFor(_mapping.Emulator.Name); + action.Type = GameActionType.Emulator; + action.EmulatorId = mapped.EmulatorId; + action.EmulatorProfileId = mapped.ProfileId; + action.IsPlayAction = true; + + // Our own write; OnItemUpdated has nothing to push to RomM for it. + _plugin.SuppressSync(game.Id); + _plugin.Playnite.Database.Games.Update(game); + + _plugin.Logger.Info($"[Importer] Repointed {game.Name}'s play action at {_mapping.Emulator.Name} " + + $"to follow the {_mapping.MappingName} mapping."); + return mapped; } private Guid DetermineCompletionStatus(RomMRom ROM) diff --git a/Games/RomMPlayAction.cs b/Games/RomMPlayAction.cs new file mode 100644 index 0000000..4be1f10 --- /dev/null +++ b/Games/RomMPlayAction.cs @@ -0,0 +1,105 @@ +using Playnite.SDK.Models; +using System; +using System.Collections.Generic; +using System.Linq; + +namespace RomM.Games +{ + /// + /// The emulator and profile the importer last wrote onto a game's play action, as recorded in + /// the ROM's sidecar. is what a sidecar written before the plugin kept + /// this record yields. + /// + internal struct AppliedPlayAction + { + public static readonly AppliedPlayAction Unknown = new AppliedPlayAction(Guid.Empty, null); + + public readonly Guid EmulatorId; + public readonly string ProfileId; + + public AppliedPlayAction(Guid emulatorId, string profileId) + { + EmulatorId = emulatorId; + ProfileId = profileId; + } + } + + /// + /// The emulator play action the importer writes onto every imported game, and the rules for + /// deciding whether a later import may repoint it. + /// + /// The action is a snapshot of the emulator mapping taken at import time. Re-imports skip games + /// that already exist, so a mapping later pointed at another emulator used to leave every game + /// it had imported launching -- and syncing saves against -- the emulator it no longer uses. Refreshing the action closes that gap, but only while the action + /// is still the one the plugin wrote: a user who repoints it themselves keeps their choice. + /// + internal static class RomMPlayAction + { + public static string NameFor(string emulatorName) => $"Play in {emulatorName}"; + + public static GameAction Build(string emulatorName, Guid emulatorId, string emulatorProfileId) + { + return new GameAction + { + Name = NameFor(emulatorName), + Type = GameActionType.Emulator, + EmulatorId = emulatorId, + EmulatorProfileId = emulatorProfileId, + IsPlayAction = true, + }; + } + + /// + /// The emulator action Playnite launches the game with. The play action comes first; a game + /// whose emulator action is not marked as one still tells us which emulator it runs on. + /// + public static GameAction Find(IEnumerable actions) + { + if (actions == null) + return null; + + var list = actions as IList ?? actions.ToList(); + return list.FirstOrDefault(a => a != null && a.IsPlayAction && a.Type == GameActionType.Emulator) + ?? list.FirstOrDefault(a => a != null && a.Type == GameActionType.Emulator); + } + + /// Whether the action already launches the given emulator and profile. + public static bool Matches(GameAction action, Guid emulatorId, string emulatorProfileId) + { + return action != null + && action.EmulatorId == emulatorId + && SameProfile(action.EmulatorProfileId, emulatorProfileId); + } + + /// + /// Whether the action is still the plugin's to repoint. + /// + /// / are what the + /// plugin last wrote, recorded in the ROM's sidecar; if the action still carries them, + /// nobody has touched it. Sidecars written before the plugin recorded that (every install + /// that predates this) carry no applied emulator, and then the generated name is the only + /// marker left: an action still called "Play in <the emulator it points at>" is one + /// the importer wrote and the user has not renamed or repointed. + /// + public static bool IsUnedited(GameAction action, Guid appliedEmulatorId, string appliedProfileId, string actionEmulatorName) + { + if (action == null) + return false; + + if (appliedEmulatorId != Guid.Empty) + return Matches(action, appliedEmulatorId, appliedProfileId); + + return !string.IsNullOrEmpty(actionEmulatorName) + && string.Equals(action.Name, NameFor(actionEmulatorName), StringComparison.Ordinal); + } + + // Playnite writes an unset profile as either null or "", and the two mean the same thing. + private static bool SameProfile(string left, string right) + { + if (string.IsNullOrEmpty(left) && string.IsNullOrEmpty(right)) + return true; + + return string.Equals(left, right, StringComparison.Ordinal); + } + } +} diff --git a/IRomm.cs b/IRomm.cs index 4b4f4af..c760986 100644 --- a/IRomm.cs +++ b/IRomm.cs @@ -17,5 +17,12 @@ internal interface IRomM string GetPluginUserDataPath(); RomMRom FetchRom(string romId); + /// + /// The emulator mapping a game was imported under, from its ROM sidecar, or null when the + /// sidecar or the mapping is gone. Unlike the game's play action this follows the mapping + /// as the user edits it, because every import rewrites the sidecar. + /// + Settings.EmulatorMapping MappingFor(Game game); + } } \ No newline at end of file diff --git a/Models/RomM/Rom/RomMRomLocal.cs b/Models/RomM/Rom/RomMRomLocal.cs index 95c393d..b275fd3 100644 --- a/Models/RomM/Rom/RomMRomLocal.cs +++ b/Models/RomM/Rom/RomMRomLocal.cs @@ -32,6 +32,13 @@ public class RomMRomLocal public string SHA1 { get; set; } public Guid MappingID { get; set; } + // The emulator and profile the importer last wrote onto the game's play action. An action + // that still carries them has not been touched since, so a later import may repoint it at + // the mapping's current emulator; one that differs is the user's own choice and is left + // alone. Empty on sidecars written before the plugin recorded this. + public Guid AppliedEmulatorID { get; set; } + public string AppliedEmulatorProfileID { get; set; } + public List ROMVersions { get; set; } } diff --git a/RomM.Tests/RomM.Tests.csproj b/RomM.Tests/RomM.Tests.csproj index 15717a1..c65994a 100644 --- a/RomM.Tests/RomM.Tests.csproj +++ b/RomM.Tests/RomM.Tests.csproj @@ -53,6 +53,8 @@ + + diff --git a/RomM.Tests/RomMPlayActionTests.cs b/RomM.Tests/RomMPlayActionTests.cs new file mode 100644 index 0000000..ca9daba --- /dev/null +++ b/RomM.Tests/RomMPlayActionTests.cs @@ -0,0 +1,129 @@ +using Playnite.SDK.Models; +using RomM.Games; +using System; +using System.Collections.Generic; +using Xunit; + +namespace RomM.Tests +{ + public class RomMPlayActionTests + { + private static readonly Guid Mapped = Guid.NewGuid(); + private static readonly Guid Other = Guid.NewGuid(); + + [Fact] + public void Build_produces_the_importers_play_action() + { + var action = RomMPlayAction.Build("RetroArch", Mapped, "profile-1"); + + Assert.Equal("Play in RetroArch", action.Name); + Assert.Equal(GameActionType.Emulator, action.Type); + Assert.Equal(Mapped, action.EmulatorId); + Assert.Equal("profile-1", action.EmulatorProfileId); + Assert.True(action.IsPlayAction); + } + + [Fact] + public void Find_prefers_the_emulator_action_marked_as_the_play_action() + { + var play = new GameAction { Type = GameActionType.Emulator, IsPlayAction = true, Name = "play" }; + + var found = RomMPlayAction.Find(new List + { + new GameAction { Type = GameActionType.URL, Name = "View in RomM" }, + new GameAction { Type = GameActionType.Emulator, Name = "secondary" }, + play, + }); + + Assert.Same(play, found); + } + + // An emulator action that isn't flagged as the play action still tells us which emulator + // the game runs on, which is all save sync needs. + [Fact] + public void Find_falls_back_to_any_emulator_action() + { + var action = new GameAction { Type = GameActionType.Emulator, Name = "secondary" }; + + Assert.Same(action, RomMPlayAction.Find(new[] + { + new GameAction { Type = GameActionType.URL, IsPlayAction = true }, + action, + })); + } + + [Fact] + public void Find_returns_null_without_an_emulator_action() + { + Assert.Null(RomMPlayAction.Find(new[] { new GameAction { Type = GameActionType.URL } })); + Assert.Null(RomMPlayAction.Find(null)); + } + + // Playnite writes an unset profile as null or "" depending on where it came from. + [Theory] + [InlineData(null, "")] + [InlineData("", null)] + [InlineData("p", "p")] + public void Matches_treats_an_unset_profile_the_same_either_way(string actionProfile, string mappedProfile) + { + var action = new GameAction { EmulatorId = Mapped, EmulatorProfileId = actionProfile }; + + Assert.True(RomMPlayAction.Matches(action, Mapped, mappedProfile)); + } + + [Fact] + public void Matches_is_false_for_another_emulator_or_profile() + { + var action = new GameAction { EmulatorId = Mapped, EmulatorProfileId = "p" }; + + Assert.False(RomMPlayAction.Matches(action, Other, "p")); + Assert.False(RomMPlayAction.Matches(action, Mapped, "q")); + Assert.False(RomMPlayAction.Matches(null, Mapped, "p")); + } + + [Fact] + public void An_action_still_carrying_what_the_plugin_applied_is_unedited() + { + var action = new GameAction { Name = "anything", EmulatorId = Mapped, EmulatorProfileId = "p" }; + + Assert.True(RomMPlayAction.IsUnedited(action, Mapped, "p", "Dolphin")); + } + + // Once the user has repointed the action, the recorded applied emulator no longer matches + // and the action is theirs -- a later mapping change must not take it back. + [Fact] + public void An_action_repointed_by_the_user_is_not_unedited() + { + var action = new GameAction { Name = "Play in Dolphin", EmulatorId = Other, EmulatorProfileId = "q" }; + + Assert.False(RomMPlayAction.IsUnedited(action, Mapped, "p", "Dolphin")); + } + + // Sidecars from before the plugin recorded what it applied: the generated name is the only + // marker that the importer wrote the action and nobody has touched it since. + [Fact] + public void Without_a_recorded_emulator_the_generated_name_marks_the_action_as_the_plugins() + { + var action = new GameAction { Name = "Play in Dolphin", EmulatorId = Other }; + + Assert.True(RomMPlayAction.IsUnedited(action, Guid.Empty, null, "Dolphin")); + } + + [Theory] + [InlineData("Play with Dolphin", "Dolphin")] + [InlineData("Play in Dolphin", "RetroArch")] + [InlineData("Play in Dolphin", null)] + public void Without_a_recorded_emulator_anything_but_the_generated_name_is_left_alone(string name, string emulatorName) + { + var action = new GameAction { Name = name, EmulatorId = Other }; + + Assert.False(RomMPlayAction.IsUnedited(action, Guid.Empty, null, emulatorName)); + } + + [Fact] + public void A_missing_action_is_never_unedited() + { + Assert.False(RomMPlayAction.IsUnedited(null, Mapped, "p", "RetroArch")); + } + } +} diff --git a/RomM.Tests/SaveEmulatorResolverTests.cs b/RomM.Tests/SaveEmulatorResolverTests.cs new file mode 100644 index 0000000..ba18674 --- /dev/null +++ b/RomM.Tests/SaveEmulatorResolverTests.cs @@ -0,0 +1,145 @@ +using Playnite.SDK.Models; +using RomM.Saves; +using RomM.Saves.Handlers; +using System; +using System.Collections.Generic; +using Xunit; + +namespace RomM.Tests +{ + public class SaveEmulatorResolverTests + { + private static readonly SaveHandlerRegistry Handlers = new SaveHandlerRegistry(); + + private static Emulator RetroArch(Guid? id = null) => + new Emulator { Id = id ?? Guid.NewGuid(), Name = "RetroArch" }; + + private static Emulator Dolphin() => + new Emulator { Id = Guid.NewGuid(), Name = "Dolphin" }; + + private static SaveEmulatorCandidate Candidate(SaveEmulatorSource source, Emulator emulator, EmulatorProfile profile = null) => + new SaveEmulatorCandidate { Source = source, Emulator = emulator, Profile = profile }; + + // The action a user repointed at another supported emulator is their choice, and outranks + // whatever the platform mapping says. + [Fact] + public void The_play_action_wins_when_its_emulator_is_supported() + { + var fromAction = RetroArch(); + + var resolution = SaveEmulatorResolver.Resolve(Handlers, new[] + { + Candidate(SaveEmulatorSource.PlayAction, fromAction), + Candidate(SaveEmulatorSource.Mapping, RetroArch()), + }); + + Assert.Same(fromAction, resolution.Emulator); + Assert.Equal(SaveEmulatorSource.PlayAction, resolution.Source); + Assert.Equal(SaveEmulatorProblem.None, resolution.Problem); + Assert.NotNull(resolution.Handler); + } + + // The reported bug: the action is a snapshot from import, so a mapping later repointed at + // RetroArch has to be consulted rather than the sync giving up on the stale emulator. + [Fact] + public void The_mapping_is_used_when_the_play_actions_emulator_is_unsupported() + { + var fromMapping = RetroArch(); + + var resolution = SaveEmulatorResolver.Resolve(Handlers, new[] + { + Candidate(SaveEmulatorSource.PlayAction, Dolphin()), + Candidate(SaveEmulatorSource.Mapping, fromMapping), + }); + + Assert.Same(fromMapping, resolution.Emulator); + Assert.Equal(SaveEmulatorSource.Mapping, resolution.Source); + Assert.Equal(SaveEmulatorProblem.None, resolution.Problem); + } + + // Reported as unsupported rather than as "no emulator": the two need different advice, and + // the emulator named is the one the user would go looking for. + [Fact] + public void No_supported_candidate_reports_the_first_emulator_as_unsupported() + { + var resolution = SaveEmulatorResolver.Resolve(Handlers, new[] + { + Candidate(SaveEmulatorSource.PlayAction, Dolphin()), + Candidate(SaveEmulatorSource.Mapping, new Emulator { Id = Guid.NewGuid(), Name = "PCSX2" }), + }); + + Assert.Equal(SaveEmulatorProblem.Unsupported, resolution.Problem); + Assert.Equal("Dolphin", resolution.UnsupportedEmulatorName); + Assert.Null(resolution.Handler); + } + + [Fact] + public void Candidates_without_an_emulator_are_reported_as_none_set() + { + var resolution = SaveEmulatorResolver.Resolve(Handlers, new[] + { + Candidate(SaveEmulatorSource.PlayAction, null), + Candidate(SaveEmulatorSource.Mapping, null), + null, + }); + + Assert.Equal(SaveEmulatorProblem.NoEmulator, resolution.Problem); + Assert.Null(resolution.Emulator); + } + + [Fact] + public void No_candidates_at_all_are_reported_as_none_set() + { + Assert.Equal(SaveEmulatorProblem.NoEmulator, + SaveEmulatorResolver.Resolve(Handlers, null).Problem); + } + + // An action can name an emulator without naming a profile. For RetroArch the profile is the + // core, and the core is a folder in the save path, so the mapping's profile for that same + // emulator is better than none. + [Fact] + public void A_profile_is_borrowed_from_another_candidate_for_the_same_emulator() + { + var id = Guid.NewGuid(); + var profile = new BuiltInEmulatorProfile { Name = "mGBA" }; + + var resolution = SaveEmulatorResolver.Resolve(Handlers, new[] + { + Candidate(SaveEmulatorSource.PlayAction, RetroArch(id)), + Candidate(SaveEmulatorSource.Mapping, RetroArch(id), profile), + }); + + Assert.Equal(SaveEmulatorSource.PlayAction, resolution.Source); + Assert.Same(profile, resolution.Profile); + } + + // A profile belongs to the emulator it was defined on; borrowing across emulators would + // name a core that emulator never runs. + [Fact] + public void A_profile_is_not_borrowed_from_a_different_emulator() + { + var resolution = SaveEmulatorResolver.Resolve(Handlers, new[] + { + Candidate(SaveEmulatorSource.PlayAction, RetroArch()), + Candidate(SaveEmulatorSource.Mapping, RetroArch(), new BuiltInEmulatorProfile { Name = "mGBA" }), + }); + + Assert.Null(resolution.Profile); + } + + [Fact] + public void The_candidates_own_profile_is_kept() + { + var id = Guid.NewGuid(); + var own = new BuiltInEmulatorProfile { Name = "Dolphin - GC/Wii" }; + + var resolution = SaveEmulatorResolver.Resolve(Handlers, new List + { + Candidate(SaveEmulatorSource.PlayAction, RetroArch(id), own), + Candidate(SaveEmulatorSource.Mapping, RetroArch(id), new BuiltInEmulatorProfile { Name = "mGBA" }), + }); + + Assert.Same(own, resolution.Profile); + } + } +} diff --git a/RomM.cs b/RomM.cs index dccfd54..9e2570a 100644 --- a/RomM.cs +++ b/RomM.cs @@ -116,6 +116,19 @@ public RomM(IPlayniteAPI api) : base(api) #region Helper functions public string CombineUrl(string baseUrl, string relativePath) => RomMUrl.Combine(baseUrl, relativePath); + /// The ROM sidecar for a game, or null when it is missing or unreadable. + internal RomMRomLocal LoadGameData(Game game) => + RomMGameData.Load(ROMDataPath, game?.GameId, Logger, game?.Name); + + public EmulatorMapping MappingFor(Game game) + { + var gameData = LoadGameData(game); + if (gameData == null) + return null; + + return Settings?.Mappings?.FirstOrDefault(x => x.MappingId == gameData.MappingID); + } + public RomMRom FetchRom(string romId) { string romUrl = CombineUrl(Settings.RomMHost, $"api/roms/{romId}"); @@ -253,7 +266,7 @@ public override void OnApplicationStarted(OnApplicationStartedEventArgs args) { if (RomMGameId.TryParse(item.GameId, out int _, out var sha1)) { - var romDataFile = $"{ROMDataPath}{sha1}.json"; + var romDataFile = RomMGameData.PathFor(ROMDataPath, sha1); if (File.Exists(romDataFile)) { File.Delete(romDataFile); @@ -355,30 +368,21 @@ public override IEnumerable GetGameMenuItems(GetGameMenuItemsArgs }); } - string romDataFile = $"{ROMDataPath}{sha1}.json"; - if (Settings.MergeRevisions && File.Exists(romDataFile) && game.IsInstalled) + if (Settings.MergeRevisions && game.IsInstalled) { - try + var gameData = LoadGameData(game); + if (gameData?.ROMVersions?.Count > 1) { - string json = File.ReadAllText(romDataFile); - var gameData = JsonConvert.DeserializeObject(json); - if(gameData.ROMVersions.Count > 1) + gameMenuItems.Add(new GameMenuItem { - gameMenuItems.Add(new GameMenuItem + //MenuSection = "@", + Description = "Switch ROM Version!", + Action = (gameMenuItem) => { - //MenuSection = "@", - Description = "Switch ROM Version!", - Action = (gameMenuItem) => - { - Playnite.InstallGame(args.Games.First().Id); - } - }); - } + Playnite.InstallGame(args.Games.First().Id); + } + }); } - catch (Exception) - { - Logger.Error($"{args.Games.First().Name} GameID is malformed or json file is corrupted!"); - } } } return gameMenuItems; @@ -402,7 +406,7 @@ public override IEnumerable GetInstallActions(GetInstallActio else { // Pull game file from RomM data directory - if (!RomMGameId.TryParse(gameID, out int _, out string romMSHA1) || !File.Exists($"{ROMDataPath}{romMSHA1}.json")) + if (!RomMGameId.TryParse(gameID, out int _, out string romMSHA1)) { Logger.Error($"{args.Game.Name} GameID is malformed!"); romData.Id = (int)InstallStatus.Cancelled; @@ -410,15 +414,10 @@ public override IEnumerable GetInstallActions(GetInstallActio yield break; } - try + gameData = LoadGameData(args.Game); + if (gameData == null) { - string json = File.ReadAllText($"{ROMDataPath}{romMSHA1}.json"); - gameData = JsonConvert.DeserializeObject(json); - } - catch (Exception) - { - Logger.Error($"{args.Game.Name} GameID is malformed or {romMSHA1} json file is corrupted!"); - romData.Id = (int)InstallStatus.Cancelled; + Logger.Error($"{args.Game.Name} has no readable ROM data file; run update game library before installing!"); } if (romData.Id == (int)InstallStatus.Cancelled || gameData?.ROMVersions == null || gameData.ROMVersions.Count == 0) @@ -495,7 +494,7 @@ public override IEnumerable GetInstallActions(GetInstallActio gameData.ROMVersions[0].IsSelected = true; } - File.WriteAllText($"{ROMDataPath}{romMSHA1}.json", JsonConvert.SerializeObject(gameData)); + RomMGameData.Save(ROMDataPath, romMSHA1, gameData); } yield return new RomMInstallController(args.Game, this, romData); @@ -505,23 +504,7 @@ public override IEnumerable GetUninstallActions(GetUninstal { if (args.Game.PluginId == Id) { - EmulatorMapping mapping = null; - - try - { - var splitID = args.Game.GameId.Split(':'); - string sidecarPath = $"{ROMDataPath}{splitID[1]}.json"; - if (File.Exists(sidecarPath)) - { - var existingJson = File.ReadAllText(sidecarPath); - var localROM = JsonConvert.DeserializeObject(existingJson); - mapping = Settings.Mappings.FirstOrDefault(x => x.MappingId == localROM.MappingID); - } - } - catch (Exception) - { - Logger.Error($"{args.Game.Name} GameID is malformed or json file is corrupted!"); - } + EmulatorMapping mapping = MappingFor(args.Game); if (mapping == null) yield return null; diff --git a/Saves/SaveEmulatorResolver.cs b/Saves/SaveEmulatorResolver.cs new file mode 100644 index 0000000..594329d --- /dev/null +++ b/Saves/SaveEmulatorResolver.cs @@ -0,0 +1,104 @@ +using Playnite.SDK.Models; +using RomM.Saves.Handlers; +using System.Collections.Generic; +using System.Linq; + +namespace RomM.Saves +{ + /// Where a candidate emulator came from, for logging and messages. + internal enum SaveEmulatorSource + { + None = 0, + PlayAction = 1, + Mapping = 2, + } + + /// Why no emulator could be used, when none could. + internal enum SaveEmulatorProblem + { + None = 0, + + /// Neither the play action nor the mapping names an emulator. + NoEmulator = 1, + + /// An emulator is set, but no handler knows where it keeps saves. + Unsupported = 2, + } + + internal class SaveEmulatorCandidate + { + public SaveEmulatorSource Source { get; set; } + public Emulator Emulator { get; set; } + public EmulatorProfile Profile { get; set; } + } + + internal class SaveEmulatorResolution + { + public Emulator Emulator { get; set; } + public EmulatorProfile Profile { get; set; } + public ISaveHandler Handler { get; set; } + public SaveEmulatorSource Source { get; set; } + public SaveEmulatorProblem Problem { get; set; } + + /// The emulator that is set but unsupported, for the message shown to the user. + public string UnsupportedEmulatorName { get; set; } + } + + /// + /// Picks the emulator a game's saves belong to, from the candidates in preference order. + /// + /// The play action leads: a user who repoints it at another emulator should have their saves + /// follow it. But the action is only a snapshot of the mapping taken at import, so when it + /// names an emulator no handler covers, the platform's own emulator mapping -- which the + /// settings screen presents as the thing that decides this -- gets its turn before sync gives + /// up. Kept free of Playnite lookups so the preference order can be tested on its own. + /// + internal static class SaveEmulatorResolver + { + public static SaveEmulatorResolution Resolve(SaveHandlerRegistry handlers, IEnumerable candidates) + { + var known = (candidates ?? Enumerable.Empty()) + .Where(c => c != null && c.Emulator != null) + .ToList(); + + if (known.Count == 0) + return new SaveEmulatorResolution { Problem = SaveEmulatorProblem.NoEmulator }; + + foreach (var candidate in known) + { + var handler = handlers?.Find(candidate.Emulator); + if (handler == null) + continue; + + return new SaveEmulatorResolution + { + Emulator = candidate.Emulator, + Profile = candidate.Profile ?? BorrowProfile(known, candidate), + Handler = handler, + Source = candidate.Source, + }; + } + + return new SaveEmulatorResolution + { + Problem = SaveEmulatorProblem.Unsupported, + UnsupportedEmulatorName = known[0].Emulator.Name, + }; + } + + /// + /// A play action can name an emulator without naming a profile, and for RetroArch the + /// profile is what identifies the core -- which is a folder in the save path when + /// sort_savefiles_enable is on. Rather than resolve a path with the core folder missing, + /// where a download would land somewhere RetroArch never reads, take the profile from + /// another candidate for the same emulator. + /// + private static EmulatorProfile BorrowProfile(IEnumerable candidates, SaveEmulatorCandidate chosen) + { + return candidates + .Where(c => c != chosen && c.Profile != null && c.Emulator.Id == chosen.Emulator.Id) + .Select(c => c.Profile) + .FirstOrDefault(); + } + } +} diff --git a/Saves/SaveSyncService.cs b/Saves/SaveSyncService.cs index 81f607c..5a836cf 100644 --- a/Saves/SaveSyncService.cs +++ b/Saves/SaveSyncService.cs @@ -97,10 +97,12 @@ public class SyncOutcome lock (_romLocks.GetOrAdd(romId, _ => new object())) { - var target = ResolveTarget(game); + // ResolveTarget words the reason it gave up: which emulator it looked at, and + // whether the problem is that none is set, that none is supported, or that the + // supported one's save path could not be worked out. + var target = ResolveTarget(game, outcome); if (target == null) { - outcome.Message = "Save sync does not know where this game's emulator keeps its saves."; return outcome; } @@ -431,66 +433,104 @@ private bool HasDeviceFor(string host) #region Save location /// - /// Finds the emulator Playnite launches this game with, hands it to whichever handler - /// recognises it, and lets that handler locate the save. Null when the game has no - /// emulator, no ROM path, or runs on an emulator no handler covers yet. + /// Finds the emulator this game's saves belong to, hands it to whichever handler recognises + /// it, and lets that handler locate the save. Null when no emulator is set, none is + /// supported, or the handler cannot work out a path -- each of which writes its own reason + /// into , because "somewhere in these three" is not something a + /// user can act on. /// - private SaveTarget ResolveTarget(Game game) + private SaveTarget ResolveTarget(Game game, SyncOutcome outcome) { var contentPath = game.Roms?.FirstOrDefault()?.Path; if (string.IsNullOrEmpty(contentPath)) + { + outcome.Message = $"{game.Name} has no ROM file for save sync to work from."; return null; + } - var action = EmulatorAction(game); - var emulator = ResolveEmulator(action); - if (emulator == null) + var resolution = ResolveEmulator(game); + if (resolution.Problem == SaveEmulatorProblem.NoEmulator) + { + outcome.Message = $"{game.Name} has no emulator set. Choose one in the game's Actions, " + + "or map its platform under RomM settings."; return null; + } - var handler = _handlers.Find(emulator); - if (handler == null) + if (resolution.Problem == SaveEmulatorProblem.Unsupported) { - Logger.Info($"[SaveSync] No save handler for emulator '{emulator.Name}', skipping {game.Name}."); + Logger.Info($"[SaveSync] No save handler for emulator '{resolution.UnsupportedEmulatorName}', skipping {game.Name}."); + outcome.Message = $"Save sync does not support {resolution.UnsupportedEmulatorName} yet. " + + $"Supported: {SupportedEmulators}."; return null; } - return handler.ResolveTarget(new SaveTargetRequest + if (resolution.Source == SaveEmulatorSource.Mapping) + { + Logger.Info($"[SaveSync] {game.Name}'s play action names no emulator save sync supports; " + + $"using {resolution.Emulator.Name} from its RomM platform mapping instead."); + } + + var target = resolution.Handler.ResolveTarget(new SaveTargetRequest { Game = game, - EmulatorInstallDir = PlaynitePath.Resolve(_romM.Playnite, emulator.InstallDir), - Profile = ResolveProfile(action, emulator), + EmulatorInstallDir = PlaynitePath.Resolve(_romM.Playnite, resolution.Emulator.InstallDir), + Profile = resolution.Profile, ContentPath = _romM.Playnite.ExpandGameVariables(game, contentPath), Logger = Logger, }); + + if (target == null) + { + outcome.Message = $"Could not work out where {resolution.Emulator.Name} keeps this game's saves."; + } + + return target; } /// - /// The emulator comes from the action Playnite actually launches with, not from the - /// the game was imported under: a user who repoints the play - /// action at another emulator should have their saves follow it. + /// The play action leads -- a user who repoints it at another emulator should have their + /// saves follow it -- but it is only a snapshot of the emulator mapping taken at import, so + /// the mapping gets its turn when the action names an emulator no handler covers. See + /// for the rules; this half is only the Playnite lookups. /// - private Emulator ResolveEmulator(GameAction action) + private SaveEmulatorResolution ResolveEmulator(Game game) { + var candidates = new List(); + + var action = RomMPlayAction.Find(game.GameActions); if (action != null && action.EmulatorId != Guid.Empty) - return _romM.Playnite.Database.Emulators?.FirstOrDefault(e => e.Id == action.EmulatorId); + { + var emulator = _romM.Playnite.Database.Emulators?.FirstOrDefault(e => e.Id == action.EmulatorId); + candidates.Add(new SaveEmulatorCandidate + { + Source = SaveEmulatorSource.PlayAction, + Emulator = emulator, + Profile = ProfileOf(emulator, action.EmulatorProfileId), + }); + } + + var mapping = _romM.MappingFor(game); + if (mapping != null) + { + candidates.Add(new SaveEmulatorCandidate + { + Source = SaveEmulatorSource.Mapping, + Emulator = mapping.Emulator, + Profile = ProfileOf(mapping.Emulator, mapping.EmulatorProfileId), + }); + } - return null; + return SaveEmulatorResolver.Resolve(_handlers, candidates); } - private static EmulatorProfile ResolveProfile(GameAction action, Emulator emulator) + private static EmulatorProfile ProfileOf(Emulator emulator, string profileId) { - var profileId = action?.EmulatorProfileId; - if (string.IsNullOrEmpty(profileId)) + if (emulator == null || string.IsNullOrEmpty(profileId)) return null; return emulator.SelectableProfiles?.FirstOrDefault(p => p.Id == profileId); } - private static GameAction EmulatorAction(Game game) - { - return game.GameActions?.FirstOrDefault(a => a.IsPlayAction && a.Type == GameActionType.Emulator) - ?? game.GameActions?.FirstOrDefault(a => a.Type == GameActionType.Emulator); - } - #endregion #region HTTP helper From 10b911dbe707982123729459cd3795bae7dd25a2 Mon Sep 17 00:00:00 2001 From: Georges-Antoine Assi Date: Mon, 21 Sep 2026 21:27:19 -0400 Subject: [PATCH 2/4] Simplify the save-sync resolution and its supporting helpers Quality pass over the previous commit, no behaviour intended beyond the one noted below. The resolver described its outcome twice: a problem enum beside the emulator and handler it had already stored. A null emulator means none was named and a null handler means none is supported, so the enum and the duplicate emulator name go, and the source of the pick becomes a bool. ResolveTarget takes an out reason rather than reaching into the caller's outcome to write one field of it. The importer's play action had two writers: RomMPlayAction.Build for a new game and a field-by-field copy of it for a refreshed one, which would have drifted the first time a field was added. Build now delegates to Apply, which both use. Matches and IsUnedited take the applied action whole instead of unpacking it into loose parameters, and the mapping lookup behind MappingFor is shared with the install path and the legacy game id through SettingsViewModel.MappingById. Three pieces of wasted work, all per ROM or on the pre-launch path: the sidecar was re-serialised to compare against what had just been parsed, where the text read off disk was already in hand; the emulator name was looked up by scanning the whole collection, eagerly, for a legacy branch that usually does not need it, and is now a keyed Get behind a thunk; and save sync read the sidecar on every game even when the play action already answered on its own, which it does whenever its emulator is supported and it names a profile. The one behaviour change: an install whose sidecar is missing or corrupt now logs one message and cancels, where it logged two and reached the same end. Co-Authored-By: Claude Opus 5 (1M context) --- Games/RomMGameData.cs | 18 +++++-- Games/RomMGameInfo.Plugin.cs | 2 +- Games/RomMImport.cs | 35 ++++++------- Games/RomMPlayAction.cs | 60 ++++++++++++----------- RomM.Tests/RomMPlayActionTests.cs | 50 +++++++++++++++---- RomM.Tests/SaveEmulatorResolverTests.cs | 52 ++++++++++---------- RomM.cs | 20 ++------ Saves/SaveEmulatorResolver.cs | 47 ++++++------------ Saves/SaveSyncService.cs | 65 ++++++++++++++----------- Settings/Settings.cs | 3 ++ 10 files changed, 188 insertions(+), 164 deletions(-) diff --git a/Games/RomMGameData.cs b/Games/RomMGameData.cs index 59a4e2f..ecc9300 100644 --- a/Games/RomMGameData.cs +++ b/Games/RomMGameData.cs @@ -7,7 +7,7 @@ namespace RomM.Games { /// - /// The per-ROM sidecar ("<sha1>.json" under the plugin's data folder): the download + /// The per-ROM sidecar ("{sha1}.json" under the plugin's data folder): the download /// descriptors for every revision, the emulator mapping the ROM was imported under, and what /// the importer last wrote onto the game's play action. /// @@ -31,18 +31,28 @@ public static RomMRomLocal Load(string romDataPath, string gameId, ILogger logge return null; } - return LoadBySha1(romDataPath, sha1, logger, gameName); + return LoadBySha1(romDataPath, sha1, logger, out string _, gameName); } - public static RomMRomLocal LoadBySha1(string romDataPath, string sha1, ILogger logger, string gameName = null) + /// + /// The file's text exactly as read, or null when there was none. A caller about to rewrite + /// the sidecar compares against this to decide whether anything changed, rather than + /// re-serialising what it has just parsed once per ROM. + /// + public static RomMRomLocal LoadBySha1(string romDataPath, string sha1, ILogger logger, out string rawJson, string gameName = null) { + rawJson = null; + var path = PathFor(romDataPath, sha1); if (!File.Exists(path)) return null; try { - return JsonConvert.DeserializeObject(File.ReadAllText(path)); + var text = File.ReadAllText(path); + var data = JsonConvert.DeserializeObject(text); + rawJson = text; + return data; } catch (Exception ex) { diff --git a/Games/RomMGameInfo.Plugin.cs b/Games/RomMGameInfo.Plugin.cs index ef135ab..e270d37 100644 --- a/Games/RomMGameInfo.Plugin.cs +++ b/Games/RomMGameInfo.Plugin.cs @@ -20,7 +20,7 @@ public EmulatorMapping Mapping { get { - return Settings.SettingsViewModel.Instance.Mappings.FirstOrDefault(m => m.MappingId == MappingId); + return Settings.SettingsViewModel.Instance.MappingById(MappingId); } } diff --git a/Games/RomMImport.cs b/Games/RomMImport.cs index c42d282..683a519 100644 --- a/Games/RomMImport.cs +++ b/Games/RomMImport.cs @@ -135,7 +135,8 @@ public List ProcessData() // What the sidecar held before this import overwrites it: the record of the // play action the plugin last wrote, which decides whether that action is // still ours to repoint at the mapping's current emulator. - var previous = RomMGameData.LoadBySha1(_plugin.ROMDataPath, ROM.SHA1, _plugin.Logger, ROM.Name); + var previous = RomMGameData.LoadBySha1(_plugin.ROMDataPath, ROM.SHA1, _plugin.Logger, + out string previousJson, ROM.Name); // Skip full import if ROM has already been imported Guid statusID = Guid.Empty; @@ -155,7 +156,7 @@ public List ProcessData() } // Save Game ROM data to file - SaveGameData(ROM, previous, RefreshPlayAction(existingGame, previous)); + SaveGameData(ROM, previous, previousJson, RefreshPlayAction(existingGame, previous)); importedGameIds.Add(gameID); continue; } @@ -167,13 +168,14 @@ public List ProcessData() // The adopted entry is an existing game re-keyed under the new id, so its // action is refreshed on the same terms as any other existing game's. _existingGames.TryGetValue(gameID, out var adoptedGame); - SaveGameData(ROM, previous, RefreshPlayAction(adoptedGame, previous)); + SaveGameData(ROM, previous, previousJson, RefreshPlayAction(adoptedGame, previous)); importedGameIds.Add(gameID); continue; } // A new game gets the mapping's emulator outright, below. - SaveGameData(ROM, previous, new AppliedPlayAction(_mapping.EmulatorId, _mapping.EmulatorProfileId)); + SaveGameData(ROM, previous, previousJson, + new AppliedPlayAction(_mapping.EmulatorId, _mapping.EmulatorProfileId)); var importedGame = ImportGame(ROM, statusID); if (importedGame != null) @@ -378,7 +380,7 @@ private bool UpdatedDeletedGame(RomMRom ROM) private MainSibling CheckForMainSibling(RomMRom ROM) => RomMSiblings.ClassifyMain(ROM, _romById); - private void SaveGameData(RomMRom ROM, RomMRomLocal previous, AppliedPlayAction applied) + private void SaveGameData(RomMRom ROM, RomMRomLocal previous, string previousJson, AppliedPlayAction applied) { RomMRomLocal toSave = new RomMRomLocal { @@ -428,7 +430,7 @@ private void SaveGameData(RomMRom ROM, RomMRomLocal previous, AppliedPlayAction } string json = JsonConvert.SerializeObject(toSave); - if (previous == null || json != JsonConvert.SerializeObject(previous)) + if (json != previousJson) RomMGameData.Save(_plugin.ROMDataPath, ROM.SHA1, toSave); } @@ -445,9 +447,8 @@ private void SaveGameData(RomMRom ROM, RomMRomLocal previous, AppliedPlayAction private AppliedPlayAction RefreshPlayAction(Game game, RomMRomLocal previous) { var mapped = new AppliedPlayAction(_mapping.EmulatorId, _mapping.EmulatorProfileId); - var applied = previous != null - ? new AppliedPlayAction(previous.AppliedEmulatorID, previous.AppliedEmulatorProfileID) - : AppliedPlayAction.Unknown; + var applied = new AppliedPlayAction(previous?.AppliedEmulatorID ?? Guid.Empty, + previous?.AppliedEmulatorProfileID); // No action to keep in step -- and no game at all, on the adoption path where the // re-keyed entry could not be found again. @@ -455,27 +456,23 @@ private AppliedPlayAction RefreshPlayAction(Game game, RomMRomLocal previous) if (action == null || _platformHasRivalMapping) return applied; - if (RomMPlayAction.Matches(action, mapped.EmulatorId, mapped.ProfileId)) + if (RomMPlayAction.Matches(action, mapped)) return mapped; - var actionEmulatorName = _plugin.Playnite.Database.Emulators? - .FirstOrDefault(e => e.Id == action.EmulatorId)?.Name; + Func actionEmulatorName = () => + _plugin.Playnite.Database.Emulators?.Get(action.EmulatorId)?.Name; - if (!RomMPlayAction.IsUnedited(action, applied.EmulatorId, applied.ProfileId, actionEmulatorName)) + if (!RomMPlayAction.IsUnedited(action, applied, actionEmulatorName)) { _plugin.Logger.Info($"[Importer] Leaving {game.Name}'s play action pointed at " + - $"{actionEmulatorName ?? ""}: it is no longer the one the plugin wrote."); + $"{actionEmulatorName() ?? ""}: it is no longer the one the plugin wrote."); return applied; } if (_mapping.Emulator == null) return applied; - action.Name = RomMPlayAction.NameFor(_mapping.Emulator.Name); - action.Type = GameActionType.Emulator; - action.EmulatorId = mapped.EmulatorId; - action.EmulatorProfileId = mapped.ProfileId; - action.IsPlayAction = true; + RomMPlayAction.Apply(action, _mapping.Emulator.Name, mapped.EmulatorId, mapped.ProfileId); // Our own write; OnItemUpdated has nothing to push to RomM for it. _plugin.SuppressSync(game.Id); diff --git a/Games/RomMPlayAction.cs b/Games/RomMPlayAction.cs index 4be1f10..1d43c04 100644 --- a/Games/RomMPlayAction.cs +++ b/Games/RomMPlayAction.cs @@ -7,13 +7,11 @@ namespace RomM.Games { /// /// The emulator and profile the importer last wrote onto a game's play action, as recorded in - /// the ROM's sidecar. is what a sidecar written before the plugin kept + /// the ROM's sidecar. An empty emulator id is what a sidecar written before the plugin kept /// this record yields. /// internal struct AppliedPlayAction { - public static readonly AppliedPlayAction Unknown = new AppliedPlayAction(Guid.Empty, null); - public readonly Guid EmulatorId; public readonly string ProfileId; @@ -37,16 +35,22 @@ internal static class RomMPlayAction { public static string NameFor(string emulatorName) => $"Play in {emulatorName}"; - public static GameAction Build(string emulatorName, Guid emulatorId, string emulatorProfileId) + public static GameAction Build(string emulatorName, Guid emulatorId, string emulatorProfileId) => + Apply(new GameAction(), emulatorName, emulatorId, emulatorProfileId); + + /// + /// Writes the importer's play action onto an existing one, for repointing a game already in + /// the library. Shares its body with so a refreshed action cannot drift + /// from a freshly imported one. + /// + public static GameAction Apply(GameAction action, string emulatorName, Guid emulatorId, string emulatorProfileId) { - return new GameAction - { - Name = NameFor(emulatorName), - Type = GameActionType.Emulator, - EmulatorId = emulatorId, - EmulatorProfileId = emulatorProfileId, - IsPlayAction = true, - }; + action.Name = NameFor(emulatorName); + action.Type = GameActionType.Emulator; + action.EmulatorId = emulatorId; + action.EmulatorProfileId = emulatorProfileId; + action.IsPlayAction = true; + return action; } /// @@ -58,39 +62,39 @@ public static GameAction Find(IEnumerable actions) if (actions == null) return null; - var list = actions as IList ?? actions.ToList(); - return list.FirstOrDefault(a => a != null && a.IsPlayAction && a.Type == GameActionType.Emulator) - ?? list.FirstOrDefault(a => a != null && a.Type == GameActionType.Emulator); + var emulatorActions = actions.Where(a => a != null && a.Type == GameActionType.Emulator).ToList(); + return emulatorActions.FirstOrDefault(a => a.IsPlayAction) ?? emulatorActions.FirstOrDefault(); } /// Whether the action already launches the given emulator and profile. - public static bool Matches(GameAction action, Guid emulatorId, string emulatorProfileId) + public static bool Matches(GameAction action, AppliedPlayAction target) { return action != null - && action.EmulatorId == emulatorId - && SameProfile(action.EmulatorProfileId, emulatorProfileId); + && action.EmulatorId == target.EmulatorId + && SameProfile(action.EmulatorProfileId, target.ProfileId); } /// /// Whether the action is still the plugin's to repoint. /// - /// / are what the - /// plugin last wrote, recorded in the ROM's sidecar; if the action still carries them, - /// nobody has touched it. Sidecars written before the plugin recorded that (every install + /// is what the plugin last wrote, recorded in the ROM's sidecar; + /// if the action still carries it, nobody has touched it. Sidecars written before the plugin recorded that (every install /// that predates this) carry no applied emulator, and then the generated name is the only - /// marker left: an action still called "Play in <the emulator it points at>" is one - /// the importer wrote and the user has not renamed or repointed. + /// marker left: an action still called "Play in {the emulator it points at}" is one + /// the importer wrote and the user has not renamed or repointed. That name costs a lookup + /// of the action's emulator, so it is passed as a thunk and only resolved on that path. /// - public static bool IsUnedited(GameAction action, Guid appliedEmulatorId, string appliedProfileId, string actionEmulatorName) + public static bool IsUnedited(GameAction action, AppliedPlayAction applied, Func actionEmulatorName) { if (action == null) return false; - if (appliedEmulatorId != Guid.Empty) - return Matches(action, appliedEmulatorId, appliedProfileId); + if (applied.EmulatorId != Guid.Empty) + return Matches(action, applied); - return !string.IsNullOrEmpty(actionEmulatorName) - && string.Equals(action.Name, NameFor(actionEmulatorName), StringComparison.Ordinal); + var name = actionEmulatorName?.Invoke(); + return !string.IsNullOrEmpty(name) + && string.Equals(action.Name, NameFor(name), StringComparison.Ordinal); } // Playnite writes an unset profile as either null or "", and the two mean the same thing. diff --git a/RomM.Tests/RomMPlayActionTests.cs b/RomM.Tests/RomMPlayActionTests.cs index ca9daba..ec42d2c 100644 --- a/RomM.Tests/RomMPlayActionTests.cs +++ b/RomM.Tests/RomMPlayActionTests.cs @@ -11,6 +11,9 @@ public class RomMPlayActionTests private static readonly Guid Mapped = Guid.NewGuid(); private static readonly Guid Other = Guid.NewGuid(); + private static AppliedPlayAction Applied(Guid emulatorId, string profileId) => + new AppliedPlayAction(emulatorId, profileId); + [Fact] public void Build_produces_the_importers_play_action() { @@ -23,6 +26,22 @@ public void Build_produces_the_importers_play_action() Assert.True(action.IsPlayAction); } + // Repointing an existing action has to leave it identical to a freshly imported one, which + // is why both go through the same writer. + [Fact] + public void Apply_rewrites_an_existing_action_into_the_importers_shape() + { + var action = new GameAction { Name = "Play in Dolphin", Type = GameActionType.URL, EmulatorId = Other }; + + RomMPlayAction.Apply(action, "RetroArch", Mapped, "profile-1"); + + Assert.Equal("Play in RetroArch", action.Name); + Assert.Equal(GameActionType.Emulator, action.Type); + Assert.Equal(Mapped, action.EmulatorId); + Assert.Equal("profile-1", action.EmulatorProfileId); + Assert.True(action.IsPlayAction); + } + [Fact] public void Find_prefers_the_emulator_action_marked_as_the_play_action() { @@ -68,7 +87,7 @@ public void Matches_treats_an_unset_profile_the_same_either_way(string actionPro { var action = new GameAction { EmulatorId = Mapped, EmulatorProfileId = actionProfile }; - Assert.True(RomMPlayAction.Matches(action, Mapped, mappedProfile)); + Assert.True(RomMPlayAction.Matches(action, Applied(Mapped, mappedProfile))); } [Fact] @@ -76,9 +95,9 @@ public void Matches_is_false_for_another_emulator_or_profile() { var action = new GameAction { EmulatorId = Mapped, EmulatorProfileId = "p" }; - Assert.False(RomMPlayAction.Matches(action, Other, "p")); - Assert.False(RomMPlayAction.Matches(action, Mapped, "q")); - Assert.False(RomMPlayAction.Matches(null, Mapped, "p")); + Assert.False(RomMPlayAction.Matches(action, Applied(Other, "p"))); + Assert.False(RomMPlayAction.Matches(action, Applied(Mapped, "q"))); + Assert.False(RomMPlayAction.Matches(null, Applied(Mapped, "p"))); } [Fact] @@ -86,7 +105,7 @@ public void An_action_still_carrying_what_the_plugin_applied_is_unedited() { var action = new GameAction { Name = "anything", EmulatorId = Mapped, EmulatorProfileId = "p" }; - Assert.True(RomMPlayAction.IsUnedited(action, Mapped, "p", "Dolphin")); + Assert.True(RomMPlayAction.IsUnedited(action, Applied(Mapped, "p"), () => "Dolphin")); } // Once the user has repointed the action, the recorded applied emulator no longer matches @@ -96,7 +115,7 @@ public void An_action_repointed_by_the_user_is_not_unedited() { var action = new GameAction { Name = "Play in Dolphin", EmulatorId = Other, EmulatorProfileId = "q" }; - Assert.False(RomMPlayAction.IsUnedited(action, Mapped, "p", "Dolphin")); + Assert.False(RomMPlayAction.IsUnedited(action, Applied(Mapped, "p"), () => "Dolphin")); } // Sidecars from before the plugin recorded what it applied: the generated name is the only @@ -106,7 +125,7 @@ public void Without_a_recorded_emulator_the_generated_name_marks_the_action_as_t { var action = new GameAction { Name = "Play in Dolphin", EmulatorId = Other }; - Assert.True(RomMPlayAction.IsUnedited(action, Guid.Empty, null, "Dolphin")); + Assert.True(RomMPlayAction.IsUnedited(action, Applied(Guid.Empty, null), () => "Dolphin")); } [Theory] @@ -117,13 +136,26 @@ public void Without_a_recorded_emulator_anything_but_the_generated_name_is_left_ { var action = new GameAction { Name = name, EmulatorId = Other }; - Assert.False(RomMPlayAction.IsUnedited(action, Guid.Empty, null, emulatorName)); + Assert.False(RomMPlayAction.IsUnedited(action, Applied(Guid.Empty, null), () => emulatorName)); + } + + // The emulator name costs a database lookup, so it is only resolved on the legacy path that + // actually needs it. + [Fact] + public void The_emulator_name_is_not_resolved_when_the_sidecar_records_what_we_applied() + { + var action = new GameAction { Name = "Play in Dolphin", EmulatorId = Mapped, EmulatorProfileId = "p" }; + var resolved = false; + + RomMPlayAction.IsUnedited(action, Applied(Mapped, "p"), () => { resolved = true; return "Dolphin"; }); + + Assert.False(resolved); } [Fact] public void A_missing_action_is_never_unedited() { - Assert.False(RomMPlayAction.IsUnedited(null, Mapped, "p", "RetroArch")); + Assert.False(RomMPlayAction.IsUnedited(null, Applied(Mapped, "p"), () => "RetroArch")); } } } diff --git a/RomM.Tests/SaveEmulatorResolverTests.cs b/RomM.Tests/SaveEmulatorResolverTests.cs index ba18674..bf23328 100644 --- a/RomM.Tests/SaveEmulatorResolverTests.cs +++ b/RomM.Tests/SaveEmulatorResolverTests.cs @@ -17,8 +17,11 @@ private static Emulator RetroArch(Guid? id = null) => private static Emulator Dolphin() => new Emulator { Id = Guid.NewGuid(), Name = "Dolphin" }; - private static SaveEmulatorCandidate Candidate(SaveEmulatorSource source, Emulator emulator, EmulatorProfile profile = null) => - new SaveEmulatorCandidate { Source = source, Emulator = emulator, Profile = profile }; + private static SaveEmulatorCandidate FromAction(Emulator emulator, EmulatorProfile profile = null) => + new SaveEmulatorCandidate { Emulator = emulator, Profile = profile }; + + private static SaveEmulatorCandidate FromMapping(Emulator emulator, EmulatorProfile profile = null) => + new SaveEmulatorCandidate { FromMapping = true, Emulator = emulator, Profile = profile }; // The action a user repointed at another supported emulator is their choice, and outranks // whatever the platform mapping says. @@ -29,13 +32,12 @@ public void The_play_action_wins_when_its_emulator_is_supported() var resolution = SaveEmulatorResolver.Resolve(Handlers, new[] { - Candidate(SaveEmulatorSource.PlayAction, fromAction), - Candidate(SaveEmulatorSource.Mapping, RetroArch()), + FromAction(fromAction), + FromMapping(RetroArch()), }); Assert.Same(fromAction, resolution.Emulator); - Assert.Equal(SaveEmulatorSource.PlayAction, resolution.Source); - Assert.Equal(SaveEmulatorProblem.None, resolution.Problem); + Assert.False(resolution.FromMapping); Assert.NotNull(resolution.Handler); } @@ -48,13 +50,13 @@ public void The_mapping_is_used_when_the_play_actions_emulator_is_unsupported() var resolution = SaveEmulatorResolver.Resolve(Handlers, new[] { - Candidate(SaveEmulatorSource.PlayAction, Dolphin()), - Candidate(SaveEmulatorSource.Mapping, fromMapping), + FromAction(Dolphin()), + FromMapping(fromMapping), }); Assert.Same(fromMapping, resolution.Emulator); - Assert.Equal(SaveEmulatorSource.Mapping, resolution.Source); - Assert.Equal(SaveEmulatorProblem.None, resolution.Problem); + Assert.True(resolution.FromMapping); + Assert.NotNull(resolution.Handler); } // Reported as unsupported rather than as "no emulator": the two need different advice, and @@ -64,12 +66,11 @@ public void No_supported_candidate_reports_the_first_emulator_as_unsupported() { var resolution = SaveEmulatorResolver.Resolve(Handlers, new[] { - Candidate(SaveEmulatorSource.PlayAction, Dolphin()), - Candidate(SaveEmulatorSource.Mapping, new Emulator { Id = Guid.NewGuid(), Name = "PCSX2" }), + FromAction(Dolphin()), + FromMapping(new Emulator { Id = Guid.NewGuid(), Name = "PCSX2" }), }); - Assert.Equal(SaveEmulatorProblem.Unsupported, resolution.Problem); - Assert.Equal("Dolphin", resolution.UnsupportedEmulatorName); + Assert.Equal("Dolphin", resolution.Emulator?.Name); Assert.Null(resolution.Handler); } @@ -78,20 +79,19 @@ public void Candidates_without_an_emulator_are_reported_as_none_set() { var resolution = SaveEmulatorResolver.Resolve(Handlers, new[] { - Candidate(SaveEmulatorSource.PlayAction, null), - Candidate(SaveEmulatorSource.Mapping, null), + FromAction(null), + FromMapping(null), null, }); - Assert.Equal(SaveEmulatorProblem.NoEmulator, resolution.Problem); Assert.Null(resolution.Emulator); + Assert.Null(resolution.Handler); } [Fact] public void No_candidates_at_all_are_reported_as_none_set() { - Assert.Equal(SaveEmulatorProblem.NoEmulator, - SaveEmulatorResolver.Resolve(Handlers, null).Problem); + Assert.Null(SaveEmulatorResolver.Resolve(Handlers, null).Emulator); } // An action can name an emulator without naming a profile. For RetroArch the profile is the @@ -105,11 +105,11 @@ public void A_profile_is_borrowed_from_another_candidate_for_the_same_emulator() var resolution = SaveEmulatorResolver.Resolve(Handlers, new[] { - Candidate(SaveEmulatorSource.PlayAction, RetroArch(id)), - Candidate(SaveEmulatorSource.Mapping, RetroArch(id), profile), + FromAction(RetroArch(id)), + FromMapping(RetroArch(id), profile), }); - Assert.Equal(SaveEmulatorSource.PlayAction, resolution.Source); + Assert.False(resolution.FromMapping); Assert.Same(profile, resolution.Profile); } @@ -120,8 +120,8 @@ public void A_profile_is_not_borrowed_from_a_different_emulator() { var resolution = SaveEmulatorResolver.Resolve(Handlers, new[] { - Candidate(SaveEmulatorSource.PlayAction, RetroArch()), - Candidate(SaveEmulatorSource.Mapping, RetroArch(), new BuiltInEmulatorProfile { Name = "mGBA" }), + FromAction(RetroArch()), + FromMapping(RetroArch(), new BuiltInEmulatorProfile { Name = "mGBA" }), }); Assert.Null(resolution.Profile); @@ -135,8 +135,8 @@ public void The_candidates_own_profile_is_kept() var resolution = SaveEmulatorResolver.Resolve(Handlers, new List { - Candidate(SaveEmulatorSource.PlayAction, RetroArch(id), own), - Candidate(SaveEmulatorSource.Mapping, RetroArch(id), new BuiltInEmulatorProfile { Name = "mGBA" }), + FromAction(RetroArch(id), own), + FromMapping(RetroArch(id), new BuiltInEmulatorProfile { Name = "mGBA" }), }); Assert.Same(own, resolution.Profile); diff --git a/RomM.cs b/RomM.cs index 9e2570a..da82db6 100644 --- a/RomM.cs +++ b/RomM.cs @@ -120,14 +120,8 @@ public RomM(IPlayniteAPI api) : base(api) internal RomMRomLocal LoadGameData(Game game) => RomMGameData.Load(ROMDataPath, game?.GameId, Logger, game?.Name); - public EmulatorMapping MappingFor(Game game) - { - var gameData = LoadGameData(game); - if (gameData == null) - return null; - - return Settings?.Mappings?.FirstOrDefault(x => x.MappingId == gameData.MappingID); - } + public EmulatorMapping MappingFor(Game game) => + Settings?.MappingById(LoadGameData(game)?.MappingID ?? Guid.Empty); public RomMRom FetchRom(string romId) { @@ -415,13 +409,9 @@ public override IEnumerable GetInstallActions(GetInstallActio } gameData = LoadGameData(args.Game); - if (gameData == null) - { - Logger.Error($"{args.Game.Name} has no readable ROM data file; run update game library before installing!"); - } - - if (romData.Id == (int)InstallStatus.Cancelled || gameData?.ROMVersions == null || gameData.ROMVersions.Count == 0) + if (gameData?.ROMVersions == null || gameData.ROMVersions.Count == 0) { + Logger.Error($"{args.Game.Name} has no usable ROM data file; run update game library before installing!"); romData.Id = (int)InstallStatus.Cancelled; yield return new RomMInstallController(args.Game, this, romData); yield break; @@ -435,7 +425,7 @@ public override IEnumerable GetInstallActions(GetInstallActio FolderName = gameData.ROMVersions[0].FolderName, HasMultipleFiles = gameData.ROMVersions[0].HasMultipleFiles, DownloadURL = gameData.ROMVersions[0].DownloadURL, - Mapping = Settings.Mappings.FirstOrDefault(x => x.MappingId == gameData.MappingID) + Mapping = Settings.MappingById(gameData.MappingID) }; // If Siblings are available prompt user with version selection diff --git a/Saves/SaveEmulatorResolver.cs b/Saves/SaveEmulatorResolver.cs index 594329d..ad93c14 100644 --- a/Saves/SaveEmulatorResolver.cs +++ b/Saves/SaveEmulatorResolver.cs @@ -5,43 +5,26 @@ namespace RomM.Saves { - /// Where a candidate emulator came from, for logging and messages. - internal enum SaveEmulatorSource - { - None = 0, - PlayAction = 1, - Mapping = 2, - } - - /// Why no emulator could be used, when none could. - internal enum SaveEmulatorProblem - { - None = 0, - - /// Neither the play action nor the mapping names an emulator. - NoEmulator = 1, - - /// An emulator is set, but no handler knows where it keeps saves. - Unsupported = 2, - } - internal class SaveEmulatorCandidate { - public SaveEmulatorSource Source { get; set; } + /// Whether this candidate came from the platform mapping rather than the play action. + public bool FromMapping { get; set; } + public Emulator Emulator { get; set; } public EmulatorProfile Profile { get; set; } } + /// + /// The outcome of the pick. A null means no candidate named one at all; + /// an emulator with a null means one is set but no handler knows where it + /// keeps its saves. The two need different advice, which is why they are distinguishable. + /// internal class SaveEmulatorResolution { public Emulator Emulator { get; set; } public EmulatorProfile Profile { get; set; } public ISaveHandler Handler { get; set; } - public SaveEmulatorSource Source { get; set; } - public SaveEmulatorProblem Problem { get; set; } - - /// The emulator that is set but unsupported, for the message shown to the user. - public string UnsupportedEmulatorName { get; set; } + public bool FromMapping { get; set; } } /// @@ -62,7 +45,7 @@ public static SaveEmulatorResolution Resolve(SaveHandlerRegistry handlers, IEnum .ToList(); if (known.Count == 0) - return new SaveEmulatorResolution { Problem = SaveEmulatorProblem.NoEmulator }; + return new SaveEmulatorResolution(); foreach (var candidate in known) { @@ -75,15 +58,13 @@ public static SaveEmulatorResolution Resolve(SaveHandlerRegistry handlers, IEnum Emulator = candidate.Emulator, Profile = candidate.Profile ?? BorrowProfile(known, candidate), Handler = handler, - Source = candidate.Source, + FromMapping = candidate.FromMapping, }; } - return new SaveEmulatorResolution - { - Problem = SaveEmulatorProblem.Unsupported, - UnsupportedEmulatorName = known[0].Emulator.Name, - }; + // An emulator is set but unsupported. The first candidate is the one the user would go + // looking for, so it is the one the message names. + return new SaveEmulatorResolution { Emulator = known[0].Emulator }; } /// diff --git a/Saves/SaveSyncService.cs b/Saves/SaveSyncService.cs index 5a836cf..1d94fb2 100644 --- a/Saves/SaveSyncService.cs +++ b/Saves/SaveSyncService.cs @@ -97,12 +97,10 @@ public class SyncOutcome lock (_romLocks.GetOrAdd(romId, _ => new object())) { - // ResolveTarget words the reason it gave up: which emulator it looked at, and - // whether the problem is that none is set, that none is supported, or that the - // supported one's save path could not be worked out. - var target = ResolveTarget(game, outcome); + var target = ResolveTarget(game, out string reason); if (target == null) { + outcome.Message = reason; return outcome; } @@ -435,36 +433,37 @@ private bool HasDeviceFor(string host) /// /// Finds the emulator this game's saves belong to, hands it to whichever handler recognises /// it, and lets that handler locate the save. Null when no emulator is set, none is - /// supported, or the handler cannot work out a path -- each of which writes its own reason - /// into , because "somewhere in these three" is not something a - /// user can act on. + /// supported, or the handler cannot work out a path -- says + /// which, because "somewhere in these three" is not something a user can act on. /// - private SaveTarget ResolveTarget(Game game, SyncOutcome outcome) + private SaveTarget ResolveTarget(Game game, out string reason) { + reason = null; + var contentPath = game.Roms?.FirstOrDefault()?.Path; if (string.IsNullOrEmpty(contentPath)) { - outcome.Message = $"{game.Name} has no ROM file for save sync to work from."; + reason = $"{game.Name} has no ROM file for save sync to work from."; return null; } var resolution = ResolveEmulator(game); - if (resolution.Problem == SaveEmulatorProblem.NoEmulator) + if (resolution.Emulator == null) { - outcome.Message = $"{game.Name} has no emulator set. Choose one in the game's Actions, " + - "or map its platform under RomM settings."; + reason = $"{game.Name} has no emulator set. Choose one in the game's Actions, " + + "or map its platform under RomM settings."; return null; } - if (resolution.Problem == SaveEmulatorProblem.Unsupported) + if (resolution.Handler == null) { - Logger.Info($"[SaveSync] No save handler for emulator '{resolution.UnsupportedEmulatorName}', skipping {game.Name}."); - outcome.Message = $"Save sync does not support {resolution.UnsupportedEmulatorName} yet. " + - $"Supported: {SupportedEmulators}."; + Logger.Info($"[SaveSync] No save handler for emulator '{resolution.Emulator.Name}', skipping {game.Name}."); + reason = $"Save sync does not support {resolution.Emulator.Name} yet. " + + $"Supported: {SupportedEmulators}."; return null; } - if (resolution.Source == SaveEmulatorSource.Mapping) + if (resolution.FromMapping) { Logger.Info($"[SaveSync] {game.Name}'s play action names no emulator save sync supports; " + $"using {resolution.Emulator.Name} from its RomM platform mapping instead."); @@ -481,7 +480,7 @@ private SaveTarget ResolveTarget(Game game, SyncOutcome outcome) if (target == null) { - outcome.Message = $"Could not work out where {resolution.Emulator.Name} keeps this game's saves."; + reason = $"Could not work out where {resolution.Emulator.Name} keeps this game's saves."; } return target; @@ -498,26 +497,34 @@ private SaveEmulatorResolution ResolveEmulator(Game game) var candidates = new List(); var action = RomMPlayAction.Find(game.GameActions); + SaveEmulatorCandidate fromAction = null; if (action != null && action.EmulatorId != Guid.Empty) { - var emulator = _romM.Playnite.Database.Emulators?.FirstOrDefault(e => e.Id == action.EmulatorId); - candidates.Add(new SaveEmulatorCandidate + var emulator = _romM.Playnite.Database.Emulators?.Get(action.EmulatorId); + fromAction = new SaveEmulatorCandidate { - Source = SaveEmulatorSource.PlayAction, Emulator = emulator, Profile = ProfileOf(emulator, action.EmulatorProfileId), - }); + }; + candidates.Add(fromAction); } - var mapping = _romM.MappingFor(game); - if (mapping != null) + // Reading the mapping means reading the ROM's sidecar off disk, and this runs on the + // pre-launch path, so it is skipped when the action can answer on its own -- which it + // cannot if its emulator is unsupported, or if it names no profile for the handler to + // take the core from. + if (fromAction?.Profile == null || _handlers.Find(fromAction.Emulator) == null) { - candidates.Add(new SaveEmulatorCandidate + var mapping = _romM.MappingFor(game); + if (mapping != null) { - Source = SaveEmulatorSource.Mapping, - Emulator = mapping.Emulator, - Profile = ProfileOf(mapping.Emulator, mapping.EmulatorProfileId), - }); + candidates.Add(new SaveEmulatorCandidate + { + FromMapping = true, + Emulator = mapping.Emulator, + Profile = mapping.EmulatorProfile, + }); + } } return SaveEmulatorResolver.Resolve(_handlers, candidates); diff --git a/Settings/Settings.cs b/Settings/Settings.cs index 0417861..31c68dd 100644 --- a/Settings/Settings.cs +++ b/Settings/Settings.cs @@ -276,6 +276,9 @@ public string ExcludeGenres public ObservableCollection Mappings { get; set; } + /// The mapping with this id, or null. The link a game keeps to its mapping. + internal EmulatorMapping MappingById(Guid id) => Mappings?.FirstOrDefault(x => x.MappingId == id); + public List RomMPlatforms { get => _romMPlatforms; From 0e1246c6a5f2a572a3cb37aa046a7c22a6df5f51 Mon Sep 17 00:00:00 2001 From: Georges-Antoine Assi Date: Thu, 24 Sep 2026 10:53:05 -0400 Subject: [PATCH 3/4] Tighten play-action refresh and share mapping checks - Keep a user's per-game core, arguments and demoted actions when repointing play actions, and save each game once per import. - Clear a mapping's profile when its emulator changes, so a mismatched pair is never written; drop the importer's guard for it. - Share the controller's mapping skip rules via EmulatorMapping so the rival-mapping check can't drift. - Consume sync suppression whether or not KeepRomMSynced is on. - Log locked sidecars as unreadable rather than corrupted, write the JSON already produced, and reuse the parsed sha1 when loading sidecars. Co-Authored-By: Claude Opus 5.5 (1M context) --- Games/RomMGameData.cs | 16 +++++- Games/RomMImport.cs | 91 +++++++++++++++++++------------ Games/RomMImportController.cs | 4 +- Games/RomMPlayAction.cs | 24 ++++---- RomM.Tests/RomMPlayActionTests.cs | 33 +++++++++-- RomM.cs | 17 +++--- Saves/SaveEmulatorResolver.cs | 5 +- Settings/EmulatorMapping.cs | 20 +++++++ 8 files changed, 145 insertions(+), 65 deletions(-) diff --git a/Games/RomMGameData.cs b/Games/RomMGameData.cs index ecc9300..151873e 100644 --- a/Games/RomMGameData.cs +++ b/Games/RomMGameData.cs @@ -54,6 +54,13 @@ public static RomMRomLocal LoadBySha1(string romDataPath, string sha1, ILogger l rawJson = text; return data; } + catch (IOException ex) + { + // Save sync reads the sidecar off the UI thread while an import may be rewriting it; + // that is a transient sharing violation, not a corrupt file. + logger?.Warn(ex, $"{gameName ?? sha1} ROM data file could not be read."); + return null; + } catch (Exception ex) { logger?.Error(ex, $"{gameName ?? sha1} ROM data file is corrupted!"); @@ -62,6 +69,13 @@ public static RomMRomLocal LoadBySha1(string romDataPath, string sha1, ILogger l } public static void Save(string romDataPath, string sha1, RomMRomLocal data) => - File.WriteAllText(PathFor(romDataPath, sha1), JsonConvert.SerializeObject(data)); + SaveJson(romDataPath, sha1, JsonConvert.SerializeObject(data)); + + /// + /// For a caller that has already serialised the sidecar to compare it with what was on + /// disk, so it is not serialised a second time just to be written. + /// + public static void SaveJson(string romDataPath, string sha1, string json) => + File.WriteAllText(PathFor(romDataPath, sha1), json); } } diff --git a/Games/RomMImport.cs b/Games/RomMImport.cs index 6b5ab5a..0acb213 100644 --- a/Games/RomMImport.cs +++ b/Games/RomMImport.cs @@ -29,6 +29,8 @@ internal class RomMImport // the play actions the other just wrote. Neither is more right than the other, so when that // is the setup the actions are left exactly as they are. bool _platformHasRivalMapping; + // The emulator and profile this import writes onto play actions. + readonly AppliedPlayAction _mapped; public RomMImport(RomM plugin, LibraryImportGamesArgs args, EmulatorMapping mapping, List roms, List favourites) { @@ -66,8 +68,12 @@ public RomMImport(RomM plugin, LibraryImportGamesArgs args, EmulatorMapping mapp _completionStatusMap = plugin.Playnite.Database.CompletionStatuses.ToDictionary(cs => cs.Name, cs => cs.Id); _favourites = favourites; + // Only mappings the import controller will actually run count: one it skips as + // misconfigured writes nothing, so it cannot fight this one over the actions. _platformHasRivalMapping = plugin.Settings?.Mappings? - .Count(m => m.Enabled && m.RomMPlatformId == mapping.RomMPlatformId) > 1; + .Count(m => m.Enabled && m.IsImportable && m.RomMPlatformId == mapping.RomMPlatformId) > 1; + + _mapped = new AppliedPlayAction(mapping.EmulatorId, mapping.EmulatorProfileId); } // Builds the per-ROM download descriptor via the shared factory (see RomMRevisionFactory). @@ -142,6 +148,8 @@ public List ProcessData() Guid statusID = Guid.Empty; if (_existingGames.TryGetValue(gameID, out var existingGame)) { + bool changed = RefreshPlayAction(existingGame, previous, out var applied); + // Sync user data if (_plugin.Settings.KeepRomMSynced) { @@ -149,33 +157,34 @@ public List ProcessData() existingGame.Favorite = _favourites.Exists(f => f == ROM.Id); if (statusID != Guid.Empty) existingGame.CompletionStatusId = statusID; - - // This is our own write of the server's values; don't let OnItemUpdated echo it back. - _plugin.SuppressSync(existingGame.Id); - _plugin.Playnite.Database.Games.Update(existingGame); + changed = true; } + if (changed) + SaveOwnWrite(existingGame); + // Save Game ROM data to file - SaveGameData(ROM, previous, previousJson, RefreshPlayAction(existingGame, previous)); + SaveGameData(ROM, previous, previousJson, applied); importedGameIds.Add(gameID); continue; } // If keep deleted games is enabled and a deleted game gets re-added back to the // server under a new romMId, update the existing playnite entry instead. - if (_plugin.Settings.KeepDeletedGames && UpdatedDeletedGame(ROM)) + if (_plugin.Settings.KeepDeletedGames && UpdatedDeletedGame(ROM, out var adoptedGame)) { // The adopted entry is an existing game re-keyed under the new id, so its // action is refreshed on the same terms as any other existing game's. - _existingGames.TryGetValue(gameID, out var adoptedGame); - SaveGameData(ROM, previous, previousJson, RefreshPlayAction(adoptedGame, previous)); + if (RefreshPlayAction(adoptedGame, previous, out var applied)) + SaveOwnWrite(adoptedGame); + + SaveGameData(ROM, previous, previousJson, applied); importedGameIds.Add(gameID); continue; } // A new game gets the mapping's emulator outright, below. - SaveGameData(ROM, previous, previousJson, - new AppliedPlayAction(_mapping.EmulatorId, _mapping.EmulatorProfileId)); + SaveGameData(ROM, previous, previousJson, _mapped); var importedGame = ImportGame(ROM, statusID); if (importedGame != null) @@ -255,7 +264,7 @@ private Game ImportGame(RomMRom ROM, Guid StatusID) metadata.InstallSize = ROM.FileSizeBytes; metadata.GameActions = new List { - RomMPlayAction.Build(_mapping.Emulator.Name, _mapping.EmulatorId, _mapping.EmulatorProfileId), + RomMPlayAction.Build(_mapping.Emulator.Name, _mapped.EmulatorId, _mapped.ProfileId), new GameAction { Type = GameActionType.URL, @@ -364,7 +373,7 @@ private bool UpdatedOldGameID(RomMRom ROM) return false; } - private bool UpdatedDeletedGame(RomMRom ROM) + private bool UpdatedDeletedGame(RomMRom ROM, out Game adopted) { // A game with the same SHA1 but a different romMId means RomM deleted and re-added it; adopt // the existing entry under the new id. @@ -377,9 +386,11 @@ private bool UpdatedDeletedGame(RomMRom ROM) _existingGames.Remove(oldId); _existingGames[newId] = oldgame; + adopted = oldgame; return true; } + adopted = null; return false; } @@ -437,12 +448,20 @@ private void SaveGameData(RomMRom ROM, RomMRomLocal previous, string previousJso string json = JsonConvert.SerializeObject(toSave); if (json != previousJson) - RomMGameData.Save(_plugin.ROMDataPath, ROM.SHA1, toSave); + RomMGameData.SaveJson(_plugin.ROMDataPath, ROM.SHA1, json); + } + + // Saves a change the importer made to a game; OnItemUpdated must not echo it back to RomM. + private void SaveOwnWrite(Game game) + { + _plugin.SuppressSync(game.Id); + _plugin.Playnite.Database.Games.Update(game); } /// /// Brings an already-imported game's play action back in step with its mapping, and reports - /// the action the sidecar should now record. + /// the action the sidecar should now record. Returns whether the action was changed; the + /// caller saves the game, so a repoint and a user-data sync share one write. /// /// Existing games are otherwise skipped wholesale by the importer, so a mapping repointed /// at another emulator left every game it had imported launching -- and resolving its saves @@ -450,20 +469,29 @@ private void SaveGameData(RomMRom ROM, RomMRomLocal previous, string previousJso /// has repointed it themselves it is theirs, and the sidecar keeps remembering what we last /// wrote so that stays true across imports. /// - private AppliedPlayAction RefreshPlayAction(Game game, RomMRomLocal previous) + private bool RefreshPlayAction(Game game, RomMRomLocal previous, out AppliedPlayAction result) { - var mapped = new AppliedPlayAction(_mapping.EmulatorId, _mapping.EmulatorProfileId); - var applied = new AppliedPlayAction(previous?.AppliedEmulatorID ?? Guid.Empty, - previous?.AppliedEmulatorProfileID); + result = new AppliedPlayAction(previous?.AppliedEmulatorID ?? Guid.Empty, + previous?.AppliedEmulatorProfileID); + var applied = result; - // No action to keep in step -- and no game at all, on the adoption path where the - // re-keyed entry could not be found again. + // No action to keep in step. var action = RomMPlayAction.Find(game?.GameActions); if (action == null || _platformHasRivalMapping) - return applied; + return false; - if (RomMPlayAction.Matches(action, mapped)) - return mapped; + if (RomMPlayAction.Matches(action, _mapped)) + { + result = _mapped; + return false; + } + + // A sidecar from before the plugin recorded what it applied leaves only the generated + // name to go on, and that name records which emulator the importer chose, not which + // profile. On the mapping's own emulator a different profile is as likely a core the + // user picked for this one game as a mapping edit, so it is left alone. + if (applied.EmulatorId == Guid.Empty && action.EmulatorId == _mapped.EmulatorId) + return false; Func actionEmulatorName = () => _plugin.Playnite.Database.Emulators?.Get(action.EmulatorId)?.Name; @@ -472,21 +500,14 @@ private AppliedPlayAction RefreshPlayAction(Game game, RomMRomLocal previous) { _plugin.Logger.Info($"[Importer] Leaving {game.Name}'s play action pointed at " + $"{actionEmulatorName() ?? ""}: it is no longer the one the plugin wrote."); - return applied; + return false; } - if (_mapping.Emulator == null) - return applied; - - RomMPlayAction.Apply(action, _mapping.Emulator.Name, mapped.EmulatorId, mapped.ProfileId); - - // Our own write; OnItemUpdated has nothing to push to RomM for it. - _plugin.SuppressSync(game.Id); - _plugin.Playnite.Database.Games.Update(game); - + RomMPlayAction.Apply(action, _mapping.Emulator.Name, _mapped.EmulatorId, _mapped.ProfileId); _plugin.Logger.Info($"[Importer] Repointed {game.Name}'s play action at {_mapping.Emulator.Name} " + $"to follow the {_mapping.MappingName} mapping."); - return mapped; + result = _mapped; + return true; } private Guid DetermineCompletionStatus(RomMRom ROM) diff --git a/Games/RomMImportController.cs b/Games/RomMImportController.cs index b35257b..6e6a5f1 100644 --- a/Games/RomMImportController.cs +++ b/Games/RomMImportController.cs @@ -59,7 +59,7 @@ public List Import(LibraryImportGamesArgs args) break; // A mapping with no emulator/profile is genuinely unconfigured — skip quietly. - if (mapping.Emulator == null || mapping.EmulatorProfile == null) + if (!mapping.HasEmulatorProfile) { Logger.Warn($"[Import Controller] Emulator {mapping.MappingId} is misconfigured, skipping."); continue; @@ -70,7 +70,7 @@ public List Import(LibraryImportGamesArgs args) // is carried over. Give the user an actionable message instead of a cryptic // "-1 not found". The <= 0 check covers both the unset RomMPlatformId (-1) and the // empty default RomMPlatform (Id 0). - if (mapping.RomMPlatformId <= 0 || mapping.RomMPlatform == null || mapping.RomMPlatform.Id <= 0) + if (!mapping.HasRomMPlatform) { var name = !string.IsNullOrWhiteSpace(mapping.MappingName) ? mapping.MappingName diff --git a/Games/RomMPlayAction.cs b/Games/RomMPlayAction.cs index 1d43c04..12a1c0b 100644 --- a/Games/RomMPlayAction.cs +++ b/Games/RomMPlayAction.cs @@ -40,8 +40,9 @@ public static GameAction Build(string emulatorName, Guid emulatorId, string emul /// /// Writes the importer's play action onto an existing one, for repointing a game already in - /// the library. Shares its body with so a refreshed action cannot drift - /// from a freshly imported one. + /// the library. Shares its body with so the fields the importer owns + /// cannot drift from a freshly imported action; anything else on the action is left as it + /// is, which is why refuses an action carrying argument overrides. /// public static GameAction Apply(GameAction action, string emulatorName, Guid emulatorId, string emulatorProfileId) { @@ -83,10 +84,18 @@ public static bool Matches(GameAction action, AppliedPlayAction target) /// marker left: an action still called "Play in {the emulator it points at}" is one /// the importer wrote and the user has not renamed or repointed. That name costs a lookup /// of the action's emulator, so it is passed as a thunk and only resolved on that path. + /// + /// Either way the importer only ever writes a play action with no argument overrides, so an + /// action the user demoted from being the play action, or gave arguments of its own, is + /// theirs: repointing it would re-promote it beside their own play action, or launch the + /// new emulator with arguments meant for the old one. /// public static bool IsUnedited(GameAction action, AppliedPlayAction applied, Func actionEmulatorName) { - if (action == null) + if (action == null + || !action.IsPlayAction + || action.OverrideDefaultArgs + || !string.IsNullOrEmpty(action.AdditionalArguments)) return false; if (applied.EmulatorId != Guid.Empty) @@ -98,12 +107,7 @@ public static bool IsUnedited(GameAction action, AppliedPlayAction applied, Func } // Playnite writes an unset profile as either null or "", and the two mean the same thing. - private static bool SameProfile(string left, string right) - { - if (string.IsNullOrEmpty(left) && string.IsNullOrEmpty(right)) - return true; - - return string.Equals(left, right, StringComparison.Ordinal); - } + private static bool SameProfile(string left, string right) => + string.Equals(left ?? "", right ?? "", StringComparison.Ordinal); } } diff --git a/RomM.Tests/RomMPlayActionTests.cs b/RomM.Tests/RomMPlayActionTests.cs index ec42d2c..0cd9a8c 100644 --- a/RomM.Tests/RomMPlayActionTests.cs +++ b/RomM.Tests/RomMPlayActionTests.cs @@ -103,7 +103,7 @@ public void Matches_is_false_for_another_emulator_or_profile() [Fact] public void An_action_still_carrying_what_the_plugin_applied_is_unedited() { - var action = new GameAction { Name = "anything", EmulatorId = Mapped, EmulatorProfileId = "p" }; + var action = new GameAction { Name = "anything", IsPlayAction = true, EmulatorId = Mapped, EmulatorProfileId = "p" }; Assert.True(RomMPlayAction.IsUnedited(action, Applied(Mapped, "p"), () => "Dolphin")); } @@ -113,7 +113,7 @@ public void An_action_still_carrying_what_the_plugin_applied_is_unedited() [Fact] public void An_action_repointed_by_the_user_is_not_unedited() { - var action = new GameAction { Name = "Play in Dolphin", EmulatorId = Other, EmulatorProfileId = "q" }; + var action = new GameAction { Name = "Play in Dolphin", IsPlayAction = true, EmulatorId = Other, EmulatorProfileId = "q" }; Assert.False(RomMPlayAction.IsUnedited(action, Applied(Mapped, "p"), () => "Dolphin")); } @@ -123,7 +123,7 @@ public void An_action_repointed_by_the_user_is_not_unedited() [Fact] public void Without_a_recorded_emulator_the_generated_name_marks_the_action_as_the_plugins() { - var action = new GameAction { Name = "Play in Dolphin", EmulatorId = Other }; + var action = new GameAction { Name = "Play in Dolphin", IsPlayAction = true, EmulatorId = Other }; Assert.True(RomMPlayAction.IsUnedited(action, Applied(Guid.Empty, null), () => "Dolphin")); } @@ -134,7 +134,7 @@ public void Without_a_recorded_emulator_the_generated_name_marks_the_action_as_t [InlineData("Play in Dolphin", null)] public void Without_a_recorded_emulator_anything_but_the_generated_name_is_left_alone(string name, string emulatorName) { - var action = new GameAction { Name = name, EmulatorId = Other }; + var action = new GameAction { Name = name, IsPlayAction = true, EmulatorId = Other }; Assert.False(RomMPlayAction.IsUnedited(action, Applied(Guid.Empty, null), () => emulatorName)); } @@ -144,7 +144,7 @@ public void Without_a_recorded_emulator_anything_but_the_generated_name_is_left_ [Fact] public void The_emulator_name_is_not_resolved_when_the_sidecar_records_what_we_applied() { - var action = new GameAction { Name = "Play in Dolphin", EmulatorId = Mapped, EmulatorProfileId = "p" }; + var action = new GameAction { Name = "Play in Dolphin", IsPlayAction = true, EmulatorId = Mapped, EmulatorProfileId = "p" }; var resolved = false; RomMPlayAction.IsUnedited(action, Applied(Mapped, "p"), () => { resolved = true; return "Dolphin"; }); @@ -152,6 +152,29 @@ public void The_emulator_name_is_not_resolved_when_the_sidecar_records_what_we_a Assert.False(resolved); } + // The importer only writes a play action. One the user demoted is theirs: repointing it + // would mark it as a play action again beside the one they chose. + [Fact] + public void An_action_no_longer_marked_as_the_play_action_is_not_unedited() + { + var action = new GameAction { Name = "Play in Dolphin", IsPlayAction = false, EmulatorId = Mapped, EmulatorProfileId = "p" }; + + Assert.False(RomMPlayAction.IsUnedited(action, Applied(Mapped, "p"), () => "Dolphin")); + Assert.False(RomMPlayAction.IsUnedited(action, Applied(Guid.Empty, null), () => "Dolphin")); + } + + // Repointing keeps every field the importer does not own, so arguments the user added for + // one emulator would be passed to the next. + [Fact] + public void An_action_with_argument_overrides_is_not_unedited() + { + var additional = new GameAction { IsPlayAction = true, EmulatorId = Mapped, EmulatorProfileId = "p", AdditionalArguments = "--fullscreen" }; + var overridden = new GameAction { IsPlayAction = true, EmulatorId = Mapped, EmulatorProfileId = "p", OverrideDefaultArgs = true }; + + Assert.False(RomMPlayAction.IsUnedited(additional, Applied(Mapped, "p"), () => "Dolphin")); + Assert.False(RomMPlayAction.IsUnedited(overridden, Applied(Mapped, "p"), () => "Dolphin")); + } + [Fact] public void A_missing_action_is_never_unedited() { diff --git a/RomM.cs b/RomM.cs index 449da42..5cb092b 100644 --- a/RomM.cs +++ b/RomM.cs @@ -364,7 +364,7 @@ public override IEnumerable GetGameMenuItems(GetGameMenuItemsArgs if (Settings.MergeRevisions && game.IsInstalled) { - var gameData = LoadGameData(game); + var gameData = RomMGameData.LoadBySha1(ROMDataPath, sha1, Logger, out _, game.Name); if (gameData?.ROMVersions?.Count > 1) { gameMenuItems.Add(new GameMenuItem @@ -408,7 +408,7 @@ public override IEnumerable GetInstallActions(GetInstallActio yield break; } - gameData = LoadGameData(args.Game); + gameData = RomMGameData.LoadBySha1(ROMDataPath, romMSHA1, Logger, out _, args.Game.Name); if (gameData?.ROMVersions == null || gameData.ROMVersions.Count == 0) { Logger.Error($"{args.Game.Name} has no usable ROM data file; run update game library before installing!"); @@ -637,14 +637,15 @@ private void OnItemUpdated(object sender, ItemUpdatedEventArgs e) DownloadQueueController?.Cancel(newGame.Id); } - if (Settings.KeepRomMSynced == true) + // The importer wrote this change itself; don't push it back. Consumed whether or not + // sync is on, so a suppression set while it is off cannot swallow a later real edit. + if (ignoredGameIds.TryRemove(newGame.Id, out byte _)) { - // The importer wrote the server's own values into this game; don't push them back. - if (ignoredGameIds.TryRemove(newGame.Id, out byte _)) - { - continue; - } + continue; + } + if (Settings.KeepRomMSynced == true) + { if(!RomMGameId.TryParse(newGame.GameId, out int romMId, out string _)) { Logger.Error($"{newGame.Name} GameID is malformed!"); diff --git a/Saves/SaveEmulatorResolver.cs b/Saves/SaveEmulatorResolver.cs index ad93c14..1d1142b 100644 --- a/Saves/SaveEmulatorResolver.cs +++ b/Saves/SaveEmulatorResolver.cs @@ -44,9 +44,6 @@ public static SaveEmulatorResolution Resolve(SaveHandlerRegistry handlers, IEnum .Where(c => c != null && c.Emulator != null) .ToList(); - if (known.Count == 0) - return new SaveEmulatorResolution(); - foreach (var candidate in known) { var handler = handlers?.Find(candidate.Emulator); @@ -64,7 +61,7 @@ public static SaveEmulatorResolution Resolve(SaveHandlerRegistry handlers, IEnum // An emulator is set but unsupported. The first candidate is the one the user would go // looking for, so it is the one the message names. - return new SaveEmulatorResolution { Emulator = known[0].Emulator }; + return new SaveEmulatorResolution { Emulator = known.FirstOrDefault()?.Emulator }; } /// diff --git a/Settings/EmulatorMapping.cs b/Settings/EmulatorMapping.cs index 32d2662..7bbadf1 100644 --- a/Settings/EmulatorMapping.cs +++ b/Settings/EmulatorMapping.cs @@ -108,6 +108,14 @@ public Emulator Emulator _emulator = value; _emulatorId = value.Id; AvailableProfiles = Emulator?.SelectableProfiles; + // A profile belongs to one emulator. Keeping the previous emulator's would pair it + // with this one and leave every game imported through the mapping unable to launch. + if (_emulatorProfileId != null && value.SelectableProfiles?.Any(p => p.Id == _emulatorProfileId) != true) + { + _emulatorProfile = null; + _emulatorProfileId = null; + OnPropertyChanged(nameof(EmulatorProfile)); + } RomMPlatform = new RomMPlatform(); MappingName = value.Name; OnPropertyChanged(); @@ -161,6 +169,18 @@ public string EmulatorProfileId } } + /// Whether an emulator and one of its profiles are picked. + [JsonIgnore] + public bool HasEmulatorProfile => Emulator != null && EmulatorProfile != null; + + /// Whether a RomM platform is picked; unset reads as -1 or an empty platform (Id 0). + [JsonIgnore] + public bool HasRomMPlatform => RomMPlatformId > 0 && RomMPlatform != null && RomMPlatform.Id > 0; + + /// Whether the import controller will run this mapping, given it is enabled. + [JsonIgnore] + public bool IsImportable => HasEmulatorProfile && HasRomMPlatform; + // (Deprecated) DON'T USE [JsonIgnore] public Platform Platform From 5d5c8ef73c75f9e8f05f3a8d3f9925755f326f68 Mon Sep 17 00:00:00 2001 From: Georges-Antoine Assi Date: Thu, 24 Sep 2026 11:02:41 -0400 Subject: [PATCH 4/4] Keep save sync on the launched emulator and guard action ownership - The mapping no longer stands in for a play action naming a different emulator: that emulator is what launches, so syncing another one's saves would download where it never reads. The mapping now only fills in when the action names no emulator, or lends a profile to that same emulator; the unsupported message names the mapping's emulator when save sync supports it. - Save sync ignores the sidecar's mapping on a platform two importable mappings cover, since it only records whichever import ran last. The rival check is shared as Settings.HasRivalMapping. - An action already matching the mapping is only recorded as plugin-applied if the plugin owned it before, so a user's matching choice is not repointed on a later mapping change. Co-Authored-By: Claude Opus 5.5 (1M context) --- Games/RomMImport.cs | 15 +++++---- RomM.Tests/SaveEmulatorResolverTests.cs | 44 ++++++++++++++++++++++--- Saves/SaveEmulatorResolver.cs | 33 +++++++++++++++---- Saves/SaveSyncService.cs | 19 +++++++---- Settings/Settings.cs | 8 +++++ 5 files changed, 96 insertions(+), 23 deletions(-) diff --git a/Games/RomMImport.cs b/Games/RomMImport.cs index 0acb213..919b3d1 100644 --- a/Games/RomMImport.cs +++ b/Games/RomMImport.cs @@ -70,8 +70,7 @@ public RomMImport(RomM plugin, LibraryImportGamesArgs args, EmulatorMapping mapp // Only mappings the import controller will actually run count: one it skips as // misconfigured writes nothing, so it cannot fight this one over the actions. - _platformHasRivalMapping = plugin.Settings?.Mappings? - .Count(m => m.Enabled && m.IsImportable && m.RomMPlatformId == mapping.RomMPlatformId) > 1; + _platformHasRivalMapping = plugin.Settings?.HasRivalMapping(mapping) == true; _mapped = new AppliedPlayAction(mapping.EmulatorId, mapping.EmulatorProfileId); } @@ -480,9 +479,16 @@ private bool RefreshPlayAction(Game game, RomMRomLocal previous, out AppliedPlay if (action == null || _platformHasRivalMapping) return false; + Func actionEmulatorName = () => + _plugin.Playnite.Database.Emulators?.Get(action.EmulatorId)?.Name; + + // Already in step. It only becomes ours to record if it was ours before: a user who set + // this emulator themselves and then pointed the mapping at the same one still owns it, + // and must not have it repointed when the mapping moves on. if (RomMPlayAction.Matches(action, _mapped)) { - result = _mapped; + if (RomMPlayAction.IsUnedited(action, applied, actionEmulatorName)) + result = _mapped; return false; } @@ -493,9 +499,6 @@ private bool RefreshPlayAction(Game game, RomMRomLocal previous, out AppliedPlay if (applied.EmulatorId == Guid.Empty && action.EmulatorId == _mapped.EmulatorId) return false; - Func actionEmulatorName = () => - _plugin.Playnite.Database.Emulators?.Get(action.EmulatorId)?.Name; - if (!RomMPlayAction.IsUnedited(action, applied, actionEmulatorName)) { _plugin.Logger.Info($"[Importer] Leaving {game.Name}'s play action pointed at " + diff --git a/RomM.Tests/SaveEmulatorResolverTests.cs b/RomM.Tests/SaveEmulatorResolverTests.cs index bf23328..2123a02 100644 --- a/RomM.Tests/SaveEmulatorResolverTests.cs +++ b/RomM.Tests/SaveEmulatorResolverTests.cs @@ -41,10 +41,11 @@ public void The_play_action_wins_when_its_emulator_is_supported() Assert.NotNull(resolution.Handler); } - // The reported bug: the action is a snapshot from import, so a mapping later repointed at - // RetroArch has to be consulted rather than the sync giving up on the stale emulator. + // The action is what launches, so syncing the mapping's emulator in its place would download + // where the launched emulator never reads. It is reported as unsupported, naming the + // mapping's emulator so the user knows what to point the action at. [Fact] - public void The_mapping_is_used_when_the_play_actions_emulator_is_unsupported() + public void The_mapping_does_not_replace_an_unsupported_play_action_emulator() { var fromMapping = RetroArch(); @@ -54,6 +55,40 @@ public void The_mapping_is_used_when_the_play_actions_emulator_is_unsupported() FromMapping(fromMapping), }); + Assert.Equal("Dolphin", resolution.Emulator?.Name); + Assert.Null(resolution.Handler); + Assert.Same(fromMapping, resolution.PassedOverMappingEmulator); + } + + // Nor does it replace a supported one the user chose. + [Fact] + public void The_mapping_is_not_passed_over_when_the_action_names_the_same_emulator() + { + var id = Guid.NewGuid(); + + var resolution = SaveEmulatorResolver.Resolve(Handlers, new[] + { + FromAction(RetroArch(id)), + FromMapping(RetroArch(id)), + }); + + Assert.False(resolution.FromMapping); + Assert.Null(resolution.PassedOverMappingEmulator); + } + + // With no emulator on the action there is nothing launched to contradict, so the mapping + // is the best answer there is. + [Fact] + public void The_mapping_is_used_when_the_play_action_names_no_emulator() + { + var fromMapping = RetroArch(); + + var resolution = SaveEmulatorResolver.Resolve(Handlers, new[] + { + FromAction(null), + FromMapping(fromMapping), + }); + Assert.Same(fromMapping, resolution.Emulator); Assert.True(resolution.FromMapping); Assert.NotNull(resolution.Handler); @@ -62,7 +97,7 @@ public void The_mapping_is_used_when_the_play_actions_emulator_is_unsupported() // Reported as unsupported rather than as "no emulator": the two need different advice, and // the emulator named is the one the user would go looking for. [Fact] - public void No_supported_candidate_reports_the_first_emulator_as_unsupported() + public void An_unsupported_play_action_emulator_is_reported_as_unsupported() { var resolution = SaveEmulatorResolver.Resolve(Handlers, new[] { @@ -72,6 +107,7 @@ public void No_supported_candidate_reports_the_first_emulator_as_unsupported() Assert.Equal("Dolphin", resolution.Emulator?.Name); Assert.Null(resolution.Handler); + Assert.Null(resolution.PassedOverMappingEmulator); } [Fact] diff --git a/Saves/SaveEmulatorResolver.cs b/Saves/SaveEmulatorResolver.cs index 1d1142b..98dc782 100644 --- a/Saves/SaveEmulatorResolver.cs +++ b/Saves/SaveEmulatorResolver.cs @@ -25,16 +25,24 @@ internal class SaveEmulatorResolution public EmulatorProfile Profile { get; set; } public ISaveHandler Handler { get; set; } public bool FromMapping { get; set; } + + /// + /// The mapping's emulator when it is supported but was passed over because the game launches + /// another one. Set so the "unsupported" advice can point at it; never a sync target. + /// + public Emulator PassedOverMappingEmulator { get; set; } } /// /// Picks the emulator a game's saves belong to, from the candidates in preference order. /// - /// The play action leads: a user who repoints it at another emulator should have their saves - /// follow it. But the action is only a snapshot of the mapping taken at import, so when it - /// names an emulator no handler covers, the platform's own emulator mapping -- which the - /// settings screen presents as the thing that decides this -- gets its turn before sync gives - /// up. Kept free of Playnite lookups so the preference order can be tested on its own. + /// The play action decides: it is what Playnite launches, so its emulator is the one that reads + /// and writes the saves. Syncing another emulator's saves in its place would download where the + /// launched emulator never reads and upload a save it never wrote, so a mapping naming a + /// different emulator is never a fallback -- a stale action is fixed by the importer repointing + /// it instead. The mapping only answers when the action names no emulator at all, or lends its + /// profile to that same emulator. Kept free of Playnite lookups so the rules can be tested on + /// their own. /// internal static class SaveEmulatorResolver { @@ -44,6 +52,15 @@ public static SaveEmulatorResolution Resolve(SaveHandlerRegistry handlers, IEnum .Where(c => c != null && c.Emulator != null) .ToList(); + var launched = known.FirstOrDefault(c => !c.FromMapping)?.Emulator; + Emulator passedOver = null; + if (launched != null) + { + passedOver = known.FirstOrDefault(c => c.FromMapping && c.Emulator.Id != launched.Id + && handlers?.Find(c.Emulator) != null)?.Emulator; + known = known.Where(c => c.Emulator.Id == launched.Id).ToList(); + } + foreach (var candidate in known) { var handler = handlers?.Find(candidate.Emulator); @@ -61,7 +78,11 @@ public static SaveEmulatorResolution Resolve(SaveHandlerRegistry handlers, IEnum // An emulator is set but unsupported. The first candidate is the one the user would go // looking for, so it is the one the message names. - return new SaveEmulatorResolution { Emulator = known.FirstOrDefault()?.Emulator }; + return new SaveEmulatorResolution + { + Emulator = known.FirstOrDefault()?.Emulator, + PassedOverMappingEmulator = passedOver, + }; } /// diff --git a/Saves/SaveSyncService.cs b/Saves/SaveSyncService.cs index 1d94fb2..4aebebf 100644 --- a/Saves/SaveSyncService.cs +++ b/Saves/SaveSyncService.cs @@ -460,12 +460,17 @@ private SaveTarget ResolveTarget(Game game, out string reason) Logger.Info($"[SaveSync] No save handler for emulator '{resolution.Emulator.Name}', skipping {game.Name}."); reason = $"Save sync does not support {resolution.Emulator.Name} yet. " + $"Supported: {SupportedEmulators}."; + if (resolution.PassedOverMappingEmulator != null) + { + reason += $" Its RomM platform mapping uses {resolution.PassedOverMappingEmulator.Name}: " + + "point the game's play action at it to sync its saves."; + } return null; } if (resolution.FromMapping) { - Logger.Info($"[SaveSync] {game.Name}'s play action names no emulator save sync supports; " + + Logger.Info($"[SaveSync] {game.Name}'s play action names no emulator; " + $"using {resolution.Emulator.Name} from its RomM platform mapping instead."); } @@ -487,10 +492,9 @@ private SaveTarget ResolveTarget(Game game, out string reason) } /// - /// The play action leads -- a user who repoints it at another emulator should have their - /// saves follow it -- but it is only a snapshot of the emulator mapping taken at import, so - /// the mapping gets its turn when the action names an emulator no handler covers. See - /// for the rules; this half is only the Playnite lookups. + /// The play action decides, since it is what launches; the mapping fills in when the action + /// names no emulator or no profile. See for the rules; + /// this half is only the Playnite lookups. /// private SaveEmulatorResolution ResolveEmulator(Game game) { @@ -512,11 +516,12 @@ private SaveEmulatorResolution ResolveEmulator(Game game) // Reading the mapping means reading the ROM's sidecar off disk, and this runs on the // pre-launch path, so it is skipped when the action can answer on its own -- which it // cannot if its emulator is unsupported, or if it names no profile for the handler to - // take the core from. + // take the core from. A platform two mappings cover records whichever import ran last, + // which says nothing about this game, so its mapping is not consulted at all. if (fromAction?.Profile == null || _handlers.Find(fromAction.Emulator) == null) { var mapping = _romM.MappingFor(game); - if (mapping != null) + if (mapping != null && !Settings.HasRivalMapping(mapping)) { candidates.Add(new SaveEmulatorCandidate { diff --git a/Settings/Settings.cs b/Settings/Settings.cs index 31c68dd..b2983d2 100644 --- a/Settings/Settings.cs +++ b/Settings/Settings.cs @@ -279,6 +279,14 @@ public string ExcludeGenres /// The mapping with this id, or null. The link a game keeps to its mapping. internal EmulatorMapping MappingById(Guid id) => Mappings?.FirstOrDefault(x => x.MappingId == id); + /// + /// Whether another mapping the importer will actually run covers the same RomM platform. + /// Both walk the same ROMs and each records itself as the ROM's mapping, so for such a + /// platform the sidecar's mapping is only whichever pass ran last. + /// + internal bool HasRivalMapping(EmulatorMapping mapping) => + mapping != null && Mappings?.Count(m => m.Enabled && m.IsImportable && m.RomMPlatformId == mapping.RomMPlatformId) > 1; + public List RomMPlatforms { get => _romMPlatforms;