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

[Enhancement] Creating a bindable property with enum backing does not select enum default #2826

Closed
adrianknight89 opened this issue May 24, 2018 · 5 comments · Fixed by #2827

Comments

@adrianknight89
Copy link
Contributor

adrianknight89 commented May 24, 2018

Description

When I create a bindable property for an enum, I have to explicitly set a default value in the Bindable.Create() method for that enum otherwise a runtime error will occur. Why is that? In my opinion, by default, the first member of an enum type should be chosen by the Create() method. See attached.

This issue seems to exist for structs as well. For example, if you create a bindable property for TimeSpan, you need to explicitly do new TimeSpan() or default(TimeSpan) while in reality a TimeSpan defaults to "00:00:00".

Reproduction Link

Demo.zip

@pauldipietro pauldipietro added this to New in Triage May 24, 2018
@charlesroddie
Copy link

charlesroddie commented May 24, 2018

If there is going to be a default, it should be explicit. Placing a value first in an enum does not imply that it is intended to be a default. No need to make bindings even more hacky than they are already.

@adrianknight89
Copy link
Contributor Author

I should have been clear about default enum values. You're right that the first enumeration isn't always the default one. Here's an example:

enum MyEnum
{
      A = 1,
      B = 0,
      C = 2
}

default(MyEnum) would return B.

Official documentation for bindable properties mentions the following for the defaultValue parameter of Create():

Providing a default property value that's different from the default for the type of the property.

which, to me, implies that Create() is supposed to set the property to its default if defaultValue is not provided.

I'd say the documentation should make it clear that for enums and structs, an explicit initialization must be provided or the Create() method should be modified to define defaults.

@activa
Copy link
Contributor

activa commented May 24, 2018

The documentation is wrong. The default value is of type object and the default is null. When the property type is a value type, the code is trying to cast null to a value type and of course that throws an exception.

So either the code needs to change so it uses default(T) in case the default value is null, or the documentation should make it clear that the provided default value should be something that can be casted the property type.

@activa
Copy link
Contributor

activa commented May 24, 2018

Fixing the code to do what the documentation says looks pretty easy.

Adding this to the BindableProperty constructor should do it:

if (ReturnType.IsValueType && Nullable.GetUnderlyingType(ReturnType) == null && DefaultValue == null)
    DefaultValue = Activator.CreateInstance(ReturnType);

I'll create a PR for it and see what they say.

@activa
Copy link
Contributor

activa commented May 24, 2018

See PR #2827

@PureWeen PureWeen removed this from New in Triage May 25, 2018
@PureWeen PureWeen changed the title Creating a bindable property with enum backing does not select enum default [Enhancement] Creating a bindable property with enum backing does not select enum default May 25, 2018
@StephaneDelcroix StephaneDelcroix added this to Done in Enhancements via automation Jun 11, 2018
@samhouts samhouts added this to Done in v3.6.0 Jun 11, 2018
@samhouts samhouts removed this from Done in Enhancements Jun 12, 2018
@samhouts samhouts removed this from Done in v3.6.0 Jun 26, 2018
@samhouts samhouts added this to Done in v3.2.0 Jun 26, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
No open projects
v3.2.0
  
Done
Development

Successfully merging a pull request may close this issue.

5 participants