louthy / language-ext

C# functional language extensions - a base class library for functional programming
MIT License
6.38k stars 415 forks source link

Binding support for Option and Some #16

Closed marcpiechura closed 9 years ago

marcpiechura commented 9 years ago

Hi,

i would like to use your framework for some WPF desktop applications, but i would need binding support for Option and Some. As a test I have written my own TypeConverter for Option, unfortunately I had to use some dirty reflection to get the correct types.

I would add A pull request, but the project isn't working with VS 2013 and i don't want to install VS 2015 on the same pc.

Of course I could wrap the option/some types in the view model to "bindable" types, but that would require a lot of boilerplate code.

    public class OptionalValueClassTypeConverter : TypeConverter
    {
        public override bool CanConvertFrom(ITypeDescriptorContext context, Type sourceType)
        {
            return true;
        }

        public override bool CanConvertTo(ITypeDescriptorContext context, Type destinationType)
        {
            return true;
        }

        public override object ConvertFrom(ITypeDescriptorContext context, CultureInfo culture, object value)
        {
            if (value == null)
                return Prelude.None;
            var valueType = value.GetType();

            var methods = typeof (Prelude).GetMethods().Where(info => info.Name.Equals("Optional")).ToList();

            var method = methods.First().MakeGenericMethod(valueType);
            var result = method.Invoke(null, new[] { value });
            return result;
        }

        public override object ConvertTo(ITypeDescriptorContext context, CultureInfo culture, object value, Type destinationType)
        {
            var optionValue = value as IOptionalValue;
            if (optionValue == null)
                return value;

            if (optionValue.IsNone)
                return null;

            dynamic dynamicOption = value;

            return dynamicOption.ToList()[0];
        }
    }
louthy commented 9 years ago

Hi @Silv3rcircl3 thanks for bringing this up. I don't ever use WPF (more of a web-guy), so I just need a bit of clarification on how the data-binding works:

I can see the ConvertFrom makes sense. It allows any value to be wrapped in an Option<_>. But ConvertTo appears to require conversion from Option<T> (?) to destinationType.

So a couple things:

  1. If optionValue is not an IOptionalValue it just returns the value, not converted. Is that right?
  2. If optionValue.IsNone then it returns null, how does that work when destinationType is a value type?
  3. Does null have the same semantic meaning to the data-binding system as None?
  4. If there is a value, then you're just getting it and returning it. Shouldn't it Convert.ChangeType(value.Value,destinationType)

Apologies for not knowing this stuff, I tend to steer clear of the big .NET APIs and prefer my own hand-rolled data-binding solutions.

marcpiechura commented 9 years ago

Hi @louthy,

that explains the lack of binding support :) Honastly, that's also for me the first time I must write a TypeConverter, usually I would write a ValueConverter and set this for a specific binding/value, for example to bind a bool value to the visual state of a control.

Also the converter I wrote yesterday was more like a workout in the morning, so it's not really perfect and only tested with Option<String> and Option<TestClass>.

Hoppfully I can answer your questions correctly, I have only the knowledge of google from yesterday about the TypeConverter and I doesn't have deep knowledge about the binding system from WPF, I "only" know how to use it ;)

  1. After I read your anwser, I thought over this and I think that can't happen. Because the converter is coupled to a specific type, in this case IOptionalValue and should only used for this type.
  2. ConverTo is for the VM -> GUI way and I think the control properties can handle null, if not I would only expect something like a "BindingError" which only shows up in the debug console from visual studio. Another example would be if you bind a textbox to a integer value and then type "ab" into the textfeld, this case is handled by the binding itself and doesn't throw any exceltion which could crash the app.
  3. Doesn't know if I understand your question correctly and I'm not sure if some gui property updates the vm with null, but if so, None would make sense in my mind. For example if you bind a DatePicker to Nullable<DateTime>, the control would set the vm property to null if no date is selected, which should be None in option case ?! If I remember me correctly, this case bound to DateTime would set the vm property to default(DateTime).
  4. I tried this method at the beginning, unfortunately the value must implement the IConvert(or something like this) interface.

I also have to say, that I read about your framework at the weekend and started a "could it be work for me" hour yesterday, so I'm really new to option, some ... ;)

