Rg.plugins.popup: PushPopupAsync() returns before popup is actually displayed

[tlhintoq]

await Navigation.PushPopupAsync(popup, false);

You can await this but... The call returns before the popup is actually displayed. So more like when the popup system receives the request for the popup to be displayed.

I've had a few situations were I await the push, do some work, dismiss the popup BEFORE IT ACTUALLY APPEARS... then the popup appears... and is on screen forever now because my code already dismissed it.

I shouldn't be running into race conditions on something I await.

[rotorgames]

@tlhintoq Hello. Thank you for question. What platforms have this problem? Android, iOS?

[alezhuk]

@tlhintoq you can wait for popup closed with help of TaskCompletionSource

[alezhuk]

@tlhintoq my code can await until popup is closed, or not await, depends on the logic :)

public async Task ShowMyPopupAsync(object context, bool waitUntilClosed = false)
{
     var myPopup = new MyPopup { BindingContext = context };
     if (waitUntilClosed)
          await myPopup.PopupClosedTask;
}

and your Page (which you showing as popup) will contain next:

public partial class MyPopup : Rg.Plugins.Popup.Pages.PopupPage
{
     private TaskCompletionSource<bool> taskCompletionSource;
     public Task PopupClosedTask { get { return taskCompletionSource.Task; } }

     public MyPopup()
     {
          InitializeComponent();
     }

     protected override void OnAppearing()
     {
          base.OnAppearing();
          taskCompletionSource = new TaskCompletionSource<bool>();
     }

     protected override void OnDisappearing()
     {
          base.OnDisappearing();
          taskCompletionSource.SetResult(true);
     }
}

You can also make PopupClosedTask as interface and implement for any Page

[rotorgames]

@alezhuk I think that he doesn't want to use callback. He has problem where await has been ended early than popup page was dissapeared.

[tlhintoq]

Clarification. In my case the popup isn't happening from a page. Its happening from a ViewModel, marshalled back to the UIthread so the ActivityIndicator on the popup still shows the animation. So the flow is...
Don't know what Page is on screen at the time.

• Push busy popup
• Do some work
• Close busy popup

The problem is not about the popup DISappearing. The problem is with code executing before the popup Appears. When calling await PushPopupAsync execution continues behind the awaited call _before_ the popup is actually on screen. The await doesn't wait.
...
That can then be compounded if only a small amount of work is done on the calling thread, then a call to dismiss the popup is made (externally. Not closed from the popup itself) - and it hasn't even really yet appeared. The dismiss call tries to tell the popup (the one that hasn't rendered yet) to close. That dismiss call get ignored because it hasn't even rendered... milliseconds tick away... the popup finally renders and displays... But the call to dismiss is long gone in our review mirror.

Now... All that being said... When I get to the office I will try to apply your same recipie that you're using for OnDisappering() to OnAppearing to see if I can get execution to actually hold up long enough for the popup to become visible. Thanks for the suggestion.

[alezhuk]

@tlhintoq I am also using aka "loading popup indicator" from ViewModel and no problems were. I'll share a code for "loading"

[rotorgames]

I think that I will create mvvm wrappers for popular mvvm frameworks soon. And you will be able to use vm to vm navigation even for popups.

[tlhintoq]

That would be awesome. Looking forward to your sample code to get over this current hump, then the wrappers down the line. Thanks a lot!

[alezhuk]

interface:

public interface IPopupService
{
     Task ShowIndicatorAsync();
     Task DismissIndicatorAsync();
}

implementation:

public class PopupService : IPopupService
{
     private IndicatorPopup indicatorPopup;
     public IndicatorPopup IndicatorPopup
     {
          get
          {
               return indicatorPopup ??
                    (indicatorPopup = new IndicatorPopup());
          }
     }

     public async Task ShowIndicatorAsync()
     {
          await PopupNavigation.Instance.PushAsync(IndicatorPopup);
     }

     public async Task DismissIndicatorAsync()
     {
          if (PopupNavigation.Instance.PopupStack.Count > uint.MinValue)
               await PopupNavigation.Instance.RemovePageAsync(IndicatorPopup, animate: false);
     }
}

