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

[Android] Double tapping on context action should not crash #609

Merged
merged 7 commits into from Feb 13, 2017

Conversation

Projects
None yet
7 participants
@adrianknight89
Contributor

adrianknight89 commented Dec 5, 2016

Description of Change

If a cell context menu is open and you double tap an action, it shouldn't crash.

Bugs Fixed

PR Checklist

  • Has tests (if omitted, state reason in description)
  • Rebased on top of master at time of PR
  • Changes adhere to coding standard
  • Consolidate commits as makes sense
@adrianknight89

This comment has been minimized.

Show comment
Hide comment
@adrianknight89

adrianknight89 Dec 5, 2016

Contributor

@rmarinho Actually, we need to disable all items not just the current one. You can tap one item and immediately tap another which will crash the app. The old change doesn't have this problem. I'll take another look tonight.

Contributor

adrianknight89 commented Dec 5, 2016

@rmarinho Actually, we need to disable all items not just the current one. You can tap one item and immediately tap another which will crash the app. The old change doesn't have this problem. I'll take another look tonight.

@adrianknight89

This comment has been minimized.

Show comment
Hide comment
@adrianknight89

adrianknight89 Dec 5, 2016

Contributor

@rmarinho Nvm. Found it.

Contributor

adrianknight89 commented Dec 5, 2016

@rmarinho Nvm. Found it.

@rmarinho

This comment has been minimized.

Show comment
Hide comment
@rmarinho

rmarinho Jan 4, 2017

Member

Can you rebase please. thanks

Member

rmarinho commented Jan 4, 2017

Can you rebase please. thanks

@adrianknight89

This comment has been minimized.

Show comment
Hide comment
@adrianknight89

adrianknight89 Jan 4, 2017

Contributor

Done.

Contributor

adrianknight89 commented Jan 4, 2017

Done.

@rmarinho

This comment has been minimized.

Show comment
Hide comment
@rmarinho

rmarinho Jan 12, 2017

Member

It's failing 100 tests.. please check android UITests

Member

rmarinho commented Jan 12, 2017

It's failing 100 tests.. please check android UITests

@rmarinho rmarinho removed the waiting-tests label Jan 12, 2017

@adrianknight89

This comment has been minimized.

Show comment
Hide comment
@adrianknight89

adrianknight89 Jan 16, 2017

Contributor

@rmarinho Changed the place where the menu was being cleared. Should be fixed now.

Contributor

adrianknight89 commented Jan 16, 2017

@rmarinho Changed the place where the menu was being cleared. Should be fixed now.

@hartez

Looks good, just needs a couple of changes to the test.

