Form Validation Status

[chabad360]

Description:

Allows interacting with the validation status of a form

Checklist:

• Tests included.
• Lint and formatter run with no errors.
• Tests all pass (only tested widgets, as other tests were already failing on this branch).

Where applicable:

• Public APIs match existing style.
• Public APIs have version numbers.

[coveralls]

Coverage increased (+0.03%) to 61.539% when pulling b921d14 on chabad360:develop into 0f13bfe on fyne-io:develop.

[Jacalz]

It seems to me like this overlaps with #2870 quite a bit, but perhaps with different end results.

[chabad360]

It does, sort of. This effectively just passes whether the submit button is disabled as a validation, while #2870 attempts to give the form its own validation abilities.
So there is definitely some overlap, but I don't think this will have a significant effect (if any) on the implementation of #2870.

[Jacalz]

So there is definitely some overlap, but I don't think this will have a significant effect (if any) on the implementation of #2870.

Sorry, but I unfortunately don't think that's true. As they are implementing the same methods, it would cause some massive merge conflicts if one is merged before the other.

[chabad360]

Ah, yes, I suppose I was speaking a bit more theoretical. What I meant was that these changes should be almost entirely compatible (in concept) with those of #2870. I agree it would cause a merge conflict, but I don't think it would be very hard to resolve, as the conflicting functions are almost identical (other than a few lines). I'll doublecheck later today just to be sure.

[chabad360]

Mmm, now that I've checked, the main complication would be in SetValidationError(), and even then it wouldn't be that bad, as #2870 would still need to implement at least part of my logic or there would be some issues (particularly around enabling the enable button despite having invalid entries). Point is, these two PRs are pretty compatible (at least from my perspective), @Cookie04DE can you give your opinion?

[Cookie04DE]

Yes, I believe so too. More specifically I think Validate should check for any errors in the child widgets (your logic) and if there are none then check for the error returned by the form's validator (my logic). Though I think my SetValidationError would replace your setValidationError, since mine is exported.

[chabad360]

There is a bit of logic from my setValidationError that you would need to use but other than that, it should be good.

Also, I should probably just make it public.

[andydotxyz]

Also, I should probably just make it public.

We try never to expose new public API unless there is a clear use-case. So "just make it public" isn't something normally said ;).

[chabad360]

Fair enough, I figured that since the only other place that has it, has it public, this should probably be public too.

[andydotxyz]

Fair enough, I figured that since the only other place that has it, has it public, this should probably be public too.

Yes I see that too, and I realise this is not straight forward. Maybe it was the right thing to do.
However not all validation has SetValidationError (public or private) so in the bigger picture of all validatable widgets, what is the right thing to do?

Apart from this point it looks like a great change and I am glad that @Cookie04DE is happy to build on it in the larger PR. What do you think @Jacalz ?

[Jacalz]

Apart from this point it looks like a great change and I am glad that @Cookie04DE is happy to build on it in the larger PR. What do you think @Jacalz ?

Sounds good to me. I'm glad that everyone seems to be happy with landing this first. It might make the other PR easier to review as well given that a subset of it already will have landed by then.

[chabad360]

In some further testing, I discovered a bug where the error value didn't update properly because it was missing an initial errFormItemInitialState (so there was no change when the validation changed). This should be fixed with 5ad11c7.

But now it makes me wonder if there might be a better way of initializing this, perhaps adding an Initialize() call or similar to the interface so that it's clear you need to set it up before use, or can the widget perhaps do this itself in SetOnValidationChange() that way it's always certain to be cleared when the callback is set, etc. Either way, there needs to be a better way communicate this requirement, because if a custom widget isn't based on Entry it will have this problem (provided the changed state it enters is a nil error).

[andydotxyz]

Either way, there needs to be a better way communicate this requirement, because if a custom widget isn't based on Entry it will have this problem (provided the changed state it enters is a nil error).

I'm not really sure what this has to do with custom widgets or Entry either - it is an internal detail of how form items are validated isn't it?

[chabad360]

