search

AvaloniaUI / Avalonia

Develop Desktop, Embedded, Mobile and WebAssembly apps with C# and XAML. The most popular .NET UI client technology
https://avaloniaui.net
MIT License
26.04k stars 2.25k forks source link

Draft: Improve Refresh-Container usability #17496

Open nickodei opened 1 week ago

nickodei commented 1 week ago

Important: this is a first draft to get more feedback and discuss some behavior that is expected to happen. The behaviour of the RefreshContainer is unintuitive for mobile users. This was picket up by this issue: #15529.

What does the pull request do?

This improved the drag-to-refresh behavior especially for mobile users and allows for a smoother experience. You are now able to start the refresh independent on the clicked position. For TopToBottom pulls it is only required that you scrolled to the top.

What is the current behavior?

15529 goes into great detail but tldr:

  1. You can only trigger a refresh in a small treshold (mostly the first item)

Aufzeichnung2024-11-13153238-ezgif com-video-to-gif-converter

  1. Weard behaviour where the items get scaled away from the container when refreshing at diffrent positions

This is especially noticable when you have configured the PullDirection to be anything than TopToBottom Aufzeichnung2-ezgif com-optimize

What is the updated/expected behavior with this PR?

You are now able to trigger a refresh independen on the position you clicked, as long as you didnt scroll down (or up when the scroll behavior is BottomToTop). Left and Right scroll can be triggerd at any time right now (because it is a List that goes from top to bottom but I didnt think about Lists or things that go from left to right).

I show the desktop version here because I could easily capture the output. On mobile the experience is a lot smoother:

Aufzeichnung3-ezgif com-optimize

I was not able to fix the weard scroll-behavior but I will still try to find the bug.

How was the solution implemented (if it's not obvious)?

I created a new implementation of the GestureRecognizer where I moved more logic into the touch-moved event to allow for a smoother scroll and refresh behavior. Just fixing the canPull condition describet in the issue was not possible because it would hinder the scroll-down and making the refresh more 'reactive' is only possible in the touch-moved event.

Checklist

Breaking changes

Discussion

There are some points I want to discuss first befor I cleaned up the implementation:

  1. Shouldn"t it be possible to refresh with the mouse as well (like shown in the GIFs) ?
  2. I was not able the fix the weard behavior in number 2. Any Tipps ?
  3. I create a new GestureRecognizer for the ScrollViewer because I needed to get the Offset reliable ? Is it possible to make it generic ?

Fixed issues

Fixes #15529

avaloniaui-bot commented 1 week ago

You can test this PR using the following package version. 11.3.999-cibuild0053260-alpha. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]

emmauss commented 1 week ago

Pushed a fix for number 2.

emmauss commented 1 week ago

It would be better to merge your gesture recognizer with the existing Pull recognizer. As long as the API is maintained, it should be fine.

avaloniaui-bot commented 1 week ago

You can test this PR using the following package version. 11.3.999-cibuild0053302-alpha. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]

nickodei commented 1 week ago

Pushed a fix for number 2.

Thank you for the commit. Unfortunately it didn't completely fix the offset-issue, especially when using BottomToTop, but it helped guide me to another working solution.

The problem arose because the offsets of the visual and visualizerVisual objects where set to the same values which is true for TopToBottom but unfortunately when using something like BottomToTop you need to add the offset caused from the visualizerVisual to your inital-offset so it "matches" the offset. This works now with all 4 directions as I would expect it

avaloniaui-bot commented 1 week ago

You can test this PR using the following package version. 11.3.999-cibuild0053304-alpha. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]

nickodei commented 1 day ago

It would be better to merge your gesture recognizer with the existing Pull recognizer. As log as the API is maintained, it should be fine.

I looked into it but I didn't find a way to do this. The problem is that Visual is just not enough for the offset calculations. ScrollViewer has the Offset, Width/Height and Extend Properties so I can even calculate the pull-direction BottomToTop with viewer.Offset.Y + viewer.Viewport.Height - viewer.Extent.Height. Event the CompositionVisual Property was not enougth. Did I miss something there?

nickodei commented 1 day ago

There are some other points I found while I was trying to improve the usability of the refresh behavior:

While this current implementation works, it is still far from perfect and does not feel that smooth like an native Android-Control. I don't know how much time I can pull into this issue, without any help.