Since you personally have no benefit from implementing binding support for WPF (as you said more a web guy) and you would have to learn the hole WPF stuff, I would suggest that I install VS 2015 in a VM and try it by my self. If I have question, I would ask again and if the converter is working, I would write a demo application and submit a pull request. Of course I have no doubt if you want to do it by yourself ;-)

louthy commented 9 years ago

Ok, so these are my thoughts:

  1. It's not entirely clear whether your implementation of the TypeProvider is the 'correct' way (or if in fact there is a correct way)
  2. It's not totally clear what the WPF data-binding system will do with the returned value.
  3. I have very little experience in this area and that will make proving the correctness of any changes difficult.

I already have a prototype version in code, with a slightly cleaned up version of yours. I think there are still a few too many unknowns here. So I'll do a little bit more digging. If you could mock up a small test harness that would be very useful?

This is what my version looks like, I've tried to add some bulletproofing in case our assumptions are wrong:

    public class OptionalTypeConverter : TypeConverter
    {
        static readonly Func<Type, MethodInfo> methods = null;
        static readonly MethodInfo optional = null;

        static OptionalTypeConverter()
        {
            optional = (from info in typeof(Prelude).GetMethods()
                        where info.Name == "Optional"
                        select info)
                       .Single();

            methods = memo((Type valueType) => optional.MakeGenericMethod(valueType));
        }

        public override bool CanConvertFrom(ITypeDescriptorContext context, Type sourceType) =>
            true;

        public override bool CanConvertTo(ITypeDescriptorContext context, Type destinationType) =>
            true;

        public override object ConvertFrom(ITypeDescriptorContext context, CultureInfo culture, object value) =>
            value == null
                ? None
                : methods(value.GetType())?.Invoke(null, new[] { value });

        public override object ConvertTo(ITypeDescriptorContext context, CultureInfo culture, object value, Type destinationType) =>
            value == null
                ? null
                : (value as IOptionalValue)?.MatchUntyped(
                      Some: x => x == null
                                     ? null
                                     : destinationType.IsAssignableFrom(x.GetType())
                                        ? x
                                        : x.GetType() == typeof(string)
                                            ? x.ToString()
                                            : typeof(IConvertible).IsAssignableFrom(destinationType)
                                                ? Convert.ChangeType(x, destinationType)
                                                : null,
                      None: () => null);
    }

Things to note that are different to yours:

  1. The reflection for finding the generic method Optional<T>(...) is now memoized, so it's cached on first use. That mitigates the performance penalty as much as possible.
  2. ConvertFrom is simplified because of 1.
  3. ConvertTo assumes that value is an IOptionalValue
  4. ConvertTo uses a new MatchUntyped method I have added to IOptionalValue which matches with object values.
  5. ConvertTo first checks if the T of Option<T> to see if it is assignable to the destinationType (therefore not requiring a conversion); if so it returns it as-is. It then checks if the destinationType is a string (which from me reading seems to be the most common requirement), if so it simply calls ToString(). If not it checks if it's IConvertible; if it is then it calls ChangeType(), if not it returns null.

The null is None thing I guess we just need to see how it works in the real world with WPF. So the test-harness is essential.

Here's a build with Some<T> and Option<T> using OptionalTypeConvertor and SomeTypeConvertor:

https://www.dropbox.com/s/x20q8rro1ms8tkm/lang-ext-type-conv-1.rar?dl=0

louthy commented 9 years ago

