Pass the button when submitting the form in the AbstractFormDialog

[Jezza]

When the form is submitted, we have no idea what button was used. Currently, I'm using a bit of a hack with #getForm(DialogButton) and a transient field. Ideally, it would be a bit better if it was passed in during the submit.

I could, I guess, just use the onClick, but that means I'm now doing error checking in the click method, rather than the submit method, which is a bit off considering that's mostly the point of submit.

Edit: I'd be fine with submitting a PR for it, if you have no qualms with this addition.

[Jezza]

I've moved my code into the onClick, but I still think the onSubmit could do with the button that caused the submit.

[sebfz1]

Hi, yes moving to #onClick was correct way. Actually I could transmit the button to the DialogFormSubmitter to pass it to onSubmit (and to #onError). But does it really worth an API break? What could be the unarguably advantage?

[Jezza]

The main advantage would be error checking in the submit, rather than doing the error checking in the click. With the button in the submit, I can just report an error on a component, form, etc, and the #onClick won't be called, because the form errored out. With the system right now, it basically means I have to do all of that inside of the #onClick itself. Process the necessary elements, check if errored out, and then return before we actually start handling the click functionality. It gets a bit worse when you consider, #onClick is a lot more likely to be overridden, meaning we need to get people to call the super, or go the annoying route, and start chaining together a bunch of "onOnClick", or something equally bizzarre. People aren't going to override #onSubmit for many things, and generally not in the dialog class itself, they'll mostly care about the button itself. Which is why, I'd imagine, you have the #onClick method in the first place.

Regarding the API break, Wicket 8 hasn't been "released" yet, and it already breaks some stuff. So, I'd say the jump between 7 -> 8 is fine, and anyone using 8, is only using a milestone, which means they already acknowledge that things can change.

[sebfz1]

I see your point. I'll make the update! :+1: