EGoverde / Mvvm.Nucleus.Maui

MVVM Framework for MAUI mobile applications. Build on top of MAUI and the MVVM Community Toolkit.
https://www.nuget.org/packages/Mvvm.Nucleus.Maui
MIT License
4 stars 2 forks source link

IsNavigating is false, Page Being Navigated to Still Isn't Opened #1

Closed turdmaggot closed 1 month ago

turdmaggot commented 3 months ago

In Android (at least), NavigationService.IsNavigating property is false even if NavigationService.NavigateAsync(navigationParameters, isAnimated) is still opening a page.

This is the case in Android, but could also happen in other platforms.

EGoverde commented 3 months ago

The 'IsNavigating' property within Nucleus is currently set to 'true' when Shell triggers the 'ShellNavigating' event and 'false' again when the 'ShellNavigated' event is finished. This means that there will be some time between the calling of the specific NavigationService function and the callback.

Is this what you're referring to, or have you encountered an issue where the value is never changed into a true at all? Can you share more details?

The benefit of the current construction is that it'll work even when directly using Shell, although that is not exactly recommended. But we can fix the above gap by setting these values directly from within the NavigationService, as long as we ensure that all paths lead to it eventually being set to 'false' again.

turdmaggot commented 3 months ago

Is this what you're referring to, or have you encountered an issue where the value is never changed into a true at all? Can you share more details?

One thing's surely happening though, await NavigationService.NavigateAsync finishes executing even if the page being navigated to isn't shown. It causes an issue when the app is running slow to open a page, the user tends to tap on the button or list row multiple times, therefore opening multiple instances of the page being opened.

For the meanwhile I did a workaround for this on the viewmodel instead and it seems to work.

EGoverde commented 3 months ago

I will investigate a little on how Shell handles the navigation calls, as well as implement some improvements of the navigation logic (specifically surrounding the IsNavigation property) I have in mind.

EGoverde commented 3 months ago

Hi @turdmaggot , I've changed the logic for the 'IsNavigating' logic. Can you please validate these changes and confirm they work as expected on your end as well? I've added logging and an event to validate the moments this value changes.

You can either test these changes by including Nucleus with the branch /features/isNavigatingImprovements, but I've also release a nuget package 0.2.6-alpha.

turdmaggot commented 3 months ago

Let me test this out and get back to you. Just one question, should we check IsNavigating before calling NavigateAsync? Is the feature IgnoreNavigationWhenInProgress enabled by default or should we still set that somewhere?

Thanks!

EGoverde commented 3 months ago

Let me test this out and get back to you. Just one question, should we check IsNavigating before calling NavigateAsync? Is the feature IgnoreNavigationWhenInProgress enabled by default or should we still set that somewhere?

Thanks!

See https://github.com/EGoverde/Mvvm.Nucleus.Maui?tab=readme-ov-file#configuration for the default values. The value for IgnoreNavigationWhenInProgress is currently by default 'false'. This might become enabled by default in a future release though.

If you use the feature, you should not need to check the IsNavigating value before triggering a navigation request. If you want your own implementation, it should be reliable in the alpha package mentioned above, as previously there was a gap between the NavigationService and the Shell event..

Note that it's best combined with an AsyncRelayCommand, as that will ensure the command itself is only run once, after which the value should be set correctly.

turdmaggot commented 3 months ago

Hi @EGoverde

I've just tested 0.2.6-alpha on Android. I made sure that IgnoreNavigationWhenInProgress is set to true, then I double-clicked the control calling the AsyncRelayCommand that calls up the NavigateAsync. It still opens up 2 instances of the same page. I could also see from the logs that IsNavigating is being set to false while the page being navigated to hasn't opened up yet.

If this gets fixed though, I think IgnoreNavigationWhenInProgress should also prevent the nucleus popup service from executing ShowPopupAsync when IsNavigating is set to true. We're currently having issues in which both pages and popups show up multiple times as users get tired or frustrated when waiting for the page or popup to load so they tend to tap multiple times.

Thanks!

EGoverde commented 3 months ago

Hi @turdmaggot I have some changes in mind to further improve this functionality, I'll get back to that. If this is an urgent issue you could consider either subclassing the NavigationService and add logic to the overrides, then registering it in IoC. Or PR a full implementation that works as expected. But I'll try to get this one done.

turdmaggot commented 3 months ago

Hi @EGoverde,

This isn't such an urgent issue for now if we're talking about page to page navigation as we are still able to make workarounds for it on a per View or ViewModel implementation. This involves having a boolean isClicked value that gets to a value that prevents the AsyncRelayCommand from executing any further, and the value of this boolean property gets reset on the OnAppearing callback, once the user navigates back to the page navigated from. Popups are a different thing, but I think a similar mechanism where in a boolean property can be set before awaiting the ShopPopupAsync command. I'll try your suggestions though, if time still permits.

