-
-
Notifications
You must be signed in to change notification settings - Fork 38
Add Steam Game Properties Button to Details View #37
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?
Add Steam Game Properties Button to Details View #37
Conversation
…ntricks, and setting il2cpp executable
…lled, fix trailing quote in winedll overrides text
|
Sounds cool. The only issue I have with this is the dotnet installer. This is unnecessary as ML 0.7 will do this by itself (the linux implementation for this is still needed however, but works fine under Proton) |
|
Or, well actually, it's both dependencies, because they won't be needed for ML 0.7. Requiring the user to have Protontricks installed is pretty unnecessary in that case |
|
Also please follow the default coding conventions. I see a lot of nests being opened on the same line, and no blank lines after nests. if (abc)
return;
if (def){
//stuff
}
Ghi();Should become: if (abc)
return;
if (def)
{
//stuff
}
Ghi();Same applies to new function nests |
|
So wait, if the dependencies are unnecessary then should I just drop it down to the game properties button and leave it at that? |
|
Yea, unfortunately :/ |
|
It's fine, I understand. It was still a good learning experience for me so the effort isn't entirely wasted (and I kinda got a game for free out of it cause someone wanted it to be a little easier to use melonloader). My bad for not consulting the main codebase for this information. With that, the steam game properties I'm using is platform agnostic and doesn't have to be a Linux Util, that's just where I put it with everything else Linux related I was working on. I think this could be implemented as a button on the main page as well if windows users want to make use of it, but let me know what you think about that. I'd also want to move the method to a different class, would MLManager be better for this? |
… the loader itself. Move OpenSteamGameProperties to MLManager. Remove ProtonTricks warning Clean up code formatting
|
Removed all the extra code, and cleaned up some code style issues I could find. |
| } | ||
|
|
||
| public static async Task InstallAsync(string gameDir, bool removeUserFiles, MLVersion version, bool linux, bool x86, InstallProgressEventHandler? onProgress, InstallFinishedEventHandler? onFinished) | ||
| public static async Task InstallAsync(string gameDir, string? id, bool removeUserFiles, MLVersion version, bool linux, bool x86, InstallProgressEventHandler? onProgress, InstallFinishedEventHandler? onFinished) |
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.
Please rename the id parameter and specify what it's for (ex. gameId)
| (errorMessage) => Dispatcher.UIThread.Post(() => OnOperationFinished(errorMessage))); | ||
| } | ||
|
|
||
| private void GamePropsHandler(object sender, RoutedEventArgs args) |
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.
Keep in mind that the installer supports multiple launchers. Make sure you only show this on Steam games or add support for other launchers
| return inited; | ||
| } | ||
|
|
||
| public static void OpenSteamGameProperties(string? appId) |
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 class is meant for MelonLoader-related operations only. Move this to the SteamLauncher class
| return; | ||
| } | ||
|
|
||
| Process.Start(new ProcessStartInfo(){ |
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.
Please abide by the default coding conventions; fix the nest format
| return; | ||
| } | ||
|
|
||
| Process.Start(new ProcessStartInfo(){ |
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.
Add a try-catch block in case it fails to open for whatever reason
|
Also, this PR should be retargeted towards the |
|
I will check the UI changes once you fix the points above and get approved for workflow checks |
|
It probably won't be until tomorrow when I get this done. Today has been exhausting at work. |
Let me know if any additional changes need to be made. This should hopefully reduce the amount of effort and confusion people have with getting MelonLoader running under linux.