-
-
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
Dispose Homepage on navigation #4472
Conversation
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.
This is a bit overkill, considering we can just call Dispose() in YourHome's OnNavigatingFrom override. The code looks good, but I'm not sure if we need the FrameExtensions at all unless there's another reason for it.
@duke7553 we could call a function similar to |
@duke7553 Should we merge this? |
@yaichenbaum could you review this one? It's fixing a memory leak 👀 |
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.
@yaichenbaum @d2dyno1 "using File.Extensions" is unused and useless in 2 files. "WidgetPage.xaml.cs" is better.
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.
Clever enough solution to call the Dispose() method, so I suppose
Resolved / Related Issues
Details of Changes
YourHome.Dispose() is now called on navigation. It is possible to modify the code for any page to be disposed on navigation however that's not the case here. The problem with disposing every page but YourHome is that any other page is being reused and in YourHome's case, the widgets are created on navigation.
This PR fixes the issue where widgets weren't being disposed on navigaion and in Bundles case, it fixes a memory leak.
Validation
How did you test these changes?