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

Fix dangling active terminal on delete #32161

Merged
merged 2 commits into from
Aug 9, 2017
Merged

Conversation

Lixire
Copy link
Contributor

@Lixire Lixire commented Aug 8, 2017

Fixes an issue where if you delete a terminal that is not the last one and you also have the active terminal as the last one, the index doesn't get updated and the terminal panel title is blank.

Steps to reproduce:

  1. Create three terminals and focus the last one
  2. Delete the second one through the quick picker
  3. Observe that the panel title is blank

@Lixire Lixire added terminal Integrated terminal issues quick-pick Quick-pick widget issues labels Aug 8, 2017
@Lixire Lixire added this to the August 2017 milestone Aug 8, 2017
@Lixire Lixire self-assigned this Aug 8, 2017
@Lixire Lixire requested a review from roblourens August 8, 2017 23:01
@@ -95,6 +95,9 @@ export class QuickKillTerminalAction extends Action {
if (terminal) {
terminal.dispose();
}
if (this.terminalService.activeTerminalInstanceIndex !== terminalIndex) {
this.terminalService.setActiveInstanceByIndex(Math.min(terminalIndex, this.terminalService.terminalInstances.length - 1));
Copy link
Member

Choose a reason for hiding this comment

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

Is it a problem if there's only one terminal, and you delete it? Does it call this with -1?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It works as expected, but we probably should check for that case

@roblourens
Copy link
Member

I just realized that I get a green square for reviewing a PR, but not for merging it.

@roblourens roblourens merged commit bac7689 into master Aug 9, 2017
@Lixire Lixire mentioned this pull request Aug 10, 2017
6 tasks
@Lixire Lixire deleted the amqi/terminal-quickopen branch August 16, 2017 17:41
@github-actions github-actions bot locked and limited conversation to collaborators Mar 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
quick-pick Quick-pick widget issues terminal Integrated terminal issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants