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

closeOnExit: graceful doesn't work right after moving a pane to a new tab #11035

Closed
zadjii-msft opened this issue Aug 25, 2021 · 7 comments · Fixed by #11039
Closed

closeOnExit: graceful doesn't work right after moving a pane to a new tab #11035

zadjii-msft opened this issue Aug 25, 2021 · 7 comments · Fixed by #11039
Labels
Area-User Interface Issues pertaining to the user interface of the Console or Terminal Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting Priority-2 A description (P2) Product-Terminal The new Windows Terminal. Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.
Milestone
Terminal v1.11

Comments

@zadjii-msft
Copy link
Member

likely regressed in #10780

repro steps:

  • set "closeOnExit": "graceful" (I bet always would work too, but haven't checked)
  • split a pane.
  • In the first pane, run a command. Or just type input. Do anything really, or heck don't.
  • open the command palette and run mp -t 2, to move this pane to a new tab.
  • type exit

expected:
pane exits, and with it, the tab

actual:
image

This is a bug that's only present in 1.11+, but since movePane only exists in 1.11+, it's not technically a regression. If we can't get it fixed for 1.11, we can probably just service this. I don't think this is worth blocking over.
@zadjii-msft zadjii-msft added Issue-Bug It either shouldn't be doing this or needs an investigation. Area-User Interface Issues pertaining to the user interface of the Console or Terminal Product-Terminal The new Windows Terminal. Priority-2 A description (P2) labels Aug 25, 2021
@zadjii-msft zadjii-msft added this to the Terminal v1.11 milestone Aug 25, 2021
@ghost ghost added the Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting label Aug 25, 2021
@DHowett
Copy link
Member

DHowett commented Aug 25, 2021

Huh. What other event handlers aren't getting hooked up? Could it be that Bell and the two focus trackers are also lost? Are we never calling Initialize?

@DHowett
Copy link
Member

DHowett commented Aug 25, 2021

No, we're calling _AttachEventHandlersToPane and all in AttachPane; perhaps we are not doing it in the "new tab with pane" case?

@DHowett
Copy link
Member

DHowett commented Aug 25, 2021

Yes, it looks like a pane converted into a new tab can't bell!

@DHowett DHowett mentioned this issue Aug 25, 2021
Closed
@Rosefield
Copy link
Contributor

Oh, oops, I'll take a look at this.

@Rosefield
Copy link
Contributor

So, sanity checking,

  1. TabManagement calls InitializeTab https://github.com/microsoft/terminal/blob/main/src/cascadia/TerminalApp/TabManagement.cpp#L228
  2. Which calls tab->Initialize https://github.com/microsoft/terminal/blob/main/src/cascadia/TerminalApp/TabManagement.cpp#L103
  3. Which appears to call AttachEventHandlersToPane https://github.com/microsoft/terminal/blob/main/src/cascadia/TerminalApp/TerminalTab.cpp#L210 (and is the same code path as creating a normal tab from a control)

Is this a problem with all moved panes, or just ones moved to new tabs? Admittedly one of the reasons I adopted wt in the first place was so I could turn the bell off, so I never would have noticed it missing. :)

@PankajBhojwani
Copy link
Contributor

PankajBhojwani commented Aug 25, 2021 •
edited
Loading

Is this a problem with all moved panes, or just ones moved to new tabs?

Just ones moved to new tabs I think! Did a quick trace and it seems TerminalPage::_MovePane calls _CreateNewTabFromPane, which does not attach any event handlers to the pane Ignore that, it does call Initialize hm

@Rosefield Rosefield mentioned this issue Aug 25, 2021
Merged
@ghost ghost added the In-PR This issue has a related PR label Aug 25, 2021
@ghost ghost closed this as completed in #11039 Aug 25, 2021
@ghost ghost added Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. and removed In-PR This issue has a related PR labels Aug 25, 2021
ghost pushed a commit that referenced this issue Aug 25, 2021
@Rosefield
Make sure we keep event handlers on the control when detaching a pane (…
2c5a35f 
…#11039)

When moving a pane to a new tab previously we removed the event handlers
on it as if we were closing it, but we are just moving it so we need to
keep them.

I tried really hard to make sure all of the events were hooked up
correctly, but I guess I missed these originally since they are normally
created in the Pane constructor.

Closes #11035

## Validation Steps Performed
created panes, moved them to new tabs, confirmed that they close and
ding appropriately.
@ghost
Copy link

ghost commented Aug 31, 2021

🎉This issue was addressed in #11039, which has now been successfully released as Windows Terminal Preview v1.11.2421.0.:tada:

Handy links:

This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Area-User Interface Issues pertaining to the user interface of the Console or Terminal Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting Priority-2 A description (P2) Product-Terminal The new Windows Terminal. Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.
Projects
None yet
Milestone
Terminal v1.11
Development

Successfully merging a pull request may close this issue.

Make sure we keep event handlers on the control when detaching a pane Rosefield/terminal
4 participants
@DHowett @Rosefield @zadjii-msft @PankajBhojwani