Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Doubletap/click couses trouble on Android. #75

Open
oddbear opened this issue Jun 7, 2016 · 15 comments
Open

Doubletap/click couses trouble on Android. #75

oddbear opened this issue Jun 7, 2016 · 15 comments

Comments

@oddbear
Copy link

oddbear commented Jun 7, 2016

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
Copy link
Owner

rid00z commented Jun 8, 2016

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.

@rid00z rid00z closed this as completed Jun 8, 2016
@oddbear
Copy link
Author

oddbear commented Jun 8, 2016

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 rid00z reopened this Jun 8, 2016
@rid00z
Copy link
Owner

rid00z commented Jun 8, 2016

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

@oddbear
Copy link
Author

oddbear commented Jun 8, 2016

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
Copy link
Author

oddbear commented Jun 8, 2016

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

@oddbear
Copy link
Author

oddbear commented Aug 23, 2016

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
Copy link
Owner

rid00z commented Aug 27, 2016

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

@oddbear
Copy link
Author

oddbear commented Aug 28, 2016

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
Copy link
Owner

rid00z commented Aug 29, 2016

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

@oddbear
Copy link
Author

oddbear commented Aug 29, 2016

Pull request #99

@joanbarros
Copy link

@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
Copy link
Author

oddbear commented Oct 23, 2018

@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
Copy link

@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
Copy link

danielkatz commented Oct 23, 2018

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
Copy link
Author

oddbear commented Oct 24, 2018

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants