Bug 321451

Comment 1 Anthony Hunter 2010-08-09 13:48:49 EDT

I am not seeing any changes to the initialization sequence in Polyline. You can going to have to elaborate further which exact method you are having issues with.

Polyline was last changed in 2008 for Bug 238874 .

Comment 2 Loic JULIEN 2010-08-15 02:46:02 EDT

(In reply to comment #1)
> I am not seeing any changes to the initialization sequence in Polyline. You can
> going to have to elaborate further which exact method you are having issues
> with.
> 
> Polyline was last changed in 2008 for Bug 238874 .

Hi Anthony,

It seems that the regression came from "[238874] gef-head Alex.Shatalin 081017 Create ScalablePolygonShape figure" when Polyline did override repaint and added a bounds = null statement. As the bounds is now null during the init of polyline we will end up figuring out the points though our decorator is not fully initialized.

Let me know if this clarifies.
~Loic

Comment 3 Anthony Hunter 2010-09-02 15:08:20 EDT

(In reply to comment #2)
> (In reply to comment #1)
> > I am not seeing any changes to the initialization sequence in Polyline. You can
> > going to have to elaborate further which exact method you are having issues
> > with.
> > 
> > Polyline was last changed in 2008 for Bug 238874 .
> 
> Hi Anthony,
> 
> It seems that the regression came from "[238874] gef-head Alex.Shatalin 081017
> Create ScalablePolygonShape figure" when Polyline did override repaint and
> added a bounds = null statement. As the bounds is now null during the init of
> polyline we will end up figuring out the points though our decorator is not
> fully initialized.
> 
> Let me know if this clarifies.
> ~Loic

So if we do not throw NPE and allow to continue, does that solve the problem?

Comment 4 Randy Hudson 2010-09-02 17:23:06 EDT

Why is this in Polyline?:

public void repaint() {
	bounds = null;
	super.repaint();
}

Comment 5 Alex Boyko 2010-09-02 17:36:23 EDT

Looks like accidental change... i.e. a parameter change method affecting the bounds didn't call bounds = null, but called repaint(). Then bounds = null have been added, but repaint() override wasn't removed.

Comment 6 Anthony Hunter 2010-09-03 15:10:10 EDT

(In reply to comment #4)
> Why is this in Polyline?:
> 
> public void repaint() {
>     bounds = null;
>     super.repaint();
> }

So we all agree that this method should be removed in 3.6.1? Please confirm and I can change before RC3.

Comment 7 Anthony Hunter 2010-09-07 15:10:55 EDT

Created attachment 178353 [details]
Patch to fix the issue.

Can you review this patch for 3.6.1?

Comment 8 Alex Boyko 2010-09-07 15:15:47 EDT

Looks good to me.

Comment 9 Anthony Hunter 2010-09-07 16:08:23 EDT

Committed to HEAD and R3_6_maintenance for 3.5.1 (Helios SR1).

Comment 10 David Williams 2010-09-10 01:03:12 EDT

Just to cross reference ... we found the fix here caused a hang in the WSDL editor in WTP (and a smaller regression in XSD editor). See bug 324884. 

We can probably work around ... but not sure if it'd effect other graphical editors?

Comment 11 Randy Hudson 2010-09-10 11:23:09 EDT

Here's the link to the old version of Polyline which was working correctly:

http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.gef/plugins/org.eclipse.draw2d/src/org/eclipse/draw2d/Polyline.java?view=markup&revision=1.25&root=Tools_Project

Someone found a couple of places where bounds were being set to null, and then repaint() was called, and decided to move the bounds=null into repaint, and also *delete* bounds=null from those callers of repaint(). (Which was wrong, since not every caller of repaint() needs bounds recalculated)

To fix this, you also have to add "bounds=null" back everywhere that it used to be:
addPoint
insertPoint
removeAllPoints
removePoint
setLineWidth
setPoint
setPoints

And possibly new API added since, like:
setLineAttributes

Comment 12 Keith Chong 2010-09-10 11:35:22 EDT

This fix regressed caused our WTP WSDL Editor to hang the workbench.  See bug 324884 for details.

Also, it regressed the drag and drop feedback in our WTP XSD Editor.  The feedback does not appear during dragging.  I will attach screen caps to show this.

Comment 13 Keith Chong 2010-09-10 11:38:12 EDT

Created attachment 178624 [details]
Drag feedback prior to this fix.

Comment 14 Keith Chong 2010-09-10 11:38:50 EDT

Created attachment 178625 [details]
Drag feedback with this fix.

Comment 15 Alex Boyko 2010-09-10 11:40:52 EDT

AbstractPointListShape doesn't null the bounds for some reason... I thought it does... should have looked first. Yes, looks like we have to override all those methods or move the bounds caluulation logic and nullification of the bounds to AbstractPointListShape.

Comment 16 Anthony Hunter 2010-09-10 12:06:10 EDT

Hi again, For RC4 and GEF 3.6.1 in Helios SR1 next week, I am going to restore the code to what was delivered in GEF 3.6 for Helios GA.

I am not sure what we should consider doing for GEF 3.6.2 in Helios SR2, if anything, but the status is this.

1) A team requested a feature *very* early in GEF 3.5 via a patch and it was delivered in 3.5 M3.
2) Nobody complained for two GEF releases, well over two years.
3) An IBM team complained that the change broke their stuff, escalated the issue requiring the need to look at the issue (IBM pays my salary).
4) We tried to fix and broke other stuff and in addition got more complaints that the original change two years ago may be invalid.

I am of the opinion that the change is in, the changes never broke my application or any of the GEF examples I have to work with and we leave the changes as is.

Comment 17 Keith Chong 2010-09-10 15:57:50 EDT

> I am of the opinion that the change is in, the changes never broke my
> application or any of the GEF examples I have to work with and we leave the
> changes as is.

Hi Anthony, I tried most if not all the GEF examples that I could find.  As far as I could tell, none use Polygon or Polyline.

Comment 18 Anthony Hunter 2010-09-13 11:32:13 EDT

(In reply to comment #16)
> Hi again, For RC4 and GEF 3.6.1 in Helios SR1 next week, I am going to restore
> the code to what was delivered in GEF 3.6 for Helios GA.

I have rolled back the change in HEAD and R3_6_maintenance for 3.5.1 (Helios SR1).

Comment 19 Anthony Hunter 2010-09-15 11:37:32 EDT

(In reply to comment #18)
> I have rolled back the change in HEAD and R3_6_maintenance for 3.5.1 (Helios
> SR1).