ReactiveUI: [BUG] Router.NavigateBack is still broken in some cases

Describe the bug

This is related to https://github.com/reactiveui/ReactiveUI/issues/2286 and https://github.com/reactiveui/ReactiveUI/pull/2315, with some more test cases

Steps To Reproduce

Sample: Test.zip

The sample is based on the one provided by https://github.com/reactiveui/ReactiveUI/issues/2286

There are three cases (all can be found inside ItemViewModel):

  1. NavigateBack inside Subscribe
    var completedInternally = Observable.Amb(
        Confirm.Select(_ => _),
        Cancel.Select(_ => _),
        Delete.Select(_ => _));
    
    completedInternally.Subscribe(_ =>
    {
        if (HostScreen.Router.NavigationStack.LastOrDefault() == this)
        {
            HostScreen.Router.NavigateBack.Execute().Subscribe(); 
            if (NextViewModel != null)
                HostScreen.Router.Navigate.Execute(NextViewModel).Subscribe();
        }
    });
    
  2. NavigateBack inside SelectMany
    var completedInternally = Observable.Amb(
            Confirm.Select(_ => _),
            Cancel.Select(_ => _),
            Delete.Select(_ => _))
        .Where(_ => HostScreen.Router.NavigationStack.LastOrDefault() == this)
        .SelectMany(_ => HostScreen.Router.NavigateBack.Execute());
    completedInternally.Subscribe(_ =>
    {
        if (NextViewModel != null)
            HostScreen.Router.Navigate.Execute(NextViewModel).Subscribe();
    });
    
  3. NavigateBack inside Do
    var completedInternally = Observable.Amb(
            Confirm.Select(_ => _),
            Cancel.Select(_ => _),
            Delete.Select(_ => _))
        .Where(_ => HostScreen.Router.NavigationStack.LastOrDefault() == this)
        .Do(_ => HostScreen.Router.NavigateBack.Execute().Subscribe());
    completedInternally.Subscribe(_ =>
    {
        if (NextViewModel != null)
            HostScreen.Router.Navigate.Execute(NextViewModel).Subscribe();
    });
    
Case RxUI 11.1.11 RxUI 11.1.6
1
2
3

Expected behavior

It should return to the Main Page when Save is tapped on the Production page.

Environment

  • OS: Android 7-API 24, physical device
  • Version 11.1.11 and 11.1.6
  • Device: Samsung S6

Additional context

I’m quite new to ReactiveUI (using this for my uni’s final year project), so it might the case that I am using it all wrong.

Thanks!

About this issue

  • Original URL
  • State: closed
  • Created 4 years ago
  • Comments: 15 (11 by maintainers)

Most upvoted comments

@garyng No apologies needed. I am digesting all the words and trying to find the sense in all this. Thank you for all the details. I will plug all this in and run some more tests. I am almost wondering if the previous change did anything, or just made it worst.

I think I fixed it… But I’m not sure… forgive me for pasting it here because I have to submit my assignment later 😅

/// <summary>
/// This is a <see cref="NavigationPage"/> that serves as a router.
/// </summary>
/// <seealso cref="Xamarin.Forms.NavigationPage" />
/// <seealso cref="ReactiveUI.IActivatableView" />
public class RoutedViewHost : NavigationPage, IActivatableView, IEnableLogger
{
    /// <summary>
    /// The router bindable property.
    /// </summary>
    public static readonly BindableProperty RouterProperty = BindableProperty.Create(
        nameof(Router),
        typeof(RoutingState),
        typeof(RoutedViewHost),
        default(RoutingState),
        BindingMode.OneWay);

