Open azoitl opened 1 year ago
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
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
github-actions[bot]
commented
11 months ago This issue is stale because it has been open for 90 days with no activity.
github-actions[bot]
commented
8 months ago This issue is stale because it has been open for 90 days with no activity.
github-actions[bot]
commented
5 months ago This issue is stale because it has been open for 90 days with no activity.
github-actions[bot]
commented
2 months ago This issue is stale because it has been open for 90 days with no activity.
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: