Skip to content
This repository was archived by the owner on Feb 25, 2026. It is now read-only.

Changed SmoothScrollIntoView method to be truly asynchronous #4129

Merged
michael-hawker merged 13 commits into from
Aug 2, 2021
Merged

Changed SmoothScrollIntoView method to be truly asynchronous #4129

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
4590172
Changed SmoothScrollIntoView method to truly asynchronous
Vijay-Nirmal Jul 25, 2021
6f1869e
Fixed SA1502 in VoidResult struct
Vijay-Nirmal Jul 26, 2021
3945999
Fixed issue where SmoothScrollIntoView doesn't await until the animat…
Vijay-Nirmal Jul 26, 2021
602e98d
Merge branch 'SmoothScrollMadeAsync' of https://github.com/Vijay-Nirm…
Vijay-Nirmal Jul 26, 2021
525bc25
Fixed review comments. 1) Remove ConfigureAwait(true), 2) Remove Void…
Vijay-Nirmal Jul 27, 2021
b1033fb
Added Scroll indicator
Vijay-Nirmal Jul 27, 2021
8a5774e
Fixed typos
Vijay-Nirmal Jul 27, 2021
4ee302d
Fixed typo
Vijay-Nirmal Jul 27, 2021
625716b
Scroll indicators test change in code behind
Vijay-Nirmal Jul 27, 2021
a09e203
Scroll indicators test change in xaml
Vijay-Nirmal Jul 27, 2021
4b70980
Fixed Task not completing issue
Vijay-Nirmal Jul 27, 2021
70d7b0e
Fixed Task not completing issue when offset is negative
Vijay-Nirmal Jul 27, 2021
b6749fc
Merge branch 'main' into SmoothScrollMadeAsync
michael-hawker Jul 28, 2021
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
4 changes: 2 additions & 2 deletions ...osoft.Toolkit.Uwp.SampleApp/SamplePages/ListViewExtensions/ListViewExtensionsPage.xaml.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ public void OnXamlRendered(FrameworkElement control)

