-
-
Notifications
You must be signed in to change notification settings - Fork 9
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
Implemented window title version number and instance+session persistent directory memory #89
base: main
Are you sure you want to change the base?
Conversation
Darojax
commented
Sep 13, 2024
- App will remember the last directory used, multiple apps will remember their own directory used. (Not persistent across app restarts.)
- Added app version displaying in the window title based on value in in ReforgerServerApp.csproj
Appended by accident
json files appended by mistake, please ignore them. |
This needs to target |
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.
Hey! Looks good, just needs a few minor tweaks - thanks so much for your contribution!
@@ -21,6 +22,9 @@ public Main() | |||
{ | |||
InitializeComponent(); | |||
|
|||
// Set the window title with the version number | |||
this.Text = $"ReforgerServerApp - {GetAppVersion()}"; |
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.
Should be "Arma Reforger Dedicated Server Tool", There are some messagebox constants in Utils/Constants.cs
, instead of having:
. . .
public static string WARN_MESSAGEBOX_TITLE_STR = "Arma Reforger Dedicated Server Tool - Warning";
public static string INFO_MESSAGEBOX_TITLE_STR = "Arma Reforger Dedicated Server Tool - Information";
. . .
we could have public static string APPLICATION_NAME = "Arma Reforger Dedicated Server Tool";
and each of the messagebox strings can be APPLICATION_NAME + blah
and then you could refer to APPLICATION_NAME
here to keep things consistent across the app
/// Retrieves the application's version number. | ||
/// </summary> | ||
/// <returns>Formatted version string.</returns> | ||
private string GetAppVersion() |
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.
Probably shouldn't be in Main, maybe this can live in FileIOManager
? There's not really a good home for it at the moment
// Use lastUsedDirectory if available | ||
if (!string.IsNullOrEmpty(instance.lastUsedDirectory)) | ||
{ | ||
sfd.InitialDirectory = instance.lastUsedDirectory; |
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.
instead of calling this instance in here which I think might be a bit weird, have a getter which means we can, for static methods go FileIOManager.GetInstance().GetLastUsedDirectory()
. This would make it consistent with the rest of the codebase too
if (success) | ||
{ | ||
// Update lastUsedDirectory | ||
instance.lastUsedDirectory = Path.GetDirectoryName(sfd.FileName) ?? instance.lastUsedDirectory; |
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.
once updated, use getter here
// Use lastUsedDirectory if available | ||
if (!string.IsNullOrEmpty(instance.lastUsedDirectory)) | ||
{ | ||
ofd.InitialDirectory = instance.lastUsedDirectory; |
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.
getter
@@ -150,14 +168,28 @@ public static bool SaveConfigurationToFile(string path) | |||
/// </summary> | |||
public static void LoadConfigurationFromFile() | |||
{ | |||
FileIOManager instance = GetInstance(); |
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.
wont need with the getter
ofd.Filter = "JSON (*.json)|*.json"; | ||
if (ofd.ShowDialog() == DialogResult.OK) | ||
{ | ||
string filePath = ofd.FileName; | ||
using StreamReader sr = File.OpenText(filePath); | ||
ConfigurationManager.GetInstance().PopulateServerConfiguration(sr.ReadToEnd()); | ||
|
||
// Update lastUsedDirectory | ||
instance.lastUsedDirectory = Path.GetDirectoryName(filePath) ?? instance.lastUsedDirectory; |
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.
getter
/// Retrieves the application's version number. | ||
/// </summary> | ||
/// <returns>Formatted version string.</returns> | ||
private string GetAppVersion() |
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 could also be used to populate the About Box if put in the FileIOManager
singleton. Please look into doing that :)
*.exe |
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.
.exe should already be in the gitignore
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 can't see it explicitly but the builds would be ignored here.
ArmaReforgerServerTool/.gitignore
Lines 16 to 26 in 472ea23
# Build results | |
[Dd]ebug/ | |
[Dd]ebugPublic/ | |
[Rr]elease/ | |
[Rr]eleases/ | |
x64/ | |
x86/ | |
bld/ | |
[Bb]in/ | |
[Oo]bj/ | |
[Ll]og/ |
// Retrieve the version from the assembly metadata using pattern matching | ||
return Assembly.GetExecutingAssembly().GetName().Version switch | ||
{ | ||
{ } version => $"v{version.Major}.{version.Minor}.{version.Build}.{version.Revision}", | ||
_ => "v0.0.0.0" // Fallback version if Version is null | ||
}; |
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'd prefer throwing on null version. It's the kind of thing that is either set or not, and it's vital for it to be set when released.