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

FileSystem dock: Fix opening a Linux terminal #88173

Merged
merged 1 commit into from
Feb 15, 2024

Conversation

capnm
Copy link
Contributor

@capnm capnm commented Feb 10, 2024

The previous implementation of opening a terminal
in the FileSystem dock was causing errors due to the use
of a bash builtin command that does not work outside the shell.

This resulted in the following error messages:

ERROR: Could not create child process: command
   at: execute (drivers/unix/os_unix.cpp:553)
ERROR: Could not create child process: command
   at: execute (drivers/unix/os_unix.cpp:553)
...

To resolve this issue,

  • the command is now executed inside a bash shell,
  • if the terminal_emulator_flags editor option is empty,
    the working directory for gnome-terminal and urxvt is now properly set.

Tested only on GNOME platforms debian/ubuntu:

godot4.3devX -v -e
...
Opening terminal emulator: gnome-terminal --working-directory ... /00-test-new_projects/new-project1/new_folder/test folder/

Opening terminal emulator: urxvt -cd ... /00-test-new_projects/new-project1/new_folder/test folder

Screenshot from 2024-02-10 22-05-50

@capnm capnm requested a review from a team as a code owner February 10, 2024 14:01
@AThousandShips AThousandShips requested a review from a team February 10, 2024 14:07
@AThousandShips AThousandShips added this to the 4.3 milestone Feb 10, 2024
@capnm capnm force-pushed the 240210-fix-fsdock-open_terminal branch from d9e3151 to 717b2e6 Compare February 10, 2024 21:25
@capnm capnm requested a review from AThousandShips February 10, 2024 21:32
@capnm capnm force-pushed the 240210-fix-fsdock-open_terminal branch from 717b2e6 to e64ec01 Compare February 12, 2024 01:37
The previous implementation of opening a terminal
in the FileSystem dock was causing errors due to the use
of a bash builtin command that does not work outside the shell.

This resulted in the following error messages:

	ERROR: Could not create child process: command
		at: execute (drivers/unix/os_unix.cpp:553)
	ERROR: Could not create child process: command
		at: execute (drivers/unix/os_unix.cpp:553)
	...

To resolve this issue,
+ the command is now executed inside a bash shell,
+ if the `terminal_emulator_flags` editor option is empty,
  the working directory for gnome-terminal and urxvt is now properly set.
@capnm capnm force-pushed the 240210-fix-fsdock-open_terminal branch from e64ec01 to 8e8f8e9 Compare February 14, 2024 13:30
@capnm
Copy link
Contributor Author

capnm commented Feb 14, 2024

Rebased to tip and tested KDE Plasma on Arch Linux:

Without this PR also broken:
Screenshot_20240214_131052

current tip + PR OK:

Screenshot_20240214_141207

@akien-mga akien-mga changed the title FileSystem dock: Fix open a terminal FileSystem dock: Fix opening a Linux terminal Feb 14, 2024
Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't reproduce the bug on Fedora 39 with KDE Plasma (Wayland), but I tested the PR and it doesn't seem to break it either. So if it solves it for you, I guess it's fine.

@Calinou
Copy link
Member

Calinou commented Feb 14, 2024

I thought command -v was standard POSIX shell stuff that didn't require Bash (that's why I chose it in fact).

Either way, it's not very important as the editor on Linux/*BSD will pretty much always have Bash available.

Copy link
Member

@Calinou Calinou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested locally on Fedora 39 KDE X11 (with Konsole and Kitty), it works as expected.

@akien-mga akien-mga merged commit ce971c2 into godotengine:master Feb 15, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants