-
-
Notifications
You must be signed in to change notification settings - Fork 61
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
feat: add Serverlist editor to the Patcher #527
base: master
Are you sure you want to change the base?
Conversation
This also added a dependency to Newtonsoft.Json Also updated .NetFramework to 4.8 since 4.6 is EoL since April 22
Doesn't this already have an equivalent in the LocalGhost Patcher? I wonder why it hasn't been added to the Peacock one |
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.
Noted some issues and other points.
Also, it would be preferable if this didn't use Newtonsoft.Json, as it would mean having to include a dll file along with the patcher exe.
@@ -128,24 +153,41 @@ private void MainForm_FormClosing(object sender, FormClosingEventArgs e) | |||
|
|||
private string GetSelectedServerHostname() | |||
{ | |||
if (!servers.TryGetValue(serverUrlComboBox.Text, out string hostname)) | |||
string hostname = ""; | |||
var result = gameServers.FirstOrDefault(server => server.ServerName == serverUrlComboBox.Text); |
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 result isn't actually used anywhere, and this function will now always return "127.0.0.1" unless you manually type something into the server combobox.
} | ||
|
||
return hostname; | ||
} | ||
|
||
private void SetSelectedServerHostname(string input) | ||
{ | ||
if (!serversReverse.TryGetValue(input, out string result)) | ||
GameServer server = null; | ||
if (input == "localhost" || input == "127.0.0.1") |
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 don't really understand why this if-statement is here - I think you always want to compare against the ServerAddress
, and then return the corresponding ServerName
.
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 think I introduced this check for the edge case a user used a custom name for "Peacock Local" at the moment the program starts. At the moment it shows 127.0.0.1 or "Peacock Local"
I think we should first move to patcher-core. That is a more modern setup (.NET instead of .NET Framework, UI separated from logic) and has support for DPI and Steam Deck (through CLI). And for size reasons, it can still back-compile to .NET 4.8. Also, couldn't we just get the actual serverlist editor from @grappigegovert version back in the source? |
add the newline back Co-authored-by: Govert de Gans <[email protected]>
I just checked out the ghostmod patchers menu, in my opinion it does not resolve the issue, because it is hidden in the options menu and not easy to find or understand how it works
I used JSON because it is easy to read and understand and most typescript devs know it. but if an additional dll is not wanted I can use another format to |
You mean C# right :P? As for the replacement: https://www.nuget.org/packages/system.text.json - this is part of .NET (not .NET Framework) by default now. For simple situations, there is no need to use Newtonsoft anymore. Another reason why I feel we should first adopt the core version, then reapply this. |
patcher/MainForm.cs
Outdated
{ | ||
gameServers = CreateDefautServerList(); | ||
} | ||
configLocation = $@"{appData}\PeacockProject\"; |
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 always use Path.Combine
.
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 had stolen this line from Settings.cs
:P
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.
Yea, there are quite a few weird things in the code that make my hands itch. But if we make sure newly added stuff to be correct, the rest will follow, haha.
patcher/MainForm.cs
Outdated
@@ -91,6 +80,26 @@ public MainForm() | |||
{ | |||
CurrentSettings = new Settings(); | |||
} | |||
try | |||
{ | |||
var fileContent = File.ReadAllText($@"{configLocation}\Servers.json", Encoding.UTF8); |
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.
Same here.
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.
As for the editor itself, we can easily port that over to the new version.
patcher/ServerListEditor.cs
Outdated
.SpecialFolder | ||
.ApplicationData); | ||
|
||
var folder = $@"{appData}\PeacockProject\"; |
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.
Same here.
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 also feel there should just be a single static readonly in a file with the path to the configuration and server.json
.
patcher/MainForm.cs
Outdated
{ | ||
GameServer[] gameServers = new GameServer[2]; | ||
gameServers[0] = new GameServer { ServerName = "IOI Official", ServerAddress = "config.hitman.io" }; | ||
gameServers[1] = new GameServer { ServerName = "Peacock Local", ServerAddress = "127.0.0.1" }; | ||
var serverList = JsonConvert.SerializeObject(gameServers); | ||
File.WriteAllText("Servers.json", serverList, Encoding.UTF8); | ||
|
||
File.WriteAllText($@"{configLocation}Servers.json", serverList, Encoding.UTF8); |
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.
Same here.
patcher/ServerListEditor.cs
Outdated
var serverString = JsonConvert.SerializeObject(GameServers); | ||
File.WriteAllText("Servers.json", serverString, Encoding.UTF8); | ||
File.WriteAllText(@$"{folder}Servers.json", serverString, Encoding.UTF8); |
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.
Same here.
Also, please change the version to 4.6. This is pre-installed on all Windows 10 and up, preventing players from having to install .NET Framework. |
I updated to 4.8 because Microsoft do not support 4.6 anymore, but will revert this change |
Introduce static ConfigLocation string use Path.Combine instead of string operations
On my win 11 Pc 4.6 is not available to compile the project is 4.6.2 okay, too? |
https://learn.microsoft.com/en-us/dotnet/framework/install/guide-for-developers I had a reason for targeting 4.6.1 for the patcher-core. Pretty sure it was related to something that was simply unavailable in .NET 4.6 and/or more trouble than it's worth to backport. Can you target that? Get this: https://dotnet.microsoft.com/en-us/download/dotnet-framework/thank-you/net461-developer-pack-offline-installer |
I managed to find the target installer for 4.6 was a bit hidden |
This will fix #428
Scope
Add a button to the patcher to open a new Dialog to edit the serverList
This pull request also added a dependency to Newtonsoft.Json
Also updated .NetFramework to 4.8 since 4.6 is EoL since April 22
Test Plan
Clicked the new button
edit the entries and save them
changes are stored in "Servers.json"
Checklist