Add titlebars to panes#7371
Add titlebars to panes#7371HunterMitchell wants to merge 4 commits intomicrosoft:mainfrom HunterMitchell:feature/7290-pane-titlebar
Conversation
Removed whitespace being introduced by Visual Studio
New misspellings found, please review:
To accept these changes, run the following commands✏️ Contributor please read thisBy default the command suggestion will generate a file named based on your commit. That's generally ok as long as you add the file to your commit. Someone can reorganize it later.
If the listed items are:
See the 🔬 You can test your commits without appending to a PR by creating a new branch with that extra change and pushing it to your fork. The :check-spelling action will run in response to your push -- it doesn't require an open pull request. By using such a branch, you can limit the number of typos your peers see you make. 😉
|
zadjii-msft
left a comment
There was a problem hiding this comment.
Alright I definitely owe you a longer review, but here's something to start with just from initial glance.
| _root.Children().Append(_border); | ||
|
|
||
| // Update the pane title to reflect the term title | ||
| _control.TitleChanged([this](winrt::hstring newTitle) { |
There was a problem hiding this comment.
Both the lambda's in this function need to capture a weak ref to this with std::weak_ptr<Pane> weakThis{ shared_from_this() };
There was a problem hiding this comment.
Attempting to get a weak reference to this results in a runtime crash. The function containing this code is called only in the Pane's constructor.
There was a problem hiding this comment.
Oh well that's annoying. We might need to do add a TODO to follow-up after #3999 to make those take weakrefs. I'm not sure how we could fix those at the moment. Maybe more coffee will help...
There was a problem hiding this comment.
Sounds good, let me know what else I can do to help! Also, please note the issue regarding a crash after a child pane is closed still exists even though I marked that comment as being resolved. ☕
Changed default "showPaneTitlebar" setting to false Prevent release by locking pointer
|
So, here's a quick update. I fixed some of the comments above, however, the issue regarding a crash after a child pane is closed still exists and I have not been able to find a fix in my free time. This maybe held up by #3999? |
|
Just wanted to get this back into the spotlight in case it got lost in the pile of other feature requests / issues. I know you showed some interested in this feature @DHowett, so would you mind also taking a look at some of the code here and maybe provide some insight. I didn't want to bother @zadjii-msft since he is out. Let me know what you think! |
DHowett
left a comment
There was a problem hiding this comment.
I love this, and I think it's the right thing for us to do.
I've got a couple minor concerns that might amount to a lot of work, unfortunately. We're not really in a good place to keep adding UI to Pane. It's my goal to eventually get us to a .xaml file that we can move all the control code into leaving Pane.cpp to host the logic. That'll require us to do a bit of work to make Pane a XAML control itself, which I'm certain is the correct thing to do.
I think a titlebar needs to be a part of that. I'm undecided as to whether this should be blocked by that work.
Thanks so much for writing this up. It's really promising, and I love how it looks!
|
As much as I want this feature done as quickly as possible, I would rather us do the harder work up front so additions to the Pane titlebar and Pane itself are easier. In this case I agree that this PR should be blocked until there is a better solution like you provided. I would be up for helping rework some of it, but I have been busy recently. |
I'll close this for now, but not as a rejection. This is a sound idea that we'll revisit when we can support it best 😄 Thanks so much for doing the lifting here. |



Summary of the Pull Request
This pull request adds titlebars to panes. Below are a few features / changes:
ChromeDisabledHigh.References
This pull request should close #7290 and provides #7075 with a solution.
PR Checklist
Detailed Description of the Pull Request / Additional comments
Current Issues with this PR
_CloseChildmethod, if I don't release_firstChildor_secondChild, it works (however creating a memory leak).Future Thoughts:
Validation Steps Performed
showPaneTitlebartotrueif not already.enteror clicking away from textbox. Pressescapeto cancel the edit.