emmauss commented 1 day ago

There are some other points I found while I was trying to improve the usability of the refresh behavior:

* Having a smooth transition between Scrolling and Refreshing is currently not possible. The problem is the `Capture(e.Pointer);` which is called in both `ScrollGestureRecoginzer` and `ScrollViewerGestureRecognizer`. It will inevitably be called by one of the recognizers and therefore the other one is skipped. So the other one does not get the important pointer events to process it. Therefore you have the feeling of "Clicking an item first to refresh" or you need to "release your finger and tap again so you can start the refresh". => E.g while it always recognizes the scroll when we match on `offset.Y >= 0`, when we capture and find out that the delta is negative (we should scroll), the `ScrollGestureRecoginzer` does not get this event and you don't scroll. Using `offset.Y > 0`, will show the behavior "Clicking an item first to refresh" because the `ScrollGestureRecoginzer` will capture all events and the  `ScrollViewerGestureRecognizer` will be starved

* I wanted to add a Dropdown on the Demo where you could choose the pull-direction and while I was able to do that, it had some side-effects that are not easy to fix. Because on PullDirection-Changed we do : `_refreshInfoProviderAdapter = new ScrollViewerIRefreshInfoProviderAdapter(PullDirection);`. This is bad because it will loose all references to the private members an therefore you cant change the PullDirection of `ScrollViewerGestureRecognizer` or when you call at some time `AdaptFromTree`, It will create a new `ScrollViewerGestureRecognizer` but the old events aren't cleared and so you have double the event-handlers with different PullDirections. My attempts where not that successful because on every route, some artifacts where sill there and every solution didn't feed good

While this current implementation works, it is still far from perfect and does not feel that smooth like an native Android-Control. I don't know how much time I can pull into this issue, without any help.

I don't quite understand the first point. If you have a list and are at the top of it, i.e. Offset = 0, pulling down will trigger the refresh. If you are not at the top, like Offset =0, pulling down will trigger scrolling with -delta Y, requiring you to release when at the top and then pulling down to trigger refresh. I think to some that behavior is preferable. But, yeah, it's not possible to transition from scrolling to pulling.

The behavior in the second point was ported directly from WinUI. It's still open for improvement, especially making it work well with binding.

Thanks for looking into this.

nickodei commented 1 day ago

Thank you for your quick answer. I was to vague on my first point so I have an example. I added some logs and the important part is that when you scroll "fast" (or just not slow enough) the first initial pointer event ScrollViewerPullRecognizer will give you a delta of 0 (PointerMoved(PointerEventArgs e) was called twice). Because I don"t know if we are pulling, I don"t capture the pointer and im not calling Capture(e.Pointer);. But because of that, the ScrollGestureRecognizer is calling Capture(e.Pointer); and therefore it will get all the event and I dont get the events that it was indeed negative and I should have scrolled (TopToBottom => Scrolling down). These events would be there on the third or next event but the ScrollViewerPullRecognizer already took these events and handled these:

[ViewRootImpl@9ec7c43[MainActivity]] ViewPostIme pointer 0
[DOTNET] Initial Position: 143.2888888888889, 145.42222222222222
[DOTNET] [ScrollViewerPullRecognizer]: Offset: 0, 0
[DOTNET] Delta: 0, 0 with current poition: 143.24166666666667, 145.42222222222222
[DOTNET] [ScrollViewerPullRecognizer]: Offset: 0, 0
[DOTNET] Delta: 0, 0 with current poition: 143.2888888888889, 145.42222222222222
[DOTNET] [ScrollGestureRecognizer]: Capturing point!
[ViewRootImpl@9ec7c43[MainActivity]] ViewPostIme pointer 1

If I now call Capture(e.Pointer); event if I the delta is 0, it reliably can detect if it needs to scroll or not (delta positiv or negative). I don't however have a possibility to say "hey, my delta is currently positive and I captured this pointer already to skip all other recognizer. I want to revert it and allow the other recognizer to react to this or the next pointers".

Edit: So in the end, if I dont Initially capture the Point to find out in the next 2 to 3 events, if my delta is positive (do scroll) or negative (do refresh), the ScrollGestureRecognizer will be faster than me and I will not get the change to refresh