It exists because Forms don't use the error of an item upon initialization (because we want to be nice to the end users on launch). To make sure that when an Entry that is invalid changes it's status we do something about it (i.e. enable submit), we set the error of the Entry to errFormItemInitialState so when onValidationChange() gets called later, it's always updated.

The issue with this behavior is that it only happens for an Entry, anything else (like a Form) needs to become invalid and then valid again (because SetValidationError() isn't in the interface, only in the Entry).

Now that I'm less tired, I think the best solution to the problem is just to make SetValidationError() part of the Validator interface. That way we can do this on all form items instead of just Entries.

[andydotxyz]

Ah I see your point. What do you think @Jacalz ?

[Jacalz]

Sorry for the late reply. I think it makes sense, but we need to be careful given that changing interfaces is a breaking change.

[chabad360]

So here's the question, should I add that change to this PR? Or should we make a separate one as an RFC to try and better alert anyone that may be caught by surprise from this change?

[andydotxyz]

Our contract is not to break things until 3.0 - so we can't just add the new method to the interface - it will break all custom validators.
Is there no way that we can do this in another way that avoids putting this on developers? It seems like we are pushing something onto their code that should not be required but is an issue with how we have implemented validation...

Looking for out of the box ideas as to how we can get this right I guess :)

[andydotxyz]

If you want to land this PR can it be done without the new method exposed? If this is to support embedding form in other form we can probably gloss over that for now...

[chabad360]

Our contract is not to break things until 3.0 - so we can't just add the new method to the interface - it will break all custom validators.

I'll admit I'm curious to see how many custom validators don't already have a SetValidationError(). Perhaps we should make a PR that can sit there until 3.0 is ready to happen just to gather comments.

Is there no way that we can do this in another way that avoids putting this on developers? It seems like we are pushing something onto their code that should not be required but is an issue with how we have implemented validation...

I'm not really sure there is, because any other container that holds validators would need to be able to do the same thing. So wither we have some not-simple logic to deal with this issue, or we just pre-set the validation error (which is nice and simple, but breaks an interface).

If you want to land this PR can it be done without the new method exposed?

Yes it can, I've dealt with it in 5ad11c7 but it's a dumb fix.

If this is to support embedding form in other form we can probably gloss over that for now...

This isn't just an issue for that use case. If someone puts anything between the Form and the Validator, it will cause a problem.

[chabad360]

Is there anything else I need to for this to get approved?

[andydotxyz]

Is there anything else I need to for this to get approved?

Yes, I don't think my previous comment about the new external function was really addressed - as far as I can see it is not needed in this implementation, so should be left private for further discussion about how we manage the complications of the initial validation state.

This isn't just an issue for that use case. If someone puts anything between the Form and the Validator, it will cause a problem.

The same is true for any combination of elements, as validation has to be passed up to the top level - anything that is not Validatable in the chain will block it.

[andydotxyz]

In summary of the above the issue of initial state for validation of a form should be separate and I think can be solved in a more elegant way (for example if the form was itself tracking if it had tested validation before assuming pass), so the additional external method may not be required when we solve it better.

[chabad360]

If you want I can make it private. I made it public simply because the other implementations I came across had it public as well.

[chabad360]

Perhaps you could leave an actual review on the code, that it's more clear what you want changed.

[chabad360]

Alrighty, lmk if you need any other changes.

[andydotxyz]

Thanks, this looks good to me now. @Jacalz did you want a quick feedback as well?

[Jacalz]

I have a few notes on the code. Sorry for the late review.

[Jacalz]

I was slightly incorrect in my last review. This is the right way to do it.

[Jacalz]

Sorry. I found two things but I swear that this is approved when you have answered :)

[Jacalz]

Looks good to me. Thanks. Sorry for all the back and forth. I have been very busy lately.
Btw, please don't force push when you don't have to. It has a habit of destroying the inline comments here on GitHub and generally makes reviewing harder.

[chabad360]

Btw, please don't force push when you don't have to. It has a habit of destroying the inline comments here on GitHub and generally makes reviewing harder.

Fair enough, I did it most recently because some commits from develop were accidentally merged into my branch and so I needed to drop them.