Community
Participate
Working Groups
MA (21.08.2001 19:00:55) I was setting the input of a tableviewer two times with the same input. The first time, mapElement was called for each element, the second time for none of them. 'setInput()' always unmaps all elements. So after the second setInput the map is empty. I think this is the problematic code: (StructuredViewer) protected void associate(Object element, Item item) { Object data = item.getData(); if (data == element) return; << returns without mapElement if (data != null) disassociate(item); item.setData(element); mapElement(element, item); } NOTES: MA (28.08.2001 20:32:18) Other experiment: 1. Open the package viewer, create java project proj, package 'pack' and CU 'A.java' 2. set breakpoints on mapElement and unmapElement 3. create a cu pack.B.java 4. Even A.java stays the same, is is mappes and unmapped several times. 5. set breakpoints on assiciate and disassociate 6. A.java is disassociated and assiciated again (several times)
PRODUCT VERSION: 129
another problem is in disassociate protected void disassociate(Item item) { Object element = item.getData(); Assert.isNotNull(element); item.setData(null); if (elementMap != null && findItem(element) == item) unmapElement(element); } It would be better if disassociate does not check 'elementMap' to be non-null.
Note that this check was recently changed to elementMap.get(element) != null rather than calling findItem, to avoid the extra work it does for checking the input element. Would it be better if it called usingElementMap() instead? We would still need to access the map directly to get the item. Note that this is done in other places as well, e.g. findItem. Why do you want this change?
I would just overwrite mapElement & unmapElement, but don't have the enable the HashLookup.
To make this change, I would have to add a new method unmapElement(Object element, Item item) so it can do the check currently in disassociate. It could call unmapElement(Object) for backwards compatibility.
Regarding the original bug re unmapAllElements(), it would be nice to avoid having to do extra work in associate to cover for the case where there is an item->element mapping but not an element->item mapping. The simplest approach would be to simply null out the data fields of the items when the map is cleared, in unmapAllElements(). Unfortunately, this won't work since disassociate() asserts that the data is not null (and I'd like to leave this assert in). It also makes unmapAllElements () inconsistent with unmapElement(), which does not do this. Another approach is to remove all items in TableViewer.inputChanged, so that there is no inconsistency. However, this is expensive. It is much more costly to delete and re-add table items than just updating their labels. This would be a big hit for large tables (e.g. the task list). The best approach would be to remove the call to unmapElements() from setInput () and only do it in the inputChanged method of subclasses where it makes sense (it does for tree viewers, but not for TableViewer). Unfortunately, we can't do this since this would be a breaking API change. So for now, I will change associate to always do a mapElement. The cost is an extra table lookup (when the element map is enabled), which is much cheaper than item creation/deletion. I will also make the change to unmapElement proposed in the previous comment, which will let you simply override mapElement(Object, Item) and unmapElement (Object, Item) (override this one rather than unmapElement(Object)). Note that you'll likely get more mapElement calls than unmapElement calls due to the change to associate. Could you try out the following changes to StructuredViewer? Although the tests pass, and Eclipse runs, I am reluctant to release them just before going away for the holidays. You may require some further changes too. Please let me know. /** * Removes the given association from the internal element to widget map. * Does nothing if mapping is disabled, or if the given element does not map * to the given item. * <p> * This method is internal to the framework; subclassers should not call * this method. * </p> * * @param element the element * @since 2.0 */ protected void unmapElement(Object element, Widget item) { // double-check that the element actually maps to the given item before unmapping it if (elementMap != null && elementMap.get(element) == item) { // call unmapElement for backwards compatibility unmapElement(element); } } protected void disassociate(Item item) { Object element = item.getData(); Assert.isNotNull(element); item.setData(null); unmapElement(element, item); } protected void associate(Object element, Item item) { Object data = item.getData(); if (data != element) { if (data != null) disassociate(item); item.setData(element); } // Always map the element, even if data == element, // since unmapAllElements() can leave the map inconsistent // See bug 2741 for details. mapElement(element, item); }
Also need to change AbstractTreeViewer.doUpdateItem to call unmapElement(data, item) instead of unmapElement(data).
As for the extra work being done as per your note MA (28.08.2001 20:32:18), this is due to 2 things: 1. When multiple items are affected on a package fragment, JavaElementContentProvider does a full refresh on the fragment. This occurs in this case because both a .java and a .class are being added with autobuild on. Note that this is expensive, and may cause performance problems for large packages. 2. Refresh in the tree viewer is suboptimal and can lead to added items, or items whose corresponding element has changed, having their labels set twice. This is a known problem which we plan to improve.
I tested your implementation and it seems to work! The behaviour mentioned (unnecessary refreshes on the package viever) is bug 4138.
Changes released. Closing PR.
