-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Move horizontal TabView outside of PaneHolderPage #4209
Conversation
I can confirm a slight reduction in memory usage after this PR. For example, 10 open tabs went from 170 MB to 150 MB. The important part is that this PR fixes weird, user-facing behavior associated with having different instances of the TabView at once. |
For #4180 |
@yaichenbaum This should be fixed now. I suggest merging this one despite the occasional weird behavior with the sidebar because it potentially fixes a crash during resize. |
@duke7553 What's the occasional weird behavior with the sidebar that you're talking about? |
@yaichenbaum The sidebar may enter a state where there is a gap between it and the content when it is in compact mode. This behavior is relatively rare and only occurs because the property change can't be communicated to the control on every open tab when their sidebar instances are unloaded due to the tab being unselected. This only occurs when multiple tabs are open and is usually easy to get back to normal by resizing the app window/toggling the sidebar open and closed. |
@duke7553 I'll check that out when I review this PR. We can probably resolve that and reduce memory usage by moving the sidebar out of PaneHolderPage as well. |
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.
Great work! LGTM
Uff this crashes the app when toggling the "Enable dual pane mode" option in settings (because |
@gave92 Is that related to this issue https://appcenter.ms/orgs/FilesApp/apps/Files/crashes/errors/2706044090u/overview? |
@yaichenbaum yeah but @duke7553 has a fix in his other PR. |
Resolved / Related Issues
Itemize resolved / related issues by this PR.
Details of Changes
Prevent duplicate TabView instances on each page by moving the TabView to MainPage. This also includes an expanded drag region allowing users to drag the app window from the sidebar while keeping the right inset drag region which is now set a more reliable way. Finally, this PR adds back the removed TabViewItem transitions.
Validation
How did you test these changes?
Screenshots (optional)
Design remains the same and still allows for sidebar resizing