Ok, so I did a bit more research, and the proper implementation of a TypeProvider for something like an Option type (i.e. a type that wraps another) is this:

    public class OptionalTypeConverter : TypeConverter
    {
        static readonly Func<Type, MethodInfo> methods = null;
        static readonly MethodInfo optional = null;

        readonly Type optionType;
        readonly Type simpleType;
        readonly TypeConverter simpleTypeConverter;

        static OptionalTypeConverter()
        {
            optional = (from info in typeof(Prelude).GetMethods()
                        where info.Name == "Optional"
                        select info)
                       .Single();

            methods = memo((Type valueType) => optional.MakeGenericMethod(valueType));
        }

        public OptionalTypeConverter(Type type)
        {
            optionType = type;
            simpleType = type.GetGenericArguments().Single();
            simpleTypeConverter = TypeDescriptor.GetConverter(simpleType);
        }

        public override bool CanConvertFrom(ITypeDescriptorContext context, Type sourceType) =>
            sourceType == simpleType
                ? true
                : simpleTypeConverter == null
                    ? base.CanConvertFrom(context, sourceType)
                    : simpleTypeConverter.CanConvertFrom(context, sourceType);

        public override object ConvertFrom(ITypeDescriptorContext context, CultureInfo culture, object value) =>
            value == null
                ? None
                : value.GetType() == simpleType
                    ? methods(value.GetType())?.Invoke(null, new[] { value })
                    : value is string && String.IsNullOrEmpty(value as string)
                        ? None
                        : simpleTypeConverter == null
                            ? base.ConvertFrom(context, culture, value)
                            : methods(value.GetType())?.Invoke(
                                null,
                                new[] { simpleTypeConverter.ConvertFrom(context, culture, value) }
                                );

        public override bool CanConvertTo(ITypeDescriptorContext context, Type destinationType) =>
            destinationType == simpleType
                ? true
                : destinationType == typeof(InstanceDescriptor)
                    ? true
                    : simpleTypeConverter == null
                        ? base.CanConvertTo(context, destinationType)
                        : simpleTypeConverter.CanConvertTo(context, destinationType);

        private object ConvertToValueNull(ITypeDescriptorContext context, CultureInfo culture, Type destinationType) =>
            destinationType == typeof(string)
                ? String.Empty
                : base.ConvertTo(context, culture, null, destinationType);

        public override object ConvertTo(ITypeDescriptorContext context, CultureInfo culture, object value, Type destinationType) =>
            value == null
                ? ConvertToValueNull(context, culture, destinationType)
                : (value as IOptionalValue)?.MatchUntyped(
                      Some: x => destinationType == simpleType
                                     ? x
                                     : destinationType == typeof(InstanceDescriptor)
                                         ? new InstanceDescriptor(
                                               optionType.GetConstructor(new Type[] { simpleType }),
                                               new object[] { value },
                                               true)
                                         : x == null
                                             ? ConvertToValueNull(context, culture, destinationType)
                                             : simpleTypeConverter == null
                                                ? base.ConvertTo(context, culture, x, destinationType)
                                                : simpleTypeConverter.ConvertTo(context, culture, x, destinationType),
                      None: () => ConvertToValueNull(context, culture, destinationType));
    }

Why anybody would use these methods and put themselves through this torture is beyond me ;)

Anyway, you can grab a new test build from here: https://www.dropbox.com/s/yvshwciz9b51kvf/lang-ext-type-conv-3.rar?dl=0

Let me know if it works for you.

marcpiechura commented 9 years ago

HI,

works great, with some modifications ;)

In ConvertFrom

: methods(value.GetType())?.Invoke(
                                null,
                                new[] { simpleTypeConverter.ConvertFrom(context, culture, value) }
                                );

you use value.GetType but it must be simpleType.

            value == null
                ? None
                : value.GetType() == simpleType
                    ? methods(value.GetType())?.Invoke(null, new[] { value })
                    : value is string && String.IsNullOrEmpty(value as string)
                        ? None
                        : simpleTypeConverter == null
                            ? base.ConvertFrom(context, culture, value)
                            : methods(simpleType)?.Invoke(
                                null,
                                new[] { simpleTypeConverter.ConvertFrom(context, culture, value) }
                                );

And Prelude contains Optional<T>(T) and Optional<T>(Nullable<T>), so this one

optional = (from info in typeof(Prelude).GetMethods()
                        where info.Name == "Optional"
                        select info)
                       .Single();

throwns an Exception and Single must changed to First.

optional = (from info in typeof(Prelude).GetMethods()
                        where info.Name == "Optional"
                        select info)
                       .First();

Another problem is binding a ListBox to Option<List<string>>, because Option implements IEnumerable the binding isn't calling the TypeConverter but calls directly IEnumerator IEnumerable.GetEnumerator() from Option. With this method it works, but I'm not sure if this would break something else or if this was the idea behind implementing IEnumerable at all.

        IEnumerator IEnumerable.GetEnumerator() =>      
            (value is IEnumerable) 
            ? (value as IEnumerable).GetEnumerator() 
            : AsEnumerable().GetEnumerator();

One last thing, do you know that the static using has changed? using LanguageExt.Prelude; doesn't work any more, must changed to using static LanguageExt.Prelude; Just asking because in the readme and sources the old way is still used and I had to change it everywhere befor I could compile the source code ;)

marcpiechura commented 9 years ago

