Community
Participate
Working Groups
Build Identifier: 20090920-1017 When binding a list-typed property of a JavaBean model to multi-selection in a ListViewer and using conversion through the UpdateListStrategy, unconverted objects can enter the model. Also model-to-view conversion fails with java.lang.IndexOutOfBoundsException. I can actually get these errors without using a Converter, see my snippets in this forum post http://www.eclipse.org/forums/index.php?t=rview&goto=629646#msg_629646If; I tried a Converter approach to fix this without success and found this issue. As I see it now, binding a model list to multi-selection in the view is simply broken unless both your model and view list elements are of the same type AND you override the equals method. A working example can be seen in the mentioned forum post. However overriding equals gives other issues as explained here https://bugs.eclipse.org/bugs/show_bug.cgi?id=307585. Reproducible: Always Steps to Reproduce: 1. Compile and run the attached SnippetModelToViewMultiSelectionWithConversion 2. Click around the list. You will see in the debug output from "...(wrapped)" that a JobWrapped object which may exist only in the UI and should be converted to a Job object has entered the model. 3. Click the "Reload Person from Dao" button twice. You will see an java.lang.IndexOutOfBoundsException when the ListBinding tries to update the UI.
Created attachment 179819 [details] Snippet which shows Bug 326507
Created attachment 179827 [details] A workaround for the problem which also uses the wrapped object in the model I can work around the problem by having my model do the conversion: I create a property of type List that contains the converted objects (which should not be persisted to the database!) and is synchronized with the real objects in the model. Quite an ugly solution but it works. I have another idea for a nicer workaround: create an IObservableValue implementation which takes an IObservableList (the multi selection in the viewer) and converts it to a value (the entire list). Then use a normal ValueBinding.
Created attachment 179839 [details] A workaround for the problem which uses a Value Binding and IElementComparer Created a much better workaround to my original problem (with having to override equals) by converting the IObservableList of multi-selection to an IObservableValue and subsequently using a Value Binding to the model property. No converter is necessary by using the usual IElementComparer on the Viewer. No change is required to the model and no converters are required.
This problem is also reproducible with Eclipse 3.6.2 (org.eclipse.core.databinding_1.3.100). The convert method is called (in ListBinding, line 173) obviously only for add, but not for replace. As a workaround we subclassed UpdateListStrategy to override useMoveAndReplace() (return false, if an converter is set, and true else). Snippet: @Override protected boolean useMoveAndReplace() { return this.converter == null; }
*** Bug 422023 has been marked as a duplicate of this bug. ***
This bug is still reproducible in 4.3.
In ListBinding, I think you just need to change this (line 201): public void handleReplace(int index, Object oldElement, Object newElement) { if (useMoveAndReplace) { IStatus setterStatus = updateListStrategy.doReplace(destination, index, newElement); mergeStatus(multiStatus, setterStatus); } else { super.handleReplace(index, oldElement, newElement); } } To this: public void handleReplace(int index, Object oldElement, Object newElement) { if (useMoveAndReplace) { IStatus setterStatus = updateListStrategy.doReplace(destination, index, updateListStrategy.convert(newElement)); mergeStatus(multiStatus, setterStatus); } else { super.handleReplace(index, oldElement, newElement); } }
New Gerrit change created: https://git.eclipse.org/r/60963
Stefan, can you review the patch?
Mass move to 4.7 as M7 is approaching. Please move back in case you are planning to fix it for Neon.
I provided the patch 5 months ago. So yes, I want to have in M7!
Mass move to 4.6 RC1. We might push out more to 4.7.
Very sorry it took me so long to get to this code review. My review queue has been severely backlogged. See my comments in the patch.
New Gerrit change created: https://git.eclipse.org/r/119631
I pushed a suggested fix: https://git.eclipse.org/r/119631 It aims be a minimal change which preserves backwards compatibility: Add a flag to UpdateListStrategy which enables the fix.
New Gerrit change created: https://git.eclipse.org/r/119713
I'd like to discuss different alternative approaches to a solution for this problem. The following are the good alternatives that I can think of. They all works by transferring information from the caller of DataBindingContext#bindList to ListBinding#doUpdate, where the correct new behaviour or the buggy old behaviour can be chosen. 1. A subclass of UpdateListStrategy. ListBinding#doUpdate would check the type of its UpdateListStrategy and apply conversion if it has the new type. This fix is implemented here: https://git.eclipse.org/r/119713 2. An overload of DataBindingContext#bindList with a new flag parameter which signals that the conversion fix should be used. 3. An argument to the UpdateListStrategy constructor(s). This fix is implemented here: https://git.eclipse.org/r/119631/ ------------------ Having though about this some more I think 1. is the best solution. The reason is that new code would not have to something "extra", such as using an extra parameter, to get the new correct conversion behaviour. The constructors of the original UpdateListStrategy can be marked as deprecated to alert clients of the bug. I have implemented this fix in the following change: https://git.eclipse.org/r/119713 One tricky part is to come up with a reasonable name for the new UpdateListStrategy subclass. I have called UpdateListStrategy2 in the implementation. That is not very good.
Jens, I'm still a bit uncertain about providing backwards compatibility for broken functionality. What do you think about the following: 1.) We fix UpdateListStrategy to always use the custom converter 2.) Optional: For safety reasons we could provide a system flag or a new method in UpdateListStrategy usePre49Convertor or something like this, which allows to restore the old behavior. WDYT?
Lars Vogel wrote: > Jens, I'm still a bit uncertain about providing > backwards compatibility for broken functionality. > What do you think about the following: > ... First I want to say that I have no special competence or experience of this. I have never personally been hit by this bug, nor do I know of any code that would be broken by a fix. I agree fully with Lars. To me it seems excessive to avoid fixing bugs to preserve backwards compatibility. Especially when it implies that the API will be polluted by a backwards compatible workaround, as in this case. ## About workarounds I also think that people who have developed workarounds should be aware of the fact that the problem will be fixed eventually, and make sure their workarounds continue to work when that happens. The workarounds that are posted here would not be affected by a fix, since they don't add converters to the bindings at all. ## About system property switch I cannot really judge whether a system property to switch off the fix should be added. On the one hand, it is easy to implement, and it could be useful to someone who have a large old application which starts to behave strangely. On the other hand, maybe it adds work and complexity in other ways. I guess it should be documented somewhere for example. If you think it is warranted with such a mechanism then I can add the code. ## Release notes There should probably be a sentence about this fix in the release notes. ## Rebase I rebased Conrads Gerrit change against the latest master. (That means the commit now is under my name, Condrad wouldn't mind that, would he?)
Jens, Just to be sure: https://git.eclipse.org/r/#/c/60963/ is the updated one, correct?
> Just to be sure: https://git.eclipse.org/r/#/c/60963/ is the updated one, correct? Yes, that's correct.
Gerrit change https://git.eclipse.org/r/60963 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=ed9c7a3703fc0c1480fb43589cd96f5bc51a8c21
Thanks Jens and Conrad. Please abandon the alternative approaches.
