Feature: Forms

[VincentFoulon80]

I started implementing forms into console_engine, the objective is to allow the developer to easily create self-managed TUI forms. They'll be enclosed in their own "form" feature. The "event" feature will also be needed in order for them to handle user input properly.

🆕 Features

• Forms that can handle multiple fields simultaneously
• Fields:
  • Text
  • HiddenText
  • Checkbox
  • Radio
• Basic Field Validation:
  • NotBlank
  • Integer
  • Number
  • Alphabetic
  • Alphanumeric
  • Callback
  • CharactersCallback

[VincentFoulon80]

This Pull Request is mostly finished, if anybody wants to review it, you're welcome !

[VincentFoulon80]

One bigger change I'd suggest is "consuming" the events.

While I agree with the idea, I'm puzzled as to implement it or not. I'd like to allow the possibility of multiple fields handling a single event. Also, as you may have seen on the examples, we not only give the event to the form (so he may or may not use it) but after that we also use it afterward to handle for example form cancelling.

Edit: Also, a custom-made FormField that would always mark events as handled could break its program if he's not careful enough (it's up to the developer to fix this though)

Maybe I've misunderstood your statement ?

[LoipesMas]

I'd like to allow the possibility of multiple fields handling a single event

This sounds like something that could potentially be a hard-to-find problem in the future (either in development of the crate or when using the crate). And I don't really see the use case for that. The possibility could maybe implemented in an explicit way, e.g. some UI element that sends events to all of it's children.

we not only give the event to the form (so he may or may not use it) but after that we also use it afterward to handle for example form cancelling

Maybe better way to describe that was that first the program checks if it wants to do something with the event (quit, cancel, etc.) and if it decides that it doesn't then pass it forward. I think this makes more sense. Then it could be implemented like this:

match event {
    Event::Key(KeyEvent { KeyCode::Escape, modifiers }) => {
        break;
    },
    event => form.handle_event(&event),
}

I'm just worried that this handling of events in multiple places could cause some unexpected behavior.

[VincentFoulon80]

Right, I see your point. I'll work on that part then, that shouldn't take too much time to implement thankfully.

[LoipesMas]

One more thing: I wonder if it wouldn't be better if get_result would return a Result type that would have the validation error as Err. So instead of checking first if there are any errors and the proceeding with getting the form data, this would happen at once. Here's how I imagine it more or less. This is more inline with Rust design (Result type was made for that). I used a separate function for getting the data from form so you can use ? operator.

[VincentFoulon80]

One more thing: I wonder if it wouldn't be better if get_result would return a Result type that would have the validation error as Err. So instead of checking first if there are any errors and the proceeding with getting the form data, this would happen at once. Here's how I imagine it more or less. This is more inline with Rust design (Result type was made for that). I used a separate function for getting the data from form so you can use ? operator.

That's genius! I didn't think of that as those two functions were made several days apart. I'll apply this once I'm back to it. Thanks for the suggestion !

[VincentFoulon80]

Okay so I implemented a few things :

• Events are now consumed by FormFields, for forms, we only pass the event on the first active field we find
• Form.get_result() now returns a Result<FormValue, FormError> value, with FormError being an enum that either tells that the field does not exist, or that the validation failed (with the errors provided)
• Number constraint now parses the input to a f32, since they allow any range of number, it's better that way.
• I also added logic constraints, in case we need to allow more complex values (like field can be empty OR a valid integer)

[VincentFoulon80]

I addressed most of your comments,

I'm wondering something about validation constraints, precisely unhandled FormValue (like Nothing, Boolean, Index most of the time), should the behavior of constraints pass them through (return true even if they doesn't match the validation, to not block HashMap validation for example), or should they block (return false) ?

Starting this PR I was for the passing through, but by working on it right now I'm thinking about switching to a blocking behavior.

Any thoughts about this ?

[LoipesMas]

I addressed most of your comments,

I'm wondering something about validation constraints, precisely unhandled FormValue (like Nothing, Boolean, Index most of the time), should the behavior of constraints pass them through (return true even if they doesn't match the validation, to not block HashMap validation for example), or should they block (return false) ?

Starting this PR I was for the passing through, but by working on it right now I'm thinking about switching to a blocking behavior.

Any thoughts about this ?

I think it makes sense for them to be blocking, since the user decides what constraint are applied where. And being more picky could potentially prevent some unexpected behavior (e.g. applying number constraint to a toggling button doesn't make sense so it's better if it invalidates the form). Now I'm wondering if maybe it should be an error (rather than just not validating)? It fits Rust to be picky about types.

[VincentFoulon80]

I addressed most of your comments, I'm wondering something about validation constraints, precisely unhandled FormValue (like Nothing, Boolean, Index most of the time), should the behavior of constraints pass them through (return true even if they doesn't match the validation, to not block HashMap validation for example), or should they block (return false) ? Starting this PR I was for the passing through, but by working on it right now I'm thinking about switching to a blocking behavior. Any thoughts about this ?

I think it makes sense for them to be blocking, since the user decides what constraint are applied where. And being more picky could potentially prevent some unexpected behavior (e.g. applying number constraint to a toggling button doesn't make sense so it's better if it invalidates the form). Now I'm wondering if maybe it should be an error (rather than just not validating)? It fits Rust to be picky about types.

I'd love to see compile time errors due to missuse of constraints but that implies that we know what kind of output a given field can return, and also what kind of output a given constraint is able to validate. But I have no idea if that's possible.

I'm not sure about runtime validation, should they return a Result or an Option (if not just a bool) ? I'm leaning toward Options but I'd like your opinion on this

[LoipesMas]

Yeah, compile time errors are probably not feasible. But returning an Option or a Result doesn't seem like a good solution either, since then you have to decide whether you block on None/Err or pass. Or you could leave it up to the user. I was also thinking that maybe it could panic, but it feels kinda off too.

[VincentFoulon80]

Maybe with a 'support' function that returns bool if the given FormValue is supported ?

[LoipesMas]

How would this work?

[VincentFoulon80]

Something around

    fn supports(&self, output: &FormValue) -> bool {
        matches!(
            output,
            FormValue::Boolean(_) | FormValue::Index(_) | FormValue::List(_)
        )
    }

I'm not sure about this idea, it feels like bypassing something alike Options I might simply return false for now (blocking), and figure out a better way later

[LoipesMas]

Leaving it for later, when you have more experience with actually using it (and maybe feedback from users of the crate), is probably the right move right now.

[VincentFoulon80]

I think that's it, By the way, I added a IsTrue constraint. I'll add some more later as I encounter some use-cases but I think I'll stop for this release. If you got any idea for more constraints I'll take them as well

[LoipesMas]

I don't have any ideas for constraints for now

[VincentFoulon80]

Okay, then I think we're done, unless you got anything else to do before merging ?

[LoipesMas]

Nope, looks good to me!

[VincentFoulon80]

So I'm merging this, thanks a million for your patience and all your reviews! The update looks amazing compared to my first attempts thanks to you.

By any chance, would you be interested in being a maintainer on this repository ?

[LoipesMas]

Glad I could be of help! I'm not familiar with or interested in TUI development so I don't think I'd contribute much, but feel free to mention me if you have any questions or want a code review and I'll try to help when I have the time.

[VincentFoulon80]

If you ever need help / review / opinion, feel free to tag me too, I would gladly try to help as well !