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

[Android] Fix issue with the use of null propagation operator #567

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
5 participants
@adrianknight89
Copy link
Contributor

commented Nov 27, 2016

Description of Change

When propagating null checks using ? and accessing a property, one should give the expression a default value or else an NRE can be thrown.

renderer?.ViewGroup?.ChildCount != 0 throws an exception when renderer is null. To fix this, we could default the expression to 0 like so:

(renderer?.ViewGroup?.ChildCount ?? 0) != 0

or use old-school null checks. I chose the second option because ReSharper isn't smart enough to remove the squiggly line under renderer.

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

left a comment

If this fixes a bug, it requires a test that fails without your patch, and works with it.

If you can't write a test for this changes, please explain why

@adrianknight89

This comment has been minimized.

Copy link
Contributor Author

commented Dec 1, 2016

@StephaneDelcroix If you look at the bug description and repro, the user is using a custom view cell renderer and overriding GetCellCore method. In there, he's inflating an axml to use as native view and returning the new native view. What happens is that as program flow continues, ListViewAdapter is trying to apply touch listeners to this new cell and the cell does not have a renderer hence the NRE. The original implementation of GetCellCore creates a renderer.

After thinking about this, I believe my fix is not the right one (although it could perhaps be nice to throw an argument null exception if renderer is null for visual guidance). It appears to me the proper fix might be invoking the base method with the new native cell instead of returning the new native cell.

protected override Android.Views.View GetCellCore(Cell item, Android.Views.View convertView, ViewGroup parent, Context context)
{
	var nativeView = convertView;

	if (nativeView == null)
		nativeView = (context as Activity).LayoutInflater.Inflate(Resource.Layout.CustomViewCell, null);

	return base.GetCellCore(item, nativeView, parent, context);
}

That said, I will let you guys decide how to proceed. The changes in this PR are not applying touch listeners if the renderer is null. Should we throw an exception instead?

@hartez hartez referenced this pull request Dec 1, 2016

Merged

Fix potential NRE in ConditionalFocusLayout #587

4 of 4 tasks complete
@hartez

This comment has been minimized.

Copy link
Member

commented Dec 1, 2016

I agree that the original submitter's custom renderer might have issues and might not work they way they expect. Regardless, ApplyTouchListenersToSpecialCells() should be able to run without crashing if a renderer isn't set. (It was clearly written with that intent; otherwise it would just assume the renderer is not null).

I whipped up an alternate PR which tests this specific scenario and does the null checking in a more concise manner: #587

@adrianknight89

This comment has been minimized.

Copy link
Contributor Author

commented Dec 1, 2016

@hartez Great.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.