Overcome a limitation of binding data variables to checkboxes

[ZombieRaccoon]

When binding data variables to checkboxes I encountered that the presence of a non-empty value attribute is required in order for them to properly function. I consider this a limitation, so I devised a solution to overcome it. I believe that the resulting changes make the underlying philosophy more streamlined.

In my opinion, channeling the changes of the checked attribute of checkboxes into imaginary changes in value and then blending them together with radio buttons confuse things quite a bit, hence my attempt at cleaning it up.

Any comments or thoughts are much appreciated!

[mikke89]

Thanks for all your contributions, happy to have them!

I'll need to take a closer look at this one. This week is quite busy so I'll get back to you after that.

[mikke89]

Hi, it took me a while, but I've finally taken a closer look at this.

I think the initial issue you report can simply be resolved by returning eg. the value "on" for checkboxes that have no specified value, this is also what web browsers seem to do.

Overall, I agree that the Checked controller is a bit hacky, and I very much appreciate the effort you put into this, but I don't think this is the right approach:

• Removing the event value is a big breaking change, additionally, I suspect it might mess up form submissions. The checkboxes in the invader sample options don't seem to save correctly anymore.
• I'm not sure if these changes work with radio buttons? I'm getting warnings in the data binding example at least.
• Changing how the default actions work (in the event dispatcher) seems a bit arbitrary for such a small change. This has a performance impact and would need more justification and a lot of testing.
• There are some stylistic changes that don't really fit the library: mainly extracting "strings" into global variables, and the name change from DataController... to ...DataController.

What do you say we instead try out my suggested fix above? If we still want to change the checked controller we can discuss that, but it needs to happen in a non-breaking way and I need confidence that it works for both checkboxes and radio buttons.

[ZombieRaccoon]

I think the initial issue you report can simply be resolved by returning eg. the value "on" for checkboxes that have no specified value, this is also what web browsers seem to do.

Overall, I agree that the Checked controller is a bit hacky, and I very much appreciate the effort you put into this, but I don't think this is the right approach:

It's my sense of order that compelled me to come up with a solution that I considered cleaner from a certain point of view. With that being said, I could totally image that this change would also imply that other parts of the code would require added complexity so as not to introduce regressions.

• Changing how the default actions work (in the event dispatcher) seems a bit arbitrary for such a small change. This has a performance impact and would need more justification and a lot of testing.

If it's this part of the patch that you're referring to, then I intended it to be a form of simplification, which I now see that it isn't - I seem to have missed the dom_distance_from_target part of the logical expression for some reason.

What do you say we instead try out my suggested fix above? If we still want to change the checked controller we can discuss that, but it needs to happen in a non-breaking way and I need confidence that it works for both checkboxes and radio buttons.

My take on this is that I'd be perfectly happy with any solution that you considered acceptable. Other than that, I'd also be absolutely willing to make improvements to this patch if you saw merit in its fundamentals.

Since it's completely understandable that you'd like to avoid breaking changes, do you think that perhaps we could always emit checked and keep value untouched?

[mikke89]

I'm all for seeing some improved code, so please go ahead, I'm just happy you are motivated for it! :)

Sounds good to me to add checked parameter and keeping value as is. When you're changing DataControllerChecked there might be related changes to be made in DataViewChecked as well. Regardless, I also think my earlier suggestion should be included, as this might help in other cases as well.

Let's try to make sure any new code doesn't break anything, and also works well with radio buttons. It would also be helpful if unrelated cleanups are kept in a separate commit, or even separated out to another PR. This way it is easier to following along the featured changes, thanks!

[ZombieRaccoon]

I went ahead and incorporated everything that has come up so far. Is there an easy way to build some or all of the sample applications, or is it manual labor all the way like the way it seems?

EDIT: I've come across the BUILD_SAMPLES CMake option, which should do the trick. I'll test the changes using some of these.

[ZombieRaccoon]

I've tested the changes using the databinding sample application and came up with something that is both generic and works for radio buttons. I personally believe that the should_affect_data_binding event parameter I introduced makes things explicit, and thus clearer.

Looking forward to hearing your feedback!

[mikke89]

Thanks, now we are getting close! This is already an improvement. And I don't see any reason this would break anything, which is good!

I have some feedback below, let me know what you think.

[ZombieRaccoon]

I tested the latest changes using the databinding sample and things still work as expected.

[mikke89]

Thanks, all of this looks good to me!

It's always nice when we can delete more lines than we add :)

[ZombieRaccoon]

I'm glad you like it! It's been a pleasure. Would you please consider making a release out of the bug fixes that have accumulated since the latest release?

[mikke89]

Sure, I'll do it soon!