Bugzilla – Full Text Bug Listing
| Summary: | GraphViewer produces invalid selections | ||
|---|---|---|---|
| Product: | [Tools] GEF | Reporter: | Daniel Krügler <daniel.kruegler> |
| Component: | GEF-Legacy Zest | Assignee: | gef-inbox <gef-inbox> |
| Status: | RESOLVED FIXED | QA Contact: | |
| Severity: | normal | ||
| Priority: | P3 | CC: | steeg |
| Version: | 3.6.1 | ||
| Target Milestone: | --- | ||
| Hardware: | PC | ||
| OS: | Windows 7 | ||
| Whiteboard: | |||
Thanks for the detailed report Daniel.
I had a look at the issue and I'm thinking: shouldn't
AbstractStructuredGraphViewer#getSelectionFromWidget return the selected widgets instead of their data? So something like this would be true:
Graph graph = new Graph(shell, SWT.NONE);
GraphNode n1 = new GraphNode(graph, SWT.NONE);
GraphNode n2 = new GraphNode(graph, SWT.NONE);
GraphConnection c = new GraphConnection(graph, SWT.NONE, n1, n2);
graph.setSelection(new GraphItem[] { n1, n2, c });
GraphViewer viewer = new GraphViewer(graph);
StructuredSelection sel = (StructuredSelection) viewer.getSelection();
assertEquals(3, sel.size());
assertEquals(n1, sel.toList().get(0)); // currently the data, i.e. null
assertEquals(n2, sel.toList().get(1));
assertEquals(c, sel.toList().get(2));
Does that make sense? Daniel, would it solve your issue as well?
(In reply to comment #1) I currently doubt that this is the correct approach, because the Items of any viewer are never considered as it's elements. Just compare with the TableItem objects of TableViewer, which are not the elements of this viewer. If GraphViewer would really do this, you need to carefully check whether the remaining parts of the viewer logic are consistent with this. E.g. the filtering of visual elements is also based on the data being null or not null. (In reply to comment #2) Yes, you are right - thanks for the sanity check. So to get it consistent with AbstractTableViewer#getSelectionFromWidget and other viewers I suggest this: https://github.com/fsteeg/zest/commit/0e530e4289b712dfa38b5fe89ae34ea44bb77d21 (In reply to comment #3) > (In reply to comment #2) > > Yes, you are right - thanks for the sanity check. So to get it consistent with > AbstractTableViewer#getSelectionFromWidget and other viewers I suggest this: > > https://github.com/fsteeg/zest/commit/0e530e4289b712dfa38b5fe89ae34ea44bb77d21 Yes, this looks good (even though I cannot test it here). I would also suggest to add one further constructor to GraphConnection that allows to set an "Object data", similarly to GraphItem. (In reply to comment #4) Fixed in 1.x head and 2.x master, see GraphViewerTests#testValidSelection Regarding constructors that set the data, I'd prefer not to add additional constructors to the Zest API. For consistency, I'd rather deprecate the existing constructors that set a data and point to Widget#setData instead. |
Since we have added a context menu to this viewer, we observe that we get reproducible NullPointerExceptions produced inside the action object contribution handler code of Eclipse (org.eclipse.ui.internal.ObjectContributorManager#getCommonClasses). It turned out that the cause for these exceptions is due to the GraphViewer which produces ISelection values which are not empty (the sequence of elements has a size > 0), but with elements have null values. But null values are not acceptable contents for any IStructuredSelection as described in the corresponding constructors of StructuredSelection. It turned out that these selection values are produced by querying the data property of the GraphNode contents of the viewer, e.g. in AbstractStructuredGraphViewer: protected List getSelectionFromWidget() { List internalSelection = getWidgetSelection(); LinkedList externalSelection = new LinkedList(); for (Iterator i = internalSelection.iterator(); i.hasNext();) { // @tag zest.todo : should there be a method on IGraphItem // to get the external data? GraphItem item = (GraphItem) i.next(); if (item instanceof GraphNode) { externalSelection.add(((GraphNode) item).getData()); } else if (item instanceof GraphConnection) { externalSelection.add(((GraphConnection) item).getExternalConnection()); } else if (item instanceof Widget) { externalSelection.add(((Widget) item).getData()); } } return externalSelection; } But looking at the GraphNode description there is nothing mentioned that the data property must have a well-defined value. In contrast, several constructors exist, that implicitly set the data value to null. IMO there are two choices to fix this: 1) The GraphViewer should document the relation between selection and data, in this case it seems that user code can never legally construct GraphNode objects with null data. 2) The GraphViewer takes care when constructing the selection values within AbstractStructuredGraphViewer#getSelectionFromWidget() to not add null data values to the complete selection. But also in this case, the relevance of the data property of GraphNode needs to be documented. There is a worse problem with GraphConnection: In this case there exists only a single constructor which does implicitly set the data property of the GraphItem to null. In this case we would strongly recommend to provide a further constructor that allows setting the data as well. As a workaround for this problem we currently have changed our code and always set a unique data value per GraphNode and per GraphConnection, but we strongly recommend to fix the contract-violations within getSelectionFromWidget() (and maybe some other places), because it did quite a time to locate the actual root of the problem: The exception trace which the user code observes does not contain any relation to zest, so there is nothing which points to this problem within the GraphViewer, unless you expect it there and trace it back.