[New] IField interface

[mever]

As requested for issue #9366.

[mever]

I found that some field implementations already conform to this interface so I decided to change the name from inputValue to value instead. The setValue returns an error for incompatible types because an exception is too severe and crashes the application when uncaught. In next commits I will implement the interface for textfield and virtual selectbox. After that I will modify the resetter and validator to support the new interface and add/modify integration tests to prove it.

[mever]

So what's in this latest commit. First I changed the form.Resetter class to use the new interface (reducing it by 93 lines). Run the tests, implemented the interface in various fields and repeated this procedure until all tests passed in the ui package. I also added an extra test case for the virtual selectbox.

[mever]

Is it possible to run the Travis tests again? The last one failing is probably a mishap.

[cajus]

Done

[mever]

Okay, so the reason why this is failing is because Travis gets a merge from this branch and master: https://github.com/qooxdoo/qooxdoo/commit/4cdbb78366421d941b5a261b97e4ba50c2dc95e2. The master branch does not pass [firefox, latest] because it contains a problem. See: https://travis-ci.org/qooxdoo/qooxdoo/builds/262999042

[mever]

@johnspackman good point. Can sub-interfaces refine methods (i.e. more specific)? Some languages do not allow it. Let's try it anyway though.

[level420]

@mever regarding the failing travis runs: this should be fixed now with https://github.com/qooxdoo/qooxdoo/pull/9368 resp. 3d6e15f20bde1c4f1940af07b80cbc6899e2c508

[mever]

@derrell Seems like a sensible idea. To be clear. The form resetter is not fully BC for fields that do not implement the interface. This change is therefore not totally BC as it changes the interface of the resetter. I have changed the fields in this project to conform to this interface. For fields defined outside of this project, implications are unsure.

[derrell]

@mever Are you able to provide instructions for converting existing derived classes to the new interface? With 6.0, we don't need to be strictly BC, but we must at least either provide the migration script for any non-BC changes, or give clear instructions for people upgrading, on how to change their classes to conform. I think this PR is worthwhile and look forward to approving it; I just want to make sure we don't impose (too much) pain on our users when the release is done.

Thanks!

[level420]

@mever do you have some hints I could start searching for incompatibilities?

[mever]

On backwards compatibility:

First of, classes inhering the standard fields do not need to change as they should already be liskov substitutable. This also applies to classes directly implementing the standard interfaces: IBooleanForm, IColorForm, IDateForm, INumberForm, IArrayForm, IModelForm and IStringForm. The IField interface is actually the same interface as listed but with weaker parameter and return types.

The only real BC breaker here is the resetter. Because fields that are using a selection are abstracted away (see the three removed private methods in the resetter class). The qx.test.ui.form.FormManager#testAll test helped me a lot here in tracking down which fields didn't conform.

A way of finding potential problems is to look at implementations of qx.data.controller.ISelection and qx.ui.core.ISingleSelection. These implementations should also conform to this new interface. In order to pass the tests earlier mentioned I had to change the MSingleSelectionHandling and MMultiSelectionHandling mixins. Fields using those now automatically implement the interface when included. Fields implementing both selection interfaces but not the mixin are not compatible as they should provide their own implementation.

[level420]

@mever there are, of course, let's name it implicit or potential BC breakers, if a class signature changes by e.g. adding new public or protected members or properties to an existing class, as this may collide with already existing code deriving from the unchanged class.

As I wrote for the qx.ui.form.RadioButtonGroup this situation may get more manageable if added functionality is override-able.

[mever]

@level420 I agreed.

This new commit should allow you to overwrite the _onChangeSelection method.

Is it possible to notify the user when collisions happen in classes inhering one of: qx.ui.form.RadioButtonGroup or qx.ui.form.VirtualSelectBox?

[level420]

@mever regarding detection of collisions: not that I'm aware of. Without knowing the history of the class both adding new methods to the derived class or, after the change in the superclass, overriding the methods are valid scenarios.

There is just one point where I had been able to immediately detect the collision: this is if you had chosen to add a property value to qx.ui.form.RadioButtonGroup, as I've also added a value property to the derived class. In this situation the generator would have reported an error.

[level420]

I'm still a bit unhappy with the situation, as what I now have to do (after this PR) is to change the value type of my class derived from qx.ui.form.RadioButtonGroup from qx.ui.form.IRadioItem to String .

