Clean up Pane #2494
Merged
Clean up Pane #2494
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Having separate Horizontal/Vertical versions made it hard to manage, and App.cpp already made use of Pane::SplitState so it made sense to have that be the descriminator
Split was used as a noun in Tab but a verb in Pane, which felt odd
Improves readibility. All 'low hanging fruit' cases where the 'if' was returning.
miniksa
approved these changes
Aug 21, 2019
There was a problem hiding this comment.
I like it, but this is @zadjii-msft's world so I'd like to see his review before the final merge.
| // our separator _is_ the correct direction, then we should be the pane | ||
| // to resize. Otherwise, just return false, as we couldn't handle it | ||
| // either. | ||
| if ((!_firstChild->_IsLeaf()) && _firstChild->_HasFocusedChild()) |
There was a problem hiding this comment.
This pattern ("find the non-leaf's child, if it has one") appears a lot. Is there a way to pull it out into a helper? I know that we do slightly different things with each one, but the pattern looks the same:
if(!<x>.leaf && <x>.child) {
make <x> do <A>, or do <_A> ourselves
}
perhaps we can:
auto x = _findFirstChildPaneMatchingThoseCriteriaOrNot();
return x.has_value() && x->A() || _A();
zadjii-msft
approved these changes
Aug 28, 2019
|
Thanks for the contribution, as always! 😄 |
4 tasks
|
🎉 Handy links: |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Cleanup of Pane.cpp based on conversations in #2450
Summary of the Pull Request
Split{Horizontal,Vertical}methods into a single method that acceptsPane::SplitStateTab::(Can)AddSplitto(Can)SplitPaneto keep the verb/noun usage consistent with Pane.cppelsestatements where theifalways returnsWhen 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
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)