-
Notifications
You must be signed in to change notification settings - Fork 17
feat: fix CD disk detection; add manual folder picker #222
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: development
Are you sure you want to change the base?
feat: fix CD disk detection; add manual folder picker #222
Conversation
200d051 to
0bd3bb0
Compare
- Fixed game recognition for CD/ISO installations. - Added manual directory selection fallback. - Added missing Zero Hour retail paths. - Updated .gitignore for releases.
0bd3bb0 to
3979893
Compare
| /// <param name="directory">The directory to check.</param> | ||
| /// <param name="executableNames">The list of executable names to look for.</param> | ||
| /// <returns>True if any of the executables exist.</returns> | ||
| private static bool HasAnyExecutable(string directory, string[] executableNames) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This whole method is basically just this, but much more verbose:
private static bool HasAnyExecutable(string directory, string[] executableNames) =>
executableNames.Any(exe => File.Exists(Path.Combine(directory, exe)));
Do you still need a separate method for it? I'd only do so if you use it in multiple places.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed the method since it wasn't needed and just bloated more
| return installation; | ||
| } | ||
| } | ||
| else |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This else block is entirely redundant as you are already doing a return from the if on line 1570. I think this could would make more sense / be more readable if the else block was removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I moved it up now so it gets reached.
undead2146
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed the code review comments
| /// <param name="directory">The directory to check.</param> | ||
| /// <param name="executableNames">The list of executable names to look for.</param> | ||
| /// <returns>True if any of the executables exist.</returns> | ||
| private static bool HasAnyExecutable(string directory, string[] executableNames) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed the method since it wasn't needed and just bloated more
| return installation; | ||
| } | ||
| } | ||
| else |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I moved it up now so it gets reached.
This PR fixes the GameInstalation not being detected for CDISO whereby users do not have a
Instal Dirregistry entry.