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
feat(textinput): do not block input on validation #185
feat(textinput): do not block input on validation #185
Conversation
❤️ This is fantastic! @meowgorithm and I were just talking about how this would be a great addition. Thank you for reading our minds @GabrielNagy 😄 |
bac2470
to
801ac44
Compare
@maaslalani let me know how I can help getting this merged 🙏🏼 |
Hey @GabrielNagy sorry for forgetting about this one! I almost wonder if the The difference would be you could then validate input and give warning errors such as "file not found" but also prevent input that will never be valid such as inserting a special character in a file name. This obviously would be a breaking API change so we might need to think about it a little more, but I'm curious to hear your thoughts on the validation function returning a bool for whether to allow the input or block it from changing. And, you can always return false or always return true for the behaviour in the current PR. |
That sounds good to me! It's definitely the better solution apart from the backwards compatibility issue. |
I really like the option to block or warn on invalid input! As for the separate validation functions, would a more general |
I just hit this issue as well, and I need to stop using the |
Hitting this issue too. Can't validate FQDN because I can't type in the first dot. What it needs for this to be merged? |
801ac44
to
b0c7209
Compare
I rebased and implemented the backwards-incompatible suggestion from @maaslalani. I'd be happy to work on some tests as well once we decide on a way forward. |
Any updates on this? Willing to help if needed as well. |
@GabrielNagy, what do you think of simply removing the blocking of the validation? That way we can keep the signature of the function the same. We would then simply put the responsibility on the user to ensure that there is no I like the idea of keeping the |
|
||
if m.Err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So essentially we just remove this line and never block.
Thanks for coming back to this. Yeah, not blocking the input would solve all our issues since we already handled blocking submit ourselves, e.g. https://github.com/ubuntu/adsys/blob/ca3458501ff5b2f72f16be266bad7a189a890acf/internal/watchdtui/watchdtui.go#L224-L227 I'll push an update to the PR! |
b0c7209
to
3d2b848
Compare
3d2b848
to
5b06afd
Compare
Thanks for all your patience on this one @GabrielNagy! |
👋 Thanks for looking at this - I just spent a couple of hours debugging this issue on my own project 😆 |
This PR builds upon the excellent work in charmbracelet#167 and charmbracelet#114 and makes a breaking change to the validation API. Currently, validation will completely block text input if the Validate function returns an error. This is now changed so the function no longer blocks input if this is the case, thus handing this responsibility to the clients. This is helpful for cases where the user is requested to type an existing system path, and the Validate function keeps asserting the existence of the path. With the current implementation such a validation is not possible. For example: > / Err: nil > /t Err: /t: No such file or directory > /tm Err: /tm: No such file or directory > /tmp Err: nil
case key.Matches(msg, m.KeyMap.DeleteWordBackward): | ||
m.deleteWordBackward() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason for deleting this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah I see it's a duplicate now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, apologies should have pulled that to a separate commit. It confused me as well when re-reading the code. 😅
5b06afd
to
e7b3760
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome work @GabrielNagy, just tested and it works great!
case key.Matches(msg, m.KeyMap.DeleteWordBackward): | ||
m.deleteWordBackward() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah I see it's a duplicate now.
This PR builds upon the excellent work in #167 and #114 and makes a breaking change to the validation API.
Currently, validation will completely block text input if the Validate function returns an error. This is now changed so the function no longer blocks input if this is the case, thus handing this responsibility to the clients.
This is helpful for cases where the user is requested to type an existing system path, and the
Validate
function keeps asserting the existence of the path. With the current implementation such a validation is not possible.For example: