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

Fix issue with ConnectedAnimation crashing due to selection being null #497

Merged
merged 1 commit into from
Jul 10, 2020

Conversation

marcelwgn
Copy link
Collaborator

Description

The sample crashed as on a freshly loaded page, the selecteditem of the RadioButtons was null. By setting SelectedIndex in XAML, the RadioButtons now always know the selected item, and thus fixing the crash.

Motivation and Context

Closes #494

How Has This Been Tested?

Opened ConnectedAnimationPage and pressed the "Navigate" button. App did not crash.

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

@stmoy
Copy link
Contributor

stmoy commented Jul 9, 2020

Thank you for the fix. I'm wondering if this is a platform bug. CC: @ranjeshj @kikisaints @YuliKl - thoughts?

@marcelwgn
Copy link
Collaborator Author

Thank you for the fix. I'm wondering if this is a platform bug. CC: @ ranjeshj @ kikisaints @ YuliKl - thoughts?

I'm inclined to say "no" here, because it isn't really like we are setting the selection on the RadioButtons, but we are rather telling the single RadioButton to be checked. I am not sure if this is a very common scenario or if it is "expected", after all "Checked" is only something you would use if you use multiple RadioButton, but asking the RadioButtons Control what is selected would be more in the realm of "RadioButtons" handle selection.

Those two seem more or less exclusive to me.

@stmoy
Copy link
Contributor

stmoy commented Jul 9, 2020

If the bar for RadioButtons control is "we expect it to work wherever StackPanel was used", then this would violate that bar. (Not sure if this is the bar, hence my question to Ranjesh/Kiki/Yulia)

I guess it's a bit of a semantic question - should RadioButton be responsible for knowing which thing is selected, or should RadioButtons? Previously it was RadioButton, but now I don't know if it is "either/or" or only RadioButtons. I think this change suggests the latter, but I don't know if that has been well articulated.

@YuliKl
Copy link

YuliKl commented Jul 9, 2020

This had felt like an app bug to me when I opened it. But I defer to @kikisaints for clarification on how these controls were designed to work.

@ranjeshj
Copy link
Contributor

If the bar for RadioButtons control is "we expect it to work wherever StackPanel was used", then this would violate that bar. (Not sure if this is the bar, hence my question to Ranjesh/Kiki/Yulia)

I guess it's a bit of a semantic question - should RadioButton be responsible for knowing which thing is selected, or should RadioButtons? Previously it was RadioButton, but now I don't know if it is "either/or" or only RadioButtons. I think this change suggests the latter, but I don't know if that has been well articulated.

This is interesting. Selection on the RadioButtons control is conveyed through the checked state of the RadioButton. Should changing checked state trigger a selection change? What if you set two RadioButtons to checked? @StephenLPeters do you remember discussing this issue ?

@stmoy
Copy link
Contributor

stmoy commented Jul 10, 2020

Even though this smells like a platform bug to me, it's not likely to be fixed anytime soon. I'll accept this PR in the meantime while we discuss if this is desired behavior or not.

@stmoy stmoy merged commit 7dcced3 into microsoft:master Jul 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Pressing Navigate in the Connected Animation page crashes the app
4 participants