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

extProtonRun: Use 'printf "%q"' when parsing RUNPROGARGS #1073

Merged
merged 3 commits into from Mar 24, 2024

Conversation

sonic2kk
Copy link
Owner

@sonic2kk sonic2kk commented Mar 23, 2024

Partial fix for #1072.
See also #1074.

When parsing PROGARGS in extProtonRun, we just use printf "%s". However it seems we need to double-escape paths with backslashes when giving them to executables. This is probably because we cannot preserve quotes in these paths, so the \ is read as an escape character and not a literal \.

We can use printf "%q" to actually escape these paths, turning a path with single backslashes into Z:\\home\\gaben\\Games\\Half-Life 3\\hl3.exe.

Manually setting any paths in CUSTOMCMD_ARGS to use double-backslashes like above will actually fix it (needs a quadruple escape from the Yad GUI). So hopefully, using "%q" here will do this automatically. This would mean we can store CUSTOMCMD_ARGS with single backslashes, and then extProtonRun will properly handle the backslash paths.

Note that saving paths with single backslashes from the Game Menu is currently broken. You will need to double-escape on each save, because they are escaped on each load of the Game Menu and thus on each save are escaped back. #1074 fixes the saving part, which means custom command arguments with single backslashes passed on the Game Menu will work with this PR.


Since posting this PR, a change was made to fix an issue caused by printf "%q" appending a stray \ to launch flags with a space. This is mentioned briefly in the first comment on this PR (#1073 (comment)).

Please see #1072 (comment) for more details and background on the fix for paths with spaces.

@sonic2kk sonic2kk added the shellcheck Run ShellCheck workflow against PR label Mar 23, 2024
@sonic2kk sonic2kk removed the shellcheck Run ShellCheck workflow against PR label Mar 23, 2024
@sonic2kk
Copy link
Owner Author

Using printf "%q" had an issue where all arguments with spaces were having those spaces escaped. So in the case of #1072, we were ending up with --launch\ , with a backslash.

To fix this, we just strip trailing single backslashes. This won't affect paths as they should not end in a single backslash to begin with. printf "%q" will always escape single backslashes, so it should always be valid to strip a single trailing backslash.


The 2nd problem I solved here was for paths with spaces, but only for paths that use backslashes. This was a journey but the fix was basically, in the args array, if a string ends in a backslash, then assume it's a cut-off path and join them, and delete the duplicate entry that we are combining. Then, because this creates null strings, we create a new array with null strings filtered out. Earlier in the loop we remove single trailing backslashes from arguments, so thanks to this, arguments should never be misunderstood as parts of paths, unless the path ends with a backslash. For example, Z:\home\username\Downloads\. But a user should not do this, and we will document this on the wiki.

More details for this can be found here: #1072 (comment)


With all of that out of the way, this PR needs testing for the use-case in #1072, and then also if possible I would like to try and handle paths with forward slashes.


One other thing of note: The changes here may eventually be generally applicable, for example for launch arguments coming from Steam if they are broken somehow. But we will simply keep this fix in mind, and if issues arise in future to do with paths and spaces, we can sort them out then.

Although ideally, where possible, users should avoid paths with spaces. This is not always possible if a game folder has a space, which is why I made extra effort to solve this problem for custom commands (some custom commands have to be placed in, and therefore ran from, the game folder, and if that folder name has a space then a user would be SOL).


So, testing and maybe implementing this for paths with forward slashes, and then this should be good to merge.

@sonic2kk
Copy link
Owner Author

sonic2kk commented Mar 23, 2024

Rebased against master now that #1074 is merged, which means the Game Menu backslash escaping fix is merged. That should make testing this PR easier, since paths can be configured from the GUI or at least, not mangled when saving from the Game Menu.

@sonic2kk
Copy link
Owner Author

sonic2kk commented Mar 24, 2024

Tested by me and @zany130, seems to work as expected. Will bump the version and merge.

Never figured out the forward-slash thing. There's a TODO though so it can be implemented later, no need for it to block this PR.

@sonic2kk sonic2kk merged commit 8f9dcbe into master Mar 24, 2024
1 check passed
@sonic2kk sonic2kk linked an issue Mar 24, 2024 that may be closed by this pull request
@sonic2kk sonic2kk deleted the extProtonRun-printf-q branch March 31, 2024 14:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unable to pass windows paths to custom commands
1 participant