Execute terminals directly

Stunkymonkey / nautilus-open-any-terminal

GNU General Public License v3.0

536 stars 54 forks source link

Execute terminals directly #98

Closed lvxnull closed 8 months ago

lvxnull commented 8 months ago

Also adds xterm support

Fixes #76 Fixes #79

Stunkymonkey commented 8 months ago

overall LGTM. Thanks for your contribution.

Stunkymonkey commented 8 months ago

the best contribution in this MR is the usage of shlex and using the current working directory. I am wondering why I have not thought of both improvements.

Thanks a lot for not only giving the ideas, but also implementing it.

I have tested it locally and with foot everything works fine (with and without spaces in folder names). Not sure about flatpack and remote folders. Have you tested it?

lvxnull commented 8 months ago

I tested the vast majority of the supported terminals natively on debian. I also tested some terminals through flatpak, and they work as long as the terminal has access to user files. This can be tweaked through flatseal by the user if it's not the case by default. ~I did not test remote folders.~ UPDATE: I tested sftp. It works.

lvxnull commented 8 months ago

Since this PR adds shell quoting with shlex, #80 is now obsolete.

Stunkymonkey commented 8 months ago

I am tested it today. Locally all works perfect.

things I noticed:

the new terminal is always spawned as a nested shell. All works fine, but might be confusing. Otherwise we could set SHLVL to 1. But to be honest lets not care about it. I see no blocking point here.

for remote folders (without spaces) I am getting:

err: slave.c:417: ssh -t u181505-sub1@u181505-sub1.your-storagebox.de cd / \; exec \$SHELL: failed to execute: No such file or directory
err: fdm.c:215: no such FD: 6

with spaces I am getting the same with quoting:

err: slave.c:417: ssh -t u181505-sub1@u181505-sub1.your-storagebox.de cd ''"'"'/test test'"'"'' \; exec \$SHELL: failed to execute: No such file or directory
err: fdm.c:215: no such FD: 6

lvxnull commented 8 months ago

@Stunkymonkey This happened because foot doesn't support passing the entire command as a single string. Should be fixed now.

Stunkymonkey commented 8 months ago

yes works now.

I am wondering why I am seeing both context entrys, but that not your fault. Screenshot-2023-12-24_132539

so all LGTM.

Stunkymonkey commented 8 months ago

thank you very much for your fast responses and your contribution.