-
Notifications
You must be signed in to change notification settings - Fork 50
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
Clean editpart children #212
Conversation
Cleaned and improved AbstractEditPart to have a generic typed Java list. Furthermore cleaned-code as suggested by Sonar.
In addition quite some clean-up of the code.
GraphicalEditParts
962fa3b
to
00a285f
Compare
I'll try to have a look this week, but don't know when. |
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.
Looks good, some minor questions.
|
||
result.addAll(rightSide); | ||
return result; | ||
} | ||
|
||
private List findNodesBetweenInclusive(EditPart left, EditPart right) { | ||
private static List<EditPart> findNodesBetweenInclusive(EditPart left, EditPart right) { |
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.
Are you sure transferring this method to a static context will not break anything?
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.
As it is private and not accessing any fields and it was suggested by the Eclipse Java compiler I'm pretty sure.
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.
As this method is private, not accessing any fields, and as it was suggested by the Eclipse Java compiler I'm pretty sure. But if you are not feeling comfortable I'm happy to change back.
return new ImportsPart(cont); | ||
|
||
case Container.TYPE_COMMENT, Container.TYPE_PARAGRAPH: |
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.
Minor formatting issue
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.
How would you format this? I thought the new case statement with commas between cases is formated on one line.
Please take your time. I have the fix for GMF ready just in case. |
GraphicalEditPart.getChildren() now returns a List<? extends GraphicalEditPart> See-also: eclipse-gef/gef-classic#212 Signed-off-by: Pierre-Charles David <[email protected]>
@pcdavid I just saw your commit. So I think your are fine with merging it? Please note that I'm happy to perform such API change commits for GMF runtime as well. |
Let me check again, I'm not sure that patch compiled properly. |
GraphicalEditPart.getChildren() now returns a List<? extends GraphicalEditPart> See-also: eclipse-gef/gef-classic#212 Signed-off-by: Pierre-Charles David <[email protected]>
GraphicalEditPart.getChildren() now returns a List<? extends GraphicalEditPart> See-also: eclipse-gef/gef-classic#212 Signed-off-by: Pierre-Charles David <[email protected]>
I've merged the GMF part, you can go ahead. |
thx. Then I will do. Have a few more patches in the pipeline that I would love to have in RC1 to have early tests. For that I also changed a few of my test setups to catch things we missed in 3.16. |
In order to address #155 this PR reattempts to turn the children list and all its handling int java generics. For most users this PR should not provide any issues.
@Destrolaric what do you think?
@pcdavid I tried hard but I break 2 lines in GMF runtime. I would submit a PR to fix it. How and when would you like to have it?