Skip to content

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

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

Discussion: We don't need INotifyDataErrorInfo for validation #4567

Closed
omikhailov opened this issue Mar 18, 2021 · 2 comments
Closed

Discussion: We don't need INotifyDataErrorInfo for validation #4567

omikhailov opened this issue Mar 18, 2021 · 2 comments
Labels
area-InputValidation discussion General discussion team-Controls Issue for the Controls team wct

Comments

@omikhailov
Copy link

Discussion: What's the practical point of supporting that interface?

Perhaps, in the times of first WPF releases the idea of highlighting textbox with red when validation fails was innovative and fresh, but today it is not enough. Validation should allow convenient customization of error messages with bindings, specialized controls and templates, but INotifyDataErrorInfo offers just event and a method to get information about errors. It would be much more convenient if it would provide bindable collection instead and would not be coupled with ViewModel.

In fact, most of the problems with validation can be solved quite simply if you would try to change the approach and instead of repeating old mistakes, start making new ones. For example, this is what I came up with after playing a couple of hours in the Visual Studio:

    <Grid>
        <Grid HorizontalAlignment="Center" VerticalAlignment="Center">
            <!-- There should be new templatable control for validation warnings instead of Popup -->
            <Popup IsOpen="{x:Bind viewModel.FirstNameValidator.HasIssues, Mode=OneWay}" Closed="{x:Bind viewModel.FirstNameValidator.Reset}" IsLightDismissEnabled="True" VerticalOffset="-80">
                <Border BorderThickness="1" BorderBrush="{StaticResource SystemControlDisabledAccentBrush}" Background="{StaticResource ApplicationPageBackgroundThemeBrush}" CornerRadius="4" Padding="16,20,16,20">
                    <winui:ItemsRepeater ItemsSource="{x:Bind viewModel.FirstNameValidator.Issues, Mode=OneWay}" />
                </Border>
            </Popup>
            <TextBox Width="200" Text="{x:Bind viewModel.FirstName, Mode=TwoWay}" PlaceholderText="First Name" />
        </Grid>
    </Grid>
    public class MainViewModel : INotifyPropertyChanged
    {
        public RulesValidator<string> FirstNameValidator { get; } = new RulesValidator<string>(
            FrameworkValidationRule.StringIsNotEmpty(), 
            FrameworkValidationRule.StringMinLength(2));

        private string _firstName;

        public string FirstName
        { 
            get
            {
                return _firstName;
            }
            set
            {
                if (FirstNameValidator.Validate(value))
                {
                    _firstName = value;

                    OnPropertyChanged();
                }
            }
        }
    }

What in this code is better than in the ObservableValidator samples:

  • ViewModel does not have to implement additional interface or inherit from base class.
  • It is clear where the validation data is and which validation object is responsible for which area.
  • You can make whatever you want with that data, it can be used with new specialized control or with custom layouts - usage is as simple as binding.
  • Bindings don't have to support anything new, both {x:Bind } and {Binding } can bind to Validator.
  • It is not required to update TextBox and other controls templates. We can add a property IsValid and new visual state to some controls where it would be appropriate to let developers bind that property to Validator.HasIssues, but once we will ship ValidationPopup, it will not be a blocking task.
  • There is no reflection.

Below is the rest of the code required to better understand or compile this sample. It's all a draft, but it demonstrates that validation without INotifyDataErrorInfo can be simple and flexible, so I encourage you to consider whether WinUI should really support the old interface or consider other options. I am sure many people here can suggest better options.

    public interface IValidator<T> : INotifyPropertyChanged // Common interface to use binding in the new ValidationPopup control
    {
        bool HasIssues { get; }

        IEnumerable Issues { get; } // Error message strings or complex objects for binding within custom templates in the new control

        bool Validate(T value);

        void Reset();
    }

    public interface IValidationRule<T> // Validation rule predefined in MVVM framework or created by developer
    {
        object Validate(T value);
    }

    public class RulesValidator<T> : IValidator<T> // Validator for validation rules
    {
        private IValidationRule<T>[] _rules;

        public RulesValidator(params IValidationRule<T>[] rules)
        {
            _rules = rules;
        }

        public bool HasIssues { get; private set; }
                
        public IEnumerable Issues { get; private set; }

        public event PropertyChangedEventHandler PropertyChanged;

        public bool Validate(T value)
        {
            var result = true;

            var issues = new List<object>(_rules.Length);

            foreach (var rule in _rules)
            {
                var validationResult = rule.Validate(value);

                if (validationResult != null)
                {
                    result = false;

                    issues.Add(validationResult);
                }
            }

            HasIssues = !result;

            Issues = issues;

            FirePropertiesChanged();

            return result;
        }

        public void Reset()
        {
            HasIssues = false;

            Issues = null;

            FirePropertiesChanged();
        }

        private void FirePropertiesChanged()
        {
            PropertyChanged?.Invoke(this, new PropertyChangedEventArgs(nameof(HasIssues)));

            PropertyChanged?.Invoke(this, new PropertyChangedEventArgs(nameof(Issues)));
        }
    }

    public static class FrameworkValidationRule // Static class with predefined validation rules
    {
        private static StringIsNotEmptyValidationRule _stringIsNotEmpty = new StringIsNotEmptyValidationRule();

        public static IValidationRule<string> StringIsNotEmpty()
        {
            return _stringIsNotEmpty;
        }
    }

    public class StringIsNotEmptyValidationRule : IValidationRule<string> // Sample validation rule
    {
        public object Validate(string value)
        {
            return string.IsNullOrWhiteSpace(value) ? "This field should not be empty!" : null;
        }
    }

    public class DataAnnotationsValidator<T> : IValidator<T> // Validator for DataAnnotation attributes
    {
        ...
    }

    public class MyValidator : IValidator<T> // Developer can make custom validators for complex cases
    {
        ...
    }

Related Links

#179 Add Input Validation Support to WinUI (UWP and Win32 Desktop) with INotifyDataErrorInfo
ObservableValidator from Community Toolkit
INotifyDataErrorInfo

@omikhailov omikhailov added the discussion General discussion label Mar 18, 2021
@ghost ghost added the needs-triage Issue needs to be triaged by the area owners label Mar 18, 2021
@StephenLPeters StephenLPeters added team-Controls Issue for the Controls team area-InputValidation and removed needs-triage Issue needs to be triaged by the area owners labels Mar 18, 2021
@StephenLPeters
Copy link
Contributor

@ranjeshj and @MikeHillberg

@michael-hawker
Copy link
Collaborator

Duplicate/Related to #5859 and #4671, alternative to #179.

@microsoft microsoft locked and limited conversation to collaborators Jul 22, 2022
@ranjeshj ranjeshj converted this issue into discussion #7464 Jul 22, 2022

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

Labels
area-InputValidation discussion General discussion team-Controls Issue for the Controls team wct
Projects
None yet
Development

No branches or pull requests

3 participants