[PR #2494] [MERGED] Clean up Pane #24953

Open
opened 2026-01-31 09:06:21 +00:00 by claunia · 0 comments
Owner

📋 Pull Request Information

Original PR: https://github.com/microsoft/terminal/pull/2494
Author: @richardszalay
Created: 8/21/2019
Status: ✅ Merged
Merged: 8/28/2019
Merged by: @DHowett-MSFT

Base: master ← Head: pane-cleanup


📝 Commits (4)

  • 138fd3f Merge pane splitting methods
  • 76ff685 Rename Tab::(Can)AddSplit to (Can)SplitPane to align with Pane methods
  • 50d7e09 Remove unused local variable in Pane::_CanSplit
  • a09a591 Remove redundant 'else' branches in Pane

📊 Changes

5 files changed (+79 additions, -162 deletions)

View changed files

📝 src/cascadia/TerminalApp/App.cpp (+2 -4)
📝 src/cascadia/TerminalApp/Pane.cpp (+63 -121)
📝 src/cascadia/TerminalApp/Pane.h (+2 -5)
📝 src/cascadia/TerminalApp/Tab.cpp (+9 -28)
📝 src/cascadia/TerminalApp/Tab.h (+3 -4)

📄 Description

Cleanup of Pane.cpp based on conversations in #2450

Summary of the Pull Request

  • Merging of all Split{Horizontal,Vertical} methods into a single method that accepts Pane::SplitState
  • Rename Tab::(Can)AddSplit to (Can)SplitPane to keep the verb/noun usage consistent with Pane.cpp
  • Cleanup of a few other Pane methods, mostly removing unnecessarily nested else statements where the if always returns

When reviewing, it might be easier to split into renames and tidy up

Each change has been committed separately to make them easier to rebase out if you're not happy with them, but feel free to squash them when merging.

References

#2450

PR Checklist

  • Closes #xxx (not directly related to an issue)
  • CLA signed. If not, go over here and sign the CLA
  • Tests added/passed (N/A - there are no tests for Pane.cpp)
  • Requires documentation to be updated
  • I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place: #2450

Detailed Description of the Pull Request / Additional comments

All covered by the "Summary" above.

Validation Steps Performed

Simple usage of the terminal, and hammering the split feature to ensure it still works (including the previously fixed crash fixed by #2450)


🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.

## 📋 Pull Request Information **Original PR:** https://github.com/microsoft/terminal/pull/2494 **Author:** [@richardszalay](https://github.com/richardszalay) **Created:** 8/21/2019 **Status:** ✅ Merged **Merged:** 8/28/2019 **Merged by:** [@DHowett-MSFT](https://github.com/DHowett-MSFT) **Base:** `master` ← **Head:** `pane-cleanup` --- ### 📝 Commits (4) - [`138fd3f`](https://github.com/microsoft/terminal/commit/138fd3fe37c9355b26d35e8b756f06f5d179b02a) Merge pane splitting methods - [`76ff685`](https://github.com/microsoft/terminal/commit/76ff6850051bdd2a14fe97b3230124832528af90) Rename Tab::(Can)AddSplit to (Can)SplitPane to align with Pane methods - [`50d7e09`](https://github.com/microsoft/terminal/commit/50d7e097314e5ccf86f130b8dc4b7e8238406949) Remove unused local variable in Pane::_CanSplit - [`a09a591`](https://github.com/microsoft/terminal/commit/a09a5917783689b6e7c7895887cba92587390ade) Remove redundant 'else' branches in Pane ### 📊 Changes **5 files changed** (+79 additions, -162 deletions) <details> <summary>View changed files</summary> 📝 `src/cascadia/TerminalApp/App.cpp` (+2 -4) 📝 `src/cascadia/TerminalApp/Pane.cpp` (+63 -121) 📝 `src/cascadia/TerminalApp/Pane.h` (+2 -5) 📝 `src/cascadia/TerminalApp/Tab.cpp` (+9 -28) 📝 `src/cascadia/TerminalApp/Tab.h` (+3 -4) </details> ### 📄 Description Cleanup of Pane.cpp based on conversations in #2450 ## Summary of the Pull Request * Merging of all `Split{Horizontal,Vertical}` methods into a single method that accepts `Pane::SplitState` * Rename `Tab::(Can)AddSplit` to `(Can)SplitPane` to keep the verb/noun usage consistent with Pane.cpp * Cleanup of a few other Pane methods, mostly removing unnecessarily nested `else` statements where the `if` always returns When reviewing, it might be easier to split into [renames](https://github.com/microsoft/terminal/pull/2494/files/76ff6850051bdd2a14fe97b3230124832528af90) and [tidy up](https://github.com/microsoft/terminal/pull/2494/files/76ff6850051bdd2a14fe97b3230124832528af90..a09a5917783689b6e7c7895887cba92587390ade) Each change has been committed separately to make them easier to rebase out if you're not happy with them, but feel free to squash them when merging. ## References #2450 <!-- Please review the items on the PR checklist before submitting--> ## PR Checklist * [ ] Closes #xxx (not directly related to an issue) * [x] CLA signed. If not, go over [here](https://cla.opensource.microsoft.com/microsoft/Terminal) and sign the CLA * [ ] Tests added/passed (N/A - there are no tests for Pane.cpp) * [ ] Requires documentation to be updated * [x] I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place: #2450 ## Detailed Description of the Pull Request / Additional comments All covered by the "Summary" above. ## Validation Steps Performed Simple usage of the terminal, and hammering the split feature to ensure it still works (including the previously fixed crash fixed by #2450) --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>
claunia added the pull-request label 2026-01-31 09:06:21 +00:00
Sign in to join this conversation.
No Label pull-request
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: starred/terminal#24953