Use Java Generics where possible

azoitl commented 1 year ago

GEF Classic's dates back to a time without generics. While this is in general nothing bad it makes maintaining GEF Classic hard. Also clients and GEF Classic have to perform more instanceof checks and cast then necessary. Therefore GEF Classic is also hard to use.

However just type all generics used is also not the solution as it may break client code. See for example the discussion in PR #81.

In the other side there are partly hidden issues in GEF Classic that a cleanup can reveal (e.g., Issue #154, or the planned upcoming PR https://github.com/azoitl/gef-classic/commit/69194fbf80e0c9b545788f662807c775d26ad6af).

As now several people are interested in improving the GEF Classic code base in this respect this issue is here to coordinate any effort. But also discuss how to best test and evaluate all proposed changes to make the transition for our clients as smooth as possible.

Furthermore the starting point for this has already been done. There are already several PRs merged for the 3.15 release but there is still some way ahead of us.

For starting this I've created a first PR #149 and based on that there are several commits in the pipeline that can be moved to PRs as soon the first one is merged:

First set of Validation and Check rules In order to ensure a smooth transition for our users I would suggest at least the following tests and rules to be performed:

PRs should be as small as possible each addressing one distinct feature
It should pass our unit tests
Our examples (e.g., logic editor) do behave as expected (e.g., for PR#149 I found an issue by doing this step)
Compile with additional clients (e.g., I do at least Eclipse GMF runtime and 4diac IDE). The more the merrier.

Destrolaric commented 1 year ago

@azoitl We discussed this with our team. We can accept the #149. And we are planning to split other parts of our https://github.com/Destrolaric/gef-classic/pull/2 into several smaller prs. Is it acceptable?

vogella commented 1 year ago

And we are planning to split other parts of our Destrolaric#2 into several smaller prs. Is it acceptable?

Yes

azoitl commented 1 year ago

@Destrolaric first of all thx a lot for your ongoing support and contributions. I'm very grateful for these. I'm also sorry that I'm causing here extra work and a burden. So also thx for that. I was thinking it would be good idea to meet in some online meeting so we can better sync on our interests. What do you think?

Destrolaric commented 1 year ago

@azoitl Thanks for the suggestion. It looks like a good idea. I think we may have the meeting next week, is that ok?

azoitl commented 1 year ago

@azoitl Thanks for the suggestion. It looks like a good idea. I think we may have the meeting next week, is that ok?

Next week sounds good. May I send you a few suggestions via email?

Destrolaric commented 1 year ago

@azoitl Yes, of course.

nitind commented 1 year ago

https://github.com/eclipse/gef-classic/blob/49d0515fa7fc09bc6b35d47643f9cc26661fb5c3/org.eclipse.draw2d/src/org/eclipse/draw2d/IFigure.java#L295 returning a list of <? extends IFigure> instead of IFigure breaks our builds (https://ci.eclipse.org/webtools/view/webtools_CI/job/webtools-jsf_master/2353/console):

[ERROR] Failed to execute goal org.eclipse.tycho:tycho-compiler-plugin:2.7.5:compile (default-compile) on project org.eclipse.jst.jsf.facesconfig.ui: Compilation failure: Compilation failure: [ERROR] /home/jenkins/agent/workspace/webtools-jsf_master/jsf/plugins/org.eclipse.jst.jsf.facesconfig.ui/src/org/eclipse/jst/jsf/facesconfig/ui/preference/BaseNodeFigure.java:[824] [ERROR] parent.getChildren().add(child); [ERROR] ^^^ [ERROR] The method add(capture#5-of ? extends IFigure) in the type List<capture#5-of ? extends IFigure> is not applicable for the arguments (IFigure) [ERROR] /home/jenkins/agent/workspace/webtools-jsf_master/jsf/plugins/org.eclipse.jst.jsf.facesconfig.ui/src/org/eclipse/jst/jsf/facesconfig/ui/preference/BaseNodeFigure.java:[839] [ERROR] parent.getChildren().add(child); [ERROR] ^^^ [ERROR] The method add(capture#7-of ? extends IFigure) in the type List<capture#7-of ? extends IFigure> is not applicable for the arguments (IFigure) [ERROR] 2 problems (2 errors) [ERROR] -> [Help 1]

azoitl commented 1 year ago

@nitind sorry that this breaks your build. I just pushed a fix with a small clean-up to webtools-jsf that fixes the issue. The reason we use the <? extends IFigure> version is documented in the discussion of PR #81.

But looking at your code I noticed that that what you like to do should be in the end offered by the IFigure interface, namely reordering a child. Because adding children by directly manipulating the children list is somewhat dangerous and may break the figure. What do you think about adding a reorderChild method to IFigure?

nitind commented 1 year ago

I don't think it's needed, given the various overloaded add() methods that already exist. I've pushed a small change atop yours in consideration of this. The more complicated scenario was the Dali build, with this error:

[ERROR] Failed to execute goal org.eclipse.tycho:tycho-compiler-plugin:2.7.5:compile (default-compile) on project org.eclipse.jpt.jpa.ui: Compilation failure: Compilation failure: [ERROR] /home/jenkins/agent/workspace/webtools-dali_master/jpa/plugins/org.eclipse.jpt.jpa.ui/src/org/eclipse/jpt/jpa/ui/internal/wizards/gen/AssociationsListComposite.java:[90] [ERROR] List associationFigures = figure.getChildren(); [ERROR] ^^^^^^^^^^^^^^^^^^^^ [ERROR] Type mismatch: cannot convert from List<capture#1-of ? extends IFigure> to List [ERROR] /home/jenkins/agent/workspace/webtools-dali_master/jpa/plugins/org.eclipse.jpt.jpa.ui/src/org/eclipse/jpt/jpa/ui/internal/wizards/gen/AssociationsListComposite.java:[105] [ERROR] List associationFigures = figure.getChildren(); [ERROR] ^^^^^^^^^^^^^^^^^^^^ [ERROR] Type mismatch: cannot convert from List<capture#2-of ? extends IFigure> to List [ERROR] 2 problems (2 errors) [ERROR] -> [Help 1]

azoitl commented 1 year ago

Thx for the feedback. Damn I really hoped that my changes would break less. Have to be more considerate for the upcoming changes. \cc @Destrolaric