feat: auto-detect of native dolphin#291
Conversation
Release 2.4.8
Release 2.4.9
fix: fixed auto-detect of data folder to be based on textbox instead of saved value
📝 WalkthroughWalkthroughAdds ChangesLinux Native Dolphin Detection
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
WheelWizard/Views/Pages/Settings/WhWzSettings.axaml.cs (1)
175-205:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid prompting Flatpak install when native Dolphin is already installed.
Line 175 only checks Flatpak absence. If native Dolphin is detected (and the user declines autofill), the flow still offers Flatpak installation, which is incorrect for an already-installed setup.
Suggested fix
- if (!EnvHelper.IsFlatpakSandboxed() && !IsFlatpakDolphinInstalled()) + if (!EnvHelper.IsFlatpakSandboxed() && !IsFlatpakDolphinInstalled() && !IsNativeDolphinInstalled()) { var wantsAutomaticInstall = await new YesNoWindow() .SetMainText(t("question.dolphin_flatpack.title")) .SetExtraText(t("question.dolphin_flatpack.extra"))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@WheelWizard/Views/Pages/Settings/WhWzSettings.axaml.cs` around lines 175 - 205, The condition guarding the Flatpak Dolphin installation prompt in the if statement at line 175 only checks whether Flatpak is sandboxed and whether Flatpak Dolphin is installed, but it does not check whether a native version of Dolphin is already available. Add an additional condition to verify that native Dolphin is not already installed before prompting the user to install the Flatpak version. Look for or create an appropriate method (similar to IsFlatpakDolphinInstalled) to check for native Dolphin installation and add it to the existing condition using logical AND operators.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@WheelWizard/Views/Pages/Settings/WhWzSettings.axaml.cs`:
- Around line 175-205: The condition guarding the Flatpak Dolphin installation
prompt in the if statement at line 175 only checks whether Flatpak is sandboxed
and whether Flatpak Dolphin is installed, but it does not check whether a native
version of Dolphin is already available. Add an additional condition to verify
that native Dolphin is not already installed before prompting the user to
install the Flatpak version. Look for or create an appropriate method (similar
to IsFlatpakDolphinInstalled) to check for native Dolphin installation and add
it to the existing condition using logical AND operators.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: f3e62b8b-5fb2-417c-b8c8-dabecaf865d3
📒 Files selected for processing (3)
WheelWizard/Features/DolphinInstaller/LinuxDolphinInstaller.csWheelWizard/Services/PathManager.csWheelWizard/Views/Pages/Settings/WhWzSettings.axaml.cs
|
lgtm. We should probably retire If you could adapt the code to drop that function as well, that would be great! |
|
I can defintely do that. But the more im looking at this the more i think that this should be an own "config wizard" that guides the player through setting up all the correct paths, autodetection and also installation itself if nothing was automatically detected. If that sounds something you guys would want I could definitely do that - in a separate PR of course |
Sure, that sounds like a good idea. One thing to note there would be that, if the Wheel Wizard application itself is running as a Flatpak (we have checks for this in some places), then, normally, the only two options the users should have would be Another thing that has been confusing is the wording "dolphin executable" in the first place -- users don't realize that it can be a command on non-Windows systems. These are just some known issues that come to mind that you should be aware of if you intend to improve the initial setup flow. A separate PR for this would be appreciated! |
|
Ok then, should I close this PR and create a new one in which I would work on the total rework or do you still wish to keep these smaller changes (with the additions you asked for) for as long as the rework is not done? |
I wouldn't close this PR, this is still a good change. One thing, though: The base branch should be changed to |
|
The pop-up would still need a different text in the future (and translations). It currently references a Dolphin "folder". |
|
Agreed |
feat: added auto-detect of native dolphin installations for linux
fix: fixed auto-detect of data folder to be based on textbox instead of saved value
Purpose of this PR:
The launcher can now detect native installations of Dolphin. If it is detected it will ask the user as with the data folder if the user wants to accept the path found
Also the data folder detection used not the text field, but possibly an older value that didn't represent the users intent aynmore
How to Test:
What Has Been Changed:
Related Issue Link:
Checklist before merging