-
Notifications
You must be signed in to change notification settings - Fork 8.5k
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.
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
Add titlebars to panes #7371
Add titlebars to panes #7371
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. 😉
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
_CloseChild
method, if I don't release_firstChild
or_secondChild
, it works (however creating a memory leak).Future Thoughts:
Validation Steps Performed
showPaneTitlebar
totrue
if not already.enter
or clicking away from textbox. Pressescape
to cancel the edit.