This implies a change of the return value of the setValue method, which should not be.

Additionally I'm not sure what happens if a form containing qx.ui.form.RadioButtonGroup now runs through the cycle of creating a qx.data.controller.form, creating a model from this controller and act with qx.data.marshal.Json on it. Does it at some point try to serialize a qx.ui.form.RadioButton because it is now the value of the radio button group?

[mever]

@level420 I agree. It is bad because it breaks liskov substitution. This next commit a0725b7e should fix that.

I have to admit that my focus was more on my own domain. Before releasing this I want to do a few more cycles and make sure BC is intact. I will also look into the form controller.

[level420]

@mever the latest push https://github.com/qooxdoo/qooxdoo/pull/9367/commits/ebeb9faa1bf9aa26f5ff72a40b904737bebf8eb9 to this PR seems unrelated

[level420]

Additionally the improved resetter error handling in 06ff20b leads a few tests to fail. See https://travis-ci.org/qooxdoo/qooxdoo/jobs/265599013

[mever]

@level420 yeah my IDE was complaining about a missing argument (capture) in removeListener in one of the commits (this one: be8a1a07). Fixed it here. Should I revert and place it in a different PR?

[level420]

@mever yes please let this PR be what it is named.

[mever]

@level420 can I perform a force push without the commit?

[level420]

@mever I think the best would be to revert https://github.com/qooxdoo/qooxdoo/commit/ebeb9faa1bf9aa26f5ff72a40b904737bebf8eb9

[level420]

re-requesting approvals.

[level420]

@mever could you please review https://github.com/qooxdoo/qooxdoo/pull/9367#issuecomment-322843623 . Thank you!

[mever]

Sorry for the delay, I had a lot on my plate. @derrell it is good you are picky, not accepting just anything makes a great framework. This commit is squashed as requested by @oetiker. The previous commits are on the https://github.com/mever/qooxdoo/commits/form.bak branch.

In 60af803 and 140d9fc I've systematically checked {qx.data.controller.ISelection} and {qx.ui.core.ISingleSelection}, respectively. In order to verify all previous widgets that could be used in combination with the resetter. I've also checked the {qx.data.controller.Form} as suggested by @level420 .

Migration instructions are: The biggest change is on the {qx.ui.form.Resetter} component. All form widgets that worked previously from the standard library should still work. Check if your class does not conflict with the {qx.ui.form.IField} interface when inheriting from one of these widgets in your application. This interface essentially specifies a value property with a weak type. When you specify custom form widgets which use the resetter (e.g.: indirectly through the form), then implementing {qx.ui.core.ISingleSelection} or {qx.data.controller.ISelection} is not enough. You need to implement the {qx.ui.form.IField} interface. When your widget(s) includes {qx.ui.core.MSingleSelectionHandling} or {qx.ui.core.MMultiSelectionHandling} it already implements the interface. Just make sure your class also specifies the interface in its definition.

These objects implement the new {qx.ui.form.IField} interface:

Widgets: {qx.ui.container.Stack} and {qx.ui.tabview.TabView}. Form fields: {qx.ui.tree.Tree}, {qx.ui.form.List}, {qx.ui.form.RadioButtonGroup}, {qx.ui.form.RadioGroup}, {qx.ui.form.SelectBox} and {qx.ui.form.VirtualSelectBox}. Any classes inheriting these widgets or form fields should work without change when not conflicting with the {qx.ui.form.IField} interface.

Mixins: {qx.data.controller.MSelection}, {qx.ui.core.MSingleSelectionHandling}, {qx.ui.core.MMultiSelectionHandling} and {qx.ui.virtual.selection.MModel}. Any classes including these mixins should work when not conflicting with the {qx.ui.form.IField} interface. When you need the implementing classes to work with the {qx.ui.form.Resetter} component they should explicitly implement the {qx.ui.form.IField} interface.

All the form field interfaces in {qx.ui.form.*}: {IArrayForm}, {IBooleanForm}, {IColorForm}, {IModelForm}, {INumberForm} and {IStringForm}. Any class explicitly (indirectly) implementing these interfaces should work without change.

[level420]

4 approvals, more than 2 business days since voting. merging. thank you @mever