Sorry, I skip that you have also implemented it for Some. Could you explain me when some should be used for member declaration(of course only in the scope of a view model)? For me it sounds like the only reason would be to be sure that the value can't be null, like int but for all types.

Normally I would handle this with Nullable and some validation which checks that HasValue is true or a ValueConverter which throws an Exception like "xyz is required and must be a number", because the default behavior in WPF when you bind a textbox to int looks like this: On start text is set to default int value image if you type in some number it converts successfully image and when you clear the textbox you get an error message "The value "" could not be converted" image

and the same for inputs which aren't a number image

and this error message in the Output window from VS

System.Windows.Data Error: 7 : ConvertBack cannot convert value '1a' (type 'String'). BindingExpression:Path=OptionalString; DataItem='MainWindowViewModel' (HashCode=46256406); target element is 'TextBox' (Name='textBox'); target property is 'Text' (type 'String') FormatException:'System.FormatException: Die Eingabezeichenfolge hat das falsche Format.
   bei System.Number.StringToNumber(String str, NumberStyles options, NumberBuffer& number, NumberFormatInfo info, Boolean parseDecimal)
   bei System.Number.ParseInt32(String s, NumberStyles style, NumberFormatInfo info)
   bei System.String.System.IConvertible.ToInt32(IFormatProvider provider)
   bei System.Convert.ChangeType(Object value, Type conversionType, IFormatProvider provider)
   bei MS.Internal.Data.SystemConvertConverter.ConvertBack(Object o, Type type, Object parameter, CultureInfo culture)
   bei System.Windows.Data.BindingExpression.ConvertBackHelper(IValueConverter converter, Object value, Type sourceType, Object parameter, CultureInfo culture)'

So for some this would mean every value that comes from the UI which is null or can't be converted to T would show such a "meaningful" error message like "Can't be null" or "Cant' be converted"... And are you returning None if string is empty? Got this error message when clearing the textbox bind to Some<int> "LanguageExt.OptionNone" could not converted to "LanguageExt.Some`1[System.Int32]"

So the some converter should not check for empt string, but rather let the simpletypeconverter handle this case, because it should work for some<string> but not for some<int>. And last but not least, this happens only for value types but with some this would also happen for reference types...

With all this in mind, I would never bind to Some, but rather use Option and some validation logic to handle these cases...

ButchersBoy commented 9 years ago

Hi, just throwing in my tuppence worth, but as a WPF developer, that TypeConverter is pure ugly. That's part of what view-models are for, to massage stuff in and out of a view. Why bind directly to the Option<>, when you can wrap/convert it in some kind of proxy for view-time? For example, I never bind a TextBox.Text to an int. Id' bind it to a string, and manage validation & conversion in the view-model without having to jump through horrible hoops.

marcpiechura commented 9 years ago

Hi @ButchersBoy

as I said I also would not bind a Textbox to an int ;-) But without the TypeProvider a conversion from every Option<T> to T would be required, if you use Option in you model. In your example you would create a facade over you model, which makes sense of course, but what about use cases where you do not need any validation or conversion? You would end up with useless getter and setter in the viewmodel wich are converting the option from the model, in these cases I would just bind directly to the model properties.

Or do you have a better idea how to solve these cases?

ButchersBoy commented 9 years ago

Hi @Silv3rcircl3 ...no, I don't really have a better idea, other than these comments:

If it's a readonly (OneTime) binding, yeah, expose the Option<>. If it's not, simplify it through a property, for various reasons:

On my current system we have our own similar Option<>, but whether its Option<>, or another domain object I normally try and follow the process:

Construct VM/Unpack domain object > user edits VM/proxies > user submit > pack into Domain object, leave VM

This also, roughly translates into

Immutable Object > Mutable Object > Immutable Object

The longer your data is immutable (e.g sitting in a Option<>), the less brittle your code will be. UI aspects are notoriously brittle. So on exit/save/when appropriate, at that point push into domain/Option<>/immutable object as the data leaves the VM.

This is just my opinion, but I've written many successful UI screens in this manner.

marcpiechura commented 9 years ago

I agree to all your opinions and you have probably more experience then I do. So maybe you could give me then an advice, because the projects I'm working on aren't from these nature, were a user clicks a button, "fills out five fields" and clicks submit.

