maui icon indicating copy to clipboard operation
maui copied to clipboard

CarouselView doesn't scroll to the CurrentItem when set

Open marcojak opened this issue 3 years ago • 5 comments

Description

I've aded the following CarouselView in my XAML: <CarouselView ItemsSource="{Binding Strings}" CurrentItem="{Binding CurrentString}"....

In my constructor I set the ObservableCollection Strings (A,B,C,D,.....H) and then I set the CurrentItem to one of the items in the collection (D). I would expect the CarouselView to scroll to that item D, instead I can see that the CurrentItem property, get the value D, then A, then null, then A, then null again and then finally A. When the page appears I can see that the CarouselView is on the item A while it should be on the item D.

Steps to Reproduce

I've created a quick test to reproduce the issue: TestCarouselView.zip

Version with bug

6.0 (current)

Last version that worked well

Unknown/Other

Affected platforms

Android, I was not able test on other platforms

Affected platform versions

Android 11

Did you find any workaround?

If I start a new thread and add a delay of 1 second, and then I set the CurrentItem, the CarouselView scrolls correctly to the selected CurrentItem.

Given this workaround, I suspect that the CarouselView set the CurrentItem while it's still processing the ItemsSource so the CurrentItem is lost.

Relevant log output

No response

marcojak avatar May 28 '22 20:05 marcojak

verified repro on android with above project.

kristinx0211 avatar Jun 07 '22 07:06 kristinx0211

I have the same kind of problem

draghetto86 avatar Aug 31 '22 13:08 draghetto86

This is still an issue with the latest Maui for .NET 7 RC1.

Sleeckx avatar Sep 26 '22 13:09 Sleeckx

Tried today again after 4 months and the issue is still present. I really don't understand how we can start to take this platform seriously if even these big problems are not solved. I really love Xamarin and I want to love MAUI but with all these problems, this is very hard.

marcojak avatar Sep 28 '22 20:09 marcojak

Yes, the bug is still present today (current latest version from everything, .Net 6). The situation is even worse on Windows.

TeguSolutions avatar Oct 28 '22 20:10 TeguSolutions

I am also getting this weird behaviour. Any news on this?

bcaceiro avatar Nov 21 '22 12:11 bcaceiro

same here

duepuntotre avatar Dec 18 '22 15:12 duepuntotre

Still facing the same issue.

<CarouselView
      ItemsSource="{Binding DefaultColors}" 
      CurrentItem="{Binding SelectedColor, Mode=TwoWay}"
      PeekAreaInsets="40"
      Margin="0, 40"
      ItemTemplate="{StaticResource ThemeColorInfoItemTemplate}"
      >
      <CarouselView.ItemsLayout>
          <LinearItemsLayout 
              SnapPointsAlignment="Center"
              SnapPointsType="MandatorySingle"
              Orientation="Horizontal"  
              ItemSpacing="20" />
      </CarouselView.ItemsLayout>
  </CarouselView>
new void LoadSettings()
{
    DefaultColors = new(ThemeManager.Instance.AvailableThemes);
    SelectedColor =
        DefaultColors?.FirstOrDefault(theme => theme.PrimaryColor?.ToArgbHex() == HexColorCode) ??
        DefaultColors?.FirstOrDefault();
}

Setting the CurrentItem to an item from the list, still the first item is shown and switching stops to work.

AndreasReitberger avatar Feb 05 '23 11:02 AndreasReitberger

same here

minglu avatar Feb 09 '23 20:02 minglu

This needs to be fixed. How is this core function of the control not tested and working. Please fix soon.

justintemplar avatar Feb 17 '23 00:02 justintemplar

It is definitely a bug. (all written below applies to Android, this issue seems not to affect iOS, there just setting Poisition works fine for me)

Adding some debug code shows that after Carousel is rendered (either ItemsSource is changed or initial rendering) it is scrolled to 0. As far as I know (and please correct me) there is no way to execute a code after item rendering is done, and the only way is to assume some time interval the rendering is done within, and keep setting the Position property if it changes back to zero (due to scroll event).

For me the workaround below workarounds the problem:

Extensions:


internal static class TaskEx
{
    /// Executes action on backround (non-main) thread.
    public static Task RunBg(Func<Task> action, CancellationToken cancellationToken = default)
        => Task.Factory.StartNew(action, cancellationToken, TaskCreationOptions.PreferFairness, TaskScheduler.Default).Unwrap();
}

