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

Resource Manager fix for F# File Resources (Bugzilla 53515) #825

Merged
merged 3 commits into from Apr 12, 2017

Conversation

Projects
None yet
5 participants
@SpiegelSoft
Contributor

SpiegelSoft commented Mar 20, 2017

Description of Change

In F#, loading images from files causes a null reference exception.

Bugs Fixed

https://bugzilla.xamarin.com/show_bug.cgi?id=53515

API Changes

None

Behavioral Changes

None

PR Checklist

@dnfclas

This comment has been minimized.

Show comment
Hide comment
@dnfclas

dnfclas Mar 20, 2017

This seems like a small (but important) contribution, so no Contribution License Agreement is required at this point. We will now review your pull request.
Thanks,
.NET Foundation Pull Request Bot

dnfclas commented Mar 20, 2017

This seems like a small (but important) contribution, so no Contribution License Agreement is required at this point. We will now review your pull request.
Thanks,
.NET Foundation Pull Request Bot

Show outdated Hide outdated Xamarin.Forms.Platform.Android/ResourceManager.cs
if (prop != null)
return (int)prop.GetValue(type);
object value = type.GetFields().FirstOrDefault(p => p.Name == propertyName)?.GetValue(type)
?? type.GetProperties().FirstOrDefault(p => p.Name == propertyName)?.GetValue(type);

This comment has been minimized.

@StephaneDelcroix

StephaneDelcroix Mar 21, 2017

Member

we prefer tabs over spaces. otherwise 👍

@StephaneDelcroix

StephaneDelcroix Mar 21, 2017

Member

we prefer tabs over spaces. otherwise 👍

This comment has been minimized.

@SpiegelSoft

SpiegelSoft Mar 21, 2017

Contributor

Weird -- I changed the Visual Studio option, but it seems Visual Studio loves its spaces.

Now that I think of it, memberName is probably a better argument name than propertyName.

Also, I'll be honest -- I wasn't able to get the solution running on my environment, so this request is a best guess rather than fully tested code. I believe it will solve my immediate issue though.

@SpiegelSoft

SpiegelSoft Mar 21, 2017

Contributor

Weird -- I changed the Visual Studio option, but it seems Visual Studio loves its spaces.

Now that I think of it, memberName is probably a better argument name than propertyName.

Also, I'll be honest -- I wasn't able to get the solution running on my environment, so this request is a best guess rather than fully tested code. I believe it will solve my immediate issue though.

This comment has been minimized.

@StephaneDelcroix

StephaneDelcroix Mar 21, 2017

Member

-- I wasn't able to get the solution running on my environment

ouch...

I'm building a nuget of this PR and will send it to you by email so you can test

@StephaneDelcroix

StephaneDelcroix Mar 21, 2017

Member

-- I wasn't able to get the solution running on my environment

ouch...

I'm building a nuget of this PR and will send it to you by email so you can test

This comment has been minimized.

@SpiegelSoft

SpiegelSoft Mar 21, 2017

Contributor

👍

@SpiegelSoft

SpiegelSoft Mar 21, 2017

Contributor

👍

@rmarinho

This comment has been minimized.

Show comment
Hide comment
@rmarinho

rmarinho Mar 27, 2017

Member

@SpiegelSoft can you rebase this please ? thanks

Member

rmarinho commented Mar 27, 2017

@SpiegelSoft can you rebase this please ? thanks

@SpiegelSoft

This comment has been minimized.

Show comment
Hide comment
@SpiegelSoft

SpiegelSoft Mar 27, 2017

Contributor

Done.

Contributor

SpiegelSoft commented Mar 27, 2017

Done.

@StephaneDelcroix

Could you change that one, then retest, and I'll merge

Show outdated Hide outdated Xamarin.Forms.Platform.Android/ResourceManager.cs
return (int)prop.GetValue(type);
object value = type.GetFields().FirstOrDefault(p => p.Name == memberName)?.GetValue(type)
?? type.GetProperties().FirstOrDefault(p => p.Name == memberName)?.GetValue(type);
if (value != null)

This comment has been minimized.

@StephaneDelcroix

StephaneDelcroix Mar 30, 2017

Member

could you add a check to make sure the cast operation won't fail ? like:

if (value!=null && value is int)
@StephaneDelcroix

StephaneDelcroix Mar 30, 2017

Member

could you add a check to make sure the cast operation won't fail ? like:

if (value!=null && value is int)
@SpiegelSoft

This comment has been minimized.

Show comment
Hide comment
@SpiegelSoft

SpiegelSoft Mar 30, 2017

Contributor

In fact, if the value is an int, the null check is redundant.

Done.

Contributor

SpiegelSoft commented Mar 30, 2017

In fact, if the value is an int, the null check is redundant.

Done.

@StephaneDelcroix

you shouldn't have merged the master in this PR, it makes everything messy. you should instead have rebased it on top of master.

The changes are ok, so 👍 once the PR is neat

@SpiegelSoft

This comment has been minimized.

Show comment
Hide comment
@SpiegelSoft

SpiegelSoft Apr 7, 2017

Contributor
Contributor

SpiegelSoft commented Apr 7, 2017

@StephaneDelcroix

This comment has been minimized.

Show comment
Hide comment
@StephaneDelcroix

StephaneDelcroix Apr 7, 2017

Member

@SpiegelSoft the easiest thing to do if you're not comfortable with git rebase -i would be to create a new branch, and only cherry-pick the commit that matters.

(if you want to keep the same pr, you'll need to rename this branch to something else, and then recreate the new one with the same name)

Member

StephaneDelcroix commented Apr 7, 2017

@SpiegelSoft the easiest thing to do if you're not comfortable with git rebase -i would be to create a new branch, and only cherry-pick the commit that matters.

(if you want to keep the same pr, you'll need to rename this branch to something else, and then recreate the new one with the same name)

SpiegelSoft added some commits Mar 20, 2017

@rmarinho rmarinho merged commit b9a94f4 into xamarin:master Apr 12, 2017

5 of 6 checks passed

Android-UITests-C8 Finished TeamCity Build Xamarin.Forms :: Debug :: Stable - Cycle 9 :: UI Tests :: OSX Test Cloud Package - Run Android 6.0.1 (Legacy Rende…
Details
OSX-Debug-C8 Finished TeamCity Build Xamarin.Forms :: Debug :: Stable - Cycle 9 :: OSX Debug : Running
Details
Windows-Debug-C8 Finished TeamCity Build Xamarin.Forms :: Debug :: Stable - Cycle 9 :: Windows Debug : Tests passed: 3768, ignored: 10
Details
iOS10-UITests-C8 Finished TeamCity Build Xamarin.Forms :: Debug :: Stable - Cycle 9 :: UI Tests :: OSX Test Cloud Package - Run iOS Unified iOS10 : Tests p…
Details
iOS8-UITests-C8 Finished TeamCity Build Xamarin.Forms :: Debug :: Stable - Cycle 9 :: UI Tests :: OSX Test Cloud Package - Run iOS Unified IOS8 : Running
Details
iOS9-UITests-C8 Finished TeamCity Build Xamarin.Forms :: Debug :: Stable - Cycle 9 :: UI Tests :: OSX Test Cloud Package - Run iOS Unified iOS9 : Tests pa…
Details

@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