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

move-pane -t (existing tab) now crashes #11037

Closed
DHowett opened this issue Aug 25, 2021 · 6 comments · Fixed by #11040
Closed

move-pane -t (existing tab) now crashes #11037

DHowett opened this issue Aug 25, 2021 · 6 comments · Fixed by #11040
Assignees
@zadjii-msft
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. Priority-1 A description (P1) Product-Terminal The new Windows Terminal. Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. Severity-Blocking We won't ship a release like this! No-siree.
Milestone
Terminal v1.11

Comments

@DHowett
Copy link
Member

DHowett commented Aug 25, 2021

This may have regressed with #10982 or other Pane changes, since I don't believe the original code from @Rosefield was broken.
@DHowett DHowett added Product-Terminal The new Windows Terminal. Severity-Blocking We won't ship a release like this! No-siree. labels Aug 25, 2021
@ghost ghost added Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting Needs-Tag-Fix Doesn't match tag requirements labels Aug 25, 2021
@DHowett DHowett added this to the Terminal v1.11 milestone Aug 25, 2021
@DHowett
Copy link
Member Author

DHowett commented Aug 25, 2021 •
edited
Loading

@zadjii-msft unlike the other one (#11035), this one I will mark blocking 😄

@DHowett
Copy link
Member Author

DHowett commented Aug 25, 2021

Call stack puts us in IControl::Focus, which might make it related to #10978?

@DHowett DHowett added Area-User Interface Issues pertaining to the user interface of the Console or Terminal Priority-1 A description (P1) labels Aug 25, 2021
@DHowett
Copy link
Member Author

DHowett commented Aug 25, 2021

@zadjii-msft okay, this only happens when I use commandline mode to mp -t directly, because doing ExecuteCommandLine does another ProcessStartupActions . . .

@zadjii-msft zadjii-msft self-assigned this Aug 25, 2021
@zadjii-msft zadjii-msft added Issue-Bug It either shouldn't be doing this or needs an investigation. and removed Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting labels Aug 25, 2021
@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label Aug 25, 2021
@Rosefield
Copy link
Contributor

Rosefield commented Aug 25, 2021 •
edited
Loading

In my attempts to reproduce, it looks like the issue is when you move the last pane from a tab to another tab, and GetActiveControl returns null because there is no longer a control on the tab being destroyed.

This follows from my decision to null out the member here: https://github.com/microsoft/terminal/blob/main/src/cascadia/TerminalApp/TerminalTab.cpp#L531. The cheap fix is to just wrap the _GetActiveControl with a if (const auto control = _GetActiveControl()) ..., but the better one is probably to change those lines since I bet they violate some expectations about when a control should exist, like seen here.

@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 #11040 Aug 25, 2021
@ghost ghost removed the In-PR This issue has a related PR label Aug 25, 2021
ghost pushed a commit that referenced this issue Aug 25, 2021
@Rosefield
Only attempt to focus if there is a control to focus (#11040)
ee8800c 
Only focus if there is a control to focus (which may be null if e.g. the focused tab is being destroyed)

Closes #11037 

## Additional comments
I tried to remove the _activePane = nullptr in `TerminalTab::DetachPane` but that actually completely broke being able to focus the control at all making the tab completely unusable. Focus does seem to transfer just fine here with this change.

## Validation Steps Performed
Used the command execution to move panes to and from existing panes, including new tabs and destroying tabs.
@ghost ghost added the Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. label Aug 25, 2021
DHowett pushed a commit that referenced this issue Aug 25, 2021
@Rosefield @DHowett
Only attempt to focus if there is a control to focus (#11040)
1d090d6 
Only focus if there is a control to focus (which may be null if e.g. the focused tab is being destroyed)

Closes #11037 

## Additional comments
I tried to remove the _activePane = nullptr in `TerminalTab::DetachPane` but that actually completely broke being able to focus the control at all making the tab completely unusable. Focus does seem to transfer just fine here with this change.

## Validation Steps Performed
Used the command execution to move panes to and from existing panes, including new tabs and destroying tabs.
@ghost
Copy link

ghost commented Aug 31, 2021

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

Handy links:

@ghost
Copy link

ghost commented Aug 31, 2021

🎉This issue was addressed in #11040, 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

@zadjii-msft zadjii-msft

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. Priority-1 A description (P1) Product-Terminal The new Windows Terminal. Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. Severity-Blocking We won't ship a release like this! No-siree.
Projects
None yet
Milestone
Terminal v1.11
Development

Successfully merging a pull request may close this issue.

Only attempt to focus if there is a control to focus Rosefield/terminal
3 participants
@DHowett @Rosefield @zadjii-msft