    /// <summary>
    /// Initializes a new instance of the <see cref="Exception"/> class.
    /// </summary>
    /// <exception cref="RoutedViewHost">You *must* register an IScreen class representing your App's main Screen.</exception>
    public RoutedViewHost()
    {
        this.WhenActivated(d =>
        {
            bool currentlyPopping = false;
            bool popToRootPending = false;
            bool userInstigated = false;

            Router.NavigationChanged
                .Where(_ => Router.NavigationStack.Count == 0)
                .Select(x =>
                {
                    // Xamarin Forms does not let us completely clear down the navigation stack
                    // instead, we have to delay this request momentarily until we receive the new root view
                    // then, we can insert the new root view first, and then pop to it
                    popToRootPending = true;
                    return x;
                })
                .Subscribe()
                .DisposeWith(d);

            Router.NavigationChanged
                .CountChanged()
                .Select(_ => Router.NavigationStack.Count)
                .StartWith(Router.NavigationStack.Count)
                .Buffer(2, 1)
                .Select(counts => new
                {
                    Delta = counts[0] - counts[1],
                    Previous = counts[0],
                    Current = counts[1], 
                    // cache current viewmodel as it might change if some other Navigation command is executed midway
                    CurrentViewModel = Router.GetCurrentViewModel()
                })
                .Where(_ => !userInstigated)
                .Where(x => x.Delta > 0)
                .SelectMany(
                    async x =>
                    {
                        // XF doesn't provide a means of navigating back more than one screen at a time apart from navigating right back to the root page
                        // since we want as sensible an animation as possible, we pop to root if that makes sense. Otherwise, we pop each individual
                        // screen until the delta is made up, animating only the last one
                        var popToRoot = x.Current == 1;
                        currentlyPopping = true;

                        try
                        {
                            if (popToRoot)
                            {
                                await PopToRootAsync(true);
                            }
                            else if (!popToRootPending)
                            {
                                for (var i = 0; i < x.Delta; ++i)
                                {
                                    await PopAsync(i == x.Delta - 1);
                                }
                            }
                        }
                        finally
                        {
                            currentlyPopping = false;
                            if (CurrentPage is IViewFor page && x.CurrentViewModel != null)
                            {
                                page.ViewModel = x.CurrentViewModel;
                            }
                        }

                        return Unit.Default;
                    })
                .Subscribe()
                .DisposeWith(d);


            Router.Navigate
                .SelectMany(_ => PageForViewModel(Router.GetCurrentViewModel()))
                .SelectMany(async page =>
                {
                    bool animated = true;
                    var attribute = page.GetType().GetCustomAttribute<DisableAnimationAttribute>();
                    if (attribute != null)
                    {
                        animated = false;
                    }

                    if (popToRootPending && Navigation.NavigationStack.Count > 0)
                    {
                        Navigation.InsertPageBefore(page, Navigation.NavigationStack[0]);
                        await PopToRootAsync(animated);
                    }
                    else
                    {
                        await PushAsync(page, animated);
                    }

                    popToRootPending = false;
                    return page;
                })
                .Subscribe()
                .DisposeWith(d);

            var poppingEvent = Observable.FromEvent<EventHandler<NavigationEventArgs>, Unit>(
                eventHandler =>
                {
                    void Handler(object sender, NavigationEventArgs e) => eventHandler(Unit.Default);
                    return Handler;
                },
                x => Popped += x,
                x => Popped -= x);

            // NB: Catch when the user hit back as opposed to the application
            // requesting Back via NavigateBack
            poppingEvent
                .Where(_ => !currentlyPopping && Router != null)
                .Subscribe(_ =>
                {
                    userInstigated = true;

                    try
                    {
                        Router.NavigationStack.RemoveAt(Router.NavigationStack.Count - 1);
                    }
                    finally
                    {
                        userInstigated = false;
                    }

                    var vm = Router.GetCurrentViewModel();
                    if (CurrentPage is IViewFor page && vm != null)
                    {
                        // don't replace view model if vm is null
                        page.ViewModel = vm;
                    }
                })
                .DisposeWith(d);
        });

        var screen = Locator.Current.GetService<IScreen>();
        if (screen == null)
        {
            throw new Exception("You *must* register an IScreen class representing your App's main Screen");
        }

        Router = screen.Router;

        this.WhenAnyValue(x => x.Router)
            .SelectMany(router =>
            {
                return router.NavigationStack.ToObservable()
                    .Select(x => (Page) ViewLocator.Current.ResolveView(x))
                    .SelectMany(x => PushAsync(x).ToObservable())
                    .Finally(() =>
                    {
                        var vm = router.GetCurrentViewModel();
                        if (vm == null)
                        {
                            return;
                        }

                        ((IViewFor) CurrentPage).ViewModel = vm;
                        CurrentPage.Title = vm.UrlPathSegment;
                    });
            })
            .Subscribe();
    }

    /// <summary>
    /// Gets or sets the <see cref="RoutingState"/> of the view model stack.
    /// </summary>
    public RoutingState Router
    {
        get => (RoutingState) GetValue(RouterProperty);
        set => SetValue(RouterProperty, value);
    }

    /// <summary>
    /// Pages for view model.
    /// </summary>
    /// <param name="vm">The vm.</param>
    /// <returns>An observable of the page associated to a <see cref="IRoutableViewModel"/>.</returns>
    [SuppressMessage("Design", "CA1822: Can be made static", Justification = "Might be used by implementors.")]
    protected IObservable<Page> PageForViewModel(IRoutableViewModel vm)
    {
        if (vm == null)
        {
            return Observable.Empty<Page>();
        }

        var ret = ViewLocator.Current.ResolveView(vm);
        if (ret == null)
        {
            var msg =
                $"Couldn't find a View for ViewModel. You probably need to register an IViewFor<{vm.GetType().Name}>";

            return Observable.Throw<Page>(new Exception(msg));
        }

        ret.ViewModel = vm;

        var pg = (Page) ret;
        pg.Title = vm.UrlPathSegment;

        return Observable.Return(pg);
    }
}
  1. I used Buffer for a sliding window on NavigationStack count changes
  2. The current view model is cached when there is a new count changes, instead of calling later. This is imo a workaround for the race condition, but I’m not sure how to fix it properly though
  3. A few null checks on view model to prevent it from replacing the existing view model