Skip to content

Conversation

@shananas
Copy link
Collaborator

@shananas shananas commented Dec 30, 2025

i probably shouldnt access the ConfigurationService variable directly and store it somewhere in setup wizard but
also the default is now emulator I dont trust someone wont see all the info on the PC option and not switch to emulator if using emulator

Summary by CodeRabbit

  • Bug Fixes

    • Fixed configuration initialization by setting a proper default value for game edition.
  • New Features

    • Added conditional visibility control for the OpenKH Game Engine option, which is now dynamically displayed based on development mode settings.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link

coderabbitai bot commented Dec 30, 2025

📝 Walkthrough

Walkthrough

Changes add a visibility control feature to the setup wizard UI by introducing a new ViewModel property that conditionally displays a ComboBox item based on development mode, and update the default GameEdition configuration value to 1.

Changes

Cohort / File(s) Summary
Configuration Default
OpenKh.Tools.ModsManager/Services/ConfigurationService.cs
Sets default value of 1 for Config.GameEdition during deserialization, ensuring a fallback when the property is absent in configuration data.
UI Visibility Feature
OpenKh.Tools.ModsManager/ViewModels/SetupWizardViewModel.cs, OpenKh.Tools.ModsManager/Views/SetupWizardWindow.xaml
Introduces new OpenKhGameEngineVisible property that exposes visibility state dependent on ConfigurationService.DevView flag, and binds it to the "OpenKH Game Engine" ComboBox item to conditionally display the option in development mode only.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

Suggested reviewers

  • Delta-473
  • kenjiuno

Poem

A rabbit hops with glee so bright, 🐰
New visibility bindings set just right,
GameEdition defaults to one, you see,
DevView controls what shown will be!
Config and UI dance in harmony tonight.

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and specifically describes the main objective of the PR: hiding the OpenKh Game Engine option in the setup wizard based on Dev View status.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
✨ Finishing touches
  • 📝 Generate docstrings

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 765df71 and ba43084.

📒 Files selected for processing (3)
  • OpenKh.Tools.ModsManager/Services/ConfigurationService.cs
  • OpenKh.Tools.ModsManager/ViewModels/SetupWizardViewModel.cs
  • OpenKh.Tools.ModsManager/Views/SetupWizardWindow.xaml
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-11-21T23:23:49.389Z
Learnt from: shananas
Repo: OpenKH/OpenKh PR: 1226
File: OpenKh.Tools.ModsManager/ViewModels/SetupWizardViewModel.cs:0-0
Timestamp: 2025-11-21T23:23:49.389Z
Learning: In OpenKh.Tools.ModsManager/ViewModels/SetupWizardViewModel.cs, the PageRegion wizard page is specific to KH2 PS2 ISO only. The WizardPageAfterGameData should only be set to PageRegion when IsoLocationKH2 is not null, and should skip this page for KH1 and Recom ISOs.

Applied to files:

  • OpenKh.Tools.ModsManager/ViewModels/SetupWizardViewModel.cs
  • OpenKh.Tools.ModsManager/Services/ConfigurationService.cs
  • OpenKh.Tools.ModsManager/Views/SetupWizardWindow.xaml
📚 Learning: 2025-11-21T23:50:34.439Z
Learnt from: Some1fromthedark
Repo: OpenKH/OpenKh PR: 1226
File: OpenKh.Tools.ModsManager/Views/SetupWizardWindow.xaml:153-175
Timestamp: 2025-11-21T23:50:34.439Z
Learning: In the OpenKH codebase, the game identifier for Kingdom Hearts Re:Chain of Memories uses PascalCase "Recom" (not lowercase "recom") as the established standard throughout the codebase, including in GameService.cs, MainViewModel.cs, and XAML CommandParameter bindings. This differs from the lowercase convention used for "kh1" and "kh2", but "Recom" is the consistent identifier for this game across the project.

Applied to files:

  • OpenKh.Tools.ModsManager/ViewModels/SetupWizardViewModel.cs
🧬 Code graph analysis (1)
OpenKh.Tools.ModsManager/ViewModels/SetupWizardViewModel.cs (1)
OpenKh.Tools.ModsManager/Services/ConfigurationService.cs (2)
  • ConfigurationService (12-617)
  • ConfigurationService (109-149)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
  • GitHub Check: Analyze (csharp)
  • GitHub Check: build
🔇 Additional comments (2)
OpenKh.Tools.ModsManager/Services/ConfigurationService.cs (1)

33-33: LGTM! Appropriate default for non-developer users.

Setting the default value to 1 (PCSX2) ensures that new users who haven't configured the application will default to PCSX2 rather than the OpenKH Game Engine option. This aligns well with the PR objective of hiding the experimental OpenKH Game Engine from non-developers.

OpenKh.Tools.ModsManager/ViewModels/SetupWizardViewModel.cs (1)

228-228: Implementation looks good and follows existing patterns.

The OpenKhGameEngineVisible property correctly returns the appropriate visibility state based on DevView. While you mentioned in the PR description that you "probably should access the ConfigurationService variable directly and store it somewhere in setup wizard," the current implementation is actually consistent with other visibility properties in this class (e.g., OpenKhGameEngineConfigVisibility at line 366, Pcsx2ConfigVisibility at line 380).

One note: This property doesn't raise OnPropertyChanged when DevView changes. If DevView can be toggled during wizard execution, you'd need to add notification. However, this seems unlikely in the wizard context, so the current implementation should be fine.

Copy link
Collaborator

@Delta-473 Delta-473 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@Delta-473 Delta-473 merged commit 9fb4d70 into OpenKH:master Dec 31, 2025
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants