Added type information to getEditPartRegisry #155

[azoitl]

trafficstars

As part of this clean-up a new helper method has been added for getting an EditPart for a model instance and for getting the LayerManager from a viewer.

@ptziegler this PR has still a problem, or better reveal a misuse of the EditPartRegistry by the palette. The SliderPaletteEditPart is registering an instance of PaletteAnimator at the EditPartRegsitry of the PaletteViewer. This PaletteAnimator is then later requested by DrawerEditParts. The problem is that PlaetteAnimator is not an EditPart. This totally violates the API description of EditPartViewer. Both SliderPaletteEditPart and DrawerEditPart are internal classes so we have a bit more freedom. Still I'm not sure how to best solve the issue. Currently I have two ideas on my mind:

1. Move the PaletteAnimator to the PaletteViewer, add there a getter and a factory method. So that clients can provide own animators. This would for me be the cleanest solution. However clients may expect that PaletteAnimator to be in the EditPart Registry.
2. Therefore my other solution would be to let PaletteAnimator implement the EditPart interface. This would definitely be less clean but not break clients.

So I'm unsure what to do.

[ptziegler]

Hm... that's indeed a tricky one.

this PR has still a problem, or better reveal a misuse of the EditPartRegistry by the palette. The SliderPaletteEditPart is registering an instance of PaletteAnimator at the EditPartRegsitry of the PaletteViewer. This PaletteAnimator is then later requested by DrawerEditParts. The problem is that PlaetteAnimator is not an EditPart.

We have a similar issue with the LayerManager, though it is not as critical here, as this interface is only implemented by edit parts.

https://github.com/eclipse/gef-classic/blob/11ae8bf359dec43733ca37ff03c7c218e0f32b0e/org.eclipse.gef/src/org/eclipse/gef/editparts/LayerManager.java#L59-L61

Therefore my other solution would be to let PaletteAnimator implement the EditPart interface. This would definitely be less clean but not break clients.

I don't like this at all. If we add this interface now, we'll be stuck with it indefinitely. Outside of this workaround, I don't see why a PaletteAnimator should be an edit part, but e.g. the LayoutAnimator should not.

Move the PaletteAnimator to the PaletteViewer, add there a getter and a factory method. So that clients can provide own animators. This would for me be the cleanest solution. However clients may expect that PaletteAnimator to be in the EditPart Registry.

As far as I can tell, there is no documented way for contributing a custom animator. So technically we wouldn't break API if we kick it out of the edit part registry. Though you are absolutely right that we have no control over what clients may or may not do.

I assume how it should've been done initially is by adding the SliderPaletteEditPart to the registry and then access the PaletteAnimator via a getter. But we might as well do it right and expose it it via the PaletteViewer, as you described.

[azoitl]

Hm... that's indeed a tricky one.

this PR has still a problem, or better reveal a misuse of the EditPartRegistry by the palette. The SliderPaletteEditPart is registering an instance of PaletteAnimator at the EditPartRegsitry of the PaletteViewer. This PaletteAnimator is then later requested by DrawerEditParts. The problem is that PlaetteAnimator is not an EditPart.

We have a similar issue with the LayerManager, though it is not as critical here, as this interface is only implemented by edit parts.

https://github.com/eclipse/gef-classic/blob/11ae8bf359dec43733ca37ff03c7c218e0f32b0e/org.eclipse.gef/src/org/eclipse/gef/editparts/LayerManager.java#L59-L61

Therefore my other solution would be to let PaletteAnimator implement the EditPart interface. This would definitely be less clean but not break clients.

I don't like this at all. If we add this interface now, we'll be stuck with it indefinitely. Outside of this workaround, I don't see why a PaletteAnimator should be an edit part, but e.g. the LayoutAnimator should not.

Move the PaletteAnimator to the PaletteViewer, add there a getter and a factory method. So that clients can provide own animators. This would for me be the cleanest solution. However clients may expect that PaletteAnimator to be in the EditPart Registry.

As far as I can tell, there is no documented way for contributing a custom animator. So technically we wouldn't break API if we kick it out of the edit part registry. Though you are absolutely right that we have no control over what clients may or may not do.

I assume how it should've been done initially is by adding the SliderPaletteEditPart to the registry and then access the PaletteAnimator via a getter. But we might as well do it right and expose it it via the PaletteViewer, as you described.

Thx for the detailed feedback. Based on that I went with the PaletteViewer version. As the SliderPaletteEditPart is GEF internal and we should definitely not expose this one.