| Summary: | KDesktopFile should be parsing Exec= line differently | ||
|---|---|---|---|
| Product: | [Frameworks and Libraries] frameworks-kconfig | Reporter: | ratijas <me> |
| Component: | general | Assignee: | Matthew Dawson <matthew> |
| Status: | RESOLVED FIXED | ||
| Severity: | normal | CC: | kde, kdelibs-bugs, nate, prettyvanilla |
| Priority: | NOR | ||
| Version: | unspecified | ||
| Target Milestone: | --- | ||
| Platform: | Other | ||
| OS: | Linux | ||
| See Also: | https://bugs.kde.org/show_bug.cgi?id=465290 | ||
| Latest Commit: | Version Fixed In: | ||
| Sentry Crash Report: | |||
|
Description
ratijas
2023-06-28 14:14:49 UTC
Git commit 9b8f7d9ac2cbc7c6048c981a040540b7c6e35666 by ivan tkachenko.
Committed on 29/06/2023 at 09:25.
Pushed by ratijas into branch 'master'.
KConfigIniBackend: Make diagnostics about escape sequences more readable
Instead of this:
kf.config.core: "KConfigIni: In file scrcpy.desktop, line 8: " "Invalid escape sequence \"\\\"\"."
We'll be getting this:
kf.config.core: KConfigIni: In file scrcpy.desktop, line 8: Invalid escape sequence: «\"»
Which is still our bug to fix, but at least now it's clear which one.
M +2 -2 src/core/kconfigini.cpp
https://invent.kde.org/frameworks/kconfig/-/commit/9b8f7d9ac2cbc7c6048c981a040540b7c6e35666
Removing automated "qt6" keyword. This is not a regression. It might even be worth backporting to KF5 if it won't end up being an ABI break. desktop-file-validate says this is valid I don't believe there is anything to really fix here on the parsing side and the warning is correct as is, although I do find the specification a bit hard to interpret or even ambiguous at times concerning the escaping rules. Still, I'm reasonably sure the current implementation matches the actual intent and this seems to be a case where the validation logic isn't fully implemented on desktop-file-utils's side, see their TODO-comment in validate.c (https://gitlab.freedesktop.org/xdg/desktop-file-utils/-/blob/master/src/validate.c?ref_type=heads#L55): > /*TODO: > * + Lecagy-Mixed Encoding (annexe D) > * + The escape sequences \s, \n, \t, \r, and \\ are supported for values of > * type string and localestring, meaning ASCII space, newline, tab, carriage > * return, and backslash, respectively. > */ According to the current version of desktop-file-validate all these are valid (but only the first one really is to my understanding): > Exec=/bin/sh -c "\\"\\$SHELL\\" -i -c scrcpy" > Exec=/bin/sh -c "\"\\$SHELL\" -i -c scrcpy" > Exec=/bin/sh -c "\"\$SHELL\" -i -c scrcpy" but as the error message also indicates when the "$" isn't quoted at all, backslashes for escaping need to be escaped themselves: > Exec=/bin/sh -c "\\"$SHELL\\" -i -c scrcpy" > error: value "/bin/sh -c "\\"$SHELL\\" -i -c 'scrcpy --pause-on-exit=if-error'"" for key "Exec" in group "Desktop Entry" contains a non-escaped character '$' in a quote, but it should be escaped with two backslashes ("\\$") This corresponds to the note in the spec for the Exec key: > Note that the general escape rule for values of type string states that the backslash character can be escaped as ("\\") as well and that this escape rule is applied before the quoting rule. As such, to unambiguously represent a literal backslash character in a quoted argument in a desktop entry file requires the use of four successive backslash characters ("\\\\"). Likewise, a literal dollar sign in a quoted argument in a desktop entry file is unambiguously represented with ("\\$"). which will also include '\\"' and "\\`". The current version of scrcpy's desktop file works again because without trying to quote the $SHELL variable itself the current escaping ended up correctly escaped with two backslashes: > Exec=/bin/sh -c "\\$SHELL -i -c 'scrcpy --pause-on-exit=if-error'" It would probably be best to instead improve the message further (thanks for the readability patch, that's already much better!) and hint at the need for the double escaping like desktop-file-validate's message above does. Parsing changed a bit here with the commits that fixed Bug 465290, but beyond that I agree that there doesn't seem to be a bug to fix here. |