Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.

Bug 335136

Summary:

Custom figures in GraphViewer are not highlighted correctly

Product:

[Tools] GEF

Reporter:

Dale Swift <daleswift>

Component:

GEF-Legacy Zest

Assignee:

gef-inbox <gef-inbox>

Status:

RESOLVED FIXED

QA Contact:

Severity:

normal

Priority:

CC:

eclipse-bugs, m, nyssen, steeg

Version:

unspecified

Target Milestone:

---

Hardware:

OS:

Windows 7

Whiteboard:

Attachments:

Description	Flags
Proposed patch to allow styling and labeling of custom figures	none
Eclipse project with two example snippets	none
Proposed patch to allow styling and labeling of custom figures (v2)	steeg: iplog+
Patch adding base classes for implementing custom shaped figures/labels	none

Description Dale Swift

2011-01-23 17:01:52 EST

Build Identifier: Zest core 1.2.0v20100525-1225

Using GraphViewer in Zest the label provider can supply custom figures by implementing IFigureProvider. The caller supplied figure is wrapped in an instance of CGraphNode. This works fine except that there is no way for the caller to know when the custom figure should be highlighted. The end result is that selected nodes in the viewer are not displayed as highlighted.

The problem is that CGraphNode overrides GraphNode.updateFigureForModel but does nothing. Whilst it makes sense that in the general case CGraphNode does not know for sure how to highlight a custom figure there is already a safety check in GraphNode which does nothing unless the figure is an instance of GraphLabel. It should be allowed that custom figures derived from GraphLabel are highlighted in the same way as GraphLabel itself.

In my case I am implementing a custom figure which is a subclass of GraphLabel but with some minor tweaks (which allow certain nodes to be displayed transparently when the user wants to focus on a subset of nodes).

There are a few possible solutions I can suggest for this problem
1) Remove updateFigureForModel from CGraphNode and allow the call to pass to GraphNode. Then if the caller's custom figure is a subclass of GraphLabel it will be highlighted in the normal way.
2) Alternately allow the user to provide an implementation of GraphNode in place of CGraphNode (e.g. allow the label provider to supply a subclass of GraphNode by some new interface IGraphNodeProvider)
3) There is no easy workaround to this problem as key methods in GraphViewer are package accessible and cannot be access or overridden. Sepcifically if the methods addGraphModelNode and getNodesMap in AbstractStructuredGraphViewer were protected they could be used by the caller to implement a custom GraphNode.

Reproducible: Always

Steps to Reproduce:
1. implement IFigureProvider in the label provider of a GraphViewer
2. The method getFigure in the label provider needs to supply an instance of GraphLabel or a subclass thereof. Make sure to initialise background and foreground colours in the object
3. Run the viewer.
4. When nodes are selected in the viewer they are not highlighted

Comment 1 Martin Burger

2011-09-30 15:54:49 EDT

I have a similar issue: my implementation of ILabelProvider-let's call it "MyLabelProvider"-which is used as label provider for a GraphViewer, implements both IEntityStyleProvider and IFigureProvider as well.

In my implementation of MyLabelProvider.getFigure(...) I return a custom IFigure for selected nodes. Furthermore, in MyLabelProvider.getBorderColor(...) I return a custom color.

I would expect that my custom node figures would be highlighted in that custom color. However, these nodes are not highlighted at all.

Comment 2 Simon Templer

2011-11-21 09:28:54 EST

Created attachment 207305 [details]
Proposed patch to allow styling and labeling of custom figures

I have attached a proposal for solving this issue.
In short, my idea was to introduce two interfaces (for a figure supporting to be labeled or styled) and base the implementation of GraphNode.updateFigureForModel(...) on the interfaces instead of the GraphLabel class (GraphLabel implements both). CGraphNode uses the updateFigureForModel implementation of its super class.

Please let me know what you think of this solution and what might have to be changed to include it in the code base.

Comment 3 Simon Templer

2011-11-21 09:37:09 EST

Created attachment 207307 [details]
Eclipse project with two example snippets

The attached eclipse project contains two snippets, the first (CustomFigureSample) for demonstrating the problem without the patch. The custom figures provided through the IFigureProvider don't change their style if they are selected (even the one that actually is a GraphLabel).
The second (CustomFigureSampleStylable) uses the changes made in the patch, the custom figures are updated when the selection changes.

Comment 4 Fabian Steeg

2011-12-18 12:35:13 EST

Thanks a lot for your patch Simon, looks good to me.

Could you resubmit with the following additions:

- Legal file headers for the two new files (see [1] for a template)
- Your CustomFigureSampleStyleable, with a legal header, in project org.eclipse.zest.examples (slimmed down a bit, e.g. without selection listener)
- A statement (as a comment on this bug) confirming that you wrote 100% of the code yourself, and that you have the right to contribute it under the EPL

[1] http://eclipse.org/legal/copyrightandlicensenotice.php

Comment 5 Simon Templer

2011-12-19 09:04:20 EST

Created attachment 208551 [details]
Proposed patch to allow styling and labeling of custom figures (v2)

Thank you very much for your feedback, Fabian.
I changed the patch according to your instructions and improved the example.

I will also add a second patch where I added a set of classes that are intended to make it easier to create a custom shaped figure/label for use in a graph. An example is also included.

Comment 6 Simon Templer

2011-12-19 09:07:07 EST

Created attachment 208552 [details]
Patch adding base classes for implementing custom shaped figures/labels

The aforementioned second patch (depends on the first). Please take a look at it, if it makes sense for you to also include it.

Comment 7 Simon Templer

2011-12-19 09:08:28 EST

Legal Message: 

I, Simon Templer, declare that I developed attached code from scratch,
without referencing any 3rd party materials except material licensed under the
EPL. I certify that I am the copyright owner and authorize this contribution.

Comment 8 Fabian Steeg

2011-12-19 12:05:18 EST

Thanks for your updated patch. Fixed in master, see CustomFigureJFaceSnippet.

http://git.eclipse.org/c/gef/org.eclipse.zest.git/commit/?id=22efd6bd97a4df90a0

About the other patch: I'm generally reluctant to add new convenience-only API. Also, as a larger contribution (> 250 lines) it would need full legal review. If you'd like to discuss this, or prepare a smaller, example-only contribution (without introducing new API), feel free to open a new bug for this.

Comment 9 Simon Templer

2011-12-20 03:27:43 EST

Thanks again, Fabian. This being included in Zest helps a lot - not being able to really use custom figures would have been a real show-stopper for me. Keep up the good work!

Comment 10 Alexander Nyßen

2014-05-22 03:43:33 EDT

Comment on attachment 208552 [details]
Patch adding base classes for implementing custom shaped figures/labels

This patch does not seem to have been applied, thus marking obsolete.