Unregister hooks in ScrollViewerAssist when the ScrollViewer is unloaded #3133
Conversation
| @@ -119,10 +124,24 @@ static void OnLoaded(ScrollViewer scrollViewer, Action<ScrollViewer> doOnLoaded) | |||
| } | |||
| } | |||
|
|
|||
| static void OnUnloaded(ScrollViewer scrollViewer, Action<ScrollViewer> doOnUnloaded) | |||
| { | |||
| if (!scrollViewer.IsLoaded) | |||
There was a problem hiding this comment.
Sorry for barging in on this code review; I was curious. 😃
This guard is a bit suspicious to me. Why do we only want to listen for the Unloaded event if the ScrollViewer is not already loaded? I think the situation where it IS loaded is equally relevant. This would be a scenario where the AP is not statically set in XAML, but applied at runtime after the ScrollViewer has loaded. I acknowledge this may not be the most common use case, but it is still possible. In short, I think we should handle both the ScrollViewer.IsLoaded=true/false cases.
Another thing to consider is using weak event handlers. Both the Loaded and Unloaded event handlers are currently registered as strong event handlers which unregister themselves when the corresponding event fires. However, I think there are scenarios where this will not work - not particularly related to your changes though:
If the ScrollViewer resides in a TabÌtem, it will be loaded and unloaded multiple times when switching between the tabs. The current implementation only seems to wire up the hooks when the AP changes which means it will register for the Loaded event at startup. However, after navigating away from the TabItem and back to it again, the Unloaded event will fire, removing the hook, and then there is nothing listening for the "second" Loaded event to re-register the hook. Perhaps this could be mitigated by simply using weak event handlers and never unregistering them?
This comment also applies for the changes to the other AP.
Thoughts?
There was a problem hiding this comment.
That guard is certainly suspicious! I'm going to step through the code again in the demo app...
There was a problem hiding this comment.
@nicolaihenriksen, you're right; this needs to support a variety of scenarios, including:
- Tab items with permanent content in which the
ScrollViewerwill be loaded and unloaded multiple times - Tab items with transient content in which the
ScrollViewerwill be loaded once, unloaded once, and then (hopefully) garbage-collected - Runtime changes to the attached properties after the
ScrollViewerhas been loaded
I've pushed some changes which should cover all of the above.
|
Awesome. Thank you for putting this together |
Fixes #3130.
This covers the necessary clean-up of registered Win32 hooks and event handlers for the
SupportHorizontalScrollandBubbleVerticalScrollattached properties.To unregister the
HwndSourceHookwe have to capture theHwndSourcein the Loaded event. I'm happy to consider an alternative approach.I was tempted to promote the duplicated
OnLoadedandOnUnloadedlocal methods to instance level methods as they are not property-specific.