internal static class CarouselViewWorkaround
{
    private sealed class SetCarouselPositionWorkaroundHelper : IDisposable
    {
        public CarouselView Carousel { get; }

        public int DesiredPosition { get; }

        public SetCarouselPositionWorkaroundHelper(CarouselView carousel, int desiredPosition)
        {
            Carousel = carousel;
            DesiredPosition = desiredPosition;
            Carousel.Position = desiredPosition;
            Carousel.PositionChanged += OnPositionChanged;
        }

        private async Task YieldAndSetPosition()
        {
            await Task.Yield();
            await MainThread.InvokeOnMainThreadAsync(() => Carousel.Position = DesiredPosition);
        }

        private void OnPositionChanged(object? sender, PositionChangedEventArgs e)
        {
            if (e.PreviousPosition == DesiredPosition && e.CurrentPosition == 0)
            {
                _ = TaskEx.RunBg(YieldAndSetPosition);
            }
        }

        public void Dispose() => Carousel.PositionChanged -= OnPositionChanged;
    }

    public static async void KeepCarouselPosition(this CarouselView carousel, int desiredPosition, TimeSpan? duration = default)
    {
        carousel.IsScrollAnimated = false;
        using var helper = new SetCarouselPositionWorkaroundHelper(carousel, desiredPosition);
        await TaskEx.RunBg(() => Task.Delay(duration ?? TimeSpan.FromSeconds(1)));
        carousel.IsScrollAnimated = true;
    }
}

Usage:


class SomeClass
{
    private CarouselView Carousel { get; } /* initialized either by x:Name or manually */

    /* ... */

    public void Initialize(IReadOnlyList<SomeItemClass> items, int index)
    {
         // Set ItemsSource either through binding or manually
        if (items.Count > index && index >= 0)
        {
#if ANDROID
            Carousel.KeepCarouselPosition(index); // duration may be specified after the index, 1 sec by default
#else
            Carousel.Position = index;
#endif
        }
    }
}

artyomszasa avatar Mar 15 '23 07:03 artyomszasa

Verified this issue with Visual Studio Enterprise 17.6.0 Preview 6.0. Can repro on Android platform with above project. TestCarouselView.zip

Zhanglirong-Winnie avatar May 10 '23 07:05 Zhanglirong-Winnie

I also have this problem with the latest version of .net 7

DeepWorksStudios avatar Jun 01 '23 13:06 DeepWorksStudios

+1

RobTF avatar Jun 13 '23 13:06 RobTF

This is how we worked around it, in case it's useful

    /// <inheritdoc />
    protected override void OnHandlerChanged()
    {
        base.OnHandlerChanged();

        if (Handler is not null)
        {
            if (BindingContext is not IActivityAreaViewModel viewModel)
            {
                return;
            }

            /*
             * Oh MAUI.. what the f*** is this mess?
             * So the Carousel doesn't respect CurrentItem properly (https://github.com/dotnet/maui/issues/7575).
             *
             * This is definitely an Android problem, iOS remains to be seen.
             *
             * We do the following :-
             * 1. Wait 500ms (if this is removed the initial render ends up being half of one page and half of another).
             * 2. Delay setting the binding (to prevent the Carousel from stomping on the value set by the activity manager).
             * 3. Manually set CurrentItem to the actual item.
             * 4. Manually scroll to it (otherwise the user is shown the wrong, inactive page of the carousel).
             * 5. Manually set the position to the index of the current item, otherwise the first "swipe" to another page does nothing.
             *
             * Lovely, an hour and a half I won't get back... (rjh)
             */
            MainThread.InvokeOnMainThreadAsync(async () =>
            {
                await Task.Delay(500).ConfigureAwait(true);
                uiCarousel.SetBinding(CarouselView.CurrentItemProperty, "Current", BindingMode.TwoWay);
                uiCarousel.CurrentItem = viewModel.Current;
                uiCarousel.ScrollTo(viewModel.Current, animate: false);
                uiCarousel.Position = viewModel.Activities.IndexOf(viewModel.Current);
            }).FireAndForgetSafeAsync();
        }
    }

RobTF avatar Jun 13 '23 13:06 RobTF

+1

satyam16998 avatar Jun 27 '23 05:06 satyam16998

@RobTF I really want to use your workaround but, honestly, can't figure out the proper use. Would you be so kind to help, please? So, I'm overriding the OnHandlerChanged event of the xaml's codebehind file where the carousel in question is defined. In your code you call FireAndForgetSafeAsync(). What exactly is it?

