Bug 312418

Comment 1 Alexander Nyßen 2010-05-11 15:29:36 EDT

The update problem can be reproduced, as reported by Syed Atif as part of bug #195527, as follows :

1. Add a  Circuit to the diagram
2. Add a LED inside the Circuit
3. Add a LED to the diagram surface (and not isnide the Circuit)
4. Resize the Circuit so that the contained LED is not visible.
5. Select the Circuit.
6. Move the Circuit around. 
Result: Notice the Connection does not move with the Circuit, rather it redraws
at the last position where the Circuit was.

Note that the issue is only reproducable when the above sketched scenario is performed completely, i.e. the circuit and all other figures are newly created. If the editor is re-opened on an already existing diagram, this cannot be reproduced.

Comment 2 Alexander Nyßen 2010-05-11 16:17:19 EDT

Created attachment 168025 [details]
Patch to fix problem.

The problem is that the feedback policy receives the event of the host figure being moved before the connection have revalidated their bounds. As we are post M7 I have attached a patch to fix this issue. 

Anthony, could you please review?

Comment 3 Anthony Hunter 2010-05-17 10:50:20 EDT

getLayer(LayerConstants.CONNECTION_LAYER).invalidateTree();
is going to repaint the connections later? Alex, can you review the patch too?

Syed, you reported this in Bug 195527

Comment 4 Syed Atif 2010-05-17 11:17:43 EDT

(In reply to comment #3)
> getLayer(LayerConstants.CONNECTION_LAYER).invalidateTree();
> is going to repaint the connections later? Alex, can you review the patch too?
> 
> Syed, you reported this in Bug 195527

Yes. Will it not be better, performance-wise, if we could refrain from doing an invalidateTree on an entire layer?

Comment 5 Alexander Nyßen 2010-05-17 11:40:59 EDT

The alternative would be to compute the incoming/outgoing connections that would have to be revalidated, which IMHO is not possible in all cases (if the ConnectionAnchors are not navigable, e.g. in case of XYAnchors) and which may probably also not be faster.

Comment 6 Alex Boyko 2010-05-17 15:18:48 EDT

Created attachment 168802 [details]
alternative patch

I have a perfect solution for you  I think.
Connection has a figure move listeners on the source and target, hence if connection gets the figure move event before this edit policy does you'll be ok, because connection would be invalid when you get to updateFeedback() method and validate connection layer.

I suggest that we add figure and layout listener when we start showing selection and remove when we hide selection instead of edit policy activation/deactivation. It would be a performance as well.

I attached the patch. Works fine for scenario described above.

Anyway, I agree we should avoid invalidating all children of the connection layer.

Let me know what you think.

Comment 7 Alex Boyko 2010-05-17 15:22:43 EDT

adding figure move and layout listeners when we show selection should ensure that figure move listreners from the connections would receive the move event before the editpolicy does and hence the editpolicy would have all appropriate connections invalidated already.
Will be a performance enhancement by itself, because the edit policy would listen to figure move events only when it needs too, i.e. the editpart is selected.
Feel free to correct me and enhance the patch further if it seems acceptable.

Comment 8 Alexander Nyßen 2010-05-17 15:32:20 EDT

Good idea! The patch looks good, just one comment: as
super.deactivate() calls hideSelection() (via setSelectedState), I think the
de-registration of the listeners within deactivate() is now superfluous and
could be removed. I would also propose to change the comment within activate()
and deactivate() to mention, that figure listeners will be
registered/deregistered upon showSelection()/hideSelection().

Comment 9 Anthony Hunter 2010-05-17 15:37:00 EDT

Do you think we should move this to RC2 so I can push the RC1 build?

Comment 10 Alexander Nyßen 2010-05-17 15:51:40 EDT

Created attachment 168810 [details]
Patch incorporating proposed changes to alternative patch.

I have incorporated my proposed changes into this patch. Alex, if you are fine with these changes, from my point of view it could be committed, resolving this as part of RC1.

Comment 11 Alex Boyko 2010-05-17 16:16:33 EDT

Yes, looks good. Of course I had to delete the old code removing the listeners in deactivate. Just forgot :-)

Comment 12 Alexander Nyßen 2010-05-17 16:27:51 EDT

Anthony, if there aren't any objections from your side, I would propose to now commit this as part of RC1.

Comment 13 Anthony Hunter 2010-05-17 21:01:26 EDT

(In reply to comment #12)
> Anthony, if there aren't any objections from your side, I would propose to now
> commit this as part of RC1.

OK, commit and then I will run RC1

Comment 14 Alexander Nyßen 2010-05-18 01:28:24 EDT

Changes committed to cvs HEAD. Verified with logic editor example that this issue is resolved.