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.

Sign up for GitHub

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

Jump to bottom

terminal: fix 'new terminal' handling #12322

Merged
merged 2 commits into from
Mar 23, 2023
Merged

terminal: fix 'new terminal' handling #12322

merged 2 commits into from
Mar 23, 2023

Conversation

vince-fugnitto
Copy link
Member

@vince-fugnitto vince-fugnitto commented Mar 17, 2023 •
edited
Loading

What it does

The pull-request fixes a regression from #12199 which prevented new terminals from being opened without a workspace present. Previously the code would prompt users to select a cwd and exit early when no cwd was selected or no workspace was present. The code now adds handling to only prompt to select the cwd if a workspace is present.

The code also refactors the logic for openTerminal and removes openTerminalFromProfile which duplicated code.

How to test

No Workspace:

  1. start the application without a workspace
  2. confirm that new terminal and new terminal (with profile) correctly spawn a terminal with the appropriate profile at the user's home (no cwd)

Non-Multi Root Workspace:

  1. start the application with a workspace
  2. confirm that new terminal and new terminal (with profile) correctly spawn a terminal with the appropriate profile at the workspace's root

Multi-Root Workspace:

  1. start the application with a multi-root workspace (you can use add folder to workspace commands)
  2. execute new terminal
  3. confirm that the prompt to select the root appears
  4. escape the prompt - a terminal should not be opened
  5. repeat steps 2-3 - select a root and confirm the cwd is correct
  6. repeat the same steps for `new terminal (with profile)

Review checklist

Reminder for reviewers

@vince-fugnitto vince-fugnitto added the terminal issues related to the terminal label Mar 17, 2023
colin-grant-work
colin-grant-work approved these changes Mar 22, 2023
View reviewed changes
Copy link
Contributor

@colin-grant-work colin-grant-work left a comment

Choose a reason for hiding this comment

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

Changes work as described. 👍

@vince-fugnitto
terminal: fix 'new terminal' handling
cc97962 
The commit fixes a regression with `new terminal` and `new terminal
(with profile)` which did not open successfully when no workspace root
was present. The code fixes the issue and also refactors the logic to
remove unnecessary duplication.

Signed-off-by: vince-fugnitto <vincent.fugnitto@ericsson.com>
@vince-fugnitto vince-fugnitto merged commit cd93bd1 into master Mar 23, 2023
@vince-fugnitto vince-fugnitto deleted the vf/fix-new-terminal branch March 23, 2023 17:39
@github-actions github-actions bot added this to the 1.36.0 milestone Mar 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@PKhamorn302531 PKhamorn302531 PKhamorn302531 approved these changes

@colin-grant-work colin-grant-work colin-grant-work approved these changes

@tsmaeder tsmaeder Awaiting requested review from tsmaeder

@msujew msujew Awaiting requested review from msujew

Assignees
No one assigned
Labels
terminal issues related to the terminal
Projects
None yet
Milestone
1.36.0
Development

Successfully merging this pull request may close these issues.
3 participants
@vince-fugnitto @colin-grant-work @PKhamorn302531