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

Refactor Validation #3586

Merged
merged 43 commits into from May 1, 2020
Merged

Conversation

danwalmsley
Copy link

@danwalmsley danwalmsley commented Apr 24, 2020

Validation is a little bit of a mess and doesnt work in some cases.

I have simplified how validation works, and it will now support mulitple errors on a single property... this was only partially supported previously.

Furthermore you can now prevent commands from executing by observing the "HasErrors" property now and this will work.

I have removed the [ValidateMethod] attribute and replaced this with naming convention.

Basically now if you have a property called Password and you add a method (public or private) called ValidatePassword(IErrorList errors) it will automagically get called during validation.

The validation logic is all inside ViewModelBase now

@jmacato iv no idea what that ErrorDescriptors to Border converter is doing... can you give some input here.. seems it shouldnt be needed anymore?
Colour converter shouldnt be a source of errors in the UI, so we need to at least refactor it, but I need help with that.

Validation methods went from:

public ErrorDescriptors ValidateUserFeeText()
		{
			return TryParseUserFee(out _)
				? ErrorDescriptors.Empty
				: new ErrorDescriptors(new ErrorDescriptor(ErrorSeverity.Error, "Invalid fee."));
		}

to

private void ValidateUserFeeText(IErrorList errors)
		{
			if (!TryParseUserFee(out _))
			{
				errors.Add(ErrorSeverity.Error, "Invalid fee.");
			}
		}

@molnard
Copy link
Collaborator

molnard commented Apr 24, 2020

I was OK with the reflection, it was cached. Is Avalonia using the same reflection solution?

@danwalmsley
Copy link
Author

danwalmsley commented Apr 24, 2020

I was OK with the reflection, it was cached. Is Avalonia using the same reflection solution?

No, you never implement this code inside Avalonia... however Avalonia is removing all reflection code from its code base.

Its cached. but the reflection takes place when you want the most performance, when the ui loads.

I will see what implementing some kind of naming convention based on reflection looks like.

BTW the only advantage is to remove the calls to RegisterValidationMethod.

@danwalmsley
Copy link
Author

danwalmsley commented Apr 24, 2020

@molnard iv implemented it using reflection and it actually makes it really easy to use... its actually very fast so probably dont need to worry about performance.

Basically now if you have a property called Password and you add a method (public or private) called ValidatePassword(IErrorList errors) it will automagically get called during validation.

@jmacato
Copy link
Contributor

jmacato commented Apr 26, 2020

@danwalmsley as far as i remember the ErrorDescriptorToBorderColorConverter unpacks the error objects it receives and transforms it into color for the borders of the textboxes. Sometimes the code received errordescriptors or just plain exceptions so i made it handle those two.

Copy link
Collaborator

@yahiheb yahiheb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Settings tab doesn't open:

2020-04-27 03:04:14 ERROR       ToolCommands (47)       System.ArgumentException: Cannot bind to the target method because its signature is not compatible with that of the delegate type.
   at System.Reflection.RuntimeMethodInfo.CreateDelegateInternal(Type delegateType, Object firstArgument, DelegateBindingFlags bindingFlags)
   at System.Reflection.RuntimeMethodInfo.CreateDelegate(Type delegateType, Object target)
   at WalletWasabi.Gui.ViewModels.ViewModelBase.RegisterValidationMethods() in C:\Users\RESEAU\Desktop\WalletWasabi\WalletWasabi.Gui\ViewModels\ViewModelBase.cs:line 46
   at WalletWasabi.Gui.ViewModels.ViewModelBase..ctor() in C:\Users\RESEAU\Desktop\WalletWasabi\WalletWasabi.Gui\ViewModels\ViewModelBase.cs:line 23
   at WalletWasabi.Gui.ViewModels.WasabiDocumentTabViewModel..ctor(String title) in C:\Users\RESEAU\Desktop\WalletWasabi\WalletWasabi.Gui\ViewModels\WasabiDocumentTabViewModel.cs:line 20
   at WalletWasabi.Gui.Tabs.SettingsViewModel..ctor() in C:\Users\RESEAU\Desktop\WalletWasabi\WalletWasabi.Gui\Tabs\SettingsViewModel.cs:line 44
   at WalletWasabi.Gui.Shell.Commands.ToolCommands.<>c.<.ctor>b__0_4() in C:\Users\RESEAU\Desktop\WalletWasabi\WalletWasabi.Gui\Shell\Commands\ToolCommands.cs:line 30
   at AvalonStudio.Shell.IShellExtensions.AddOrSelectDocument[T](IShell me, Func`1 factory)
   at WalletWasabi.Gui.Shell.Commands.ToolCommands.<>c.<.ctor>b__0_0() in C:\Users\RESEAU\Desktop\WalletWasabi\WalletWasabi.Gui\Shell\Commands\ToolCommands.cs:line 30
   at ReactiveUI.ReactiveCommand.<>c__DisplayClass0_0.<Create>b__1(IObserver`1 observer) in d:\a\1\s\src\ReactiveUI\ReactiveCommand\ReactiveCommand.cs:line 108
   at System.Reactive.Linq.QueryLanguage.CreateWithDisposableObservable`1.SubscribeCore(IObserver`1 observer) in D:\a\1\s\Rx.NET\Source\src\System.Reactive\Linq\QueryLanguage.Creation.cs:line 35
   at System.Reactive.ObservableBase`1.Subscribe(IObserver`1 observer) in D:\a\1\s\Rx.NET\Source\src\System.Reactive\ObservableBase.cs:line 58

