Improve handling of variables and relative paths when reading paths from env vars #537
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
David1Socha
commented
Feb 2, 2025
| using ConvertCiv3Media; | ||
| using C7GameData; | ||
| using Godot; | ||
| using QueryCiv3; | ||
|
|
||
| public partial class Util { | ||
| static public string Civ3Root = Civ3Location.GetCiv3Path(); |
There was a problem hiding this comment.
Not strictly relevant to this PR, but I was wondering if this property is truly meant to use Civ3Location.GetCiv3Path() and not the class method Util.GetCiv3Path(). Currently Util.GetCiv3Path has no refs, and the C7Settings.GetSettingValue check seems like something that might be expected when calling Util.Civ3Root. From git history it looks like this property was updated 2 years ago, but I wanted to double check if this setup was intentional
There was a problem hiding this comment.
I can't say one way or another if it was intentional, but if it doesn't seem needed it might be worth a separate PR to change it.
TomWerner
approved these changes
Feb 7, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Should resolve #460 .
Some notes on the approach taken:
Dotnet has Environment.ExpandEnvironmentVariables which seems like it'd be suitable for expanding any nested environment variables, but by design that method only supports Windows-style
%VARIABLES%. I could not find any ready-made .NET functions or util libraries that fully addressed the concerns flagged in issue 460, so I added some ad-hoc checks and updates to theGetCiv3Pathmethod inCiv3Location.cs.In
Util.cs, I noticed that itsGetCiv3Pathmethod seems to be unused at the moment, with theCiv3Rootproperty forwarding out toCiv3Location.GetCiv3Path. I thought it made sense to consolidate the similarGetCiv3Pathmethods inUtil.csandCiv3Location.csto a single method, so I updatedUtil.GetCiv3Pathto point toCiv3Location.GetCiv3Path(but first checking C7Settings for a saved civ3 location, since that seemed like a significant difference between the two methods). I'd be happy to either fully removeUtil.GetCiv3Pathor restore its original method body and then also add my changes to that method if either of those would be preferred.I updated the
GetHomefunction from usingEnvironment.SpecialFolder.Personalto usingEnvironment.SpecialFolder.UserProfile, this is because .NET 8 (which I see is on the roadmap) has a breaking change toSpecialFolder.Personalchanging behavior for Linux systems from returning$HOMEto returning "XDG_DOCUMENTS_DIRif available; otherwise$HOME/Documents".Environment.SpecialFolder.UserProfilereturns $HOME under both .NET 7 and .NET 8.Tested relative directory handling and environment variable replacement on a local Windows game build with these changes, both worked successfully. I tested the logic for relative directory handling, Unix-style environment variable replacement, and
~home path replacement in a Console app in WSL and that also worked as expected. Haven't tested the full game build with these changes on Linux or OSX because I don't currently have setups for those operating systems, but if that's needed before merge I can setup a Linux VM and try to validate there as well.