Added type information to getEditPartRegisry #155 #500
Conversation
|
Hm... that's indeed a tricky one.
We have a similar issue with the LayerManager, though it is not as critical here, as this interface is only implemented by edit parts.
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.
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
force-pushed
the
editPartRegistry
branch
from
8317d3a
to
745fcf8
Compare
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. |
azoitl
force-pushed
the
editPartRegistry
branch
from
745fcf8
to
f0ec225
Compare
| @@ -844,7 +844,9 @@ protected final void registerAccessibility() { | |||
| * this method if they need to register this EditPart in additional ways. | |||
| */ | |||
| protected void registerModel() { | |||
| getViewer().getEditPartRegistry().put(getModel(), this); | |||
| @SuppressWarnings("unchecked") | |||
| Map<Object, EditPart> registry = (Map<Object, EditPart>) getViewer().getEditPartRegistry(); | |||
There was a problem hiding this comment.
I wonder if it's really a good idea for the getEditPartRegistry() to return a map with wildcards. All clients that register their own models now have to do the same cast in their edit parts...
There was a problem hiding this comment.
Thx for bringing it up. I noticed that also and was not sure. I was either thinking of non wildcard returns or an additional helper registerEditPart(EditPart ep, Object model). What do you think is better?
There was a problem hiding this comment.
I believe the cases where the registry only contains edit parts of a single type (i.e. where one would cast the map to something other than Map<Object, EditPart>) are mostly academical. So I would simply remove the wildcard, which would then hopefully minimize the amount of change required by downstream projects.
There was a problem hiding this comment.
You have a point here. Will change accordingly.
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. eclipse-gef#155
azoitl
force-pushed
the
editPartRegistry
branch
from
f0ec225
to
9aab252
Compare
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
SliderPaletteEditPartis registering an instance ofPaletteAnimatorat the EditPartRegsitry of the PaletteViewer. ThisPaletteAnimatoris then later requested byDrawerEditParts. The problem is thatPlaetteAnimatoris not anEditPart. This totally violates the API description ofEditPartViewer. BothSliderPaletteEditPartandDrawerEditPartare 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:So I'm unsure what to do.