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

Modify the logic to allow terminal emulators to be installed outside /usr/bin #752

Closed
wants to merge 1 commit into from

Conversation

amtlib-dot-dll
Copy link

For example, one may install a newer version of GNOME Terminal or XTerm in ~/.local/bin or somewhere else

Modify the logic to allow terminal emulators to be installed outside `/usr/bin`
@amtlib-dot-dll
Copy link
Author

This is a Unix-only patch, and I don't know why the "Windows Debug" check failed

@pieandcakes
Copy link
Collaborator

@dotnet-bot test this please

@amtlib-dot-dll
Copy link
Author

@pieandcakes Thanks

Copy link
Collaborator

@pieandcakes pieandcakes left a comment

Choose a reason for hiding this comment

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

Leave a comment

private string _terminalPath;
private string _bashCommandPrefix;

private bool ExistInPath(string fileName)
{
if (File.Exists(fileName))
Copy link
Collaborator

Choose a reason for hiding this comment

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

This check seems invalid now, unless you are checking /usr/bin/.

I think the proper thing should be to short circuit it with a /usr/bin/ and if it isn't there, then allow it to search path.

Copy link
Author

Choose a reason for hiding this comment

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

You are right, only but it's customary to maintain a backward compatibility 😂

@pieandcakes
Copy link
Collaborator

Is the goal to support specifically if the user has gnometerminal installed twice, that it uses path to determine where it is? Or are you trying to support checking path for these apps instead of only assuming the one installed is in /usr/bin?

@pieandcakes pieandcakes self-assigned this Aug 15, 2018
@amtlib-dot-dll
Copy link
Author

Yeah, I have XTerm installed in /app/bin/xterm but nowhere else, so MIEngine complains.

@amtlib-dot-dll
Copy link
Author

This check may be modified some time in the future because there are complaints about the extra terminal window while Visual Studio Code have its own internal one microsoft/vscode-cpptools#35

@pieandcakes
Copy link
Collaborator

I think the right fix would be to return the string that gnometerminal is found in and start the search with /usr/bin and fallback to path if it isn't found and if it is still not found then try xterm and then error.

@amtlib-dot-dll
Copy link
Author

What if someone just want to override the one installed under /usr/bin? PATH=/usr/bin:/usr/local/bin and PATH=/usr/local/bin:/usr/bin is totally different. The returned string is only used to spawn the terminal process and a bare executable name rather than a full path is OK for the spawning.

However, your idea will make the code more neat and descriptive.

@pieandcakes
Copy link
Collaborator

pieandcakes commented Oct 30, 2018

@amtlib-dot-dll We removed the requirement for terminal clients by switching to the use of VSCode Debug Protocol's RunInTerminalRequest. This will allow you to specify the terminal client through the VS Code settings instead and will not need us to set it. For more information on how to change that setting, look at microsoft/vscode-cpptools#2998.

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

Successfully merging this pull request may close these issues.

2 participants