enkore / j4-dmenu-desktop

A fast desktop menu
GNU General Public License v3.0
673 stars 69 forks source link

j4-dmenu-desktop --no-exec improperly handles terminal applications #166

Closed benjamb closed 3 months ago

benjamb commented 3 months ago

Version 2.18 would output/launch the application using $term -e <script>, however this now just prints the plain command, which when piped to something like xargs swaymsg exec -- fails to run any terminal commands such as aerc.

Example desktop file:

[Desktop Entry]
Version=1.0
Name=aerc

GenericName=Mail Client
GenericName[de]=Email Client
Comment=Launches the aerc email client
Comment[de]=Startet den aerc Email-Client
Keywords=Email,Mail,IMAP,SMTP
Categories=Office;Network;Email;ConsoleOnly

Type=Application
Icon=utilities-terminal
Terminal=true
Exec=aerc %u
MimeType=x-scheme-handler/mailto

Previous output:

% j4-dmenu-desktop --no-exec --dmenu bemenu --term=foot
Read 85 .desktop files, found 55 apps.
User input is: aerc 
foot -e "/tmp/j4-dmenu-g3vqLO"

New output:

% j4-dmenu-desktop --no-exec --dmenu bemenu --term=foot
Read 85 .desktop files, found 55 apps.
User input is: aerc
aerc

This is a breaking change not announced in the changelogs.

meator commented 3 months ago

This is a breaking change not announced in the changelogs.

This is a bug & not an intended change. I will work on a fix. Thanks for reporting this issue!

[...] when piped to something like xargs swaymsg exec -- fails to run any terminal commands such as aerc.

j4-dmenu-desktop r3.0 adds support for i3 IPC. Sway IPC appears to be compatible with i3 IPC, so the -I (or --i3-ipc) flag should work the same as your xargs solution while providing tighter and faster integration with j4-dmenu-desktop + it handles quoting well when forwarding desktop file executables.

I will document Sway support in the Examples section. I am not a Sway user though, so I'll have to test this further. Would you mind trying it out if you have spare time? I assume that you are a Sway user because you make mention of it. I'd like to know whether the following command:

j4-dmenu-desktop -I

works as expected on Sway. If it doesn't, I'd like to know whether the following change

I3SOCK="$(sway --get-socketpath)" j4-dmenu-desktop -I

fixes it (if the first command doesn't work).

Both of these commands will require the i3 executable to be present. j4-dmenu-desktop calls i3 --get-socketpath internally to get the socket path. i3 isn't used otherwise. I will fix this in the next release (this is a very simple fix).


The r3.0 release was made recently. I would like to fix these issues (and any other issues that should occur) as soon as possible, but I don't want to spam releases. I was considering making release r3.1 around next month.

meator commented 3 months ago

A workaround that would remove the requirement for i3 executable would be to create the following script:

#!/bin/sh
exec sway --get-socketpath

name it i3, make it executable and execute j4-dmenu-desktop with altered $PATH so that the script gets executed when j4-dmenu-desktop requests i3 --get-socketpath.

This workaround should work well in the r3.0 release. I haven't tested it though.

meator commented 3 months ago

j4-dmenu-desktop from develop branch should now recognize Sway by simply calling

j4-dmenu-desktop -I

I will have to test it further.


j4-dmenu-desktop r2.18 used a temporary file to handle terminal applications. This approach was overly complicated and largely unnecessary. From r3.0 onwards, j4-dmenu-desktop executes programs itself without any wrapper scripts. This means that j4-dmenu-desktop doesn't have to worry about managing and removing temporary files.

It also removes the dependency on the /tmp directory. It doesn't have to exist or be writable (but it usually is).

These changes complicate quoting though. There can be up to two levels of indirection when executing commands. Different shells have different quoting rules (j4-dmenu-desktop strictly uses /bin/sh for executing stuff when appropriate which makes quoting reliable, but "external executors" parsing the output of --no-exec like xargs can have special quoting rules which can complicate things).

The --i3-ipc flag should be preferred, because it gives j4-dmenu-desktop full control of quoting.

benjamb commented 3 months ago

I've just given the latest commit a go with:

j4-dmenu-desktop -I

Note that I3SOCK already appears to be set (I presume by sway) on the environment, along with SWAYSOCK, if that's of any note:

I3SOCK=/run/user/1000/sway-ipc.1000.2544.sock
SWAYSOCK=/run/user/1000/sway-ipc.1000.2544.sock

j4-dmenu-desktop now successfully launches both graphical and terminal applications, though it seems to expect some kind of response from sway via IPC that may not match up with what it receives, and subsequently aborts and core dumps:

$ j4-dmenu-desktop -I --no-generic --term=foot --dmenu='bemenu -i'
Read 28 .desktop files, found 17 apps.
User input is: aerc
[2024-06-06 16:54:52.129] [error] [I3Exec.cc:308] A parsing error occurred while reading i3's response (executing command 'foot -e /bin/sh -c 'exec aerc'')!
Aborted (core dumped)
User input is: Thunderbird
[2024-06-06 16:56:41.554] [error] [I3Exec.cc:308] A parsing error occurred while reading i3's response (executing command 'thunderbird --name thunderbird')!
Aborted (core dumped)
benjamb commented 3 months ago

@meator Thanks for jumping on this so quickly!

benjamb commented 3 months ago

With debug logging enabled:

[2024-06-06 18:03:12.956] [info] [main.cc:463] User input is: aerc
[2024-06-06 18:03:12.962] [debug] [I3Exec.cc:277] I3 IPC response: [ { "success": true } ]
[2024-06-06 18:03:12.962] [error] [I3Exec.cc:308] A parsing error occurred while reading i3's response (executing command '/nix/store/ybsgi7i42x27p9hjbj1wrn2q608gbsdv-foot-1.17.2/bin/foot -e /bin/sh -c 'exec aerc'')!
Aborted (core dumped)
benjamb commented 3 months ago

From a quick glance at the code, looks like the rudimentary JSON parsing is falling over due to differences in whitespace in the sway IPC response.

meator commented 3 months ago

All problems should be fixed now. It was pretty easy to fix i3 IPC, but the --no-exec problem required more work. My changes broke the --wrapper flag. I haven't reimplemented it yet.

--no-exec is now more paranoid when quoting things. You should be able to see that single quotes are used a lot now. j4-dmenu-desktop doesn't use them when it executes things itself (unless i3 IPC mode is enabled), but they are used in --no-exec mode. This should mean that you should be able to pass output of --no-exec to any shell without worrying about escape sequences causing syntax errors or shell injections being possible.

@benjamb Would you be willing to test develop again & close this issue if everything's working?

benjamb commented 3 months ago

@meator Everything appears to be working as expected, thanks!