I know I am not the first person to suffer the problem of lossy Converters causing the Binder to [falsely] update the domain model value during binding. As soon as we call BindingBuilder.bind(), a lossy converter will trigger a setValue on the domain property. We only get to call setConvertBackToPresentation(false) when it is too late.
Binding<BEAN, Double> binding = bindingBuilder.bind(getter, setter); //argh! This calls setter if converter is lossy
binding.setConvertBackToPresentation(false);// Too late
Afaik, the only solution is to defer calling binder.setBean() until after binding.
A Builder is supposed to allow us to set critical options. Why not:
bindingBuilder = bindingBuilder.setConvertBackToPresentation(false);
Binding<BEAN, Double> binding = bindingBuilder.bind(getter, setter); //won't happen
//binding.setConvertBackToPresentation(false);// no need
Or make convertBackToPresentation false by default. Personally, I can’t think of a reason why I’d ever want it to be true, but YMMV.
Lossy converters are common in my application. Round-off and precision is everywhere for integers, doubles and timestamps. Who wants to see a java Instant with nanosecond precision?
Apologies if this has been discussed ad infinitum and I missed it.
That’s the way Binder has been designed: configure it first and then give it data.
Is there some technical reason for why you need to configure the binder only after the data has already been set?
The main point of the fluent binding builder is for the steps where order matters and maybe even impacts subsequent types. The main example of this is the difference between adding a validator before or after a converter. Anything that can practicaly be outside the builder and that someone might in some cases want to toggle on-the-fly after creating the binding is in the binding rather than in the builder.
The big one is that you find out the hard way rather than reading it clearly in the documentation.
And it is harder to follow code when important lines are far apart:
public MyForm(MyData bean){
Binder<MyData> binder = new Binder<>(bean.getClass());
// lots and lots of binding definitions all in the same constructor method because I use final fields and cannot delegate to an init() method.
binder.setBean(bean);
}
But I do get it. If I was the one responsible for the Binder internal code, I’d also push for keeping it simple by disallowing unnecessary flexibility.
I am far more in the camp that there is no good reason for convertBackToPresentation ever to be true. Make it false by default and let people set it to true in those rare cases. To me, that is the best solution of all. See:
I agree, in hindsight. The challenge is that changing the default at this point comes with a risk of subtle changes to existing application applications when upgrading to a version where the default is changed. We try to be particularly mindful when it comes to potentially breaking changes that don’t even show up as compilation error.
Binder.setBean calls getBindings().forEach(b -> b.initFieldValue(bean, true));
That true is the writeBackChangedValues parameter, which calls setter.accept if value is “changed”
I ended up having to override Binder.setBean, so that I could pass in false
That was a pain, since most of the methods it calls are private.
readBean calls binding.initFieldValue(bean, false); but it has more changes as well, so I’ve never cheked if it could be a better choice.
Imho, what we need isn’t Binding. setConvertBackToPresentation(false), it is Binder. setConvertBackToPresentation(false); Displaying values should never change them.
package com.vaadin.flow.data.binder;
import java.util.Collection;
import java.util.stream.Collectors;
import com.vaadin.flow.data.binder.Binder.Binding;
import com.vaadin.flow.data.binder.Binder.BindingImpl;
/**
* Even when all the Fields are set to setConvertBackToPresentation(false), some will still try and update the bean.
* So make the bean readonly during the setter.
* However, setting the bean back to readWrite will update ALL the bindings,
* including the ones we want to keep as readonly.
* So we have to hold onto a list for undo.
*
* Need a package level definition because BinderImpl.bindings is a protected field.
*/
public class BinderWhileReadOnly {
public static void doWhileReadOnly(Binder<?> binder, Runnable run) {
Collection<Binding<?,?>> writeBindings = BinderWhileReadOnly.getWritableBindings(binder);
setReadOnly(writeBindings, true);
try {
run.run();
} finally {
setReadOnly(writeBindings, false);
}
}
private static Collection<Binding<?,?>> getWritableBindings(Binder<?> binder) {
return binder.getBindings().stream()
.filter(BinderWhileReadOnly::isReadWrite)
.collect(Collectors.toList());
}
static boolean isReadWrite(BindingImpl<?, ?, ?> binding) {
return binding.getSetter() != null && !binding.isReadOnly();
}
private static void setReadOnly(Collection<Binding<?,?>> readWriteBindings, boolean readOnly) {
for (Binding<?,?> binding : readWriteBindings) {
binding.setReadOnly(readOnly);
}
}
}
We can make Binder do many things as long as there’s a clear understanding of exactly what it should do under all circumstances and in combination with all existing features.
The main limitation is that any new behavior has to be opt-in rather than enabled by default if there’s a risk that existing application code might break.
The other alternative is really to design a new form binding API with different defaults and then keep supporting Binder with its current defaults.
That is why I’m saying “Displaying a value should never change it”; A high-level statement that it should be easier to reason about, and possibly disprove.
I totally agree that you should avoid introducing breaking changes. I also see why you need to be careful what you introduce of new features and variants, since you’ll have to support it “forever”.
