Skip to content
This repository was archived by the owner on May 1, 2024. It is now read-only.

Fix NullReferenceException thrown in PopupRenderer.SetViewController #1762

Merged
merged 8 commits into from
Dec 20, 2021

Conversation

VladislavAntonyuk
Copy link
Contributor

@VladislavAntonyuk VladislavAntonyuk commented Dec 10, 2021

Description of Bug

  1. Fix NRE when renderer if currentPageRenderer is null
  2. Allow using Popup in Xamarin Native

Issues Fixed

PR Checklist

  • Has a linked Issue, and the Issue has been approved
  • Has tests (if omitted, state reason in description)
  • Has samples (if omitted, state reason in description)
  • Rebased on top of main at time of PR
  • Changes adhere to coding standard

SkyeHoefling
SkyeHoefling previously approved these changes Dec 10, 2021
Copy link
Contributor

@SkyeHoefling SkyeHoefling left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks to me, thanks for making the change so quickly. 🚀🚀

Copy link
Contributor

@pictos pictos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've a question

}
else
{
currentPageRenderer = Platform.GetRenderer(mainPage);
ViewController ??= currentPageRenderer.ViewController;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we sure that we just want to set the ViewController if it's null?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

previously, we always set it. it was done by CreateControl. it was the only 1 place where we set it.
now we also set ViewController in ctor. we do not want to set it here again.
hope I correctly understand your question.

Copy link
Contributor

@TheCodeTraveler TheCodeTraveler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Vlad!

Let's remove the Null Forgiving Operator

@TheCodeTraveler TheCodeTraveler dismissed their stale review December 20, 2021 17:43

Null Forgiving Operator removed ✅

if (modalStackCount > 0)
{
var index = modalStackCount - 1;
page = page!.Navigation!.ModalStack![index];
page = page?.Navigation.ModalStack[index];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if ModalStack is > 0 Page will not be null

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know, but we receive an error if remove null propagation. and we discussed not to use Null forgiving operator
image

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rewrite it and add check page for null

@TheCodeTraveler TheCodeTraveler merged commit 7443f05 into main Dec 20, 2021
@TheCodeTraveler TheCodeTraveler deleted the 1758-popup-nre branch December 20, 2021 18:47
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

NullReferenceException thrown in PopupRenderer.SetViewController when using in embedded forms scenario
4 participants