MichaelShapiro avatar Jun 30 '23 01:06 MichaelShapiro

@MichaelShapiro Sure, FireAndForgetSafeAsync is just a catch all method to provide some logging/error handing on a sync/async boundary.

All it does is prevent an async void from simply disappearing should it fail, as by default these will just silently disappear.

Our implementation is this;

    /// <summary>
    /// Wraps "async void" methods in an exception handler backed by a logger.
    /// </summary>
    /// <param name="task">The task.</param>
    /// <param name="logger">The logger.</param>
    /// <param name="continueOnCapturedContext">if set to <c>true</c> the task will continue on its captured context, if any.</param>
    /// <remarks>
    /// This is to prevent async/void exceptions being "eaten" invisibly at runtime.
    /// </remarks>
    public static async void FireAndForgetSafeAsync(this Task task, ILogger? logger = null, bool continueOnCapturedContext = true)
    {
        try
        {
            if (continueOnCapturedContext)
            {
                await task.ConfigureAwait(true);
            }
            else
            {
                await task.ConfigureAwait(false);
            }
        }
#pragma warning disable CA1031 // Intentional: Exceptions should only be reported to prevent app crash.
        catch (Exception e)
#pragma warning restore CA1031
        {
            logger?.LogError(e, "Error in async/void function.");

#if DEBUG
            if (logger == null)
            {
                Debug.Write(e.ToString());
                Debug.Fail("Error in async/void function.");
                Debugger.Break();
            }
#endif
        }
    }

In principal the carousel specific code simply manually forces the carousel's "selected item" to the item specified in a property of the view model (in our case the view model is of type IActivityAreaViewModel.

We found that if you simply bind up the CurrentItem property on the carousel to the view model, when the carousel attaches to a platform specific handler, the first thing it does it try to figure out which item is selected based on its scroll position see here. Because it has just been attached the scroll position is invariably 0 and it always decides it has item 0 selected and stomps the value in the view model (due to it being a two way binding).

So instead, we don't put a binding on CurrentItem in the XAML to avoid this, and instead defer the setup until the handler has been attached and hopefully the above has already happened, then we...

  • Set the CurrentItem binding up in code (to defer the binding being in place until after the platform specific stuff has "settled down".
  • Manually set the CurrentItem property of the carousel. This makes the carousel sync up with the view model but does not update the actual view, so the user is still shown item 0 in the UI.
  • Tell the carousel to manually scroll to the correct item. This updates the UI, so the user sees the correct thing, however the scroll mechanics of the carousel are broken as some parts of the carousel still think we're at position 0. This ends up with the first few swipes a user might make fail whist the carousels internal data structures "catch up" with reality.
  • Manually set the Position property - this has the impact of preventing the issue above.

Even after all that, we found that simply running these steps results more often than not in a page which renders with the carousel "between items" so the user would see half of one item and half of another. This is likely some form of race condition with whatever UI shenanigans the platform handler is doing. The only way we found to mitigate this was to add a 500ms delay to the whole process. After that we found 100% success on the test devices we have. We try to avoid arbitrary stuff like delays and consider it a failure to come to a proper solution, but it's the best we could find in this case.

Part of the issue is that the carousel seems to have a number of properties and methods for switching items (CurrentItem, Position, ScrollTo()), and they can de-sync between themselves and the underlying handler. I don't think this control was ever tested by its authors beyond the absolute basics.

If you like I'm happy to discuss further outside of this thread, just message me (rob at codemlia dot com).

RobTF avatar Jun 30 '23 06:06 RobTF

I just wish that microsoft would fix this problem quickly

DeepWorksStudios avatar Jul 01 '23 09:07 DeepWorksStudios

@DeepWorksStudios quite... sometimes I think Microsoft thinks customers are just making this stuff up though.

RobTF avatar Jul 01 '23 10:07 RobTF

+1

flipper09112 avatar Jul 05 '23 14:07 flipper09112

@RobTF this workarround work to change view but after change view i dont reveived the events of item changed/ position changed. You can get that?

flipper09112 avatar Jul 05 '23 14:07 flipper09112

Hello lovely human, thank you for your comment on this issue. Because this issue has been closed for a period of time, please strongly consider opening a new issue linking to this issue instead to ensure better visibility of your comment. Thank you!

ghost avatar Oct 04 '23 18:10 ghost