It's more like a reporting tool, so the user opens a "document", in my case a patient, then has to fill out ~150 fields, some with validation, some without and then closes the program. Of course, in some cases there are some "submit" dialogs and I handle these as you discriped. But in general, I "just" have to expose many fields as long as the programm is running,

So, how would you handle these cases? Would you sill use a "proxy" for all properties? Currently I bind 90% directly to the model(which is exposed through the vm) and the other 10%, are fields where I need conversion or validation, so I bind them to the VM. I know, that's not as it should be, but is the effort worth the outcome?

Furthermore a subset of ~50 fields need to be displayed in a live report, for printing... So these would be your read only case, but without the TypeConvert, I would need to expose these fields via proxy again.

So, hopfully you understeand why I would like to bind directly to my model properties without exposing them again through the vm. Having said that, I would of course change my opinions and use the vm as proxy instead of the TypeConverter.

So I would be thankful for your advice!

ButchersBoy commented 9 years ago

Reading your use case, I still stand by what I have said. 5 fields or 150, your users opens the app, edits it then closes the screen, and potentially the app.

Two things stood out for me from what you have described:

Hope that helps!

marcpiechura commented 9 years ago

Sounds good. I already read about immutable models and replacing instead of updating in context of F# and event sourcing and I found it quite usefull but never get my head around how to apply it for my use case ;-)

I definitly give it a try! So, many thanks for your adivce!

@louthy I think you can close this, I will try @ButchersBoy approach, sorry for coming up with this ;)

louthy commented 9 years ago

you use value.GetType but it must be simpleType.

Fixed

And Prelude contains Optional(T) and Optional(Nullable)

Fixed

Another problem is binding a ListBox to Option<List>, because Option implements IEnumerable the binding isn't calling the TypeConverter but calls directly IEnumerator IEnumerable.GetEnumerator() from Option.

You'd have to manually unwrap that I think:

    var list = from x in option
               from y in x
               select y;

(You'll get an empty list if option is None)

One last thing, do you know that the static using has changed?

Yep, I know that. I'd rather not keep chasing the last minute syntax changes, so I'm going to keep it as-is until the final release then I'll go through and update everything. Although I guess if people are using the later CTP for VS2015 then it could come to a head. I do make sure I keep nuget up to date though.

Could you explain me when some should be used for member declaration(of course only in the scope of a view model)?

Some<T> is a bit of a problem child due to the problem of uninitialised structs not calling a default ctor. So I personally only use them for method parameters to enforce non-null arguments.

So the some converter should not check for empt string, but rather let the simpletypeconverter handle this case, because it should work for some but not for some.

Fixed (I think)

With all this in mind, I would never bind to Some

I think it's probably best.

Hi, just throwing in my tuppence worth, but as a WPF developer, that TypeConverter is pure ugly.

I agree it's ugly, I'm glad I don't have to use .NET binding on a regular basis! ;-)

Why bind directly to the Option<>, when you can wrap/convert it in some kind of proxy for view-time?

I think mainly because Option<T> should be seen as a fundamental type like Nullable<T>. It would be very un-DRY to keep having to unwrap I would have thought? I looked at the source for the Nullable<T> TypeConverter and followed the pattern laid down there. So that's where the ugliness comes from, ha!

I don't doubt there are more optimal ways to do this; but I suspect not everyone needs to have a super high-performance UI for a few textboxes/checkboxs/lists and the like. I don't mind working on this to get it to work for those use cases.

Your model/domain layer is mutable. I always strive to have an immutable domain object. If it needs to change, you replace it. Which is the same with Option<>. Even if the backing store (whatever that may be) only updates on application exit, it is good practice.

Sound advice.

Latest version: https://www.dropbox.com/s/d58ggeqfaa77712/lang-ext-type-conv-4.rar?dl=0

marcpiechura commented 9 years ago

Works now! Should I create a demo application for you?

louthy commented 9 years ago

That would be appreciated, yes. Then I can create some unit-tests using that as a reference.

Thanks :)

marcpiechura commented 9 years ago

Apologies for the late response but I was ill the last month.

I just want to ask if you still need the demo project, I could create one tomorrow ;-)

regards

samirahmed commented 9 years ago

To be fair, its not just WPF that needs this ... in MVC and WEBApi TypeConvertors are used to bind incoming requests. Without a typeconvertor it would mean I can't use Optionals at the Request object binding layer (which is imperative since I would have to validate incoming requests and at times allow for optionally null)