WalletWasabi.Gui/Tabs/SettingsViewModel.cs Outdated Show resolved Hide resolved
WalletWasabi.Gui/Tabs/SettingsViewModel.cs Outdated Show resolved Hide resolved
WalletWasabi.Gui/ViewModels/Validation/Validator.cs Outdated Show resolved Hide resolved
danwalmsley and others added 3 commits April 27, 2020 11:05
Co-Authored-By: yahiheb <52379387+yahiheb@users.noreply.github.com>
Co-Authored-By: yahiheb <52379387+yahiheb@users.noreply.github.com>
yahiheb
yahiheb previously approved these changes Apr 30, 2020
Copy link
Collaborator

@yahiheb yahiheb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tACK

@molnard
Copy link
Collaborator

molnard commented Apr 30, 2020

ACK!
Why is it not look the same?

image

molnard
molnard previously approved these changes Apr 30, 2020
@molnard molnard dismissed stale reviews from yahiheb, jmacato, and themself via 4c692ac April 30, 2020 16:54
@danwalmsley
Copy link
Author

ACK!
Why is it not look the same?

image

you have 1 of the textboxes selected (or activated) the other one not.

@danwalmsley
Copy link
Author

danwalmsley commented Apr 30, 2020

Done some refactoring to support:

  1. knowing if there are Any validations.
  2. AnyErrors AnyWarnings ,etc
  3. Allow enumeration of Errors, Warnings, Infos, etc
  4. separated validation code in to validation namespace.
  5. hidden validation apis from end users (people using viewmodelbase)
  6. Validation property for users to read and deal with validations.

Copy link
Collaborator

@molnard molnard left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Concept ACK

WalletWasabi.Gui/ViewModels/ViewModelBase.cs Show resolved Hide resolved
WalletWasabi.Gui/ViewModels/ViewModelBase.cs Show resolved Hide resolved
WalletWasabi.Gui/Validation/Validations.cs Outdated Show resolved Hide resolved
@danwalmsley
Copy link
Author

Codefactor is yet again showing an issue found, but it doesnt actually exist.

WalletWasabi.Gui/Validation/Validations.cs Outdated Show resolved Hide resolved
WalletWasabi.Gui/Validation/Validations.cs Outdated Show resolved Hide resolved
WalletWasabi.Gui/ViewModels/ViewModelBase.cs Outdated Show resolved Hide resolved
Copy link
Collaborator

@yahiheb yahiheb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tACK

Copy link
Collaborator

@molnard molnard left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very good! Cheers!

@danwalmsley
Copy link
Author

Very good! Cheers!

@molnard

image

@molnard molnard merged commit aefe492 into zkSNACKs:master May 1, 2020
Dev meeting automation moved this from danwalmsley to Done May 1, 2020
@danwalmsley danwalmsley deleted the refactor/validation branch May 1, 2020 19:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

5 participants