Community
Participate
Working Groups
The TargetToModel convert method is called, even though POLICY_ON_REQUEST is set. (Validators would also be called. The model anyhow remained unchanged.) In contrast the convert method of the ModelToTarget converter is not called, although it was also set to POLICY_ON_REQUEST. I am using Eclipse 3.5M6 The dialog code (the used model just has one String attribute "id"): public class DatabindingPolicyOnRequestTestDialog extends TitleAreaDialog { public DatabindingPolicyOnRequestTestDialog(Shell parentShell) { super(parentShell); } @Override protected Control createContents(Composite parent) { Control contents = super.createContents(parent); setTitle("Test Policy On Request"); setMessage("Test Policy On Request", IMessageProvider.NONE); return contents; } @Override protected Control createDialogArea(Composite parent) { Composite comp = new Composite(parent, SWT.NONE); comp.setLayoutData(new GridData(SWT.FILL, SWT.FILL, true, true)); GridLayout gl = new GridLayout(1, false); comp.setLayout(gl); DataBindingContext bindingContext = new DataBindingContext(); Text text = new Text(comp, SWT.BORDER); text.setLayoutData(new GridData(SWT.FILL, SWT.TOP, true, false)); IdModel model = new IdModel(); model.setId("an id"); bindingContext.bindValue(SWTObservables.observeText(text, SWT.Modify), BeansObservables.observeValue(model, "id"), new UpdateValueStrategy(UpdateValueStrategy.POLICY_ON_REQUEST).setConverter(new TargetToModelConverter()), new UpdateValueStrategy(UpdateValueStrategy.POLICY_ON_REQUEST).setConverter(new ModelToTargetConverter())); System.out.println(model.getId()); bindingContext.updateModels(); System.out.println(model.getId()); return comp; } private class ModelToTargetConverter extends Converter { public ModelToTargetConverter() { super(String.class, String.class); } @Override public Object convert(Object fromObject) { System.out.println("here1"); return "another id"; } } private class TargetToModelConverter extends Converter { public TargetToModelConverter() { super(String.class, String.class); } @Override public Object convert(Object fromObject) { System.out.println("here2"); return "just id"; } } }
Pasted code is missing source for IdModel class.
Really simple mode: public class IdModel { private String id; public void setId(String id) { this.id = id; } public String getId() { return id; } }
Created attachment 130856 [details] Full source version of snippet
Created attachment 130857 [details] Full Plugin Project Example Added a full plug-in project.
Created attachment 130861 [details] Fix Kai, can you verify that this fixes the problem?
Created attachment 130862 [details] mylyn/context/zip
Yes, it works correctly now (I also tested it with validators). Thanks Matthew.
There seems to be some new problems now. POLICY_ON_REQUEST works as expected, but POLICY_CONVERT is now the problem (please use new attached snippet). Before the fix it also had problems. When executing the new snippet, only the targetToModelValidator is called. But it is expected that both validators should be initially called. After the fix none of those validators are called, but both should be called when using POLICY_CONVERT.
Created attachment 130925 [details] DatabindingPolicyConvertTestDialog
Created attachment 130975 [details] DatabindingPolicyTestDialog I created another snippet that makes things a bit clearer (it only comprises ValueBinding). Below you see a test output when using the unpatched databinding plug-in and using the patched one. Everything works fine except the initial validation/conversion. Here is what I expect if it would work correctly: Testing POLICY_NEVER = 1 Testing POLICY_ON_REQUEST = 2 Testing POLICY_CONVERT = 4 Testing POLICY_UPDATE = 8 Initial modelToTargetConverter 4 modelToTargetValidator 4 targetToModelConverter 4 targetToModelValidator 4 modelToTargetConverter 8 modelToTargetValidator 8 targetToModelConverter 8 targetToModelValidator 8 .... (rest is ok) And here the output of the snippet: Patched Databinding Plug-In: Testing POLICY_NEVER = 1 Testing POLICY_ON_REQUEST = 2 Testing POLICY_CONVERT = 4 Testing POLICY_UPDATE = 8 Initial modelToTargetConverter 8 modelToTargetValidator 8 targetToModelConverter 8 targetToModelValidator 8 Update Model targetToModelConverter 2 targetToModelValidator 2 targetToModelConverter 4 targetToModelValidator 4 targetToModelConverter 8 targetToModelValidator 8 Update Target modelToTargetConverter 2 modelToTargetValidator 2 modelToTargetConverter 4 modelToTargetValidator 4 modelToTargetConverter 8 modelToTargetValidator 8 Unpatched Databindig Plug-In: Testing POLICY_NEVER = 1 Testing POLICY_ON_REQUEST = 2 Testing POLICY_CONVERT = 4 Testing POLICY_UPDATE = 8 Initial targetToModelConverter 2 targetToModelValidator 2 targetToModelConverter 4 targetToModelValidator 4 modelToTargetConverter 8 modelToTargetValidator 8 targetToModelConverter 8 targetToModelValidator 8 Update Model targetToModelConverter 2 targetToModelValidator 2 targetToModelConverter 4 targetToModelValidator 4 targetToModelConverter 8 targetToModelValidator 8 Update Target modelToTargetConverter 2 modelToTargetValidator 2 modelToTargetConverter 4 modelToTargetValidator 4 modelToTargetConverter 8 modelToTargetValidator 8 I am working on a little patch right now.
Created attachment 130978 [details] Fix Fix. This is a patch against CVS HEAD. Matthew, does it work now as expected?
Is it valid to validate target to model if the target has not been populated from the model?
Thanks for the hint, Matthew. I wondered a bit why you implemented it that way. I always expected that model and target are treated equally at initialization. This is a design decision. I am not sure which solution is better, but now that I know the reason, I tend toward your fix (a hint in the javadocs would be nice).
Actually I think your solution is more appropriate, with one small change: ValueBinding.postInit() should not push a round-trip on initialization, as this may break existing clients. For example, existing code can bind an unmodifiable model observable (e.g. ComputedValue) to a modifiable target observable (e.g. SWTObservables.observeEnabled) without having to worry about setting the update policy. However if we add the round-trip then existing code will have an error raised on the return trip.
Hm, ok. So what do you suggest? Do you have some code? Why doesn't that problem occur when changing the target (as POLICY_UPDATE is set implicitly when not providing an UpdateStrategy) or calling bindingContext.updateModel()?
Ok, I tested my fix with your scenario (ComputedValue as model and Text as target). postInit() worked with that "round-trip" without any Exception being thrown. Are you sure that there is a problem? Or maybe I just got your pointed out problem wrong.
The problem doesn't occur because most of the time it is a case of *not* setting the update policy appropriately because the target observable will never be modified except by the binding, e.g. ComputedValue enableOther = new ComputedValue(Boolean.TYPE) { protected Object calculate() { // compute value } }; dbc.bindValue(SWTObservables.observeEnabled(otherField), enableOther); vs. dbc.bindValue(SWTObservables.observeEnabled(otherField), enableOther, null, new UpdateValueStrategy(UpdateValueStrategy.POLICY_NEVER)); Since nobody is going to modify the observeEnabled observable or directly modify the enablement of otherField we can save ourselves some typing. Adding a round-trip in initialization would break this code. (In reply to comment #16) > Ok, I tested my fix with your scenario (ComputedValue as model and Text as > target). postInit() worked with that "round-trip" without any Exception being > thrown. > Are you sure that there is a problem? Or maybe I just got your pointed out > problem wrong. An error is definitely being raised (since ComputedValue.#doSetValue() throw an UnsupportedOperationException) but the exception is caught and embedded in the validation status. However you are not seeing the validation status since WizardPageSupport initially hides validation errors until it detects a change on one of the target observables. Try moving the code that creates the ComputedValue binding into a button selection listener and click the button and you'll see what I mean.
Created attachment 131068 [details] Update on postInit(), validate from target to model rather than update
Created attachment 131069 [details] mylyn/context/zip
Created attachment 131070 [details] Fix Ok, I see ... thanks. I created another fix (against HEAD). What do you think?
I looked at your Update. It would be nice to validate also when targetToModel has POLICY_UPDATE strategy (see my fix).
Created attachment 131071 [details] Demo round-trip problem I've added a check button, text field, and a push button. Clicking the push button creates a binding from the check button's selection to the text field's enablement. I've wrapped the check button selection observable in a ComputedValue to make it unmodifiable. If ValueBinding.postInit calls updateTargetToModel() instead of validateTargetToModel(), then clicking the button will display an error message as the binding is initialized.
I plan to commit this patch once I have some tests to got with it.
Mathew, thanks for that descriptive example snippet. So do you also validate targetToModel when targetToModel update strategy is POLICY_UPDATE, like in my new fix (it is not present in your Update)? I have to know, cause it has impact of how I design my dialogs. I use custom TitleAreaDialog where the data binding has influence on the enable state of the Ok button, like in a Wizard Page. With the above solution, this is a bit easier to handle as the fields are initially validated when POLICY_UPDATE is set on targetToModel.
Target-to-model is validated for POLICY_CONVERT or POLICY_UPDATE. The code is testing for getUpdatePolicy() >= POLICY_CONVERT which is satisfied by both of those constants. However given the confusion this caused, we could probably make the code more self-explanatory by testing for equality on both constants.
Created attachment 131438 [details] Fix with tests I don't think these tests completely cover this bug but I'm out of time to work on this. I will commit this fix for now, if you can provide more unit tests along these lines I will gladly accept them.
Created attachment 131439 [details] mylyn/context/zip
Released to HEAD > 20090409
Thanks Matthew. I will take a closer look at the tests in the upcoming days.
