Conversation
| _size = size.ToSize(); | ||
| _needsArrange = true; | ||
|
|
||
| if (IsCarouselViewCell && PlatformHandler?.VirtualView is { } virtualView) |
There was a problem hiding this comment.
Consider adding an inline comment explaining the rationale for applying a custom measurement override for carousel view cells to aid future maintainers.
|
Azure Pipelines successfully started running 3 pipeline(s). |
|
Hi! I tried this, it seems ItemsLayout="HorizontalList" is not respected. All CarouselViews in my project become vertical (at least child Grid elements are arranged vertically) |
|
@albyrock87 @kubaflo I tested it again. CV1 seems to work as expected. But CV2 doesn't - all child elements are arranged vertically. This is also easily reproduced in the original sample:
Direct ItemsLayout="HorizontalList" does not affect the behavior. scr_rec.movYou can also replace the MainPage.xml code in the original example with this simpler one, and then scroll the CarouselView in different directions: |
|
@jsuarezruiz the fix is ready here kubaflo#4 I really don't understand why, but passing the scroll direction makes the Carousel2 handler crash the app without any message :/ We can wait for the merge or you can cherry pick and run AZP immediately |
|
I have a proposal. It doesn't seem to be directly related to this PR, but since it's called "Improve iOS CarouselView performance", maybe we should fix this right away? If I set the scroll index for a CarouselView with CV2 on iOS "too early" I get a random exception "cannot access disposed object NSPathIndex". Perhaps we should move the projectedPosition calculation a little lower in CarouselViewController2.cs by changing this part: to something like this? Thanks |
|
@Bamich I think that's reasonable. |
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
|
@albyrock87 Yes, I checked and confirm that the last commit fixes the scroll orientation issue |
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
| var element = app.FindElements(automationId).FirstOrDefault(); | ||
| if (element != null && (element.GetText()?.Contains(text, StringComparison.OrdinalIgnoreCase) ?? false)) | ||
|
|
||
| if (element != null && element.TryGetText(out var s) && s.Contains(text, StringComparison.OrdinalIgnoreCase)) |
There was a problem hiding this comment.
If you need to touch the PR again, this can be:
| if (element != null && element.TryGetText(out var s) && s.Contains(text, StringComparison.OrdinalIgnoreCase)) | |
| if (element is not null && element.TryGetText(out var s) && s.Contains(text, StringComparison.OrdinalIgnoreCase)) |
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
|
|
||
| if (newLayout is UICollectionViewCompositionalLayout compositionalLayout) | ||
| { | ||
| // Note: on carousel layout, the scroll direction is always vertical |
There was a problem hiding this comment.
I don t understand this comment, can t we have a CarouselView for scroll left and right or up and down? So 2 directions?
There was a problem hiding this comment.
What I'm saying is that this variable on carousel is always set to vertical, even when having a horizontal carousel.
Horizontal carousel is made in a different way.
There was a problem hiding this comment.
I think we do it in this way (always set to vertical) to achieve horizontal paging with snapping (GroupPagingCentered). Thanks to it, we can use OrthogonalScrollingBehavior.GroupPagingCentered to scroll the section horizontally. And even if CarouselView is vertically oriented, each section scrolls horizontally — which results in the carousel-style behavior
|
|
||
| private protected virtual (Type CellType, string CellTypeReuseId) DetermineTemplatedCellType() | ||
| { | ||
| return (ItemsViewLayout.ScrollDirection == UICollectionViewScrollDirection.Vertical ? typeof(VerticalCell) : typeof(HorizontalCell), "maui"); |
There was a problem hiding this comment.
Shouldn't this use the ReuseID ?
There was a problem hiding this comment.
It wasn't using it, so I kept the old reuseId creation logic.
There was a problem hiding this comment.
Oh this was just reverted to what it was?
There was a problem hiding this comment.
I'm referring to line 508 above
var reuseId = $"_maui_{cellOrientation}_{dataTemplate.Id}";
It was using this maui string so I simply kept it like that.
While CV2 is a different story: there it was using the reuseId so I used it.
|
/rebase |
…50407.3 (dotnet#28892) Microsoft.DotNet.XHarness.CLI , Microsoft.DotNet.XHarness.TestRunners.Common , Microsoft.DotNet.XHarness.TestRunners.Xunit From Version 9.0.0-prerelease.25203.2 -> To Version 9.0.0-prerelease.25207.3 Co-authored-by: dotnet-maestro[bot] <dotnet-maestro[bot]@users.noreply.github.com>
* [ai]Add release notes prompt * Improve prompt release notes * [ai] improve prompt * [ai] Try map commits to github users * Small update to get correct github user * [ai] Add list of common users * [ai] Remove extra vriable
5e7524c to
4f0d868
Compare
|
/rebase |
|
Closed in favour of #29035 |

Description
A proposed fix for a minor regression introduced in this pr #28225
Issues Fixed
Fixes #28930
@albyrock87