Skip to content

Changed SmoothScrollIntoView method to be truly asynchronous #4129

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

Merged

Conversation

Vijay-Nirmal
Copy link
Contributor

@Vijay-Nirmal Vijay-Nirmal commented Jul 25, 2021

Fixes #4128

PR Type

What kind of change does this PR introduce?

  • Bugfix

What is the current behavior?

Currently, SmoothScrollIntoViewWithIndexAsync is not truly asynchronous but it will return a Task.

What is the new behavior?

SmoothScrollIntoView method should be truly asynchronous. It should wait until the animation completes

PR Checklist

Please check if your PR fulfills the following requirements:

  • Tested code with current supported SDKs
  • Pull Request has been submitted to the documentation repository instructions. Link:
  • Sample in sample app has been added / updated (for bug fixes / features)
  • New major technical changes in the toolkit have or will be added to the Wiki e.g. build changes, source generators, testing infrastructure, sample creation changes, etc...
  • Tests for the changes have been added (for bug fixes / features) (if applicable)
  • Header has been added to all new source files (run build/UpdateHeaders.bat)
  • Contains NO breaking changes

Other information

I have not tested this since I am encountering an issue while building the application locally. Below is the issue I am facing while building (It's not related to this code change)
image

I will try to solve this issue. In meantime, can anyone test this PR if possible? sorry for this inconvenience

@ghost
Copy link

ghost commented Jul 25, 2021

Thanks Vijay-Nirmal for opening a Pull Request! The reviewers will test the PR and highlight if there is any conflict or changes required. If the PR is approved we will proceed to merge the pull request 🙌

@ghost ghost requested review from michael-hawker, azchohfi and Rosuavio July 25, 2021 21:02
@ghost ghost added the bug 🐛 An unexpected issue that highlights incorrect behavior label Jul 25, 2021
Copy link
Contributor

@Rosuavio Rosuavio left a comment

Choose a reason for hiding this comment

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

I tested this with showing a dialog message after invoking the smooth scrolling in the sample, and it seemed that the task did not wait for the animation to complete. Is this the intended behavior?

Copy link
Contributor

@Rosuavio Rosuavio left a comment

Choose a reason for hiding this comment

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

I tested this with showing a dialog message after invoking the smooth scrolling in the sample, and it seemed that the task did not wait for the animation to complete. Is this the intended behavior?

@Vijay-Nirmal
Copy link
Contributor Author

@RosarioPulella Fixed the issue. Please re-review it. Again, thanks for testing in the first place :)

/// <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
Contributor Author

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
Member

@michael-hawker michael-hawker Jul 27, 2021

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
Contributor Author

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
Member

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.

@Rosuavio
Copy link
Contributor

@RosarioPulella Fixed the issue. Please re-review it. Again, thanks for testing in the first place :)

Happy to help @Vijay-Nirmal C: , thanks for contributing!

Also I wonder if its worth changing the sample to include an option to show a message when the scrolling finishes? (Like my experiment)

Comment on lines 232 to 234
private struct VoidResult
{
}
Copy link
Member

Choose a reason for hiding this comment

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

This type is unnecessary here, and it's also causing a non-shared generic instantiation of TaskCompletionSource<T> to be created. The previous usage of TaskCompletionSource<object> was completely fine and I don't really see a reason to change it. Besides, in the .NET 5 branch for the WinUI 3 target that'll just be moved to TaskCompletionSource (non generic) as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This type is unnecessary here

Agree. This will just prevent 4 bytes allocation in heap, that's all. I know it will be negotiable. I will remove it.

Copy link
Member

Choose a reason for hiding this comment

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

Not sure I'm following, I don't really see how this change is avoiding any heap allocations.
Also where is that 4 bytes number coming from? Even on a 32 bit system, every allocated object would always have a size of at least 8 bytes due to the space for the object header and method table pointer. Can you elaborate? 🙂

Copy link
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.

@Sergio0694 Sorry about the memory allocations, got confused with the generic implementation.

The below comment is from docs.microsoft.com

The TaskCompletionSource class doesn't have a non-generic counterpart........ (Boolean is a good default choice, but if you're concerned about the user of the Task downcasting it to a Task, you can use a private TResult type instead).

Source: https://docs.microsoft.com/en-us/dotnet/standard/asynchronous-programming-patterns/implementing-the-task-based-asynchronous-pattern#io-bound-tasks

I understand this is overkill 🤣, just need to point this out.

Copy link
Member

Choose a reason for hiding this comment

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

That but you quoted from the docs is outdated, there is a non-generic TaskCompletionSource type from .NET 5. As for your other point, I don't think we should be concerned about consumers potentially down casting to that generic task type and doing...? I don't think that'd be an issue either way, and besides we'll move to the non-generic type in the WinUI branch anyway (which has .NET 5), so this is all just temporary 🙂

Copy link
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.