private void Load()
{
SampleController.Current.RegisterNewCommand("Start Smooth Scroll", (sender, args) =>
SampleController.Current.RegisterNewCommand("Start Smooth Scroll", async (sender, args) =>
{
var index = int.TryParse(IndexInput.Text, out var i) ? i : 0;
var itemPlacement = ItemPlacementInput.SelectedItem switch
Expand All @@ -55,7 +55,7 @@ private void Load()
var scrollIfVisibile = ScrollIfVisibileInput.IsChecked ?? true;
var additionalHorizontalOffset = int.TryParse(AdditionalHorizontalOffsetInput.Text, out var ho) ? ho : 0;
var additionalVerticalOffset = int.TryParse(AdditionalVerticalOffsetInput.Text, out var vo) ? vo : 0;
sampleListView.SmoothScrollIntoViewWithIndexAsync(index, itemPlacement, disableAnimation, scrollIfVisibile, additionalHorizontalOffset, additionalVerticalOffset);
await sampleListView.SmoothScrollIntoViewWithIndexAsync(index, itemPlacement, disableAnimation, scrollIfVisibile, additionalHorizontalOffset, additionalVerticalOffset);
});
Comment thread
Vijay-Nirmal marked this conversation as resolved.

if (sampleListView != null)
Expand Down
61 changes: 40 additions & 21 deletions Microsoft.Toolkit.Uwp.UI/Extensions/ListViewBase/ListViewExtensions.SmoothScrollIntoView.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ public static partial class ListViewExtensions
/// <param name="scrollIfVisible">Set false to disable scrolling when the corresponding item is in view</param>
/// <param name="additionalHorizontalOffset">Adds additional horizontal offset</param>
/// <param name="additionalVerticalOffset">Adds additional vertical offset</param>
/// <returns>Note: Even though this return <see cref="Task"/>, it will not wait until the scrolling completes</returns>
/// <returns>Returns <see cref="Task"/> that completes after scrolling</returns>
public static async Task SmoothScrollIntoViewWithIndexAsync(this ListViewBase listViewBase, int index, ScrollItemPlacement itemPlacement = ScrollItemPlacement.Default, bool disableAnimation = false, bool scrollIfVisible = true, int additionalHorizontalOffset = 0, int additionalVerticalOffset = 0)
{
if (index > (listViewBase.Items.Count - 1))
Expand Down Expand Up @@ -56,15 +56,15 @@ public static async Task SmoothScrollIntoViewWithIndexAsync(this ListViewBase li
previousXOffset = scrollViewer.HorizontalOffset;
previousYOffset = scrollViewer.VerticalOffset;

var tcs = new TaskCompletionSource<object>();
var tcs = new TaskCompletionSource<VoidResult>();

void ViewChanged(object obj, ScrollViewerViewChangedEventArgs args) => tcs.TrySetResult(result: null);
void ViewChanged(object obj, ScrollViewerViewChangedEventArgs args) => tcs.TrySetResult(result: default);

try
{
scrollViewer.ViewChanged += ViewChanged;
listViewBase.ScrollIntoView(listViewBase.Items[index], ScrollIntoViewAlignment.Leading);
await tcs.Task;
await tcs.Task.ConfigureAwait(true);
Copy link
Copy Markdown
Member

@Sergio0694 Sergio0694 Jul 27, 2021

Choose a reason for hiding this comment

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

All these ConfigureAwait(true) calls don't actually do anything and they just create more noise, we should remove them. ConfigureAwait(bool) should really only ever be used with false, and the only reason it takes a parameter is because it might possibly be useful in some niche test scenarios where you might want to control that behavior at runtime. But ConfigureAwait(true) on its own is just meaningless.

Copy link
Copy Markdown
Contributor Author

@Vijay-Nirmal Vijay-Nirmal Jul 27, 2021

Choose a reason for hiding this comment

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

I will remove it.

My thought process: In this case, ConfigureAwait must be true otherwise my code will break. Somewhere in the guidelines or in an official blog, I saw that we should specify ConfigureAwait as true if we are purposefully not using ConfigureAwait(false). Anyway, I will remove it.

Copy link
Copy Markdown
Member

@Sergio0694 Sergio0694 Jul 27, 2021

Choose a reason for hiding this comment

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

Not sure where that was from, but the official guideline is to just remove it, as it doesn't do anything.
See this blog post from Stephen Toub 😄

Copy link
Copy Markdown
Contributor Author

@Vijay-Nirmal Vijay-Nirmal Aug 8, 2021

Choose a reason for hiding this comment

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

From the blog, you point out. In my case, this must be running on the same thread (UI Thread) otherwise my code will break.

You wouldn’t, unless you were using it purely as an indication that you were purposefully not using ConfigureAwait(false)

But I agree with you, personally, I also don't like it. Just trying to follow standards while raising a PR for Microsoft itself. 😁

}
finally
{
Expand All @@ -80,20 +80,7 @@ public static async Task SmoothScrollIntoViewWithIndexAsync(this ListViewBase li
// Scrolling back to previous position
if (isVirtualizing)
{
var tcs = new TaskCompletionSource<object>();

void ViewChanged(object obj, ScrollViewerViewChangedEventArgs args) => tcs.TrySetResult(result: null);

try
{
scrollViewer.ViewChanged += ViewChanged;
scrollViewer.ChangeView(previousXOffset, previousYOffset, zoomFactor: null, disableAnimation: true);
await tcs.Task;
}
finally
{
scrollViewer.ViewChanged -= ViewChanged;
}
await scrollViewer.ChangeViewAsync(previousXOffset, previousYOffset, zoomFactor: null, disableAnimation: true).ConfigureAwait(true);
}

var listViewBaseWidth = listViewBase.ActualWidth;
Expand Down Expand Up @@ -185,7 +172,7 @@ public static async Task SmoothScrollIntoViewWithIndexAsync(this ListViewBase li
}
}

scrollViewer.ChangeView(finalXPosition, finalYPosition, zoomFactor: null, disableAnimation);
await scrollViewer.ChangeViewAsync(finalXPosition, finalYPosition, zoomFactor: null, disableAnimation).ConfigureAwait(true);
}

/// <summary>
Expand All @@ -198,10 +185,42 @@ public static async Task SmoothScrollIntoViewWithIndexAsync(this ListViewBase li
/// <param name="scrollIfVisibile">Set true to disable scrolling when the corresponding item is in view</param>
/// <param name="additionalHorizontalOffset">Adds additional horizontal offset</param>
/// <param name="additionalVerticalOffset">Adds additional vertical offset</param>
/// <returns>Note: Even though this return <see cref="Task"/>, it will not wait until the scrolling completes</returns>
/// <returns>Returns <see cref="Task"/> that completes after scrolling</returns>
public static async Task SmoothScrollIntoViewWithItemAsync(this ListViewBase listViewBase, object item, ScrollItemPlacement itemPlacement = ScrollItemPlacement.Default, bool disableAnimation = false, bool scrollIfVisibile = true, int additionalHorizontalOffset = 0, int additionalVerticalOffset = 0)
{
await SmoothScrollIntoViewWithIndexAsync(listViewBase, listViewBase.Items.IndexOf(item), itemPlacement, disableAnimation, scrollIfVisibile, additionalHorizontalOffset, additionalVerticalOffset);
await SmoothScrollIntoViewWithIndexAsync(listViewBase, listViewBase.Items.IndexOf(item), itemPlacement, disableAnimation, scrollIfVisibile, additionalHorizontalOffset, additionalVerticalOffset).ConfigureAwait(true);
}

/// <summary>
/// Changes the view of <see cref="ScrollViewer"/> asynchronous.
/// </summary>
/// <param name="scrollViewer">The scroll viewer.</param>
/// <param name="horizontalOffset">The horizontal offset.</param>
/// <param name="verticalOffset">The vertical offset.</param>
/// <param name="zoomFactor">The zoom factor.</param>
/// <param name="disableAnimation">if set to <c>true</c> disable animation.</param>
private static async Task ChangeViewAsync(this ScrollViewer scrollViewer, double? horizontalOffset, double? verticalOffset, float? zoomFactor, bool disableAnimation)
Copy link
Copy Markdown
Contributor Author

@Vijay-Nirmal Vijay-Nirmal Jul 26, 2021

Choose a reason for hiding this comment

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

Do you guys think this method should be public? Will this extension method be useful for developers as well?

Copy link
Copy Markdown
Member

@michael-hawker michael-hawker Jul 27, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah, we could expose this. I guess the idea is if you have animation the original ChangeView can take longer to return? (Like if that's the case, we should probably call that out in the method doc comment so folks know when to use this over the built-in one.)

Copy link
Copy Markdown
Contributor Author

@Vijay-Nirmal Vijay-Nirmal Jul 27, 2021

Choose a reason for hiding this comment

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

I guess the idea is if you have animation the original ChangeView can take longer to return?

I am not sure what you mean exactly. If we have animation in the original ChangeView then it will act as fire and forget not as a synchronous call. There is no built-in way to asynchronously wait until the animation completes. ChangeViewAsync methods does that, it can asynchronously wait until the animation completes.

Copy link
Copy Markdown
Member

@michael-hawker michael-hawker Jul 28, 2021

Choose a reason for hiding this comment

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

Cool, in either case, this could be helpful. @JustinXinLiu would you use something like this?

Since this PR is good to go right now, we can always open a different one later, as we'd want to improve docs with an example maybe.

{
var tcs = new TaskCompletionSource<VoidResult>();

void ViewChanged(object _, ScrollViewerViewChangedEventArgs __) => tcs.TrySetResult(result: default);

try
{
scrollViewer.ViewChanged += ViewChanged;
scrollViewer.ChangeView(horizontalOffset, verticalOffset, zoomFactor, disableAnimation);
await tcs.Task.ConfigureAwait(true);
}
finally
{
scrollViewer.ViewChanged -= ViewChanged;
}
}

/// <summary>
/// Used as a placeholder TResult to indicate that a <![CDATA[Task<TResult>]]> has a void TResult
/// </summary>
/// <see href="https://referencesource.microsoft.com/#System.Core/System/Threading/Tasks/TaskExtensions.cs,6e36a68760fb02e6,references"/>
private struct VoidResult { }
Comment thread
Vijay-Nirmal marked this conversation as resolved.
Outdated
}
}