Show outdated Hide outdated ...ms.Controls.Issues/Xamarin.Forms.Controls.Issues.Shared/Bugzilla45027.cs
{
[Preserve(AllMembers = true)]
[Issue(IssueTracker.Bugzilla, 45027, "App crashes when double tapping on ToolbarItem or MenuItem very quickly", PlatformAffected.Android)]
public class Bugzilla45027 : TestContentPage // or TestMasterDetailPage, etc ...

This comment has been minimized.

@hartez

hartez Jan 31, 2017

Member

Since this is a race condition (and since UI test taps are so slow), I'm guessing we can't reliably make this an automated UI test.
Can you add some instructions for the tester (e.g. "long press an item, tap Action twice quickly")?

@hartez

hartez Jan 31, 2017

Member

Since this is a race condition (and since UI test taps are so slow), I'm guessing we can't reliably make this an automated UI test.
Can you add some instructions for the tester (e.g. "long press an item, tap Action twice quickly")?

This comment has been minimized.

@rmarinho

rmarinho Feb 13, 2017

Member

@adrianknight89 can you add the comment?

@rmarinho

rmarinho Feb 13, 2017

Member

@adrianknight89 can you add the comment?

This comment has been minimized.

@adrianknight89

adrianknight89 Feb 13, 2017

Contributor

@rmarinho Already added.

@adrianknight89

adrianknight89 Feb 13, 2017

Contributor

@rmarinho Already added.

@hartez

This comment has been minimized.

Show comment
Hide comment
@hartez

hartez Jan 31, 2017

Member

That Android test failure (45926) is because of a bad test which has been fixed in master - it'll go away once you rebase.

Member

hartez commented Jan 31, 2017

That Android test failure (45926) is because of a bad test which has been fixed in master - it'll go away once you rebase.

@adrianknight89

This comment has been minimized.

Show comment
Hide comment
@adrianknight89

adrianknight89 Feb 2, 2017

Contributor

Done.

Contributor

adrianknight89 commented Feb 2, 2017

Done.

@CSkoubo

This comment has been minimized.

Show comment
Hide comment
@CSkoubo

CSkoubo Feb 9, 2017

Hi Adrian, i've been working on a fix for this issue too. I think you hide the real problem with your solution. The issue is that the Item in OnActionItemClicked is null. I would much rather just have it
If (Item == null) return true;

CSkoubo commented Feb 9, 2017

Hi Adrian, i've been working on a fix for this issue too. I think you hide the real problem with your solution. The issue is that the Item in OnActionItemClicked is null. I would much rather just have it
If (Item == null) return true;

@rmarinho

This comment has been minimized.

Show comment
Hide comment
@rmarinho

rmarinho Feb 9, 2017

Member

Keeps failing tests on iOS 10 :(

Member

rmarinho commented Feb 9, 2017

Keeps failing tests on iOS 10 :(

@adrianknight89

This comment has been minimized.

Show comment
Hide comment
@adrianknight89

adrianknight89 Feb 9, 2017

Contributor

Why does it fail iOS tests if the change was made for Android?

@CSkoubo I will test your suggestion tonight. I think I had checked Item before, and it wasn't null for me, but it's been a while since the time of the PR, so I'll go back and verify. Thanks. :)

Contributor

adrianknight89 commented Feb 9, 2017

Why does it fail iOS tests if the change was made for Android?

@CSkoubo I will test your suggestion tonight. I think I had checked Item before, and it wasn't null for me, but it's been a while since the time of the PR, so I'll go back and verify. Thanks. :)

@adrianknight89

This comment has been minimized.

Show comment
Hide comment
@adrianknight89

adrianknight89 Feb 10, 2017

Contributor

@CSkoubo So, I looked at this again. I wasn't able to reproduce a null Item. When onActionItemClicked is called, Android is providing the item that was clicked. Here's the documentation. Does it make sense for Android to pass a null object?

Note that ActionModeContext is null. If you look at the commit history, I was handling this first, but as @rmarinho suggested ActionModeContext should not be null if the menu is open.

The if blocks below seem to be never hit for me:

public bool OnActionItemClicked(ActionMode mode, IMenuItem item)
{
	if (item == null)
	{
		System.Diagnostics.Debug.WriteLine("Item is null");
		return true;
	}
	//mode.Menu.Clear();
	OnActionItemClickedImpl(item);
	_actionMode?.Finish();
	return true;
}

bool global::Android.Support.V7.View.ActionMode.ICallback.OnActionItemClicked(global::Android.Support.V7.View.ActionMode mode, IMenuItem item)
{
	if (item == null)
	{
		System.Diagnostics.Debug.WriteLine("Item is null");
		return true;
	}
	//mode.Menu.Clear();
	OnActionItemClickedImpl(item);
	_supportActionMode?.Finish();
	return true;
}

When I was searching for a solution, mode.Menu.Clear(); seemed to do the trick. To be honest, I cannot think of a better solution.

Contributor

adrianknight89 commented Feb 10, 2017

@CSkoubo So, I looked at this again. I wasn't able to reproduce a null Item. When onActionItemClicked is called, Android is providing the item that was clicked. Here's the documentation. Does it make sense for Android to pass a null object?

Note that ActionModeContext is null. If you look at the commit history, I was handling this first, but as @rmarinho suggested ActionModeContext should not be null if the menu is open.

The if blocks below seem to be never hit for me:

public bool OnActionItemClicked(ActionMode mode, IMenuItem item)
{
	if (item == null)
	{
		System.Diagnostics.Debug.WriteLine("Item is null");
		return true;
	}
	//mode.Menu.Clear();
	OnActionItemClickedImpl(item);
	_actionMode?.Finish();
	return true;
}

bool global::Android.Support.V7.View.ActionMode.ICallback.OnActionItemClicked(global::Android.Support.V7.View.ActionMode mode, IMenuItem item)
{
	if (item == null)
	{
		System.Diagnostics.Debug.WriteLine("Item is null");
		return true;
	}
	//mode.Menu.Clear();
	OnActionItemClickedImpl(item);
	_supportActionMode?.Finish();
	return true;
}

When I was searching for a solution, mode.Menu.Clear(); seemed to do the trick. To be honest, I cannot think of a better solution.

@CSkoubo

This comment has been minimized.

Show comment
Hide comment
@CSkoubo

CSkoubo Feb 13, 2017

@adrianknight89 you are right! sorry for that. Spend some time looking into this.

So either use your current solution or use the "enabled" as you said -perhaps a "canExecute" would be of use. I haven't been able to get my local XF to work, so i am unable to investigate some more. (only having a mac, so need to install a VM)

CSkoubo commented Feb 13, 2017

@adrianknight89 you are right! sorry for that. Spend some time looking into this.

So either use your current solution or use the "enabled" as you said -perhaps a "canExecute" would be of use. I haven't been able to get my local XF to work, so i am unable to investigate some more. (only having a mac, so need to install a VM)

@rmarinho rmarinho merged commit f0711c6 into xamarin:master Feb 13, 2017

6 checks passed

Android-UITests-C8 Finished TeamCity Build Xamarin.Forms :: Debug :: Cycle 8 :: UI Tests :: OSX Test Cloud Package - Run Android 6.0.1 : Tests passed: 352, i…
Details
OSX-Debug-C8 Finished TeamCity Build Xamarin.Forms :: Debug :: Cycle 8 :: OSX Debug : Running
Details
Windows-Debug-C8 Finished TeamCity Build Xamarin.Forms :: Debug :: Cycle 8 :: Windows Debug : Tests passed: 3719, ignored: 10
Details
iOS10-UITests-C8 Finished TeamCity Build Xamarin.Forms :: Debug :: Cycle 8 :: UI Tests :: OSX Test Cloud Package - Run iOS Unified iOS10 : Tests passed: 34…
Details
iOS8-UITests-C8 Finished TeamCity Build Xamarin.Forms :: Debug :: Cycle 8 :: UI Tests :: OSX Test Cloud Package - Run iOS Unified IOS8 : Tests passed: 348…
Details
iOS9-UITests-C8 Finished TeamCity Build Xamarin.Forms :: Debug :: Cycle 8 :: UI Tests :: OSX Test Cloud Package - Run iOS Unified iOS9 : Tests passed: 350…
Details
@pratius

This comment has been minimized.

Show comment
Hide comment
@pratius

pratius May 25, 2017

@adrianknight89 Yes, Can you please provide the small POC in Xamarin form? Because it's giving exception for me on "ListView with ContextActions throws NullReferenceException when Multi-touch Tapped on empty and non-empty cell"

pratius commented May 25, 2017

@adrianknight89 Yes, Can you please provide the small POC in Xamarin form? Because it's giving exception for me on "ListView with ContextActions throws NullReferenceException when Multi-touch Tapped on empty and non-empty cell"

@samhouts samhouts added D-15.4 and removed cla-already-signed labels Oct 10, 2017

@samhouts samhouts added this to the 2.3.5 milestone Jun 27, 2018

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