and:

public partial class IndicatorPopup : Rg.Plugins.Popup.Pages.PopupPage
     {
          public IndicatorPopup()
          {
               InitializeComponent();
          }

          protected override bool OnBackButtonPressed()
          {
               return true;
          }
    }

Use DI and bla-bla to get instance of PopupService, so finally usage in ViewModel:

{
     await popupService.ShowIndicatorAsync();
     //... some useless lines of code
}
finally
{
     await popupService.DismissIndicatorAsync();
}

[tlhintoq]

Thanks for sharing that. I'll have to give some thought on how to push that into my existing app and NavigationVM.

[tlhintoq]

await PopupNavigation.Instance.PushAsync(IndicatorPopup);

Maybe I'm just missing something basic here but...
PopupNavigation. doesn't have an .Instance. It goes straight to PopupNavigation.PushAsync

[alezhuk]

It's from version 1.1.0-pre1, just remove Instance if your version is 1.0.4 and that's all

[alezhuk]

Sorry for that, I was testing the latest version of this plugin (which is not stable yet)

[tlhintoq]

Still working on this. Unfortunetly there is enough infrastrure in our project for managing popups, prioritizing them etc. that I can't just drop in a bunch of changes at once.

After more work with this... I'm starting to think this popup framework isn't thread-safe.
I can call await PopupNavigation.PopAllAsync(); and when execution breakpoints on the next line PopupNavigation.PopupStack still has content.

[rotorgames]

@tlhintoq 1.1.0-pre1 is prerelease. It can have some problems. I will work on it.

[tlhintoq]

I'm not using prelease. I'm using current stable release.

[rotorgames]

@tlhintoq You should use 1.1.0-pre1 if you want to get the last updates. v1.0.x will not be supported and will not be discussed.

[tlhintoq]

I get that will not be supported in the future, when the new version is finalized and released as stable. When 1.1.0 isn't prerelease.

But you just said yourself that 1.1.0-pre1 can have problems.

1.1.0-pre1 is prerelease. It can have some problems.

That's why its "pre". And that's why we don't use "pre" packages.

[rotorgames]

@tlhintoq Yes, 1.1.0-pre1 can have problems but I can fix them faster then for 1.0.x because v1.1.x has new opening system. Also v1.1.0 will open a page with await methods on iOS. Please just use v1.1.x and if you have error, give me a report. Thank you.

[tlhintoq]

I'll see what I can do about getting special approval for using the 'pre' package. We have a system in place for keeping our own curated repo of Nuget packages that have passed testing to ensure no new bugs are introduced, are compatible with other Nugets and so on.

I'm totally sympathetic to the situation of being able to (and wanting to) work on your current (newest) version and not spending time on an older version you know you are working to replace/update. I just need to persuade those above me to see it that way.

[rotorgames]

@tlhintoq you can wait for new stable release but I can't say when it will be. Maybe for week maybe for month. I don't know. But I can say exactly that I have finished the biggest part of all work for v1.1.0. And new release can be soon.

[tlhintoq]

I've decided to go with "Here let me show you why" as far as my bosses are concerned.
So I just updated on my machine/my branch.

Updated all the calls to PopupStack that were obsoleted and now use Instance.PopupStack.

05-16 12:53:36.845 I/MonoDroid(22636): UNHANDLED EXCEPTION:
05-16 12:53:36.895 I/MonoDroid(22636): System.MissingMethodException: Method 'Rg.Plugins.Popup.Services.PopupNavigation.get_Instance' not found.
05-16 12:53:36.905 I/MonoDroid(22636):   at System.Runtime.CompilerServices.AsyncTaskMethodBuilder.Start[TStateMachine] (TStateMachine& stateMachine) [0x00031] in /Users/builder/data/lanes/4468/f913a78a/source/mono/mcs/class/referencesource/mscorlib/system/runtime/compilerservices/AsyncMethodBuilder.cs:316 
05-16 12:53:36.905 I/MonoDroid(22636):   at HomePageViewModel.RemoveTopPopup () [0x00013] in <786e35a281804def922f88fddf7bff9d>:0 
05-16 12:53:36.905 I/MonoDroid(22636):   at ViewModelBase+<NavRemovePopup>d__7.MoveNext () [0x0000e] in {redacted}
05-16 12:53:36.905 I/MonoDroid(22636): --- End of stack trace from previous location where exception was thrown ---

