-
Notifications
You must be signed in to change notification settings - Fork 49
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
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. |
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. |
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You have a point here. Will change accordingly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
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
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
SliderPaletteEditPart
is registering an instance ofPaletteAnimator
at the EditPartRegsitry of the PaletteViewer. ThisPaletteAnimator
is then later requested byDrawerEditPart
s. The problem is thatPlaetteAnimator
is not anEditPart
. This totally violates the API description ofEditPartViewer
. BothSliderPaletteEditPart
andDrawerEditPart
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:So I'm unsure what to do.