I know its outdated, I have raised an issue in their repo to fix the doc issue 😉. Just had a chance to point it out before fixing 😁


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

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
Contributor Author

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
Member

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
Contributor Author

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. 😁

@ghost
Copy link

ghost commented Jul 27, 2021

This PR has been marked as "needs attention 👋" and awaiting a response from the team.

@Vijay-Nirmal
Copy link
Contributor Author

Also I wonder if its worth changing the sample to include an option to show a message when the scrolling finishes? (Like my experiment)

@RosarioPulella Sure, I will add it

@Vijay-Nirmal Vijay-Nirmal requested a review from Sergio0694 July 27, 2021 08:59
@Vijay-Nirmal
Copy link
Contributor Author

Vijay-Nirmal commented Jul 27, 2021

@RosarioPulella Added an indicator to show when scroll started and completed

4quwU0PjaI

Copy link
Contributor

@Rosuavio Rosuavio left a comment

Choose a reason for hiding this comment

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

I love the indicator! I just think that verbiage can be clearer and more accurate.

@Rosuavio
Copy link
Contributor

Rosuavio commented Jul 27, 2021

It seems making the sample show the behavior of the control more clearly has helped reveal an issue. If add a horizontally offset and scroll, the indicator changes to scrolling and never changes back. I put brake points to confirm that the task never completes.

Also I am seeing another behavior with the offsets. If I set a vertical offset and scroll, it seems to move to a position relative to its current position and not the set index. This means if I set a vertical offset to 15 it will move every time I click scroll, even if I did not change any values afterwords. Is this the intended behavior?

@ghost ghost removed the needs attention 👋 label Jul 27, 2021
@Vijay-Nirmal
Copy link
Contributor Author

It seems making the sample show the behavior of the control more clearly has helped reveal an issue. If add a horizontally offset and scroll, the indicator changes to scrolling and never changes back. I put brake points to confirm that the task never completes.

Yes, my bad. I know why this issue exists. I will fix it.

Also I am seeing another behavior with the offsets. If I set a vertical offset and scroll, it seems to move to a position relative to its current position and not the set index. This means if I set a vertical offset to 15 it will move every time I click scroll, even if I did not change any values afterwords. Is this the intended behavior?

I believe this will happen when item placement is Default and Scroll If Visible is true. I am confused about whether it's intended or not. Half of my mind says "User has set ScrollIfVisible so we must scroll with the offset", another half says "Item is already visible then why should we scroll"

Currently, I will say "No" it's not the intended behavior and write a special case for this situation.

@RosarioPulella What's your opinion?

@Rosuavio
Copy link
Contributor

I believe this will happen when item placement is Default and Scroll If Visible is true. I am confused about whether it's intended or not. Half of my mind says "User has set ScrollIfVisible so we must scroll with the offset", another half says "Item is already visible then why should we scroll"

Currently, I will say "No" it's not the intended behavior and write a special case for this situation.

@RosarioPulella What's your opinion?

I have a few thoughts, but maybe we should open an issue and discuss there, to keep this PR focused. I wanted to make sure this behavior was not introduced in this PR. It was my oversight to not discuss this part of the behavior more in the original PR.

@michael-hawker michael-hawker added next preview ✈️ Label for marking what we want to include in the next preview release for developers to try. improvements ✨ labels Jul 27, 2021
@michael-hawker michael-hawker added this to the 7.1 milestone Jul 27, 2021
@Vijay-Nirmal
Copy link
Contributor Author

If add a horizontally offset and scroll, the indicator changes to scrolling and never changes back. I put brake points to confirm that the task never completes.

Fixed this issue.

I have a few thoughts, but maybe we should open an issue and discuss there, to keep this PR focused

@RosarioPulella I will open an issue to discuss this.

Copy link
Contributor

@Rosuavio Rosuavio left a comment

Choose a reason for hiding this comment

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

Looks good now! Thanks @Vijay-Nirmal!

@michael-hawker
Copy link
Member

@Sergio0694 you good now too?

@ghost
Copy link

ghost commented Jul 28, 2021

Hello @michael-hawker!

Because this pull request has the auto merge :zap: label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

@michael-hawker
Copy link
Member

@msftbot merge once @Sergio0694 approves.

@ghost
Copy link

ghost commented Jul 28, 2021

Hello @michael-hawker!

Because you've given me some instructions on how to help merge this pull request, I'll be modifying my merge approach. Here's how I understand your requirements for merging this pull request:

  • I'll only merge this pull request if it's approved by @Sergio0694

If this doesn't seem right to you, you can tell me to cancel these instructions and use the auto-merge policy that has been configured for this repository. Try telling me "forget everything I just told you".

@michael-hawker michael-hawker merged commit 51cbc9a into CommunityToolkit:main Aug 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto merge ⚡ bug 🐛 An unexpected issue that highlights incorrect behavior improvements ✨ next preview ✈️ Label for marking what we want to include in the next preview release for developers to try.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SmoothScrollIntoView method should be truly asynchronous
5 participants