Don't hardcode Form widget validation for Entry #4250
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
|
The APIs here are very rough around the edges but I would like some opinions on it to try to shape it into something more fit for landing in the codebase. This should solve the problems we had with extended widgets. |
|
Hmm. This does still not seem to fix the issue mentioned above. However, it no longer relates to there being an extended entry any more so that's a win. It only replicates inside the form dialog but not the form widget on its own. I suspect that there is a bug in ow and when he form updates the validation status to the parent. |
Jacalz
changed the title
There was a problem hiding this comment.
I think the logic in naming ShouldDisplayValidation is backwards...
Also this introduces a new type with 4 methods for what looks like a single new function - can you share why?
Lastly, can this not be done with an anonymous interface instead of defining new APIs? If the fix was to resolve extended Entries we don't need to expose any new type as it is a case of Entry implementing a known function only...
Hehe yeah indeed. Nice catch 🙈
I'm entirely aware that this API is very rough around the edges. I just basically added a new method incrementally for each place where we were using internal API details from Entry. It definitely needs to be cleaned up, like #4250 (comment) and potentially more. However, I doubt that all four methods can be added as only one. I'm more than happy to receive suggestions, or work together with someone, to try to find a cleaner solution :) I will try to clean up the API and slim it down now that it actually works like before minus the Entry specific code. However, it would be nice with some thoughts about better solutions to the internal API usage that gets removed here.
It can yes, but it avoids parts of what I'm trying to achieve here. The idea was never to work on this just to fix the issue. I was just hoping to fix it at the same time by more generally making the Form not hardcode validation to Entry. Basically, anyone should be able to create a widget that has enough context to display useful validation messages in a Form. This also doesn't even fix the issue linked above. It just happens to make the extended entry work the same as the not extended one 😅 |
|
I have looked at this some more now and I don't quite know how to simplify it any more. I can't think of a way of reworking Form to not require the methods that I have added. |
Jacalz
force-pushed
the
form-should-not-hardcode-entry-for-validation
branch
from
6f401ff
to
1905597
Compare
|
Sorry I let this sit for a while, I will take a closer look and see why each of these methods is needed and try to think of simplifications myself. |
There was a problem hiding this comment.
Thanks for this, good to support extendible types etc for validation.
However I think the new interface exposes, or requires, to much - see my comments inline. I think we can leave it at HasValidator and ShouldDisplayValidation - these are clearly extra metadata about validation so fits within the interface idea.
| @@ -47,6 +63,14 @@ func (e *Entry) SetValidationError(err error) { | |||
| } | |||
| } | |||
|
|
|||
| // ShouldDisplayValidation returns true if the entry has not been interacted with | |||
There was a problem hiding this comment.
The dirty check seems to be backwards here - if it is not dirty it should not display validation?
|
|
||
| // SetOnFocusChanged is intended for parent widgets or containers to hook into the change of focus. | ||
| // The function might be overwritten by a parent that cares about child validation (e.g. widget.Form). | ||
| SetOnFocusChanged(func(bool)) |
There was a problem hiding this comment.
This feels like exposing internal details - I can see why it was needed but I don't think it's the cleanest way.
Can't we wrap the widgets if they are already Focusable and intercept the FocusGained and FocusLost?
| SetOnFocusChanged(func(bool)) | ||
|
|
||
| // SetValidationError manually updates the validation status until the next input change. | ||
| SetValidationError(error) |
There was a problem hiding this comment.
I'm not sure about this either - it means that validation moves from a action to a state.
I would look carefully at where it is used now - I think that the new methods added should remove the necessity to set an initial state, especially if we have interception of focus events.
Description:
This will likely fix #4147
Checklist:
Where applicable: