Community
Participate
Working Groups
There are really two problems here: 1. ObservableSetContentProvider does a remove followed by an add of all objects in the observable set. If the content provider has a comparer (IElementComparer implementation) set then what is a different object as far as the data binding is concerned will be the same object as far as the viewer is concerned. In this situation it is very important that we do an update on the element and not a remove and add. This fixes three problems: a. It stops flicker, b. It stops the order changing when elements are equal according to the sorter, c. It preserves selection. 2. The second problem is that this is unusably slow when we get up to 10,000 elements. It takes many minutes when a refresh takes less than a second. Both these problems can be fixed by replacing the following two lines in the nested ObservableCollectionContentProvider class: <code> viewerUpdater.remove(removals.toArray()); viewerUpdater.add(additions.toArray()); </code> With something like the following: <code> /* * If we have more than 100 additions or more than 1000 removals * then we replace the input to the viewer. This should keep * selection. However if there are rows with arbitrary order then * the order may change, but speed is more important. */ if (additions.size() > 100 || removals.size() > 1000) { viewer.setInput(viewer.getInput()); } else { /* * Instead of removing all removed and adding all added elements, we need to be smarter. If there * is an object in the old and in the new set that is 'equal' according * to the viewer's comparer (although not 'equal' according to the elements' * 'equal' methods so not 'equal' as far as data binding is concerned) then * we want to do an update. This ensures both ordering and selection is preserved. * <P> * Although the TableViewerUpdater has a check to see if there is a comparator set, * it does not preserve order when there is a comparator set but the comparator returns * zero for a large proportion of the pairs of elements. * */ Set removed = new HashSet(); for (Object removedObj : removals.toArray()) { removed.add(removedObj); } for (Object addedObj : additions.toArray()) { Object match = null; for (Object removedObj : removed) { if (comparer.equals(addedObj, removedObj)) { match = removedObj; break; } } if (match != null) { removed.remove(match); viewer.update(addedObj, null); } else { viewerUpdater.add(new Object[] { addedObj }); } } viewerUpdater.remove(removed.toArray()); } </code> Note that this requires a bit of hacking to make the viewer accessible inside ObservableCollectionContentProvider. It may be a cleaner solution would be to be to add the neccessary methods to IViewerUpdater. I would be happy to provide a patch if there is agreement in principle to this change and some guidance on whether methods should be added to IViewerUpdater.
(In reply to comment #0) > There are really two problems here: > > 1. ObservableSetContentProvider does a remove followed by an add of all objects > in the observable set. If the content provider has a comparer (IElementComparer > implementation) set then what is a different object as far as the data binding > is concerned will be the same object as far as the viewer is concerned. In this > situation it is very important that we do an update on the element and not a > remove and add. This fixes three problems: a. It stops flicker, b. It stops the > order changing when elements are equal according to the sorter, c. It preserves > selection. I see what you're saying here. We already have an internal class ViewerElementSet (and ObservableViewerElementSet) to deal with the IElementComparer thing. However we are not doing anything about duplication in the input observable set. See more comments at the bottom. > 2. The second problem is that this is unusably slow when we get up to 10,000 > elements. It takes many minutes when a refresh takes less than a second. > > Both these problems can be fixed by replacing the following two lines in the > nested ObservableCollectionContentProvider class: > > <code> > viewerUpdater.remove(removals.toArray()); > viewerUpdater.add(additions.toArray()); > </code> > > With something like the following: > > <code> > /* > * If we have more than 100 additions or more than 1000 removals > * then we replace the input to the viewer. This should keep > * selection. However if there are rows with arbitrary order then > * the order may change, but speed is more important. > */ > if (additions.size() > 100 || removals.size() > 1000) { > viewer.setInput(viewer.getInput()); > } else { This sounds good in principle, but I'd like to do some profiling on a couple platforms to get a better idea where the threshold ought to be. Does the performance penalty increase with the number of already existing elements? With the number of added/removed elements? Should we increase the refresh threshold with the total number of items? > /* > * Instead of removing all removed and adding all added elements, we need to > be smarter. If there > * is an object in the old and in the new set that is 'equal' according > * to the viewer's comparer (although not 'equal' according to the elements' > * 'equal' methods so not 'equal' as far as data binding is concerned) then > * we want to do an update. This ensures both ordering and selection is > preserved. > * <P> > * Although the TableViewerUpdater has a check to see if there is a > comparator set, > * it does not preserve order when there is a comparator set but the > comparator returns > * zero for a large proportion of the pairs of elements. > * > */ > Set removed = new HashSet(); > for (Object removedObj : removals.toArray()) { > removed.add(removedObj); > } > > for (Object addedObj : additions.toArray()) { > Object match = null; > for (Object removedObj : removed) { > if (comparer.equals(addedObj, removedObj)) { > match = removedObj; > break; > } > } > > if (match != null) { > removed.remove(match); > viewer.update(addedObj, null); > } else { > viewerUpdater.add(new Object[] { addedObj }); > } > } > > viewerUpdater.remove(removed.toArray()); > } > </code> > > Note that this requires a bit of hacking to make the viewer accessible inside > ObservableCollectionContentProvider. It may be a cleaner solution would be to > be to add the neccessary methods to IViewerUpdater. > > I would be happy to provide a patch if there is agreement in principle to this > change and some guidance on whether methods should be added to IViewerUpdater. The real solution here is to make ObservableViewerElementSet (which uses an IElementComparer for element equality). In the short term: if the viewer has an element comparer, we could filter out the elements found in both additions and removals: Set removals = event.diff.getRemovals(); Set additions = event.diff.getAdditions(); if (viewer instanceof StructuredViewer && ((StructuredViewer) viewer).getComparer() != null) { IElementComparer comparer = ((StructuredViewer) viewer) .getComparer(); Set filteredRemovals = ViewerElementSet.withComparer(comparer); filteredRemovals.addAll(removals); filteredRemovals.removeAll(additions); Set filteredAdditions = ViewerElementSet.withComparer(comparer); filteredAdditions.addAll(additions); filteredAdditions.removeAll(removals); removals = filteredRemovals; additions = filteredAdditions; } However this has the potential to incur a performance penalty as we are duplicating work. Also we are making the assumption that these are always replaced elements (removed then added) as opposed to added then removed. There's really no way to tell from a set diff. Aside: The IViewerUpdater interface is still experimental and not official API, so we are still free to add new methods if that would help. I'm curious though, if an element is removed and re-added, does it make more sense to call viewer.update(element) on that element, or just do nothing?
> The real solution here is to make ObservableViewerElementSet (which uses an IElementComparer for element equality). I did not complete my sentence: to make ObservableViewerElementSet _public API_.
This bug hasn't had any activity in quite some time. Maybe the problem got resolved, was a duplicate of something else, or became less pressing for some reason - or maybe it's still relevant but just hasn't been looked at yet. If you have further information on the current state of the bug, please add it. The information can be, for example, that the problem still occurs, that you still want the feature, that more information is needed, or that the bug is (for whatever reason) no longer relevant.
This bug hasn't had any activity in quite some time. Maybe the problem got resolved, was a duplicate of something else, or became less pressing for some reason - or maybe it's still relevant but just hasn't been looked at yet. As such, we're closing this bug. If you have further information on the current state of the bug, please add it and reopen this bug. The information can be, for example, that the problem still occurs, that you still want the feature, that more information is needed, or that the bug is (for whatever reason) no longer relevant. -- The automated Eclipse Genie.
