Skip to content
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

Fix trailing space mod load and system dialogs #916

Closed
wants to merge 0 commits into from

Conversation

jakerosado
Copy link
Contributor

@jakerosado jakerosado commented Oct 13, 2023

Resolves #851, resolves #905

@jakerosado jakerosado changed the title Fix directory trailing space load failure Fix trailing space mod load and system dialogs Oct 13, 2023
@Pathoschild Pathoschild added this to the 4.1.x milestone Dec 1, 2023
@Pathoschild Pathoschild force-pushed the develop branch 2 times, most recently from 4c7eac9 to 8837303 Compare March 19, 2024 04:30
@Pathoschild Pathoschild force-pushed the develop branch 2 times, most recently from e201c96 to 3793d2e Compare May 31, 2024 03:55
Pathoschild added a commit that referenced this pull request Jul 6, 2024
@Pathoschild Pathoschild closed this Jul 6, 2024
@Pathoschild
Copy link
Owner

I merged the open-folder command changes into develop manually for the upcoming SMAPI 4.1.0. Thanks for the help!

I was going to keep this PR open for this other change in StardewModdingAPI.Toolkit.Utilities.PathUtilities:

        public static string? NormalizePath(string? path)
        {
-            path = path?.Trim();
-            if (string.IsNullOrEmpty(path))
-                return path;
+            // safety check if directory is null or contains trailing spaces
+            string? trailingPath = path;
+
+            trailingPath = trailingPath?.Trim();
+            if (string.IsNullOrEmpty(trailingPath))
+                return trailingPath;

Unfortunately splitting the PR confused GitHub, so it closed the PR.

It seems like that change would cause unintended consequences though. For example, paths everywhere would no longer be trimmed, which may cause errors in cases where user input is normalized (e.g. Content Patcher content packs which should ignore the surrounding spaces in {{HasFile: asset/somePath.png }}).

@Pathoschild Pathoschild added the bug This is an unintended error or behavior that can be addressed with specific development changes. label Jul 6, 2024
@jakerosado
Copy link
Contributor Author

Made a fix in #958. I changed the null/empty check on the path from the get-go and perform the trimming only if there's no trailing space. I tested with several mods (FastAnimations, Fishnets, FrameRateLogger) by swapping which had trailing spaces. It also handles duplicate instances where there would be two of the same mods, i.e. FastAnimations, with and without a trailing space.

Pathoschild added a commit that referenced this pull request Jul 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This is an unintended error or behavior that can be addressed with specific development changes.
Projects
None yet
2 participants