Bug 322248

Comment 1 Aurelien Pupier 2011-02-21 17:05:45 EST

Hi,

I can't reproduce your issue.

Which version of GMF are you using?

I am on Windows Vista, you are using Windows 7, did you already tried reproduce it on Vista?

Ho wdo you configure to use TreeRouter? To activate it, I need to modify RelationshipEditPart, Relationship2EditPart.

The NPE pop up a window or is it only in the log that you have the error?

Thanks for report
Regards,
Aurelien

Comment 2 Yu Li 2011-03-03 10:33:06 EST

(In reply to comment #1)
> Hi,
> 
> I can't reproduce your issue.
> 
> Which version of GMF are you using?
> 
> I am on Windows Vista, you are using Windows 7, did you already tried reproduce
> it on Vista?
> 
> Ho wdo you configure to use TreeRouter? To activate it, I need to modify
> RelationshipEditPart, Relationship2EditPart.
> 
> The NPE pop up a window or is it only in the log that you have the error?
> 
> Thanks for report
> Regards,
> Aurelien

Hi,

According to my test a short while ago, the NullPointerException remains reproducible.

I'm using GMF SDK 2.3.0.v20100508 and Windows 7, but haven't tried on Vista.

To configure to use TreeRouter, you need only to modify

org.eclipse.gmf.examples.mindmap.rcp.diagram.preferences.DiagramPreferenceInitializer.java

as described in step 2.

The error message will appear on the console of the eclipse workbench, from which the mindmap application is started.

Regards,
Yu

Comment 3 Johannes Michler 2011-04-21 09:44:45 EDT

I can reproduce this with windows 7 and the Helios 3.6.2 GMF-Builds ().

Please note: The error doesn't occur on every creation of a connection but is somewhat random. But when trying to create 10 connections, be assured at least once there will be the mentioned null-pointer-exception. 

I tried to track this down by adding a breakpoint to the first line of the method 

org.eclipse.gmf.runtime.draw2d.ui.internal.routers.TreeRouter.getTargetAnchorRelativeBounds(Connection)

with the following condition:
conn.getTargetAnchor().getOwner()==null

(This is where Yu Li has his NPE in line 130 of the TreeRouter.java).

I think the problem is caused in TreeRouter.invalidate-Method:

public void invalidate(Connection conn) {
		if (conn.getSourceAnchor() == null || conn.getSourceAnchor().getOwner() == null ||
			conn.getTargetAnchor() == null || conn.getTargetAnchor().getOwner() == null)
			return;

		ListIterator li = connectionList.listIterator();
		while (li.hasNext()) {
			Connection connNext = (Connection)li.next();
			
			if (!trunkVertexEqual(connNext, conn)) {
				updateConstraint(connNext);
			}
		}
	}


There is a check if conn.getTargetAnchor().getOwner() is null. But this is done on the conn-Object, whereas some lines further-down updateConstraint is called with some Connection from the connectionList.

When having the mentioned NPE, the situation while being in the invalidate-Method is as follows:

conn.getSourceAnchor() and conn.getTargetAnchor() are both the same SlideableAnchor pointing to a DefaultSizeNodeFigure. Therefore in the first line of invalidate everything is fine

The connectionList consists of one single PolyLineConnection. This connNext-Object has the same SlideableAnchor as its source as the conn-Object. But the target of the connNext-Object points to an XYAnchor. And calling the getOwner() method of an XYAnchor returns null. This is what leads to the NPE later on in the getTargetAnchorRelativeBounds-Method.


My proposed fix for this is to check the owner of the sourceAnchor and targetAnchor before calling udpateConstraint on connNext. I've attached a patch that fixes the problem on the mindmap-sample.

Yu Li: Could you please verify?

I'd propse to bring this into 3.7, since the bug is really annoying when using the tree-router...

Best regards

Johannes Michler

Horus software GmbH
Incorporating the Power of Knowledge from Business Communities: http://www.horus.biz

Comment 4 Johannes Michler 2011-04-21 09:46:00 EDT

Created attachment 193820 [details]
propsed patch for gmf 1.4.2_v20101217-0030

fix according to my comment. Please verify if this fixes the bug and if it has any negative side-effects...

Comment 5 Yu Li 2011-04-21 11:53:49 EDT

Thanks, Johannes, the patch works according to my test. It would be fine if this patch can be incorporated into the new GMF version...

Comment 6 Aurelien Pupier 2011-04-21 12:23:29 EDT

(In reply to comment #5)
> Thanks, Johannes, the patch works according to my test. It would be fine if
> this patch can be incorporated into the new GMF version...

Thanks for the patch Johannes

I will try to take a look at this next week

Comment 7 Aurelien Pupier 2011-04-23 15:12:09 EDT

Created attachment 193960 [details]
second version of patch

I attach another patch. You were checking if the targetAnchor on ConneExt is null and not different from null.
Can you check that it is what you wanted to do?

Can you also provide a test case?

Regards,

Comment 8 Johannes Michler 2011-04-24 08:50:55 EDT

Created attachment 193966 [details]
fix:check for !=null instead of ==null on every parameter

Of cause I meant to execute the updateConstraint-Method only if source and target anchor and their owner are NOT null. So this was a typo, thank you for fixing it. But you just fixed one check, I've fixed the second too...

Yu Li provided a test-case, unfortunately I have no better or always reproducible way to test this :-(

Best regards
Johannes Michler

Comment 9 Aurelien Pupier 2011-05-05 16:29:45 EDT

patch applied

Comment 10 Anthony Hunter 2011-05-05 17:21:49 EDT

Comment on attachment 193966 [details]
fix:check for !=null instead of ==null on every parameter

Add iplog tag for Indigo since the patch came from a non committer.