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

Improve Open in Terminal in Mac #6136

Merged
merged 1 commit into from Jun 7, 2016
Merged

Improve Open in Terminal in Mac #6136

merged 1 commit into from Jun 7, 2016

Conversation

@sijad
Copy link
Contributor

sijad commented May 5, 2016

This also fixes #391

Tested in Mac OS X 10.11.4 (15E65)
With Terminal Version 2.6.1 (361.1) and iTerm2 Build 2.1.4

DEFAULT_TERMINAL_MAC also can be set as terminal.app or even terminal but I can't test if that works with older version of open command in previous Mac OS

Add Open in Terminal test case for Mac

Remove unused files
@mention-bot

This comment has been minimized.

Copy link

mention-bot commented May 5, 2016

By analyzing the blame information on this pull request, we identified @weinand and @egamma to be potential reviewers

@msftclas

This comment has been minimized.

Copy link

msftclas commented May 5, 2016

Hi @sijad, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!
You've already signed the contribution license agreement. Thanks!

The agreement was validated by Microsoft and real humans are currently evaluating your PR.

TTYL, MSBOT;

@Tyriar

This comment has been minimized.

Copy link
Member

Tyriar commented May 16, 2016

Going to adding to @joaomoreno as I don't have access to a mac/vm right now.

@Tyriar

This comment has been minimized.

Copy link
Member

Tyriar commented May 17, 2016

Getting an OS X VM setup so I can do it as @joaomoreno is out.

@weinand

This comment has been minimized.

Copy link
Member

weinand commented May 17, 2016

@sijad thanks for the PR.

Could you please provide some details about your improvements to 'Open in Terminal' on OS X.

I quickly reviewed the code and noticed that you have removed the AppleScripts that try to reuse the same terminal if it is not already running a command. Without that functionality, every 'Open in Terminal' creates a new Terminal window which quickly clutter the desktop.

@sijad

This comment has been minimized.

Copy link
Contributor Author

sijad commented May 17, 2016

@weinand that's true but applescript uses cd command and sometime it can be a little annoying
using open command is the preferred way to do this, I also did look into a few other editors and most of them uses same way to do this. but maybe using apple script is a better way, I really not sure either.

@weinand

This comment has been minimized.

Copy link
Member

weinand commented May 17, 2016

@sijad here is a first analysis of your PR. Please correct me if I'm wrong and add anything that is missing.

The PR:

  • simplifies the implementation by using open to launch the Terminal.app instead of using multiple AppleScripts,
  • makes the terminal application configurable through settings,
  • adds a test.

The PR removes the following features:

  • a user installed iTerm.app is no longer automatically used but must be configured manually in settings.
  • 'Open in Terminal' does no longer reuse a non-busy terminal but always opens a new terminal. To get back the previous functionality a user has to install a custom application and configure it in the settings.

Verdict:
I think that the added configurability outweighs the loss of functionality of this PR.

@Tyriar

This comment has been minimized.

Copy link
Member

Tyriar commented May 17, 2016

@weinand as I understand it the main part of it is #5462 (OS X support for custom terminal launch settings #3495 )

@sijad

This comment has been minimized.

Copy link
Contributor Author

sijad commented May 18, 2016

@weinand you're right. As told applescript run cd and it will be saved in shell history. I just thought it's cleaner
I'm going to revert applescript

@weinand

This comment has been minimized.

Copy link
Member

weinand commented May 18, 2016

@sijad I'm not suggesting to revert the PR to use the AppleScript because that will complicate the launch settings: What do you specify in the setting instead of Terminal.app?
I think your PR is fine, I just wanted to understand what the PR does and how VS Code's existing behavior will change.

@sijad

This comment has been minimized.

Copy link
Contributor Author

sijad commented May 18, 2016

@weinand it's still possible to use AppleScript to lunch Terminal application

@weinand

This comment has been minimized.

Copy link
Member

weinand commented May 18, 2016

@sijad sure, instead of specifying Terminal.app in the settings, I can specify an AppleScript based application. But the AppleScript would not be the default and VS Code would not ship with it.

@Tyriar Tyriar modified the milestones: June 2016, May 2016 May 23, 2016
@Tyriar

This comment has been minimized.

Copy link
Member

Tyriar commented May 27, 2016

@weinand do you want to take this PR? I wouldn't have identified any issues with the AppleScript stuff.

@Tyriar Tyriar assigned weinand and unassigned Tyriar May 27, 2016
@weinand weinand merged commit 04e29ce into microsoft:master Jun 7, 2016
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
weinand added a commit that referenced this pull request Jun 7, 2016
@sijad sijad deleted the sijad:mac-terminal branch Jun 7, 2016
weinand added a commit that referenced this pull request Jun 7, 2016
weinand added a commit that referenced this pull request Jun 7, 2016
@weinand

This comment has been minimized.

Copy link
Member

weinand commented Jun 7, 2016

@sijad today I've merged your PR into VS Code. Thanks a lot!
I could not accept the full PR because too many terminal related changes had occurred in the meantime. But I cherry picked your main commit and added three minor tweaks on top of that.

@weinand weinand mentioned this pull request Jun 27, 2016
1 of 1 task complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.