Check Downloaded and Drag & Dropped Mods for Targetted Game and Warn If None #769
Conversation
…ere no compatible applications are found
TheBestAstroNOT
changed the title
|
Only works for drag and dropping right now, working on support for mods installed via the R2 protocol (it isn't going well) |
|
PR for #751 |
|
I also made these two mods to test it https://github.com/TheBestAstroNOT/psychic-happiness/releases/tag/1.0.0 |
|
@dreamsyntax are there any changes you would like to see? |
|
Ok, now I've properly tested this build. Drag & Drop "No" instead of "Cancel" makes more sense, since the mod itself still gets installed, we just don't set the supported app. Ideally there would be three options Since this happens after a download for example though, I think its fine to just change it to -- Mod Loader Download Button
R2 Links (Gamebanana)
|
|
@dreamsyntax Can you test the mod loader's one click download button again, I'd do it myself but I don't know of any mods that I can test with. |
I'll try later today. If you want to test in interim: If you add any exe renamed as tsonic_win.exe, it will add Sonic Heroes as a gamebanana source. You'll be able to see 64 Mario skin for that game in the download manager, which has the issue. |
1 similar comment
I'll try later today. If you want to test in interim: If you add any exe renamed as tsonic_win.exe, it will add Sonic Heroes as a gamebanana source. You'll be able to see 64 Mario skin for that game in the download manager, which has the issue. |
Ok it seems to be working
|
| public XamlResourceMessageBoxOkCancel(string titleResourceName, string descriptionResourceName, string okButtonTextResourceName, string cancelButtonTextResourceName) : base(new XamlResource<string>(titleResourceName).Get(), new XamlResource<string>(descriptionResourceName).Get()) | ||
| public XamlResourceMessageBoxOkCancel(string title, string message, string okText, string cancelText) : base(title, message) | ||
| { | ||
| this.CancelBtn.Content = Lib.Static.Resources.ResourceProvider.Get<string>(cancelButtonTextResourceName); | ||
| this.OKBtn.Content = Lib.Static.Resources.ResourceProvider.Get<string>(okButtonTextResourceName); |
There was a problem hiding this comment.
Unless I'm missing something, wouldn't this replace every instance of Ok/Cancel to Yes/No?
There was a problem hiding this comment.
Uhh no? You are passing the text every time you call it. Plus other message boxes use another delegate to display their text. Only I'm using this one.
|
Tested. Feature works as expected now. @Sewer56 on any objections with the code? I left a comment about the resource, the rest looks fine. |
|
Bump |
|
Looks reasonable to me. Just doing all sorts of different things as always :p |
|
@coderabbitai full-review |
|
✅ Actions performedFull review triggered. |
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughThis pull request refactors mod configuration handling and adds validation for mod-application compatibility. The EditModDialogViewModel constructor is updated to accept collections directly instead of service instances. A new ModValidationHelper utility is introduced to validate mod-app compatibility and prompt users to configure supported applications when necessary. Compatibility validation is integrated into multiple mod installation flows: package downloads, startup initialization, and drag-drop operations. New UI resources and dialog strings (Yes, No, configuration prompts) are added to support user interactions. The XamlResourceMessageBoxOkCancel dialog is refactored to accept string parameters directly rather than resource keys. Changes are documented in the changelog template. 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
source/Reloaded.Mod.Launcher.Lib/Models/ViewModel/DownloadPackagesViewModel.cs (1)
302-312: Race condition:ForceRefresh()may execute before download completes.
ExecuteWithApplicationDispatcherdispatches the async download operation but doesn't wait for it to complete. TheForceRefresh()call on line 312 executes immediately, potentially before the mod files are written to disk. This would cause the new-mod detection logic to find nothing.Consider awaiting the download completion before refreshing:
🔧 Suggested approach
- ActionWrappers.ExecuteWithApplicationDispatcher(async () => - { - var viewModel = new DownloadPackageViewModel(_package!, IoC.Get<LoaderConfig>()); - var dl = viewModel.StartDownloadAsync(); // Fire and forget. - Actions.ShowFetchPackageDialog(viewModel); - - await dl; - await Update.ResolveMissingPackagesAsync(); - }); - - modConfigService.ForceRefresh(); + ActionWrappers.ExecuteWithApplicationDispatcher(async () => + { + var viewModel = new DownloadPackageViewModel(_package!, IoC.Get<LoaderConfig>()); + var dl = viewModel.StartDownloadAsync(); + Actions.ShowFetchPackageDialog(viewModel); + + await dl; + await Update.ResolveMissingPackagesAsync(); + + modConfigService.ForceRefresh(); + // Move the new-mod detection logic inside this async block + });source/Reloaded.Mod.Launcher.Lib/Models/ViewModel/Dialog/EditModDialogViewModel.cs (1)
86-109: Snapshot app/mod collections before iterating to avoid concurrent-modification exceptions.
ObservableCollection<T>throws if modified during enumeration. With the new download/drag-drop flow, these collections can change while the dialog is building, so aliasing the collection doesn’t protect you. Take a snapshot before iterating.✅ Suggested fix (snapshot + null-safety)
- var apps = appItems; + var apps = appItems?.ToList() ?? new List<PathTuple<ApplicationConfig>>(); foreach (var app in apps) { bool isAppEnabled = modTuple.Config.SupportedAppId.Contains(app.Config.AppId, StringComparer.OrdinalIgnoreCase); Applications.Add(new BooleanGenericTuple<IApplicationConfig>(isAppEnabled, app.Config)); } - var mods = modsItems; // In case collection changes during window open. + var mods = modsItems?.ToList() ?? new List<PathTuple<ModConfig>>(); // Snapshot to avoid concurrent modification. foreach (var mod in mods) { bool isModEnabled = modTuple.Config.ModDependencies.Contains(mod.Config.ModId, StringComparer.OrdinalIgnoreCase); var dep = new BooleanGenericTuple<IModConfig>(isModEnabled, mod.Config); Dependencies.Add(dep);
🤖 Fix all issues with AI agents
In `@source/Reloaded.Mod.Launcher.Lib/Static/Resources.cs`:
- Around line 229-237: Update the version placeholder in the comment above the
new resource properties to the actual release version and confirm the new
resource properties are approved; specifically change the comment "Update
1.XX.XX: ..." in Resources.cs (class Resources) to the real version string and
keep the added IDictionaryResource<string> properties NoAppsInConfigTitle,
NoAppsInConfigDescription, NoCompatibleAppsInConfigTitle,
NoCompatibleAppsInConfigDescription, AppSelectionQuestion, Yes, and No as-is to
be approved for inclusion.
In `@source/Reloaded.Mod.Launcher/Assets/Languages/en-GB.xaml`:
- Around line 749-752: Update the version placeholder in the XML comment that
currently reads "Update 1.XX.X: Uhh no strings for Yes and No?" to the actual
release version (e.g., "Update 1.2.3") before merging; the comment sits
immediately above the resource entries with keys "Yes" and "No" (sys:String
x:Key="Yes" and sys:String x:Key="No") so replace "1.XX.X" with the real version
string in that comment.
♻️ Duplicate comments (1)
source/Reloaded.Mod.Launcher/MainWindow.xaml.cs (1)
136-169: Consider extracting duplicated validation logic into a shared helper.This mod-validation and prompt logic (check
IsUniversalMod, checkSupportedAppId, prompt for app selection) is duplicated inDownloadPackagesViewModel.csandStartup.cs. Extracting it into a shared helper method would improve maintainability and ensure consistent behavior across all mod installation paths.Additionally, this file creates
EditModDialogdirectly, whileStartup.csandDownloadPackagesViewModel.csuseActions.EditModDialog(...). Consider using a consistent pattern.
🧹 Nitpick comments (5)
source/Reloaded.Mod.Launcher.Lib/Models/ViewModel/DownloadPackagesViewModel.cs (2)
331-334: Unused variable assignment.The
createModDialogvariable is assigned but never read. The return value ofActions.EditModDialogis discarded.♻️ Suggested fix
- var createModDialog = Actions.EditModDialog(viewmodel, null); + Actions.EditModDialog(viewmodel, null);
342-345: Unused variable assignment.Same issue as above -
createModDialogis assigned but never used.♻️ Suggested fix
- var createModDialog = Actions.EditModDialog(viewmodel, null); + Actions.EditModDialog(viewmodel, null);source/Reloaded.Mod.Launcher.Lib/Startup.cs (2)
152-155: Unused variable assignment.The
createModDialogvariable is assigned but never read.♻️ Suggested fix
- var createModDialog = Actions.EditModDialog(viewmodel, null); + Actions.EditModDialog(viewmodel, null);
163-166: Unused variable assignment.Same issue -
createModDialogis assigned but never used.♻️ Suggested fix
- var createModDialog = Actions.EditModDialog(viewmodel, null); + Actions.EditModDialog(viewmodel, null);source/Reloaded.Mod.Launcher.Lib/Models/ViewModel/Dialog/EditModDialogViewModel.cs (1)
101-108: Consider tracking known app IDs in a HashSet to avoid repeated linear scans.
Applications.Any(...)inside nested loops can get expensive with many mods/apps. AHashSet<string>(case-insensitive) keeps this O(1) and simplifies the duplicate check.♻️ Example refactor
- foreach (var app in apps) + var knownAppIds = new HashSet<string>(StringComparer.OrdinalIgnoreCase); + foreach (var app in apps) { bool isAppEnabled = modTuple.Config.SupportedAppId.Contains(app.Config.AppId, StringComparer.OrdinalIgnoreCase); Applications.Add(new BooleanGenericTuple<IApplicationConfig>(isAppEnabled, app.Config)); + knownAppIds.Add(app.Config.AppId); } ... foreach (var appId in mod.Config.SupportedAppId) { - if (!Applications.Any(x => x.Generic.AppId.Equals(appId, StringComparison.OrdinalIgnoreCase))) + if (!knownAppIds.Contains(appId)) { bool isAppEnabled = modTuple.Config.SupportedAppId.Contains(appId, StringComparer.OrdinalIgnoreCase); Applications.Add(new BooleanGenericTuple<IApplicationConfig>(isAppEnabled, new UnknownApplicationConfig(appId))); + knownAppIds.Add(appId); } }
Extract duplicated validation logic from MainWindow.xaml.cs, Startup.cs, and DownloadPackagesViewModel.cs into ModValidationHelper.ValidateModAppCompatibility(). This consolidates ~90 lines of repeated code into a single reusable method, improving maintainability and ensuring consistent behavior across all mod installation paths (drag-drop, download, and protocol handler).
|
@coderabbitai full-review |
|
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
source/Reloaded.Mod.Launcher.Lib/Models/ViewModel/DownloadPackagesViewModel.cs (1)
302-324: Remove unusednewConfigsvariable.The
newConfigslist (line 313) is created and populated in the loop (lines 314-324) but never used afterward. Either use it for a specific purpose or remove it.Additionally, the async download operation does complete before
ForceRefresh()is called—ExecuteWithApplicationDispatcherusesSynchronizationContext.Send()which blocks until the async lambda completes, including the awaitedResolveMissingPackagesAsync()call. The code structure is correct in this regard.
🤖 Fix all issues with AI agents
In `@source/Reloaded.Mod.Launcher.Lib/Utility/ModValidationHelper.cs`:
- Around line 55-57: The call to Actions.DisplayResourceMessageBoxOkCancel is
using the null-forgiving operator and can cause a NullReferenceException; update
ModValidationHelper to check Actions.DisplayResourceMessageBoxOkCancel for null
before invoking it (e.g., if (Actions.DisplayResourceMessageBoxOkCancel == null)
handle fallback/throw a clear exception or use a safe default behavior), then
call it to set loadAppPage; reference the delegate by name
(Actions.DisplayResourceMessageBoxOkCancel) and ensure loadAppPage is assigned
only after the null check to avoid runtime null dereference.
🧹 Nitpick comments (1)
source/Reloaded.Mod.Launcher.Lib/Models/ViewModel/DownloadPackagesViewModel.cs (1)
297-298: Redundant service retrieval:_modConfigServiceis already available.The
DownloadPackageCommandclass already receivesModConfigServicevia its constructor and stores it in_modConfigService(line 257). UsingIoC.GetConstant<ModConfigService>()here is redundant and could potentially return a different instance.Suggested fix
- var modConfigService = IoC.GetConstant<ModConfigService>(); - var modsBefore = new Dictionary<string, PathTuple<ModConfig>>(modConfigService.ItemsById); + var modsBefore = new Dictionary<string, PathTuple<ModConfig>>(_modConfigService.ItemsById);And use
_modConfigServiceconsistently throughout the method.
3 participants
No description provided.