Yeah, anyways issue #2 is still the more pressing issue on our project's plate right now as it affecting multiple things.

Thanks for looking at this. -Enneth

EGoverde commented 2 months ago

Hello @turdmaggot , I have released Nucleus 0.3.0 that has additional logic for avoiding multiple navigation requests. It features two approaches - one is a check for the IsNavigating value when starting a request, the other is a 'cooldown' of sorts that ignores requests shortly after one has started. Let me know if this solves your issue.

I have looked into a similar approach for popups, but decided against it. In the PopupService you await a result, so even if we 'ignore' a request, it would still have to return a value. In this case it would be better to avoid triggering the service at all.

Perhaps a value, similar to IsNavigating, in the PopupService, could be useful for projects to check before showing a popup. I will experiment a bit with that, but it would still require logic on the client side to be useful.

turdmaggot commented 1 month ago

Thanks!

I suggest maybe a mechanism in the popup service, upon instantiating a popup, and before showing it, we can set a boolean property called AllowMultiple which could be true by default. When this is set to false, as long as the same popup hasn't been closed/dismissed, showing it would be ignored?

Apologies for the late reply. I am currently assigned to support, which involves multiple context switching in between issues.

EGoverde commented 1 month ago

Thanks!

I suggest maybe a mechanism in the popup service, upon instantiating a popup, and before showing it, we can set a boolean property called AllowMultiple which could be true by default. When this is set to false, as long as the same popup hasn't been closed/dismissed, showing it would be ignored?

Apologies for the late reply. I am currently assigned to support, which involves multiple context switching in between issues.

The mechanism to decide not to show the popup could be identical to the navigation service. The problem is that since the various ShowPopupAsync() functions need to return a value. So any code that would trigger a popup would still get it's callback, e.a. the default value given, and handle it accordingly. Even if no popup is shown on Nucleus. Which might trigger additional logic after it on the project side.

The best way to prevent the issue, is to ensure to not call the PopupService multiple times in the first place. If you wrap the call in an AsyncRelayCommand and ensure you use it's CanExecute, this should be no issue though. By default it should block triggers while it's still running. I'll try and demo this in the sample project. Note that if you have some kind of custom button, rather than the MAUI button, ensure it takes the CanExecute into account.

Regarding the navigation issue, can you validate if it is working as you expect on your end, so I can close this? Note that we depend on Shell for knowing when the navigation request is finished, which is likely before the page fully appears, but when the navigation is set into motion.

turdmaggot commented 1 month ago

Ahh yeah I forgot about that, unlike NavigateAsync(), ShowPopupAsync() needs to return a value as a callback result.

I'll take note of the AsyncRelayCommand and CanExcute, and I do remember converting most of what I've touched to use [RelayCommand]. I'm not sure though if this automatically wraps those calls into a IAsyncRelayCommand in the process.

Either way, I think you could close this. I've already did an additional project-level provision in which the base class of our ViewModels have this newly-added property called IsBusyNavigating which is set to true whenever NavigateAsync is called. It only gets set to false once the calling ViewModel's OnAppearing lifecycle callback has been fired. This should supplement your update on nucleus and I believe our team has just upgraded the nucleus package from their end as well today.

I think you can close this, but if you still want me to test for confirmation, I can do so. For the popups, I'll try to check on your suggestion, by utilizing IAsyncRelayCommand and CanExcute.

Just to add though, I've also did a few reading on a few issues from MAUI's repo, and I think there's still an issue with Android's command calls being executed more than twice.

Once again, thanks for looking at this.

turdmaggot commented 1 month ago

One last thing btw,

I just got an idea for the popups, maybe future improvements? I would put this on my "nucleus wishlist" :D

Maybe we can have another implementation of ShowPopupAsync() which should return a hypothetical PopupResult object, which contains 2 properties:

This idea was inpsired from: https://developer.android.com/reference/android/app/Activity#onActivityResult(int,%20int,%20android.content.Intent)

EGoverde commented 1 month ago

Noted @turdmaggot, I'll give it some thought. I'd like to keep things simple and even this approach would mean that when you call the popupservice you'd then always have to check for the situation that something has triggered multiple times (which would be different from a 'dismissal' of the popup).

That seems a bit tedious and messy from the perspective of the implementing project, it seems better to ensure you only call this service when you're not already doing it elsewhere.

I'll close this for now.

Best to avoid calling the service when you're still awaiting