-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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 an animation to pane entrance/exit #7364
Conversation
…rformance. Let's do _better_
This doesn't work, so I'm reverting it. The grid layout is created first and foremost, so the first pane is already clipped to the smaller size, no matter what. So let's just not.
…animations-2020 # Conflicts: # src/cascadia/TerminalApp/pch.h
What about the reverse animation on close pane? |
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.
The code looks fine. I just had the one comment I left outside. I feel like the animations should be paired (in and out) before merge.
…animations-2020 # Conflicts: # src/cascadia/TerminalApp/GlobalAppSettings.cpp # src/cascadia/TerminalApp/GlobalAppSettings.h
Alright so regarding animating the closing of panes. This will be extra tricky. The simplest way to do it seems like it would be to:
But that won't actually work for us. If we leave the row/col definitions untouched for the parent when one of their children closes, then even if we make the other child bigger, the child only has half the parent to work with. So the child will get bigger, but still be clipped by the existing rol/col defs Oh but what if we
Alright so I was about to say "this is too hard for us to do now", but with the auto-sizing of the parent grid this could be possible. |
Does this feature honor the "Show animations in Windows" setting (which also powers |
@miniksa you wanted closing animations? You get closing animations |
Weird, this doesn't. But the new tab menu flyout does. But if you disable animations app-wide with I wonder why that is. I'm gonna ping some XAML folks internally for some answers on 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.
I'd definitely like for us to follow the system API -- there was an e-mail about how we can do it, so I'm gonna block for that and my couple comments.
I won't be around on Friday or Monday. I give permission to clear my review if they are fixed. I sign off apart from that.
I understand why we want a separate setting. It does feel weird though.
|
||
// Create the dummy grid. This grid will be the one we actually animate, | ||
// in the place of the closed pane. | ||
Controls::Grid dummyGrid; |
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.
shouldn't this be a border? we'll never use its containment properties..
idk, nit
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.
(Still curious tho)
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 just went with a Grid because that's my mental go-to "I need a 2d space" control. Is there a real difference the way I'm using 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.
naw
…animations-2020 # Conflicts: # src/cascadia/TerminalApp/GlobalAppSettings.h
@DHowett Hey turns out I did the OS setting thing right before I went on leave, wanna take another look? |
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.
Augh! Sorry Mike. I wrote up comments and NEVER HIT SUBMIT
// 200ms was chosen because it's quick enough that it doesn't break your | ||
// flow, but not too quick to see | ||
static const int AnimationDurationInMilliseconds = 200; | ||
static const Duration AnimationDuration = DurationHelper::FromTimeSpan(winrt::Windows::Foundation::TimeSpan(std::chrono::milliseconds(AnimationDurationInMilliseconds))); |
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.
the annoying bit here is that this has to be constructed during llibrary initialization :(
|
||
// Create the dummy grid. This grid will be the one we actually animate, | ||
// in the place of the closed pane. | ||
Controls::Grid dummyGrid; |
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.
(Still curious tho)
/azp run |
|
||
// Create the dummy grid. This grid will be the one we actually animate, | ||
// in the place of the closed pane. | ||
Controls::Grid dummyGrid; |
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.
naw
COPIED FROM BODY Yesterday, I day-of-learned about animation in XAML. The result? Summary of the Pull RequestAdds an entrance animation when panes are created. This animation can be disabled with the and in 60FPS: References
PR Checklist
Detailed Description of the Pull Request / Additional commentsAlthough the XAML animation documentation was pretty heavy on the do it in XAML route, our panes are created pretty much entirely in code, so we've got to create the animations in code as well. 200ms as the duration of the animation was picked super arbitrarily. 300ms felt too long, and 166ms felt like it was only visible for a single frame. see also:
Validation Steps PerformedMan have I been opening panes |
Azure Pipelines successfully started running 1 pipeline(s). |
Hello @DHowett! Because this pull request has the p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (
|
Adds an entrance animation when panes are created. This animation can be disabled with the `disableAnimations` global setting. Although the XAML animation documentation was pretty heavy on the _do it in XAML_ route, our panes are created pretty much entirely in code, so we've got to create the animations in code as well. 200ms as the duration of the animation was picked _super_ arbitrarily. 300ms felt too long, and 166ms felt like it was only visible for a single frame. see also: * [Motion in practice](https://docs.microsoft.com/en-us/windows/uwp/design/motion/motion-in-practice) * [This example](https://docs.microsoft.com/en-us/uwp/api/Windows.UI.Xaml.Media.Animation.Storyboard?view=winrt-19041#examples) what what I ended up using, albeit ported to cppwinrt. * [`Timeline.AllowDependentAnimations`](https://docs.microsoft.com/en-us/uwp/api/windows.ui.xaml.media.animation.timeline.allowdependentanimations?view=winrt-19041#Windows_UI_Xaml_Media_Animation_Timeline_AllowDependentAnimations) * [easing functions](https://docs.microsoft.com/en-us/windows/uwp/design/motion/key-frame-and-easing-function-animations#easing-functions) ## Validation Steps Performed Man have I been opening panes Closes microsoft#1001 Closes microsoft#7366
## Summary of the Pull Request Since we've started working on the Settings UI, a few settings have been added on `main`. This adds those missing settings over. ## References Missing settings include... - #7364: `disableAnimations` - #7873: `launchMode` `focus` and `maximizedFocus` - #7793: `bellStyle` ## Validation Steps Performed Verified that those settings appear properly in the Settings UI.
🎉 Handy links: |
Adds an entrance animation when panes are created. This animation can be
disabled with the
disableAnimations
global setting.Although the XAML animation documentation was pretty heavy on the do it
in XAML route, our panes are created pretty much entirely in code, so
we've got to create the animations in code as well.
200ms as the duration of the animation was picked super arbitrarily.
300ms felt too long, and 166ms felt like it was only visible for a
single frame.
see also:
Timeline.AllowDependentAnimations
Validation Steps Performed
Man have I been opening panes
Closes #1001
Closes #7366