lxqt / libqtxdg

Qt implementation of freedesktop.org xdg specs
https://lxqt.github.io
GNU Lesser General Public License v2.1
72 stars 35 forks source link

Support string literals in Exec keys of desktop entries #271

Closed tsujan closed 2 years ago

tsujan commented 2 years ago

It is assumed that a string literal starts with a non-escaped, non-quoted single/double quote and ends with the next single/double quote. This assumption is made based on the behavior of GLib, as reflected by pcmanfm-qt (→ libfm-qt).

The parameters and EVs are not expanded inside string literals.

Closes https://github.com/lxqt/lxqt-panel/issues/1705

tsujan commented 2 years ago

@palinek, This time, it's self-consistent:

  1. Single or double quote(s) can be inside the name.
  2. Arguments like "%f" or '%f' are NOT expanded, as they shouldn't — they are string literals.
  3. Quoted delimiters are ignored.
  4. It passed many tests by me — some as weird as Exec='/home/username/new 'folder/executable, which should run /home/username/new folder/executable.
  5. Well-known values like Exec=bach -c "fpad %U" are supported as before, in addition to Exec=bach -c 'fpad %U' (their parameters don't need expanding).

That being said, I may have missed some thing or some things. A review would be appreciated (I know it's boring).

palinek commented 2 years ago

Do we need to do the parsing by hand? You've said previously, that some glib functionality could be used. Can't we simply use posix's wordexp(3)? I know, it does more, but we're going beyond the standard anyway... and by using it we use some well defined standard behavior without troubles as when doing everything by hand.

tsujan commented 2 years ago

You've said previously, that some glib functionality could be used.

That would be ideal but it needs rewriting the whole code.

Exec's behavior isn't that of Shell, as far as I know.

I compared GLib's behavior with this; couldn't find a difference.

tsujan commented 2 years ago

Not related to this patch; just for the record:

After spending hours to investigate various aspects of Exec today, I found not only odd cases in GLib but also differences between it and KDE's handling.

String literals aside, apparently, there are contradictory interpretations of freedesktop's spec. For example, we remove the backslash from \" and don't include " in the argument, while GLib includes it. I think, in this case, we follow the spec correctly, although the spec itself isn't specific enough. Also, GLib doesn't expand EVs inside \"...\", while we do; etc.

The reason may be the complexity of the logic and its incompatibility with Shell. A complex logic results in an ugly code and, once it's written, people prefer to stay away from it. Didn't find a better explanation for the current discrepancies…

So, perhaps, we'd better stick with our code, instead of rewriting it based on GLib or another tool.

palinek commented 2 years ago

I found not only odd cases in GLib but also differences between it and KDE's handling.

A complex logic results in an ugly code and, once it's written, people prefer to stay away from it.

For this both reasons I would prefer to handover the hard work to someone (no matter if glib, wordexp or anything else). Then we don't need to maintain the core logic and can rely on a more widely used solution.

tsujan commented 2 years ago

no matter if glib, wordexp or anything else

It might matter. A widely used solution needs a widely used tool, and that's GLib.

Jobs like syntax parsing have a complex logic by nature and the core code has worked pretty fine. IMO, it would be sad if we threw it away on encountering a problem in it. As I said, GLib's solution also has problems but who wants to fix them? Adding workarounds for them might not be worth the effort.

palinek commented 2 years ago

I'm too lazy to review the parsing logic. We must trust you :)

tsujan commented 2 years ago

I understand. If this PR was made by another dev or a contributor, its review would be so boring for me.

I'll use and test it for a while to make sure everything is OK. Will recheck its logic too.

stefonarch commented 2 years ago

I stumpled upon the tor browser's desktop file. ./start-tor-browser.desktop --register-app' creates a file . desktop in ~/.local/share/applications/

Exec=sh -c '"/home/stef/bin/tor-browser_en-US/Browser/start-tor-browser" --detach || ([ ! -x "/home/stef/bin/tor-browser_en-US/Browser/start-tor-browser" ] && "$(dirname "$*")"/Browser/start-tor-browser --detach)' dummy %k

It starts only in a pcmanfm-qt window -runner, menu and quickstart no result. The command line works.

EDIT: I thought this was in master already, it works in everyone.

tsujan commented 2 years ago

I tested with Exec=sh -c '"featherpad" --standalone "/etc/protocols" & "feathernotes"' (somehow similar to your command because it has both single and double quotes as well as an option) and it worked in Panel's main menu with this patch.

tsujan commented 2 years ago

I thought this was in master already, it works in everyone.

So, there's no need to wait more. Merging...