rid00z / FreshMvvm

FreshMvvm is a super light Mvvm Framework designed specifically for Xamarin.Forms. It's designed to be Easy, Simple and Flexible.
Apache License 2.0
594 stars 172 forks source link

Doubletap/click couses trouble on Android. #75

Open oddbear opened 8 years ago

oddbear commented 8 years ago

Problems with double click/tap on binded navigation commands.

Ex.

        public Command AddContact {
            get {
                return new Command (async () => {
                    await CoreMethods.PushPageModel<ContactPageModel> ();
                });
            }
        }

Can be reproduced in the FreshMvvmSampleApp.Droid project on Nexus 5 (lollipop) emulator (mac os x).

"Basic Navigation" -> 2x click on Quotes. Opens the Quotes page 2 times. Two pages is put on the stack, before the application navigates to the one of them. (I have the same problem on my testapp on a physical Nexus 5X)

Clicking on the ADD button (ex. on Master detail page) like a maniac, crashes the app with NullReferenceException.

exception

rid00z commented 8 years ago

Hi, In this case you can disable the button until the navigation has finished. Not much was can build into the Framework for this case.

oddbear commented 8 years ago

Thanks. I see this is a Xamarin bug. However if Xamarin never fixes this, FreshMvvm could probably compensate. At least for the double tap problem.

Simple isPushing flag, would help in most cases, and a Interlock way probably fix it all together (however I am not sure what the impact on performance would be, especially on ARM).

I would probably just override the PushPage of the FreshNavigationContainer with som guarding code, until Xamarin fixes this.

rid00z commented 8 years ago

Yes we will look into this, or accept a PR.

oddbear commented 8 years ago

This seems to work for my case, however it will ignore all other PushPage events until the one executing is finished.

        private static int _isPushing = 0;

        public override async Task PushPage(Page page, FreshBasePageModel model, bool modal = false, bool animate = true)
        {
            if (0 == System.Threading.Interlocked.Exchange(ref _isPushing, 1))
            {
                try
                {
                    await base.PushPage(page, model, modal, animate);
                }
                finally
                {
                    System.Threading.Interlocked.Exchange(ref _isPushing, 0);
                }
            }
            else
            {
                System.Diagnostics.Debug.WriteLine("Double PushPage, Ignored!");
            }
        }

Edit: != changed to ==, see: https://msdn.microsoft.com/en-us/library/d3fxt78a(v=vs.110).aspx

oddbear commented 8 years ago

Looks like others have the same problem (this from 2014): https://forums.xamarin.com/discussion/18184/bug-in-xamarin-forms-navigationpage-for-android

oddbear commented 8 years ago

So I have looked a little on this one, and thought about some possible solutions. And I also see that this is a problem on iOS, but normally you won't see it as the view gets "locked".

I think the best way to solve this is to create a custom command from ICommand, and implement a way to create these commands from CoreMethods.

This will probably be the safest way. And it will be backward compatible. It can also be implemented as Extensions to Fresh, instead of in the Fresh core itself. So it could be optional.

I have created some example code on: https://github.com/oddbear/DoubleTapTest I can do I pull request, if you want, but it would be nice with some feedback.

The only thing a developer needs to do, is change void to Task in the command logic, create a shared object (or reuse the commands, with parameters for execution), and use the ICommand interface instead of Command or Command.

Other commands can also be created, but then there must be some setup/configuration logic. I think about commands like: Synchronised queued execution: if B is executed before A is finished, B waits to start until A is done. Return simultaneous: If B is executed before A is finished. B is ignored, but waits for A to be done (typical usage A and B calls the same web service, with the same query).

rid00z commented 8 years ago

@oddbear Looks good, could you package into the FreshMvvm core and do a pull request?

oddbear commented 8 years ago

I am working on a branch now: https://github.com/oddbear/FreshMvvm/tree/feature/navigation-command

I am not 100% sure about the naming, and if using the CoreMethod directly is the best ide (as you cannot do this in the constructor, but need to use the init method). I am now testing it out on the sample project, and have removed some of the problems here. But the native functions like the "home" button in the right corner (testet on Android), will still have som issues. I want to test out a little bit on those with some similar alternatives. :)

rid00z commented 8 years ago

Great work, look forward to the PR. It will solve the issue for many people.

oddbear commented 8 years ago

Pull request #99

joanbarros commented 6 years ago

@oddbear would that PR resolve the issue in which two pages are added to the navigation stack when a button is pressed twice in rapid succession?

I'm having that problem currently and I'm figuring out the best way of solving this.

oddbear commented 6 years ago

@joanbarros the PR itself does not fix anything like that, but adds a couple of "command" you optional can use as a workaround. The PR is so old so so the easiest to do might be to just copy the code into your project. For that particular scenario, ignoring the second pressed event is probably the one you want.

The implementation for ignoring would be like:

  1. Increment a number (thread safe preferably).
  2. If the number incremented is "first value", no one else is executing, if higher, ignore the push.
  3. Execute the push, in a try/finally, and await
  4. Deincrement in the finally block to reset the number.

PR Implementation: FreshNavigationCommand.cs PageModelCoreMethods.cs

PR Example of usage: MainMenuPageModel.cs

This would probably still work, but I have not tested it for a while. The important part is to await the push. Now if there has been any changes so the await is a await for the push to stack, and not show pages, then this would need some additional logic, but should be feasible.

If you have any issues, I would gladly help. There is a long time since I actually worked with Fresh, and full-time with Xamarin.Forms, but I do find this things quite interesting (and have also worked a lot more with C# concurrency issues after this).

joanbarros commented 6 years ago

@oddbear gotcha, thnx. That's what I got from reading the code but was not completely sure if something else needed to be done in the framework end.

I'll try this workaround. It seems to solve my issue rather easily.

danielkatz commented 6 years ago

I think that it's better to generalize the problem for disabling the command during any async process by implementing AsyncCommand. There are several implementations of this pattern, we used this as a base for our solution.

oddbear commented 6 years ago

@danielkatz Nice. This seems like a great solution, and actually quite similar to the PR.

It has a interesting approach to solve the "sharelock" issue also (through the ExecuteSubmitAsync, CanExecuteSubmit), that is, if you press one button, then immediately another button. Therefor a shared state between the commands is needed to not allow this.

I guess the biggest difference would be that the AsyncCommand is more Xamarin.Forms focused, and probably easier and more familiar to work with. While the approach in the PR is probably easier if you need that extra level of control, or also use it outside of XF.