Skip to content
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

Fix: Enable seamless forward and backward transition in VirtualizingCarouselPanel #14125

Conversation

LuckyGeorge1975
Copy link
Contributor

@LuckyGeorge1975 LuckyGeorge1975 commented Jan 5, 2024

What does the pull request do?

The calculation of the 'forward' flag has been changed. The change only applies when using more than 2 items in the carousel. If using only 2 items the flag is calculated as before.

What is the current behavior?

Currently the flag changes from forward to backward when transitioning from the last element to the first element in the item list and the flag changes from backward to forward when transitioning from the first item to the last item. The transition is not like a carousel but more like a linear stack.

What is the updated/expected behavior with this PR?

The transition direction does not change when transitioning from the first or last item when using more than 2 items. The transition is now seamless.

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

When using more than 2 items it is checked if the _transitionFromIndex is the last one and the _realizedIndex is the first or if the _transitionFromIndex is the first and the _realizedIndex is the last.

Checklist

Breaking changes

Obsoletions / Deprecations

Fixed issues

Fixes #14115

Changes the calculation of the "forward" flag for the page transition to enable seamless forward and backward transitions.
@LuckyGeorge1975
Copy link
Contributor Author

@dotnet-policy-service agree

Added Check for more than 2 items in carousel as there is actually no way to determine the correct transition with only 2 items. If there are only 2 items the transition behaviour is not changed.
@LuckyGeorge1975
Copy link
Contributor Author

There is currently no way to calculate the correct transition direction when using only 2 items. So the fix only applies when using more than 2 items in a carousel.

Description
When using only two items the panel does not know if the transition should be forward or backward when transitioning from index 1 to index 0.

@avaloniaui-bot
Copy link

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

Added Unit Tests for cycling through list forward and backward
@jp2masa
Copy link
Contributor

jp2masa commented Jan 5, 2024

Maybe the carousel should be reworked to work with deltas? I think that's the only way to get a circular behavior.

@avaloniaui-bot
Copy link

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

@LuckyGeorge1975
Copy link
Contributor Author

LuckyGeorge1975 commented Jan 5, 2024

@jp2masa

Thought about it. But how would you calculate the delta direction if the user changes SelectedItem or SelectedIndex via DataBinding or programmatically? I'm currently thinking about a change to calculate the "shortest" distance between _transitionFromIndex and _realizedIndex. Imagine a carousel with 10 items and you want to transition from 2 to 9 - in that case the transition should go backward and not forward.

But this will not solve the issue when using only 2 items as you have no idea what the user really wants. The only way to make this work correctly is to make SelectedItem and SelectedIndex readonly and only allow transitions with the Carousel.Next() and Carousel.Previous() methods. But that change would cripple the control.

@LuckyGeorge1975 LuckyGeorge1975 marked this pull request as ready for review January 5, 2024 14:45
@jp2masa
Copy link
Contributor

jp2masa commented Jan 5, 2024

The only way to make this work correctly is to make SelectedItem and SelectedIndex readonly and only allow transitions with the Carousel.Next() and Carousel.Previous() methods. But that change would cripple the control.

In my opinion, the carousel, being a circular control, should have readonly selection properties and some kind of Move(int delta) public method.

Anyway, I don't make decisions here, so someone from the core team will have to confirm what approach is preferred for the future.

@avaloniaui-bot
Copy link

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

@maxkatz6 maxkatz6 merged commit 56f118d into AvaloniaUI:master Feb 8, 2024
4 of 6 checks passed
@maxkatz6 maxkatz6 added the backport-candidate-11.0.x Consider this PR for backporting to 11.0 branch label Feb 8, 2024
maxkatz6 pushed a commit that referenced this pull request Feb 8, 2024
…arouselPanel (#14125)

* Update VirtualizingCarouselPanel.cs

Changes the calculation of the "forward" flag for the page transition to enable seamless forward and backward transitions.

* Update VirtualizingCarouselPanel.cs

Added Check for more than 2 items in carousel as there is actually no way to determine the correct transition with only 2 items. If there are only 2 items the transition behaviour is not changed.

* Update VirtualizingCarouselPanelTests.cs

Added Unit Tests for cycling through list forward and backward
@maxkatz6 maxkatz6 added backported-11.0.x and removed backport-candidate-11.0.x Consider this PR for backporting to 11.0 branch labels Feb 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Change calculation of "forward" flag in VirtualizingCarouselPanel
4 participants