Does this new version now REQUIRE the use of the IPopupService and getting an instance of it as a DependencyService as in your earlier code example?

[rotorgames]

@tlhintoq It exception is thrown on release mode or debug also?

[rotorgames]

@tlhintoq P.S. You updated only PCL project or android and ios also? I see that there is not Instance in the PopupNavigation, but then you couldn't compile this project.

[tlhintoq]

Debug mode.
Updated all projects: PCL and Platforms.

[rotorgames]

@tlhintoq Thank you for quick answer. Can you show a part of the code where you invoke this method and have this exception.

[alezhuk]

@tlhintoq IPopupService it is name in my application, which uses Popup Plugin, you can name whatever you want or use as is

[tlhintoq]

On it. I did a total PC shutdown and reboot. Just because.
TFS takes a while to load this huge project.

[rotorgames]

@tlhintoq OK

[tlhintoq]

The top block with the lambda to the second block, is from the ViewModelBase so all ViewModels can call RemoveTopPopup without knowing where the call lives. That way we can reorganize in the future without breaking everything.

So its really the reference in the second block, line 442 that is failing.

[tlhintoq]

FYI: I'm just using the static reference of PopupNavigation. I'm not making any hard instance of this... no inherited classes... No concreate object from the Interface... I didn't have to do that for 1.0.4 so I didn't expect to have to do it now.

[rotorgames]

@tlhintoq I'm thinking how I will fix it. You still can use static methods from PopupNavigation but for mvvm, DI and tests, it will be better to use the IPopupNavigation from instance.

[tlhintoq]

but for mvvm, DI and tests, it will be better to use the IPopupNavigation from instance.

Ok. So... If my HomeViewModel inherits IPopupNavigation I need to implement several methods from that Interface. What should those methods contain?

[tlhintoq]

You said the static calls could still be used. And at this point my HomePageViewModel has gone so sideways that its easier to roll it back than try to find all the RndD changes I've made.

If I'm reading what you're writing, correctly - all the call syntax to version 1.0.4 should still work with version 1.1.0-pre, right? I shouldn't be any worse off, right?

[rotorgames]

@tlhintoq Yes you still be able to use the static methods in PopupService.

[tlhintoq]

