Sync Status and Favorites between RomM and Playnite #89
Conversation
…nChange from playnite to romm
merge sync userdata
Summary of ChangesHello @dravorle, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a significant feature to enable seamless bi-directional synchronization of game favorites and completion statuses between RomM and Playnite. It provides users with a new setting to control this sync, ensuring that game metadata remains consistent across both platforms. The implementation involves new API calls to RomM and event handling within Playnite to capture and apply changes. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a feature to synchronize game status and favorites between RomM and Playnite. The implementation adds a setting to enable this bi-directional sync. Data is synced from RomM during library updates and to RomM via an event handler for game item updates in Playnite.
The changes are well-structured, but there are several areas for improvement regarding correctness, performance, and robustness. My review highlights a critical bug that could cause a crash, potential race conditions leading to data loss, and inefficient code patterns that should be refactored. Addressing these points will significantly improve the quality and reliability of the new synchronization feature.
| Favorite = favorites.Exists(f => f == item.Id), | ||
| LastActivity = item.RomUser.LastPlayed, | ||
| UserScore = item.RomUser.Rating * 10, //RomM-Rating is 1-10, Playnite 1-100, so it can unfortunately only by synced one direction without loosing decimals | ||
| CompletionStatus = new MetadataNameProperty(PlayniteApi.Database.CompletionStatuses.FirstOrDefault(cs => cs.Name == completionStatus).Name) ?? null, |
There was a problem hiding this comment.
This line will throw a NullReferenceException if FirstOrDefault returns null (i.e., no completion status with the given name is found). Accessing .Name on a null reference will crash the import process. This is a critical bug.
You should handle this case gracefully. For example:
var status = PlayniteApi.Database.CompletionStatuses.FirstOrDefault(cs => cs.Name == completionStatus);
var completionStatusProperty = status != null ? new MetadataNameProperty(status.Name) : null;
// ... then in the metadata object initializer:
CompletionStatus = completionStatusProperty,| internal async void UpdateFavorites(RomMCollection favoriteCollection, List<int> romIds) | ||
| { | ||
| if (favoriteCollection == null) | ||
| { | ||
| Logger.Error($"Can't update favorites, collection is null"); | ||
| return; | ||
| } | ||
|
|
||
| string apiCollectionUrl = CombineUrl(Settings.RomMHost, "api/collections"); | ||
| try | ||
| { | ||
| var formData = new MultipartFormDataContent(); | ||
| formData.Add(new StringContent(JsonConvert.SerializeObject(romIds)), "rom_ids"); | ||
| HttpResponseMessage putResponse = HttpClientSingleton.Instance.PutAsync($"{apiCollectionUrl}/{favoriteCollection.Id}", formData).GetAwaiter().GetResult(); | ||
| putResponse.EnsureSuccessStatusCode(); | ||
| } | ||
| catch (HttpRequestException e) | ||
| { | ||
| Logger.Error($"Request exception: {e.Message}"); | ||
| } | ||
| } |
There was a problem hiding this comment.
This method is async void, which should generally be avoided except for event handlers. It makes error handling and awaiting completion impossible for the caller. It should be changed to return async Task.
Additionally, this method and others (FetchFavorites, CreateFavorites, Games_ItemUpdated) use blocking calls like .GetAwaiter().GetResult() on asynchronous HTTP calls. This can lead to deadlocks and should be replaced with await.
Example for this method:
internal async Task UpdateFavoritesAsync(RomMCollection favoriteCollection, List<int> romIds)
{
if (favoriteCollection == null)
{
Logger.Error($"Can't update favorites, collection is null");
return;
}
string apiCollectionUrl = CombineUrl(Settings.RomMHost, "api/collections");
try
{
var formData = new MultipartFormDataContent();
formData.Add(new StringContent(JsonConvert.SerializeObject(romIds)), "rom_ids");
HttpResponseMessage putResponse = await HttpClientSingleton.Instance.PutAsync($"{apiCollectionUrl}/{favoriteCollection.Id}", formData);
putResponse.EnsureSuccessStatusCode();
}
catch (HttpRequestException e)
{
Logger.Error($"Request exception: {e.Message}");
// Consider re-throwing or returning a result to indicate failure
}
}You would then need to await the call to this method in Games_ItemUpdated.
| try | ||
| { | ||
| ignoredGameIds.TryAdd(game.Id, 0); | ||
| Playnite.Database.Games.Update(game); | ||
| } | ||
| finally | ||
| { | ||
| Task.Delay(500).ContinueWith(_t => ignoredGameIds.TryRemove(game.Id, out _)); | ||
| } |
There was a problem hiding this comment.
The use of Task.Delay(500) to temporarily ignore game updates introduces a race condition. If a user modifies a game within this 500ms window, that change will be ignored by the Games_ItemUpdated event handler because the game's ID will be in ignoredGameIds. This will prevent the user's change from being synced to RomM, leading to data inconsistency.
The 500ms delay is arbitrary and doesn't guarantee correctness. A more robust, non-time-based mechanism should be used to prevent sync loops while not dropping user-initiated changes. One possible approach is to have the Games_ItemUpdated handler remove the ID from ignoredGameIds if it finds it, rather than using a delayed task.
|
|
||
| internal RomMCollection CreateFavorites() | ||
| { | ||
| string apiCollectionUrl = CombineUrl(Settings.RomMHost, "api/collections?is_favorite=true&is_public=false"); |
There was a problem hiding this comment.
The query parameter here is is_favorite=true (snake_case), while in FetchFavorites (line 110) it is isFavorite=true (camelCase). This inconsistency might indicate a bug. Please verify with the RomM API documentation which format is correct for both endpoints to ensure consistent behavior.
| { | ||
| backlogged = status == "Plan to Play", | ||
| now_playing = status == "Playing", | ||
| status = RomMRomUser.CompletionStatusMap.FirstOrDefault((kv) => kv.Value == status && kv.Value != "Playing" && kv.Value != "Plan to Play" && kv.Value != "Not Played").Key |
There was a problem hiding this comment.
Using FirstOrDefault for a reverse lookup on the CompletionStatusMap is inefficient, especially within an event handler that could be triggered frequently. A more performant and readable approach is to create a pre-computed reverse dictionary.
Consider adding a reverse map to RomMRomUser:
// In RomMRomUser.cs
public static readonly Dictionary<string, string> PlayniteToRomMStatusMap =
CompletionStatusMap.ToDictionary(kv => kv.Value, kv => kv.Key);Then you can simplify this line to a more efficient lookup. The current filtering logic (kv.Value != "Playing" && ...) is complex and seems to exclude valid statuses like "Not Played", which may be a bug. Using a reverse map would simplify this greatly.
| if (Settings.KeepRomMSynced == true) | ||
| { | ||
| game.Favorite = favorites.Exists(f => f == item.Id); | ||
| game.CompletionStatusId = Playnite.Database.CompletionStatuses.FirstOrDefault(cs => cs.Name == completionStatus).Id; |
There was a problem hiding this comment.
Calling FirstOrDefault on Playnite.Database.CompletionStatuses inside a loop (foreach (var item in allRoms)) is inefficient and can lead to performance degradation when processing a large number of games.
It's better to fetch and map the completion statuses to a dictionary once before the loop for O(1) lookups.
Example:
// Before the loop
var completionStatusMap = Playnite.Database.CompletionStatuses.ToDictionary(cs => cs.Name, cs => cs.Id);
// Inside the loop
if (completionStatusMap.TryGetValue(completionStatus, out var statusId))
{
game.CompletionStatusId = statusId;
}
|
Personally I would add some code to GetGames at about line 551 as you are adding a new member to the gameID so that users don't have to reimport there entire library, something like this:
Although the |
|
Good Point! I will check the AI responses tomorrow and think about how to better store the id, maybe we don't need to use in the RomMGameInfo. But it would be useful. |
|
No problem, I noticed when I was doing some metadata changes of my own that when I edited the HasMultipleFiles member that it causes duplicates to appear as it has a different ID, the only thing I haven't solved is why updating the ID seems to remove the activity data |
2 participants
Quick Overview
This PR addresses introduces a toggle that tries to keep RomM Status and favorites in sync with playnite and vice versa. It does so by applying data from RomM whenever a library update is performed (on start or pressing F5, ...) and syncing data to RomM by subscribing to the
ItemUpdatedevent from playnite.This implements #59
Caveats