WPF MVVM Sample App #353
WPF MVVM Sample App #353
Conversation
There was a problem hiding this comment.
I think this looks great, just a couple of minor things you could look at.
It's awesome you took the time to prepare a sample to share with others. I think we should be able to reduce the amount of reflection code, maybe with some tweaks in the library, but this is more of a potential improvement and does not in anyway block merging in this PR.
I'd still like to get some input from the other collaborators before merging this in.
Samples/WpfMVVMSample/Readme.md
Outdated
|
|
||
| It performs a simple calculation allowing flexibility in the units for parameters and results. Default units for each are specified in the settings drop down. | ||
|
|
||
| A key feature enabling this sample is the UnitToStringConverter class |
| @@ -0,0 +1,13 @@ | |||
| ## WPF MVVM Sample | |||
| //if (actualEnumType == this._enumType) | ||
| // return enumValues; | ||
|
|
||
| //ommits the first enum element, typically "undefined" |
| Type actualEnumType = Nullable.GetUnderlyingType(this._enumType) ?? this._enumType; | ||
| Array enumValues = Enum.GetValues(actualEnumType); | ||
|
|
||
| //if (actualEnumType == this._enumType) |
There was a problem hiding this comment.
Consider removing commented out code.
|
|
||
| //ommits the first enum element, typically "undefined" | ||
| Array tempArray = Array.CreateInstance(actualEnumType, enumValues.Length -1); | ||
| Array.ConstrainedCopy(enumValues,1,tempArray, 0,enumValues.Length-1); |
There was a problem hiding this comment.
This is probably a stupid question as I don't fully understand this, but can't you just return enumValues.Skip(1).ToArray() here if the idea is to exclude the first/default enum value?
There was a problem hiding this comment.
Great tip. Thought I was stuck with the old array methods. Just needed a simple cast() to simplify things.
|
|
||
| var result = unitType | ||
| .GetMethod("ToString", new[] { unitEnumType, typeof(IFormatProvider), typeof(int) }) | ||
| .Invoke(value, new object[] { unitEnumValue, null, significantDigits }); |
There was a problem hiding this comment.
My only concern is that this is quite heavy on reflection and might easily break at runtime when subtle changes to the library are made that are backwards compatible on calling code, but incompatible with the reflection method signatures expected here.
I think maybe the library could be adapted a bit to facilitate this better. I have only given this a brief thinking, but I I think it would help to make https://github.com/angularsen/UnitsNet/blob/master/UnitsNet/CustomCode/QuantityParser.cs public in order to give access to the quantity value+unit parsing without having to use all the reflection bits?
I would need to spend more time evaluating this, but I think we should be able to simplify this type of integration to avoid so much reflection code. For now just consider this a rambling comment as I don't have a clear vision for how to best achieve that yet.
@JKSnd @eriove @ferittuncer I'm seeing some potential usecases for exposing BaseUnit and ToString overloads in an interface or base class for quantities here, as discussed earlier.
There was a problem hiding this comment.
I agree that the reflection is a bit clunky. Any library enhancements that better enable this strategy would be welcome.
There was a problem hiding this comment.
Well, we have experienced these problems too when we used this library in our GUI. Lacking base classes for units and quantities forces implementation to reflection as presenter layer doesn't know which unit or quantity it's dealing with at the runtime.
Perhaps there are other ways we couldn't see, I'm not sure. I just have an instinct that we should have base classes. You know better, your call @angularsen
There was a problem hiding this comment.
A common interface for all quantities would be a simple addition (at least if it contained members that are already there) and wouldn't incur any performance hit (boxing) as long as it isn't used.
That's of course not part of this pull request but it is much easier to see in this context.
Updated enum binding source.
|
Updates complete |
|
Alright, so I think we agree this is ready to merge and that there is some possibility to improve this sample by looking for ways for the library to provide the information that is now gathered by the reflection code. |
|
Thanks @dayewah ! |
|
I won't personally pursue the case of improving the reflection code as discussed above, but at least this gives us a concrete usecase to refer to in future discussion. If someone wants to draft up a proposal on how to avoid reflection code in this sample and similar usecases, just create an issue for it and I would be happy to participate in the discussion and bring that into the library. |
|
Great! Thanks @angularsen. I'll continue to monitor progress with library changes and perhaps make suggestions if I come up with anything. |
Here's a simple sample app that I think would have been useful to me when I first came across this excellent library. Not sure how many people are doing UI apps with this but I thought I would share my strategy as a sample. Let me know if any thing is amiss. Hope it is found to be a useful addition to the library. Btw this is my first pull request on github. Hoping its all done right and I promise I won't be offended if it is rejected :)
Thanks