Rolled back to same code as when using 1.0.4, but still have 1.1.0-pre installed.
App runs and at least I'm back to an operational state.
I had been trying to set .Parent = null already but in the pushing of the reused popup, in the event it wasn't null when I tried to push it. The other bug thread you pointed me at made a good point of trying to null it on closing the popup. I figured it couldn't hurt.
So updated my closing methods to do this. That meant not using .PopAllAsync for example because I had to loop through the collection to get the element-instance to set its .Parent to null. IE:

        public async Task RemoveAllPopups()
        {
            if (PopupNavigation.PopupStack.Any())
            {
                foreach (var popupPage in PopupNavigation.PopupStack.Reverse())
                {
                    await RemovePopup(popupPage);
                }
            }
            return;

{still composing. Hit send too soon.}

[rotorgames]

@tlhintoq @tlhintoq Why did you set Parent = null? Popup plugin does it itself! v1.1.0-pre1 fixed all memory leaks on all platforms

[tlhintoq]

Force of habit. It costs nothing. No harm, no foul. if the framework is doing then setting null to null shouldn't break anything.

Because I am re-using popups (they render faster after the first use) if I try to push it and it has a parent then Java screams about needing to call the view to release the child.

I can comment out those lines and confirm it doesn't break. I Just needed to get back to level again before trying to move forward again.

[rotorgames]

@tlhintoq If you did RemovePageAsync or PopAsync then all renderers was be destroyed and removed from UI stack, and Parent was set to null. But it will work only in v1.1.x

[tlhintoq]

Ok. I've just confirmed all is good at this point. Confirmed better than when I had 1.0.4. Going to check that in to source control. Then back out some of those hacks for 1.0.4 and check that all is _still_ good.

[rotorgames]

@tlhintoq Do you have a MissingMethodException now?

[tlhintoq]

If I use .Instance then yes it still throws that exception.
If I use the older syntax then it works fine. So I hope you don't pull those obsoleted calls any time soon.

I did get this exception

05-16 15:01:20.669 I/MonoDroid( 4665): UNHANDLED EXCEPTION:
05-16 15:01:20.719 I/MonoDroid( 4665): System.IndexOutOfRangeException: There is not page in PopupStack

From making a "just to be safe" call to clear all popups when there weren't any.
So I updated that with a safety check... Without checking for .Any() the pop fails.

        public async Task RemoveAllPopups()
        {
            if (PopupNavigation.PopupStack.Any())
                PopupNavigation.PopAllAsync(false);

[tlhintoq]

I've pulled all the .Parent = null calls.
I've trimmed out all the wrapped threading calls I had that were marshaling the pop to the main thread.
I still need to invoke the push back to the MainUIthread but that makes sense.
Hoping to streamline this a bit. Will let you know how that goes.

        public async Task PushPopup(PopupPage popup, EventHandler closedHandler = null)
        {
            Device.BeginInvokeOnMainThread(async () =>
            {
                //popup.Parent = null;
                //if (popup.Parent != null)
                //{
                //    //Then the popup being pushed is already being displayed.
                //    //We can't re-assign the parent in this state.  Someone is
                //    //hammering on displaying the same object.
                //    return;
                //}
                //popup.Disappearing -= closedHandler; //CYA for reused popup
                if (closedHandler != null) popup.Disappearing += closedHandler;
                await Navigation.PushPopupAsync(popup);
            });

So far so good. It will take a few hours more beating for me to let my guard down, but things are looking really good.

Thank you so much## for being at the keyboard, fast responses etc. today. Absolutely wonderful !

[tlhintoq]

05-16 16:35:16.498 I/MonoDroid(28314): Java.Lang.IllegalStateException: The specified child already has a parent. You must call removeView() on the child's parent first.

I'm going to back to setting the Parent to null.

[rotorgames]

@tlhintoq Disappearing handler is invoked before Parent = null. Parent will be null only after Navigation.PopPopupAsync or Navigation.RemovePopupPageAsync

[tlhintoq]

Okay... Starting to make some sense now.
I can reuse some popups without a problem. Those are popups that get displayed now and again and generally have a long period of time between appearances. No problem with them and I can show them 50 times without issue.

Where I run into trouble re-using a dialog is when rapidly scanning pages on a USB connected scanner. The dialog popups up saying "you have x pages... do you have more" - If the user is scanning back to back to back... Then I get the "Child already has a parent" exception around the 3rd or 4th document. But never the 2nd. So I can reuse the popup at least once... but by the 2nd or 3rd reuse the exception happens.

I can only think that my workflow of reusing the popup is faster than the framework can dispose of the handles/references and (maybe) garbage collection can take place actually _releasing_ those references.

So... This is going back to what I said originally. awaited calls are returning before all the work is really done. awaiting a push returns before the popup is actually rendered and on screen. awaiting a pop is returned before the popup is real gone completely including all of its references, handles, parent connections etc.

Or am I just misunderstanding something?

[rotorgames]

@tlhintoq You should invoke push, pop, remove only in UI thread. Also, you should watch for a navigation queue. You can reuse all popups pages but you must know that when popup is pushed then its renderer is disposed!

[phamthanhtong]

I have the same issue like as @tlhintoq . And I must invoke push/ pop inside method Device.BeginInvokeOnMainThread() to sure that Popup page was displayed.

[rotorgames]

@phamthanhtong What version of the popup plugin do you use? You can fix this problem if you update the popup plugin to 1.1.0-pre2

[rotorgames]

The main problem was resolved in v.1.1.x-pre.

Summary: You must call Push/Pop/Remove a popup page only in main the main thread with await operator.

[rraallvv]

Is this still and issue in 1.1.4.145-pre?

I'm asking because I need to await for the popup to close but the code below doesn't seem to work:

Device.BeginInvokeOnMainThread(() =>
{
    Application.Current.MainPage.Navigation.PushPopupAsync(new PopupPage());
    Debug.WriteLine("Returned");
});

A breakpoint in the line Debug.WriteLine("Returned"); would be immediately reached even if the popup hasn't finished showing up yet.

[rraallvv]

Neither does this:

Device.BeginInvokeOnMainThread(async () =>
{
    await Application.Current.MainPage.Navigation.PushPopupAsync(new PopupPage());
    Debug.WriteLine("Returned");
});

[rotorgames]

@rraallvv You should use await operator. See https://github.com/rotorgames/Rg.Plugins.Popup/wiki/Navigation#xamarinformsinavigation-navigationextension

[rraallvv]

@rotorgames, I know it should work with await, but for some reason it doesn't. What I did was to pass an action that is called after the popup is closed like this:

public partial class MyPopupPage : PopupPage
{
    Action onCloseAction;

    public MyPopupPage(Action onCloseAction = null)
    {
        InitializeComponent();
        this.onCloseAction = onCloseAction;
    }

    void OnClose(object sender, EventArgs e)
    {
        onCloseAction?.Invoke();
    }
}

public partial class MyMainPage : ContentPage
{
    async public void OnItemSelected(object sender, SelectedItemChangedEventArgs e)
    {
        await Application.Current.MainPage.Navigation.PushPopupAsync(new MyPopupPage(() =>
        {
            Debug.WriteLine("OK!!! Just finished waiting...");
        }));

        Debug.WriteLine("What!!! Should have waited...");
    }
}

I'll try to put together a small project that reproduces the issue. Thanks for looking into it.

[rotorgames]

@rraallvv Ok I got it. You would like to wait when a popup returns a result but it works not that. See #14

[rraallvv]

@rotorgames, that should do the trick, thanks!

[solomonfried]

@tlhintoq my code can await until popup is closed, or not await, depends on the logic :)

public async Task ShowMyPopupAsync(object context, bool waitUntilClosed = false)
{
     var myPopup = new MyPopup { BindingContext = context };
     if (waitUntilClosed)
          await myPopup.PopupClosedTask;
}

and your Page (which you showing as popup) will contain next:

public partial class MyPopup : Rg.Plugins.Popup.Pages.PopupPage
{
     private TaskCompletionSource<bool> taskCompletionSource;
     public Task PopupClosedTask { get { return taskCompletionSource.Task; } }

     public MyPopup()
     {
          InitializeComponent();
     }

     protected override void OnAppearing()
     {
          base.OnAppearing();
          taskCompletionSource = new TaskCompletionSource<bool>();
     }

     protected override void OnDisappearing()
     {
          base.OnDisappearing();
          taskCompletionSource.SetResult(true);
     }
}

You can also make PopupClosedTask as interface and implement for any Page

This is just what I was looking for.
With the new tuple support in c#, I can easily return multiple objects without having to create a new class.
In my popup, I am accepting a string for a prompt which is stored in a UserText property. I also have an Accept and Cancel button.
The Accept button assigns 'true' to an Accepted property.
Then your OnDisappearing method becomes.

private TaskCompletionSource<(bool isAccepted , string userText)> _taskCompletionSource;
public Task<(bool isAccepted, string userText)> PopupClosedTask => _taskCompletionSource.Task;

protected override void OnDisappearing()
{
     base.OnDisappearing();
     taskCompletionSource.SetResult((Accepted, UserText));
}

My calling code looks like...

```
await PopupNavigation.Instance.PushAsync(inputPopup,true);
var ret = await inputPopup.PopupClosedTask;

Debug.WriteLine($"{ret.isAccepted} - {ret.userText}");

```

Thanks.