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

Cannot use ALT to split panes after opening Close... tab submenu #7941

Closed
DHowett opened this issue Oct 16, 2020 · 5 comments · Fixed by #7961
Closed

Cannot use ALT to split panes after opening Close... tab submenu #7941

DHowett opened this issue Oct 16, 2020 · 5 comments · Fixed by #7961
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.5

Comments

@DHowett
Copy link
Member

DHowett commented Oct 16, 2020

Environment

Windows Terminal version (if applicable): 1.5.2892

Regression from #7728.

It looks like every time we try to split a pane, we try to re-register the menu items against the tab.
We try to reparent the menu items into a new context menu. Because of that, we eat an exception that they already have parents.

All Tab event registration fails afterwards for that tab.
@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 Oct 16, 2020
@DHowett DHowett added Priority-0 Bugs that we consider release-blocking/recall-class (P0) Area-User Interface Issues pertaining to the user interface of the Console or Terminal Product-Terminal The new Windows Terminal. Severity-Blocking We won't ship a release like this! No-siree. labels Oct 16, 2020
@DHowett
Copy link
Member Author

DHowett commented Oct 16, 2020

(/cc @mpela81, if you want to have a fun debugging session 😄)

@DHowett DHowett 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 Oct 16, 2020
@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label Oct 16, 2020
@DHowett DHowett added Needs-Tag-Fix Doesn't match tag requirements Priority-1 A description (P1) and removed Priority-0 Bugs that we consider release-blocking/recall-class (P0) labels Oct 16, 2020
@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label Oct 16, 2020
@DHowett DHowett added this to the Terminal v1.5 milestone Oct 16, 2020
@mpela81
Copy link
Contributor

mpela81 commented Oct 16, 2020

Fantastic 😄

As far as I can see, when splitting panes we call Tab::Initialize again, which re-builds the context menu from scratch. But we keep a reference to the closeTabsAfter/closeOtherTabs sub-menu items (to enable/disable them at runtime), so the same end up being appended to a new parent and 💣

To fix it, we could re-create the sub-items as well. Alternatively, the question is why creating the context menu again in the first place?

@DHowett
Copy link
Member Author

DHowett commented Oct 16, 2020 •
edited
Loading

why creating the context menu again in the first place?

If I had to guess, I'd say... "oversight"!

@mpela81
Copy link
Contributor

mpela81 commented Oct 16, 2020

why creating the context menu again in the first place?

If I had to guess, I'd say... "oversight"!

so... we could just move the _CreateContextMenu() call to the Tab's constructor, couldn't we?

@mpela81 mpela81 mentioned this issue Oct 19, 2020
Merged
2 tasks
@ghost ghost added the In-PR This issue has a related PR label Oct 19, 2020
@ghost ghost closed this as completed in #7961 Oct 19, 2020
ghost pushed a commit that referenced this issue Oct 19, 2020
@mpela81
Fix crash when creating tab menu multiple times (#7961)
8eaaa8a 
Fix for crash occurring when splitting a pane, due to tab context menu created multiple times.

## References
#7728 

## PR Checklist
* [x] Closes #7941 
* [x] CLA signed. 

## Detailed Description of the Pull Request / Additional comments
When splitting panes the `Tab::Initialize` function is called again. This rebuilt the context menu from scratch and appended the existing Close... sub-menu items to a new parent, thus causing the crash.
It is not necessary to re-create the context menu every time you split panes, it can be created only once.

## Validation Steps Performed
Manual verification:
- Play with the context menu, the Close... submenu is functioning
- Split panes (ALT + New tab), no crash occurs and context menu still functioning
@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 Oct 19, 2020
@ghost
Copy link

ghost commented Nov 11, 2020

🎉This issue was addressed in #7961, which has now been successfully released as Windows Terminal Preview v1.5.3142.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. 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.5
Development

Successfully merging a pull request may close this issue.

Fix crash when creating tab menu multiple times mpela81/terminal
2 participants
@DHowett @mpela81