Skip to content

Unregister hooks in ScrollViewerAssist when the ScrollViewer is unloaded #3133

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.

Sign up for GitHub

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

Merged
Keboo merged 2 commits into from
Mar 16, 2023
Merged

Unregister hooks in ScrollViewerAssist when the ScrollViewer is unloaded #3133

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8458eae
Unregister hooks when the ScrollViewer is unloaded
chris-crowther Mar 10, 2023
54572af
Use weak events to manage hooks
chris-crowther Mar 10, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
Unregister hooks when the ScrollViewer is unloaded
  • Loading branch information
@chris-crowther
chris-crowther committed Mar 10, 2023
commit 8458eaefe5a21cf30fd792a3e863e67af028658f
38 changes: 37 additions & 1 deletion MaterialDesignThemes.Wpf/ScrollViewerAssist.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,9 @@ public static bool GetShowSeparators(DependencyObject element)
private static readonly DependencyProperty HorizontalScrollHookProperty = DependencyProperty.RegisterAttached(
"HorizontalScrollHook", typeof(HwndSourceHook), typeof(ScrollViewerAssist), new PropertyMetadata(null));

private static readonly DependencyProperty HorizontalScrollSourceProperty = DependencyProperty.RegisterAttached(
"HorizontalScrollSource", typeof(HwndSource), typeof(ScrollViewerAssist), new PropertyMetadata(null));

public static readonly DependencyProperty SupportHorizontalScrollProperty = DependencyProperty.RegisterAttached(
"SupportHorizontalScroll", typeof(bool), typeof(ScrollViewerAssist), new PropertyMetadata(false, OnSupportHorizontalScrollChanged));

Expand All @@ -88,6 +91,8 @@ private static void OnSupportHorizontalScrollChanged(DependencyObject d, Depende
RegisterHook(sv);
}
});

OnUnloaded(scrollViewer, RemoveHook);
}
else
{
Expand Down Expand Up @@ -119,10 +124,24 @@ static void OnLoaded(ScrollViewer scrollViewer, Action<ScrollViewer> doOnLoaded)
}
}

static void OnUnloaded(ScrollViewer scrollViewer, Action<ScrollViewer> doOnUnloaded)
{
if (!scrollViewer.IsLoaded)
Copy link
Contributor

@nicolaihenriksen nicolaihenriksen Mar 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@chris-crowther chris-crowther Mar 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That guard is certainly suspicious! I'm going to step through the code again in the demo app...

Copy link
Contributor Author

@chris-crowther chris-crowther Mar 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nicolaihenriksen, you're right; this needs to support a variety of scenarios, including:

  • Tab items with permanent content in which the ScrollViewer will be loaded and unloaded multiple times
  • Tab items with transient content in which the ScrollViewer will be loaded once, unloaded once, and then (hopefully) garbage-collected
  • Runtime changes to the attached properties after the ScrollViewer has been loaded

I've pushed some changes which should cover all of the above.

{
RoutedEventHandler? onUnloaded = null;
onUnloaded = (_, _) =>
{
scrollViewer.Unloaded -= onUnloaded;
doOnUnloaded(scrollViewer);
};
scrollViewer.Unloaded += onUnloaded;
}
}

static void RemoveHook(ScrollViewer scrollViewer)
{
if (scrollViewer.GetValue(HorizontalScrollHookProperty) is HwndSourceHook hook &&
PresentationSource.FromVisual(scrollViewer) is HwndSource source)
scrollViewer.GetValue(HorizontalScrollSourceProperty) is HwndSource source)
{
source.RemoveHook(hook);
scrollViewer.SetValue(HorizontalScrollHookProperty, null);
Expand All @@ -135,6 +154,7 @@ static void RegisterHook(ScrollViewer scrollViewer)
if (PresentationSource.FromVisual(scrollViewer) is HwndSource source)
{
HwndSourceHook hook = Hook;
scrollViewer.SetValue(HorizontalScrollSourceProperty, source);
scrollViewer.SetValue(HorizontalScrollHookProperty, hook);
source.AddHook(hook);
}
Expand Down Expand Up @@ -175,6 +195,8 @@ private static void OnBubbleVerticalScrollChanged(DependencyObject d, Dependency
RegisterHook(sv);
}
});

OnUnloaded(scrollViewer, RemoveHook);
}
else
{
Expand Down Expand Up @@ -206,6 +228,20 @@ static void OnLoaded(ScrollViewer scrollViewer, Action<ScrollViewer> doOnLoaded)
}
}

static void OnUnloaded(ScrollViewer scrollViewer, Action<ScrollViewer> doOnUnloaded)
{
if (!scrollViewer.IsLoaded)
{
RoutedEventHandler? onUnloaded = null;
onUnloaded = (_, _) =>
{
scrollViewer.Unloaded -= onUnloaded;
doOnUnloaded(scrollViewer);
};
scrollViewer.Unloaded += onUnloaded;
}
}

static void RemoveHook(ScrollViewer scrollViewer)
{
scrollViewer.RemoveHandler(UIElement.MouseWheelEvent, (RoutedEventHandler)ScrollViewerOnMouseWheel);
Expand Down